CommandAPI Make NativeProxyCommandSender extend CraftBukkit class ProxiedNativeCommandSender

This fixes https://github.com/JorelAli/CommandAPI/issues/477. See that issue for context and a deeper analysis of the problem.

When VanillaCommandWrapper runs a command, it uses its getListener method to convert a Bukkit CommandSender into a Vanilla CommandListenerWrapper. When the sender implements org.bukkit.command.ProxiedCommandSender, it runs this code:

if (sender instanceof ProxiedCommandSender) {
	return ((ProxiedNativeCommandSender) sender).getHandle();
}

Previously, dev.jorel.commandapi.wrappers.NativeProxyCommandSender implemented ProxiedCommandSender, but not ProxiedNativeCommandSender, so a ClassCastException was thrown if a NativeProxyCommandSender ever tried to execute a Vanilla command.

To fix this issue, instances of NativeProxyCommandSender need to be ProxiedNativeCommandSender objects. This isn't trivial to do, since ProxiedNativeCommandSender is a version-specific CraftBukkit class (org.bukkit.craftbukkit.v1_20_R1.command.ProxiedNativeCommandSender for example). It is still possible, you just need to create an implementation of NativeProxyCommandSender for each NMS version, and that is what this PR does.

NativeProxyCommandSender is now an interface, like this:

public interface NativeProxyCommandSender extends ProxiedCommandSender {
	Location getLocation();

	World getWorld();
}

It still inherits all the methods from ProxiedCommandSender and adds the getLocation and getWorld methods, as described in the docs.

In each of the commandapi-bukkit-nms modules, there is now a NativeProxyCommandSender_(VERSION) class. For example, there is this class in commandapi-bukkit-nms-1.20:

import org.bukkit.craftbukkit.v1_20_R1.command.ProxiedNativeCommandSender;

public class NativeProxyCommandSender_1_20_R1 extends ProxiedNativeCommandSender implements NativeProxyCommandSender {
	private final World world;
	private final Location location;

	public NativeProxyCommandSender_1_20_R1(CommandSourceStack css, CommandSender caller, CommandSender callee) {
		super(css, caller, callee);

		Vec3 pos = css.getPosition();
		Vec2 rot = css.getRotation();
		this.world = CommandAPIBukkit.get().getWorldForCSS(css);
		this.location = new Location(this.world, pos.x(), pos.y(), pos.z(), rot.y, rot.x);
	}

	@Override
	public Location getLocation() {
		return location;
	}

	@Override
	public World getWorld() {
		return world;
	}
}

The logic for creating a Location and World from the CommandSourceStack was moved from the implementation of CommandAPIBukkit#getSenderForCommand. Extending ProxiedNativeCommandSender takes care of the implementation for all the ProxiedCommandSender methods (I checked and they look equivalent to the implementations that used to be in NativeProxyCommandSender), so these classes only have to implement getLocation and getWorld.

Luckily, NativeProxyCommandSender was being constructed in the NMS implementations anyway, so it was easy to change those:

@Override
public BukkitCommandSender<? extends CommandSender> getSenderForCommand(CommandContext<CommandSourceStack> cmdCtx, boolean isNative) {
	CommandSourceStack css = cmdCtx.getSource();

	CommandSender sender = css.getBukkitSender();
	if (sender == null) {
		// Sender CANNOT be null. This can occur when using a remote console
		// sender. You can access it directly using this.<MinecraftServer>getMinecraftServer().remoteConsole
		// however this may also be null, so delegate to the next most-meaningful sender.
		sender = Bukkit.getConsoleSender();
	}

	Entity proxyEntity = css.getEntity();
	CommandSender proxy = proxyEntity == null ? null : proxyEntity.getBukkitEntity();
	if (isNative || (proxy != null && !sender.equals(proxy))) {
		if (proxy == null) {
			proxy = sender;
		}
		return new BukkitNativeProxyCommandSender(new NativeProxyCommandSender_1_20_R1(css, sender, proxy));
	} else {
		return wrapCommandSender(sender);
	}
}

Instead of constructing a NativeProxyCommandSender directly, each NMS uses its version-specific implementation. The logic for extracting the Location and World has also been moved into the constructor for NativeProxyCommandSender_(VERSION), so it isn't in here anymore.

IMPORTANT: This is a breaking API change

Unfortunately, NativeProxyCommandSender cannot be a class anymore, because ProxiedNativeCommandSender is a class, and Java doesn't have multi-class inheritance. This is problematic in two ways.

First, developers could have created their own instances of NativeProxyCommandSender with something like new NativeProxyCommandSender(caller, callee, location, world). Since NativeProxyCommandSender is now an interface, it can't be instantiated like that anymore. I wouldn't expect anyone to do that, since usually you are just given a NativeProxyCommandSender when using executesNative, but it might be a problem.

Second, a java.lang.IncompatibleClassChangeError occurs when using the plugin version of the CommandAPI. For example, say I compile this command against version 9.0.3:

