From d33a5a23c32a1e5af2f8faa0c77105d0aebd8bc6 Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Tue, 6 Jul 2021 14:06:39 +0200 Subject: [PATCH] [core] Introduce Builder.failOnUnknownStates() and unit tests The previous approach of emitting a severe log message when a state (descriptor) was unknown was misleading. There are valid cases where some states are not known, if, for example, a module was explicitly disabled. Using Builder.failOnUnknownStates() in unit tests is far cleaner, as the existence of unknown states is tested in a controlled environment: one where are states are supposed to be known. --- ...ClientToServerConnectionConfiguration.java | 15 +++++++++- .../smack/fsm/StateDescriptorGraph.java | 28 +++++++++---------- ...lientToServerConnectionStateGraphTest.java | 26 ++++++++++++++++- 3 files changed, 52 insertions(+), 17 deletions(-) diff --git a/smack-core/src/main/java/org/jivesoftware/smack/c2s/ModularXmppClientToServerConnectionConfiguration.java b/smack-core/src/main/java/org/jivesoftware/smack/c2s/ModularXmppClientToServerConnectionConfiguration.java index 03982d647..74e20ba50 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/c2s/ModularXmppClientToServerConnectionConfiguration.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/c2s/ModularXmppClientToServerConnectionConfiguration.java @@ -54,7 +54,7 @@ public final class ModularXmppClientToServerConnectionConfiguration extends Conn } try { - initialStateDescriptorVertex = StateDescriptorGraph.constructStateDescriptorGraph(backwardEdgeStateDescriptors); + initialStateDescriptorVertex = StateDescriptorGraph.constructStateDescriptorGraph(backwardEdgeStateDescriptors, builder.failOnUnknownStates); } catch (InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException | NoSuchMethodException | SecurityException e) { @@ -91,6 +91,8 @@ public final class ModularXmppClientToServerConnectionConfiguration extends Conn private final Map, ModularXmppClientToServerConnectionModuleDescriptor> modulesDescriptors = new HashMap<>(); + private boolean failOnUnknownStates; + private Builder() { SmackConfiguration.addAllKnownModulesTo(this); } @@ -163,6 +165,17 @@ public final class ModularXmppClientToServerConnectionConfiguration extends Conn return getThis(); } + /** + * Fail if there are unknown states in Smack's state descriptor graph. This method is used mostly for testing + * the internals of Smack. Users can safely ignore it. + * + * @return a reference to this builder. + */ + public Builder failOnUnknownStates() { + failOnUnknownStates = true; + return getThis(); + } + @Override protected Builder getThis() { return this; diff --git a/smack-core/src/main/java/org/jivesoftware/smack/fsm/StateDescriptorGraph.java b/smack-core/src/main/java/org/jivesoftware/smack/fsm/StateDescriptorGraph.java index bbece8898..35144f73a 100644 --- a/smack-core/src/main/java/org/jivesoftware/smack/fsm/StateDescriptorGraph.java +++ b/smack-core/src/main/java/org/jivesoftware/smack/fsm/StateDescriptorGraph.java @@ -1,6 +1,6 @@ /** * - * Copyright 2018 Florian Schmaus + * Copyright 2018-2021 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -28,7 +28,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.logging.Logger; import org.jivesoftware.smack.c2s.ModularXmppClientToServerConnection.DisconnectedStateDescriptor; import org.jivesoftware.smack.c2s.internal.ModularXmppClientToServerConnectionInternal; @@ -47,8 +46,6 @@ import org.jivesoftware.smack.util.MultiMap; */ public class StateDescriptorGraph { - private static final Logger LOGGER = Logger.getLogger(StateDescriptorGraph.class.getName()); - private static GraphVertex addNewStateDescriptorGraphVertex( Class stateDescriptorClass, Map, GraphVertex> graphVertexes) @@ -102,7 +99,8 @@ public class StateDescriptorGraph { } private static void handleStateDescriptorGraphVertex(GraphVertex node, - HandleStateDescriptorGraphVertexContext context) + HandleStateDescriptorGraphVertexContext context, + boolean failOnUnknownStates) throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException, NoSuchMethodException, SecurityException { Class stateDescriptorClass = node.element.getClass(); boolean alreadyHandled = context.recurseInto(stateDescriptorClass); @@ -126,7 +124,7 @@ public class StateDescriptorGraph { case 1: GraphVertex soleSuccessorNode = successorStateDescriptors.values().iterator().next(); node.addOutgoingEdge(soleSuccessorNode); - handleStateDescriptorGraphVertex(soleSuccessorNode, context); + handleStateDescriptorGraphVertex(soleSuccessorNode, context, failOnUnknownStates); return; } @@ -144,9 +142,8 @@ public class StateDescriptorGraph { StateDescriptor successorStateDescriptor = successorStateDescriptorGraphNode.element; Class successorStateDescriptorClass = successorStateDescriptor.getClass(); for (Class subordinateClass : successorStateDescriptor.getSubordinates()) { - if (!successorClasses.contains(subordinateClass)) { - LOGGER.severe(successorStateDescriptor + " points to a subordinate '" + subordinateClass + "' which is not part of the successor set"); - continue; + if (failOnUnknownStates && !successorClasses.contains(subordinateClass)) { + throw new IllegalStateException(successorStateDescriptor + " points to a subordinate '" + subordinateClass + "' which is not part of the successor set"); } GraphVertex> superiorClassNode = lookupAndCreateIfRequired( @@ -157,10 +154,9 @@ public class StateDescriptorGraph { superiorClassNode.addOutgoingEdge(subordinateClassNode); } for (Class superiorClass : successorStateDescriptor.getSuperiors()) { - if (!successorClasses.contains(superiorClass)) { - LOGGER.severe(successorStateDescriptor + " points to a superior '" + superiorClass + if (failOnUnknownStates && !successorClasses.contains(superiorClass)) { + throw new IllegalStateException(successorStateDescriptor + " points to a superior '" + superiorClass + "' which is not part of the successor set"); - continue; } GraphVertex> subordinateClassNode = lookupAndCreateIfRequired( @@ -193,11 +189,13 @@ public class StateDescriptorGraph { node.addOutgoingEdge(successorVertex); // Recurse further. - handleStateDescriptorGraphVertex(successorVertex, context); + handleStateDescriptorGraphVertex(successorVertex, context, failOnUnknownStates); } } - public static GraphVertex constructStateDescriptorGraph(Set> backwardEdgeStateDescriptors) + public static GraphVertex constructStateDescriptorGraph( + Set> backwardEdgeStateDescriptors, + boolean failOnUnknownStates) throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException, NoSuchMethodException, SecurityException { Map, GraphVertex> graphVertexes = new HashMap<>(); @@ -219,7 +217,7 @@ public class StateDescriptorGraph { } HandleStateDescriptorGraphVertexContext context = new HandleStateDescriptorGraphVertexContext(graphVertexes, inferredForwardEdges); - handleStateDescriptorGraphVertex(initialNode, context); + handleStateDescriptorGraphVertex(initialNode, context, failOnUnknownStates); return initialNode; } diff --git a/smack-java8-full/src/test/java/org/jivesoftware/smack/full/ModularXmppClientToServerConnectionStateGraphTest.java b/smack-java8-full/src/test/java/org/jivesoftware/smack/full/ModularXmppClientToServerConnectionStateGraphTest.java index 624679481..961e7527d 100644 --- a/smack-java8-full/src/test/java/org/jivesoftware/smack/full/ModularXmppClientToServerConnectionStateGraphTest.java +++ b/smack-java8-full/src/test/java/org/jivesoftware/smack/full/ModularXmppClientToServerConnectionStateGraphTest.java @@ -1,6 +1,6 @@ /** * - * Copyright 2020 Florian Schmaus + * Copyright 2020-2021 Florian Schmaus * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ package org.jivesoftware.smack.full; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; import java.io.PrintWriter; @@ -25,6 +26,8 @@ import java.io.StringWriter; import java.net.URL; import java.nio.charset.StandardCharsets; +import org.jivesoftware.smack.c2s.ModularXmppClientToServerConnectionConfiguration; +import org.jivesoftware.smack.compression.CompressionModuleDescriptor; import org.jivesoftware.smack.util.EqualsUtil; import org.jivesoftware.smack.util.HashCode; @@ -34,6 +37,7 @@ import org.jgrapht.graph.DirectedPseudograph; import org.jgrapht.io.DOTImporter; import org.jgrapht.io.ImportException; import org.junit.jupiter.api.Test; +import org.jxmpp.stringprep.XmppStringprepException; public class ModularXmppClientToServerConnectionStateGraphTest { @@ -80,4 +84,24 @@ public class ModularXmppClientToServerConnectionStateGraphTest { assertEquals(expectedStateGraph, currentStateGraph); } + @Test + public void testNoUnknownStates() throws XmppStringprepException { + ModularXmppClientToServerConnectionConfiguration.builder() + .setUsernameAndPassword("user", "password") + .setXmppDomain("example.org") + .failOnUnknownStates() // This is the actual option that enableds this test. + .build(); + } + + @Test + public void throwsOnUnknownStates() throws XmppStringprepException { + assertThrows(IllegalStateException.class, () -> + ModularXmppClientToServerConnectionConfiguration.builder() + .setUsernameAndPassword("user", "password") + .setXmppDomain("example.org") + .removeModule(CompressionModuleDescriptor.class) + .failOnUnknownStates() // This is the actual option that enableds this test. + .build() + ); + } }