Shopkeepers

Shopkeepers

Bukkit Plugins
2M Downloads

Can't Add Shopkeeper to existing Citizen NPC using /trait shopkeeper

frost-byte opened this issue ยท 3 comments

commented

Spigot 1.13.1
Shopkeepers v2.4.0

I receive the error below when trying to use the /trait command to add a shop to an existing npc.

Looking at the code, there is some confusion about valid values for the creator parameter for ShopCreationData. The comment on the constructor states

	/**
	 * Creates a {@link ShopCreationData}.
	 * 
	 * @param creator  the creator, can be <code>null</code>

However, on line 61-62 in AbstractShopType there's this:

		// the creator, can not be null when creating a shopkeeper via this method:
		Player creator = shopCreationData.getCreator();
		Validate.notNull(creator, "Creator cannot be null!");

Is there no way to assign the command issuer as the creator? or maybe add a field to
ShopCreationData that indicates whether or not the creator field can be null?

[Shopkeepers] Task #4300 for Shopkeepers v2.4.0 generated an exception
  java.lang.IllegalArgumentException: Creator cannot be null!
  at org.apache.commons.lang.Validate.notNull(Validate.java:192) ~[server.jar:git-Spigot-2440e18-479ec05]
  at com.nisovin.shopkeepers.shopkeeper.AbstractShopType.handleShopkeeperCreation(AbstractShopType.java:62) ~[?:?]
  at com.nisovin.shopkeepers.SKShopkeepersPlugin.handleShopkeeperCreation(SKShopkeepersPlugin.java:462) ~[?:?]  
  at com.nisovin.shopkeepers.SKShopkeepersPlugin.handleShopkeeperCreation(SKShopkeepersPlugin.java:69) ~[?:?]
  at com.nisovin.shopkeepers.shopobjects.citizens.CitizensShopkeeperTrait.lambda$onAttach$1(CitizensShopkeeperTrait.java:134) ~[?:?]
  at org.bukkit.craftbukkit.v1_13_R2.scheduler.CraftTask.run(CraftTask.java:76) ~[server.jar:git-Spigot-2440e18-479ec05]
  at org.bukkit.craftbukkit.v1_13_R2.scheduler.CraftScheduler.mainThreadHeartbeat(CraftScheduler.java:361) [server.jar:git-Spigot-2440e18-479ec05]
  at net.minecraft.server.v1_13_R2.MinecraftServer.b(MinecraftServer.java:889) [server.jar:git-Spigot-2440e18-479ec05]
  at net.minecraft.server.v1_13_R2.DedicatedServer.b(DedicatedServer.java:411) [server.jar:git-Spigot-2440e18-479ec05]
  at net.minecraft.server.v1_13_R2.MinecraftServer.a(MinecraftServer.java:831) [server.jar:git-Spigot-2440e18-479ec05]
  at net.minecraft.server.v1_13_R2.MinecraftServer.run(MinecraftServer.java:729) [server.jar:git-Spigot-2440e18-479ec05]
  at java.lang.Thread.run(Thread.java:748) [?:1.8.0_171]
commented

Yes this is a bug. However, I cannot fix it any time soon (i'm on vacation).

Regarding the creator being required: The handleShopkeeperCreation method requires a player / creator, because it is meant to handle 'shopkeeper creation as if done by a player' (potentially including restriction checks that require a player, like checking world guard protection, chest access, ..)). I have added a null-check recently (after moving even more logic to that method that requires a player).

Possible solutions might be:

  • Handle npc loading (trait re-attachement after npc was loaded, no player available there) separately from 'player initially attaches the trait via command' (assuming Citizens offers an API for that) (this might however conflict with plugins that allow players to attach the trait 'indirectly' / by other means not taken into account by Citizens, resulting in no player being available there).
  • Disable npc shopkeeper creation via trait for player shopkeepers (admin shopkeepers might not perform checks that require a player, at least currently) (trying to avoid that) and then:
  • Create the shopkeeper differently there (without taking restrictions into account etc,).
  • Let the handleShopkeeperCreation function skip any checks that require a player if the player is null.

All of these seem to have caveats. So if you see another possible solution, let me know.
Currently I intend to fix it by just ignore any checks that require a player there..

commented

Your final option is what I've used as a temporary workaround. In this case I'm creating an admin shop by adding the trait using Citizens, which requires the command issuer to have admin level permissions for both Citizens and Shopkeeper.

I tried checking the UUID provided by the Owner trait, after making myself the owner of the NPC, but that didn't appear to have a valid UUID, as I was hoping that might be an option.

public T handleShopkeeperCreation(ShopCreationData shopCreationData) {
	this.validateCreationData(shopCreationData);
	SKShopkeeperRegistry shopkeeperRegistry = SKShopkeepersPlugin.getInstance().getShopkeeperRegistry();

	// the creator, can not be null when creating a shopkeeper via this method:
	Player creator = shopCreationData.getCreator();
	//Validate.notNull(creator, "Creator cannot be null!");

	ShopType<?> shopType = shopCreationData.getShopType();
	ShopObjectType<?> shopObjectType = shopCreationData.getShopObjectType();

	if (creator != null)
	{
		// can the selected shop type be used?
		if (!shopType.hasPermission(creator)) {
			Utils.sendMessage(creator, Settings.msgNoPermission);
			return null;
		}
		if (!shopType.isEnabled()) {
			Utils.sendMessage(creator, Settings.msgShopTypeDisabled, "{type}", shopType.getIdentifier());
			return null;
		}

		// can the selected shop object type be used?
		if (!shopObjectType.hasPermission(creator)) {
			Utils.sendMessage(creator, Settings.msgNoPermission);
			return null;
		}
		if (!shopObjectType.isEnabled()) {
			Utils.sendMessage(creator, Settings.msgShopObjectTypeDisabled, "{type}", shopObjectType.getIdentifier());
			return null;
		}
	}

	Location spawnLocation = shopCreationData.getSpawnLocation();
	BlockFace targetedBlockFace = shopCreationData.getTargetedBlockFace();

	// check if the shop can be placed there (enough space, etc.):
	if (!shopObjectType.isValidSpawnLocation(spawnLocation, targetedBlockFace)) {
		// invalid spawn location or targeted block face:
		if (creator != null)
			Utils.sendMessage(creator, Settings.msgShopCreateFail);
		return null;
	}

	if (!shopkeeperRegistry.getShopkeepersAtLocation(spawnLocation).isEmpty()) {
		// there is already a shopkeeper at that location:
		if (creator != null)
			Utils.sendMessage(creator, Settings.msgShopCreateFail);
		return null;
	}

	try {
		// shop type specific handling:
		if (!this.handleSpecificShopkeeperCreation(shopCreationData)) {
			return null;
		}

		// create and spawn the shopkeeper:
		@SuppressWarnings("unchecked")
		T shopkeeper = (T) shopkeeperRegistry.createShopkeeper(shopCreationData);
		assert shopkeeper != null;

		// send creation message to creator:
		if (creator != null)
			Utils.sendMessage(creator, this.getCreatedMessage());

		// save:
		shopkeeper.save();

		return shopkeeper;
	} catch (ShopkeeperCreateException e) {
		// unexpected issue (hints to a bug):
		if (creator != null)
			Utils.sendMessage(creator, ChatColor.RED + "Shopkeeper creation failed: " + e.getMessage());
		return null;
	}
}
commented

Should be fixed in the next version (v2.4.1) (please test and let me know if you encounter any issues).
It handles shopkeeper creation differently now for when a player initially creates the shopkeeper via trait command (also produces nice 'shopkeeper creation' messages there now).