new CommandAPICommand("test")
        .executesNative((sender, args) -> {
            sender.getCallee().sendMessage("You are the target of the test command");
        })
        .register();

Notably, this code uses the method ProxiedCommandSender#getCallee on a NativeProxyCommandSender object. If I try to run this command using the CommandAPI plugin jar for this PR, I get this exception:

[Server thread/ERROR]: [CommandAPI] Unhandled exception executing '/test'
java.lang.IncompatibleClassChangeError: Found interface dev.jorel.commandapi.wrappers.NativeProxyCommandSender, but class was expected
        at me.willkroboth.CommandAPITest.Main.lambda$onEnable$0(Main.java:24) ~[?:?]
        at dev.jorel.commandapi.executors.NativeCommandExecutor.run(NativeCommandExecutor.java:49) ~[?:?]
        at dev.jorel.commandapi.executors.NormalExecutor.executeWith(NormalExecutor.java:44) ~[?:?]
        at dev.jorel.commandapi.CommandAPIExecutor.execute(CommandAPIExecutor.java:138) ~[?:?]
        at dev.jorel.commandapi.CommandAPIExecutor.execute(CommandAPIExecutor.java:113) ~[?:?]
        at dev.jorel.commandapi.CommandAPIExecutor.execute(CommandAPIExecutor.java:96) ~[?:?]
        at dev.jorel.commandapi.CommandAPIHandler.lambda$generateCommand$0(CommandAPIHandler.java:258) ~[?:?]
        at com.mojang.brigadier.CommandDispatcher.execute(CommandDispatcher.java:265) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:?]
        at net.minecraft.commands.CommandDispatcher.performCommand(CommandDispatcher.java:314) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.commands.CommandDispatcher.performPrefixedCommand(CommandDispatcher.java:293) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at org.bukkit.craftbukkit.v1_20_R1.command.VanillaCommandWrapper.execute(VanillaCommandWrapper.java:45) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at org.bukkit.command.SimpleCommandMap.dispatch(SimpleCommandMap.java:149) ~[spigot-api-1.20.1-R0.1-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_20_R1.CraftServer.dispatchCommand(CraftServer.java:875) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at org.bukkit.craftbukkit.v1_20_R1.CraftServer.dispatchServerCommand(CraftServer.java:860) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.server.dedicated.DedicatedServer.bf(DedicatedServer.java:413) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.server.dedicated.DedicatedServer.b(DedicatedServer.java:389) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.server.MinecraftServer.a(MinecraftServer.java:1198) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.server.MinecraftServer.w(MinecraftServer.java:1015) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at net.minecraft.server.MinecraftServer.lambda$0(MinecraftServer.java:304) ~[spigot-1.20.1-R0.1-SNAPSHOT.jar:3838-Spigot-3374045-a2fafdd]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]
[Server thread/INFO]: An unexpected error occurred trying to execute that command

As the exception explains, the bytecode for my command expected NativeProxyCommandSender to be a class, but it found an interface instead. To fix this, I just need to recompile my plugin since the available methods don't change. Still, code compiled against older versions may not work with this PR if they use methods in NativeProxyCommandSender.

There might be some way to avoid these issues, but for now, this PR is incompatible with certain interactions with NativeProxyCommandSender.

TODO:

[x] Apply the changes to the test framework (whoops, forgot that)
[x] Make sure tests run as before
Test NMS code on real servers
- [x] getSenderForCommand
- [x] createNativeProxyCommandSender

versions

[x] 1.15, 1.15.1, 1.15.2
[x] 1.16.1
[x] 1.16.2, 1.16.3
[x] 1.16.4
[x] 1.16.5
[x] 1.17
[x] 1.17.1
[x] 1.18, 1.18.1
[x] 1.18.2
[x] 1.19
[x] 1.19.1, 1.19.2
[x] 1.19.3
[x] 1.19.4
[x] 1.20, 1.20.1
[x] 1.20.2

Aug 07 '23 15:08 willkroboth

Created branch dev/10.0.0 for backwards-incompatible changes. Please ensure this PR is retargeted to dev/10.0.0 instead of dev/dev before merging.

Aug 08 '23 15:08 JorelAli

First, developers could have created their own instances of NativeProxyCommandSender with something like new NativeProxyCommandSender(caller, callee, location, world). Since NativeProxyCommandSender is now an interface, it can't be instantiated like that anymore. I wouldn't expect anyone to do that, since usually you are just given a NativeProxyCommandSender when using executesNative, but it might be a problem.

There should be a new way for developers to create their own NativeProxyCommandSenders with this information.

TODO:

[x] Add API method for constructing a NativeProxyCommandSender
[x] Add tests for constructing a NativeProxyCommandSender
[x] Document constructor on the relevant page: https://commandapi-live-docs.jorel.dev/native.html
[x] Add a note to the upgrading guide

Aug 08 '23 16:08 willkroboth