From bdb65e9ed79b2e890d75eed932c919efe110ffd6 Mon Sep 17 00:00:00 2001 From: mezz Date: Mon, 26 Feb 2018 22:05:08 -0800 Subject: [PATCH] Improve the "Mod rejections" error message when connecting to a server (#4761) --- .../fml/client/FMLClientHandler.java | 5 +- .../handshake/FMLHandshakeClientState.java | 6 +- .../handshake/FMLHandshakeServerState.java | 6 +- .../network/internal/FMLNetworkHandler.java | 35 +++--- .../network/internal/NetworkModHolder.java | 102 ++++++++++++++++-- 5 files changed, 125 insertions(+), 29 deletions(-) diff --git a/src/main/java/net/minecraftforge/fml/client/FMLClientHandler.java b/src/main/java/net/minecraftforge/fml/client/FMLClientHandler.java index 7ed25afe6e..40460a6ce0 100644 --- a/src/main/java/net/minecraftforge/fml/client/FMLClientHandler.java +++ b/src/main/java/net/minecraftforge/fml/client/FMLClientHandler.java @@ -815,7 +815,8 @@ public class FMLClientHandler implements IFMLSidedHandler } Map modListMap = modListBldr.build(); - serverDataTag.put(data, new ExtendedServerListData(type, FMLNetworkHandler.checkModList(modListMap, Side.SERVER) == null, modListMap, !moddedClientAllowed)); + String modRejections = FMLNetworkHandler.checkModList(modListMap, Side.SERVER); + serverDataTag.put(data, new ExtendedServerListData(type, modRejections == null, modListMap, !moddedClientAllowed)); } else { @@ -825,7 +826,7 @@ public class FMLClientHandler implements IFMLSidedHandler { moddedClientAllowed = !serverDescription.endsWith(":NOFML§r"); } - serverDataTag.put(data, new ExtendedServerListData("VANILLA", false, ImmutableMap.of(), !moddedClientAllowed)); + serverDataTag.put(data, new ExtendedServerListData("VANILLA", false, ImmutableMap.of(), !moddedClientAllowed)); } startupConnectionData.countDown(); } diff --git a/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeClientState.java b/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeClientState.java index 4fd0bb18aa..e38f82096e 100644 --- a/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeClientState.java +++ b/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeClientState.java @@ -105,12 +105,12 @@ enum FMLHandshakeClientState implements IHandshakeState @Override public void accept(ChannelHandlerContext ctx, FMLHandshakeMessage msg, Consumer cons) { - String result = FMLNetworkHandler.checkModList((FMLHandshakeMessage.ModList) msg, Side.SERVER); - if (result != null) + String modRejections = FMLNetworkHandler.checkModList((FMLHandshakeMessage.ModList) msg, Side.SERVER); + if (modRejections != null) { cons.accept(ERROR); NetworkDispatcher dispatcher = ctx.channel().attr(NetworkDispatcher.FML_DISPATCHER).get(); - dispatcher.rejectHandshake(result); + dispatcher.rejectHandshake(modRejections); return; } if (!ctx.channel().attr(NetworkDispatcher.IS_LOCAL).get()) diff --git a/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeServerState.java b/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeServerState.java index 500daff10a..cd48ff88f9 100644 --- a/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeServerState.java +++ b/src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeServerState.java @@ -67,11 +67,11 @@ enum FMLHandshakeServerState implements IHandshakeState NetworkDispatcher dispatcher = ctx.channel().attr(NetworkDispatcher.FML_DISPATCHER).get(); dispatcher.setModList(client.modList()); FMLLog.log.info("Client attempting to join with {} mods : {}", client.modListSize(), client.modListAsString()); - String result = FMLNetworkHandler.checkModList(client, Side.CLIENT); - if (result != null) + String modRejections = FMLNetworkHandler.checkModList(client, Side.CLIENT); + if (modRejections != null) { cons.accept(ERROR); - dispatcher.rejectHandshake(result); + dispatcher.rejectHandshake(modRejections); return; } cons.accept(WAITINGCACK); diff --git a/src/main/java/net/minecraftforge/fml/common/network/internal/FMLNetworkHandler.java b/src/main/java/net/minecraftforge/fml/common/network/internal/FMLNetworkHandler.java index 521404f4d7..83bb291018 100644 --- a/src/main/java/net/minecraftforge/fml/common/network/internal/FMLNetworkHandler.java +++ b/src/main/java/net/minecraftforge/fml/common/network/internal/FMLNetworkHandler.java @@ -23,10 +23,11 @@ import io.netty.channel.ChannelPipeline; import io.netty.channel.embedded.EmbeddedChannel; import java.util.ArrayList; +import java.util.Comparator; import java.util.EnumMap; import java.util.List; import java.util.Map; -import java.util.Map.Entry; +import java.util.stream.Collectors; import net.minecraft.entity.Entity; import net.minecraft.entity.player.EntityPlayer; @@ -54,9 +55,9 @@ import net.minecraftforge.fml.common.registry.EntityRegistry.EntityRegistration; import net.minecraftforge.fml.relauncher.Side; import net.minecraftforge.fml.relauncher.SideOnly; -import com.google.common.collect.Lists; import com.google.gson.JsonArray; import com.google.gson.JsonObject; +import org.apache.commons.lang3.tuple.Pair; import javax.annotation.Nullable; @@ -140,26 +141,34 @@ public class FMLNetworkHandler return checkModList(modList, side); } + /** + * @param listData map of modId string to version string, represents the mods available on the given side + * @param side the side that listData is coming from, either client or server + * @return null if everything is fine, returns a string error message if there are mod rejections + */ @Nullable public static String checkModList(Map listData, Side side) { - List rejects = Lists.newArrayList(); - for (Entry networkMod : NetworkRegistry.INSTANCE.registry().entrySet()) - { - boolean result = networkMod.getValue().check(listData, side); - if (!result) - { - rejects.add(networkMod.getKey()); - } - } + List> rejects = NetworkRegistry.INSTANCE.registry().entrySet().stream() + .map(entry -> Pair.of(entry.getKey(), entry.getValue().checkCompatible(listData, side))) + .filter(pair -> pair.getValue() != null) + .sorted(Comparator.comparing(o -> o.getKey().getName())) + .collect(Collectors.toList()); if (rejects.isEmpty()) { return null; } else { - FMLLog.log.info("Rejecting connection {}: {}", side, rejects); - return String.format("Mod rejections %s",rejects); + List rejectStrings = new ArrayList<>(); + for (Pair reject : rejects) + { + ModContainer modContainer = reject.getKey(); + rejectStrings.add(modContainer.getName() + ": " + reject.getValue()); + } + String rejectString = String.join("\n", rejectStrings); + FMLLog.log.info("Rejecting connection {}: {}", side, rejectString); + return String.format("Server Mod rejections:\n%s", rejectString); } } diff --git a/src/main/java/net/minecraftforge/fml/common/network/internal/NetworkModHolder.java b/src/main/java/net/minecraftforge/fml/common/network/internal/NetworkModHolder.java index 4c80a410b2..18f880f264 100644 --- a/src/main/java/net/minecraftforge/fml/common/network/internal/NetworkModHolder.java +++ b/src/main/java/net/minecraftforge/fml/common/network/internal/NetworkModHolder.java @@ -43,8 +43,27 @@ import javax.annotation.Nullable; public class NetworkModHolder { + /** + * Validates that the mods versions on the client and server are compatible with mod. + */ public abstract class NetworkChecker { + /** + * @deprecated use {@link #checkCompatible(Map, Side)} + */ + @Deprecated // TODO remove in 1.13 public abstract boolean check(Map remoteVersions, Side side); + + /** + * @param remoteVersions map of modIds to version strings, represents all the mods on the given side + * @param side the side that the remoteVersions are from + * @return null if these mod versions are compatible with this mod, + * an error string reason if these mod versions are not compatible with this mod. + */ + @Nullable + public String checkCompatible(Map remoteVersions, Side side) + { + return check(remoteVersions, side) ? toString() : null; + } } private class IgnoredChecker extends NetworkChecker { @@ -53,6 +72,14 @@ public class NetworkModHolder { return true; } + + @Nullable + @Override + public String checkCompatible(Map remoteVersions, Side side) + { + return null; + } + @Override public String toString() { @@ -63,28 +90,81 @@ public class NetworkModHolder @Override public boolean check(Map remoteVersions, Side side) { - return remoteVersions.containsKey(container.getModId()) ? acceptVersion(remoteVersions.get(container.getModId())) : side == Side.SERVER; + return checkCompatible(remoteVersions, side) == null; } + + @Nullable + @Override + public String checkCompatible(Map remoteVersions, Side side) + { + String version = remoteVersions.get(container.getModId()); + if (version != null && acceptVersion(version)) + { + return null; + } + if (side == Side.SERVER) + { + return null; + } + String versionString; + if (acceptableRange != null) + { + if (acceptableRange.hasRestrictions()) + { + versionString = String.format("version %s", acceptableRange.toStringFriendly()); + } + else + { + versionString = String.format("version range %s", acceptableRange.toStringFriendly()); + } + } + else + { + versionString = String.format("version %s", container.getVersion()); + } + if (version != null) + { + return String.format("Requires %s but client has %s.", versionString, version); + } + else + { + return String.format("Requires %s but mod is not found on client.", versionString); + } + } + @Override public String toString() { - return acceptableRange != null ? String.format("Accepting range %s", acceptableRange) : String.format("Accepting version %s", container.getVersion()); + return acceptableRange != null ? String.format("Accepting range %s", acceptableRange.toStringFriendly()) : String.format("Accepting version %s", container.getVersion()); } } private class MethodNetworkChecker extends NetworkChecker { @Override public boolean check(Map remoteVersions, Side side) + { + return checkCompatible(remoteVersions, side) == null; + } + + @Nullable + @Override + public String checkCompatible(Map remoteVersions, Side side) { try { - return (Boolean) checkHandler.invoke(container.getMod(), remoteVersions, side); + Boolean result = (Boolean) checkHandler.invoke(container.getMod(), remoteVersions, side); + if (result != null && result) + { + return null; + } + return String.format("Failed mod's custom NetworkCheckHandler %s", container); } catch (Exception e) { FMLLog.log.error("Error occurred invoking NetworkCheckHandler {} at {}", container, e); - return false; + return String.format("Error occurred invoking NetworkCheckHandler %s", container); } } + @Override public String toString() { @@ -209,7 +289,7 @@ public class NetworkModHolder public boolean acceptVersion(String version) { - if (acceptableRange!=null) + if (acceptableRange != null) { return acceptableRange.containsVersion(new DefaultArtifactVersion(version)); } @@ -219,7 +299,13 @@ public class NetworkModHolder public boolean check(Map data, Side side) { - return checker.check(data, side); + return checker.checkCompatible(data, side) == null; + } + + @Nullable + public String checkCompatible(Map data, Side side) + { + return checker.checkCompatible(data, side); } public int getLocalId() @@ -243,8 +329,8 @@ public class NetworkModHolder } public void testVanillaAcceptance() { - acceptsVanillaClient = check(ImmutableMap.of(), Side.CLIENT); - acceptsVanillaServer = check(ImmutableMap.of(), Side.SERVER); + acceptsVanillaClient = check(ImmutableMap.of(), Side.CLIENT); + acceptsVanillaServer = check(ImmutableMap.of(), Side.SERVER); } public boolean acceptsVanilla(Side from) { return from == Side.CLIENT ? acceptsVanillaClient : acceptsVanillaServer;