Improve the "Mod rejections" error message when connecting to a server (#4761)

src/main/java/net/minecraftforge/fml/client/FMLClientHandler.java

@@ -815,7 +815,8 @@ public class FMLClientHandler implements IFMLSidedHandler
             }
 
             Map<String,String> 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.<String,String>of(), !moddedClientAllowed));
+            serverDataTag.put(data, new ExtendedServerListData("VANILLA", false, ImmutableMap.of(), !moddedClientAllowed));
         }
         startupConnectionData.countDown();
     }

src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeClientState.java

@@ -105,12 +105,12 @@ enum FMLHandshakeClientState implements IHandshakeState<FMLHandshakeClientState>
         @Override
         public void accept(ChannelHandlerContext ctx, FMLHandshakeMessage msg, Consumer<? super FMLHandshakeClientState> 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())

src/main/java/net/minecraftforge/fml/common/network/handshake/FMLHandshakeServerState.java

@@ -67,11 +67,11 @@ enum FMLHandshakeServerState implements IHandshakeState<FMLHandshakeServerState>
             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);

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<String,String> listData, Side side)
     {
-        List<ModContainer> rejects = Lists.newArrayList();
-        for (Entry<ModContainer, NetworkModHolder> networkMod : NetworkRegistry.INSTANCE.registry().entrySet())
-        {
-            boolean result = networkMod.getValue().check(listData, side);
-            if (!result)
-            {
-                rejects.add(networkMod.getKey());
-            }
-        }
+        List<Pair<ModContainer, String>> 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<String> rejectStrings = new ArrayList<>();
+            for (Pair<ModContainer, String> 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);
         }
     }
 

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<String,String> 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<String,String> 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<String, String> remoteVersions, Side side)
+        {
+            return null;
+        }
+
         @Override
         public String toString()
         {
@@ -63,28 +90,81 @@ public class NetworkModHolder
         @Override
         public boolean check(Map<String,String> 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<String, String> 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<String,String> remoteVersions, Side side)
+        {
+            return checkCompatible(remoteVersions, side) == null;
+        }
+
+        @Nullable
+        @Override
+        public String checkCompatible(Map<String, String> 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<String,String> data, Side side)
     {
-        return checker.check(data, side);
+        return checker.checkCompatible(data, side) == null;
+    }
+
+    @Nullable
+    public String checkCompatible(Map<String,String> data, Side side)
+    {
+        return checker.checkCompatible(data, side);
     }
 
     public int getLocalId()
@@ -243,8 +329,8 @@ public class NetworkModHolder
     }
 
     public void testVanillaAcceptance() {
-        acceptsVanillaClient = check(ImmutableMap.<String,String>of(), Side.CLIENT);
-        acceptsVanillaServer = check(ImmutableMap.<String,String>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;