Merge branch 'cleanup/plugin-message-channel-handling' into dev/3.0.0

proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BungeeCordMessageResponder.java

@@ -20,6 +20,7 @@ package com.velocitypowered.proxy.connection.backend;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.proxy.Player;
 import com.velocitypowered.api.proxy.ServerConnection;
+import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
 import com.velocitypowered.api.proxy.server.RegisteredServer;
@@ -316,9 +317,9 @@ public class BungeeCordMessageResponder {
     });
   }
 
-  static String getBungeeCordChannel(ProtocolVersion version) {
+  static ChannelIdentifier getBungeeCordChannel(ProtocolVersion version) {
-    return version.noLessThan(ProtocolVersion.MINECRAFT_1_13) ? MODERN_CHANNEL.getId()
+    return version.noLessThan(ProtocolVersion.MINECRAFT_1_13) ? MODERN_CHANNEL
-        : LEGACY_CHANNEL.getId();
+        : LEGACY_CHANNEL;
   }
 
   // Note: this method will always release the buffer!
@@ -329,8 +330,8 @@ public class BungeeCordMessageResponder {
   // Note: this method will always release the buffer!
   private static void sendServerResponse(ConnectedPlayer player, ByteBuf buf) {
     MinecraftConnection serverConnection = player.ensureAndGetCurrentServer().ensureConnected();
-    String chan = getBungeeCordChannel(serverConnection.getProtocolVersion());
+    ChannelIdentifier chan = getBungeeCordChannel(serverConnection.getProtocolVersion());
-    PluginMessagePacket msg = new PluginMessagePacket(chan, buf);
+    PluginMessagePacket msg = new PluginMessagePacket(chan.getId(), buf);
     serverConnection.write(msg);
   }
 

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java

@@ -30,8 +30,6 @@ import com.velocitypowered.api.event.player.TabCompleteEvent;
 import com.velocitypowered.api.event.player.configuration.PlayerEnteredConfigurationEvent;
 import com.velocitypowered.api.network.ProtocolVersion;
 import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
-import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
-import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
 import com.velocitypowered.proxy.VelocityServer;
 import com.velocitypowered.proxy.connection.ConnectionTypes;
 import com.velocitypowered.proxy.connection.MinecraftConnection;
@@ -162,7 +160,7 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
   @Override
   public void activated() {
     configSwitchFuture = new CompletableFuture<>();
-    Collection<String> channels =
+    Collection<ChannelIdentifier> channels =
         server.getChannelRegistrar().getChannelsForProtocol(player.getProtocolVersion());
     if (!channels.isEmpty()) {
       PluginMessagePacket register = constructChannelsPacket(player.getProtocolVersion(), channels);
@@ -310,22 +308,14 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
         logger.warn("A plugin message was received while the backend server was not "
             + "ready. Channel: {}. Packet discarded.", packet.getChannel());
       } else if (PluginMessageUtil.isRegister(packet)) {
-        List<String> channels = PluginMessageUtil.getChannels(packet);
+        List<ChannelIdentifier> channels = PluginMessageUtil.getChannels(packet, this.player.getProtocolVersion());
         player.getClientsideChannels().addAll(channels);
-        List<ChannelIdentifier> channelIdentifiers = new ArrayList<>();
-        for (String channel : channels) {
-          try {
-            channelIdentifiers.add(MinecraftChannelIdentifier.from(channel));
-          } catch (IllegalArgumentException e) {
-            channelIdentifiers.add(new LegacyChannelIdentifier(channel));
-          }
-        }
         server.getEventManager()
             .fireAndForget(
-                new PlayerChannelRegisterEvent(player, ImmutableList.copyOf(channelIdentifiers)));
+                new PlayerChannelRegisterEvent(player, ImmutableList.copyOf(channels)));
         backendConn.write(packet.retain());
       } else if (PluginMessageUtil.isUnregister(packet)) {
-        player.getClientsideChannels().removeAll(PluginMessageUtil.getChannels(packet));
+        player.getClientsideChannels().removeAll(PluginMessageUtil.getChannels(packet, this.player.getProtocolVersion()));
         backendConn.write(packet.retain());
       } else if (PluginMessageUtil.isMcBrand(packet)) {
         String brand = PluginMessageUtil.readBrandMessage(packet.content());
@@ -589,7 +579,7 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler {
 
     // Tell the server about the proxy's plugin message channels.
     ProtocolVersion serverVersion = serverMc.getProtocolVersion();
-    final Collection<String> channels = server.getChannelRegistrar()
+    final Collection<ChannelIdentifier> channels = server.getChannelRegistrar()
         .getChannelsForProtocol(serverMc.getProtocolVersion());
     if (!channels.isEmpty()) {
       serverMc.delayedWrite(constructChannelsPacket(serverVersion, channels));

proxy/src/main/java/com/velocitypowered/proxy/connection/client/ConnectedPlayer.java

@@ -175,7 +175,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player,
   private final InternalTabList tabList;
   private final VelocityServer server;
   private ClientConnectionPhase connectionPhase;
-  private final Collection<String> clientsideChannels;
+  private final Collection<ChannelIdentifier> clientsideChannels;
   private final CompletableFuture<Void> teardownFuture = new CompletableFuture<>();
   private @MonotonicNonNull List<String> serversToTry = null;
   private final ResourcePackHandler resourcePackHandler;
@@ -1351,7 +1351,7 @@ public class ConnectedPlayer implements MinecraftConnectionAssociation, Player,
    *
    * @return the channels
    */
-  public Collection<String> getClientsideChannels() {
+  public Collection<ChannelIdentifier> getClientsideChannels() {
     return clientsideChannels;
   }
 

proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java

@@ -22,13 +22,20 @@ import static com.google.common.base.Preconditions.checkNotNull;
 
 import com.google.common.collect.ImmutableList;
 import com.velocitypowered.api.network.ProtocolVersion;
+import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
+import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
+import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
 import com.velocitypowered.api.util.ProxyVersion;
 import com.velocitypowered.proxy.protocol.ProtocolUtils;
+import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder;
 import com.velocitypowered.proxy.protocol.packet.PluginMessagePacket;
+import com.velocitypowered.proxy.util.except.QuietDecoderException;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.regex.Pattern;
@@ -85,13 +92,16 @@ public final class PluginMessageUtil {
         .equals(UNREGISTER_CHANNEL);
   }
 
+  private static final QuietDecoderException ILLEGAL_CHANNEL = new QuietDecoderException("Illegal channel");
+
   /**
    * Fetches all the channels in a register or unregister plugin message.
    *
    * @param message the message to get the channels from
    * @return the channels, as an immutable list
    */
-  public static List<String> getChannels(PluginMessagePacket message) {
+  public static List<ChannelIdentifier> getChannels(PluginMessagePacket message,
+                                                    ProtocolVersion protocolVersion) {
     checkNotNull(message, "message");
     checkArgument(isRegister(message) || isUnregister(message), "Unknown channel type %s",
         message.getChannel());
@@ -100,8 +110,25 @@ public final class PluginMessageUtil {
       // has caused issues with 1.13+ compatibility. Just return an empty list.
       return ImmutableList.of();
     }
-    String channels = message.content().toString(StandardCharsets.UTF_8);
+    String payload = message.content().toString(StandardCharsets.UTF_8);
-    return ImmutableList.copyOf(channels.split("\0"));
+    String[] channels = payload.split("\0");
+    ImmutableList.Builder<ChannelIdentifier> channelIdentifiers = ImmutableList.builderWithExpectedSize(channels.length);
+    try {
+      for (String channel : channels) {
+        if (protocolVersion.noLessThan(ProtocolVersion.MINECRAFT_1_13)) {
+          channelIdentifiers.add(MinecraftChannelIdentifier.from(channel));
+        } else {
+          channelIdentifiers.add(new LegacyChannelIdentifier(channel));
+        }
+      }
+    } catch (IllegalArgumentException e) {
+      if (MinecraftDecoder.DEBUG) {
+        throw e;
+      } else {
+        throw ILLEGAL_CHANNEL;
+      }
+    }
+    return channelIdentifiers.build();
   }
 
   /**
@@ -112,16 +139,31 @@ public final class PluginMessageUtil {
    * @return the plugin message to send
    */
   public static PluginMessagePacket constructChannelsPacket(ProtocolVersion protocolVersion,
-                                                            Collection<String> channels) {
+                                                            Collection<ChannelIdentifier> channels) {
     checkNotNull(channels, "channels");
     checkArgument(!channels.isEmpty(), "no channels specified");
     String channelName = protocolVersion.noLessThan(ProtocolVersion.MINECRAFT_1_13)
         ? REGISTER_CHANNEL : REGISTER_CHANNEL_LEGACY;
     ByteBuf contents = Unpooled.buffer();
-    contents.writeCharSequence(String.join("\0", channels), StandardCharsets.UTF_8);
+    contents.writeCharSequence(joinChannels(channels), StandardCharsets.UTF_8);
     return new PluginMessagePacket(channelName, contents);
   }
 
+  private static String joinChannels(Collection<ChannelIdentifier> channels) {
+    checkNotNull(channels, "channels");
+    checkArgument(!channels.isEmpty(), "no channels specified");
+    StringBuilder sb = new StringBuilder();
+    Iterator<ChannelIdentifier> iterator = channels.iterator();
+    while (iterator.hasNext()) {
+      ChannelIdentifier channel = iterator.next();
+      sb.append(channel.getId());
+      if (iterator.hasNext()) {
+        sb.append('\0');
+      }
+    }
+    return sb.toString();
+  }
+
   /**
    * Rewrites the brand message to indicate the presence of Velocity.
    *

proxy/src/main/java/com/velocitypowered/proxy/util/VelocityChannelRegistrar.java

@@ -79,10 +79,10 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
    *
    * @return all legacy channel IDs
    */
-  public Collection<String> getLegacyChannelIds() {
+  public Collection<ChannelIdentifier> getLegacyChannelIds() {
-    Collection<String> ids = new HashSet<>();
+    Collection<ChannelIdentifier> ids = new HashSet<>();
     for (ChannelIdentifier value : identifierMap.values()) {
-      ids.add(value.getId());
+      ids.add(new LegacyChannelIdentifier(value.getId()));
     }
     return ids;
   }
@@ -92,13 +92,13 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
    *
    * @return the channel IDs for Minecraft 1.13 and above
    */
-  public Collection<String> getModernChannelIds() {
+  public Collection<ChannelIdentifier> getModernChannelIds() {
-    Collection<String> ids = new HashSet<>();
+    Collection<ChannelIdentifier> ids = new HashSet<>();
     for (ChannelIdentifier value : identifierMap.values()) {
       if (value instanceof MinecraftChannelIdentifier) {
-        ids.add(value.getId());
+        ids.add(value);
       } else {
-        ids.add(PluginMessageUtil.transformLegacyToModernChannel(value.getId()));
+        ids.add(MinecraftChannelIdentifier.from(PluginMessageUtil.transformLegacyToModernChannel(value.getId())));
       }
     }
     return ids;
@@ -114,7 +114,7 @@ public class VelocityChannelRegistrar implements ChannelRegistrar {
    * @param protocolVersion the protocol version in use
    * @return the list of channels to register
    */
-  public Collection<String> getChannelsForProtocol(ProtocolVersion protocolVersion) {
+  public Collection<ChannelIdentifier> getChannelsForProtocol(ProtocolVersion protocolVersion) {
     if (protocolVersion.noLessThan(ProtocolVersion.MINECRAFT_1_13)) {
       return getModernChannelIds();
     }

proxy/src/test/java/com/velocitypowered/proxy/util/VelocityChannelRegistrarTest.java

@@ -20,8 +20,10 @@ package com.velocitypowered.proxy.util;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import com.google.common.collect.ImmutableSet;
+import com.velocitypowered.api.proxy.messages.ChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.LegacyChannelIdentifier;
 import com.velocitypowered.api.proxy.messages.MinecraftChannelIdentifier;
+import java.util.stream.Collectors;
 import org.junit.jupiter.api.Test;
 
 class VelocityChannelRegistrarTest {
@@ -46,9 +48,9 @@ class VelocityChannelRegistrarTest {
     // Two channels cover the modern channel (velocity:test) and the legacy-mapped channel
     // (legacy:velocitytest). Make sure they're what we expect.
     assertEquals(ImmutableSet.of(MODERN.getId(), SIMPLE_LEGACY_REMAPPED), registrar
-        .getModernChannelIds());
+        .getModernChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));
     assertEquals(ImmutableSet.of(SIMPLE_LEGACY.getId(), MODERN.getId()), registrar
-        .getLegacyChannelIds());
+        .getLegacyChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));
   }
 
   @Test
@@ -57,9 +59,10 @@ class VelocityChannelRegistrarTest {
     registrar.register(SPECIAL_REMAP_LEGACY, MODERN_SPECIAL_REMAP);
 
     // This one, just one channel for the modern case.
-    assertEquals(ImmutableSet.of(MODERN_SPECIAL_REMAP.getId()), registrar.getModernChannelIds());
+    assertEquals(ImmutableSet.of(MODERN_SPECIAL_REMAP.getId()),
+        registrar.getModernChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));
     assertEquals(ImmutableSet.of(MODERN_SPECIAL_REMAP.getId(), SPECIAL_REMAP_LEGACY.getId()),
-        registrar.getLegacyChannelIds());
+        registrar.getLegacyChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));
   }
 
   @Test
@@ -68,7 +71,9 @@ class VelocityChannelRegistrarTest {
     registrar.register(MODERN, SIMPLE_LEGACY);
     registrar.unregister(SIMPLE_LEGACY);
 
-    assertEquals(ImmutableSet.of(MODERN.getId()), registrar.getModernChannelIds());
+    assertEquals(ImmutableSet.of(MODERN.getId()),
-    assertEquals(ImmutableSet.of(MODERN.getId()), registrar.getLegacyChannelIds());
+        registrar.getModernChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));;
+    assertEquals(ImmutableSet.of(MODERN.getId()),
+        registrar.getLegacyChannelIds().stream().map(ChannelIdentifier::getId).collect(Collectors.toSet()));
   }
 }