fix: keep component formatting failures from hiding state tree errors

flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java

@@ -35,6 +35,8 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import org.slf4j.LoggerFactory;
+
 import com.vaadin.flow.component.Component;
 import com.vaadin.flow.component.ComponentUtil;
 import com.vaadin.flow.component.UI;
@@ -847,27 +849,42 @@
     }
 
     private String formatOwnerComponentToString() {
-        final Element ownerElement = ElementUtil.from(this).orElse(null);
-        if (ownerElement == null) {
-            return "unknown element";
-        }
-        final Component component = ownerElement.getComponent().orElse(null);
-        if (component == null) {
-            return "element " + ownerElement + ", no component";
-        }
-        final ComponentTracker.Location createLocation = ComponentTracker
-                .findCreate(component);
-        final ComponentTracker.Location attachLocation = ComponentTracker
-                .findAttach(component);
-        if (createLocation != null || attachLocation != null) {
-            // the location.toString() includes the component class as well
-            return "created: " + createLocation + ", attached: "
-                    + attachLocation;
+        // This is only used to describe a component in an error message, so a
+        // misbehaving application implementation of e.g. toString() or
+        // hashCode() must not replace the original error with its own.
+        Component component = null;
+        try {
+            final Element ownerElement = ElementUtil.from(this).orElse(null);
+            if (ownerElement == null) {
+                return "unknown element";
+            }
+            component = ownerElement.getComponent().orElse(null);
+            if (component == null) {
+                return "element " + ownerElement + ", no component";
+            }
+            final ComponentTracker.Location createLocation = ComponentTracker
+                    .findCreate(component);
+            final ComponentTracker.Location attachLocation = ComponentTracker
+                    .findAttach(component);
+            if (createLocation != null || attachLocation != null) {
+                // the location.toString() includes the component class as well
+                return "created: " + createLocation + ", attached: "
+                        + attachLocation;
+            }
+            // createLocation is null in production mode. Just return the
+            // component's toString() which should provide enough information to
+            // the programmer.
+            return component.toString();
+        } catch (RuntimeException e) {
+            final String describedComponent = component == null
+                    ? "the component"
+                    : "the component of type " + component.getClass().getName();
+            LoggerFactory.getLogger(StateNode.class).debug(
+                    "Failed to describe the owner component of a state node",
+                    e);
+            return "unavailable, describing " + describedComponent + " threw "
+                    + e.getClass().getName();
         }
-        // createLocation is null in production mode. Just return the
-        // component's toString() which should provide enough information to the
-        // programmer.
-        return component.toString();
     }
 
     private boolean handleOnAttach() {
@@ -1233,6 +1250,9 @@
      * contains the element tag and, when available, the component class, the
      * routing target the component is used in, and the location where the
      * component was created.
+     * <p>
+     * This method never throws: if describing the node fails, the description
+     * says so instead and contains the details gathered so far.
      *
      * @return a description of this node, not <code>null</code>
      */
@@ -1241,39 +1261,48 @@
                 .append(getId());
         // The node is not necessarily usable as an element even when it has
         // the feature, and a description for a log message must never throw
-        if (BasicElementStateProvider.get().supports(this)) {
-            Element element = Element.get(this);
-            targetInfo.append(", element with tag '").append(element.getTag())
-                    .append("'");
-            Optional<Component> component = element.getComponent();
-            if (component.isPresent()) {
-                targetInfo.append(", component '")
-                        .append(component.get().getClass().getName())
-                        .append("'");
-                /*
-                 * The routing target is identified by its class since the path
-                 * in its annotation is not necessarily the path it is served
-                 * from: the path may be a placeholder for a name derived from
-                 * the class, and it doesn't include the prefixes that parent
-                 * layouts contribute.
-                 */
-                ComponentUtil.getRouteComponent(component.get()).filter(
-                        routeComponent -> routeComponent != component.get())
-                        .ifPresent(routeComponent -> targetInfo
-                                .append(", used in '")
-                                .append(routeComponent.getClass().getName())
-                                .append("'"));
-
-                // Only available while component tracking is enabled, which
-                // is the case in development mode
-                ComponentTracker.Location location = ComponentTracker
-                        .findCreate(component.get());
-                if (location != null) {
-                    targetInfo.append(", created at ")
-                            .append(location.filename()).append(":")
-                            .append(location.lineNumber());
+        try {
+            if (BasicElementStateProvider.get().supports(this)) {
+                Element element = Element.get(this);
+                targetInfo.append(", element with tag '")
+                        .append(element.getTag()).append("'");
+                Optional<Component> component = element.getComponent();
+                if (component.isPresent()) {
+                    targetInfo.append(", component '")
+                            .append(component.get().getClass().getName())
+                            .append("'");
+                    /*
+                     * The routing target is identified by its class since the
+                     * path in its annotation is not necessarily the path it is
+                     * served from: the path may be a placeholder for a name
+                     * derived from the class, and it doesn't include the
+                     * prefixes that parent layouts contribute.
+                     */
+                    ComponentUtil.getRouteComponent(component.get()).filter(
+                            routeComponent -> routeComponent != component.get())
+                            .ifPresent(routeComponent -> targetInfo
+                                    .append(", used in '")
+                                    .append(routeComponent.getClass().getName())
+                                    .append("'"));
+
+                    // Only available while component tracking is enabled,
+                    // which is the case in development mode
+                    ComponentTracker.Location location = ComponentTracker
+                            .findCreate(component.get());
+                    if (location != null) {
+                        targetInfo.append(", created at ")
+                                .append(location.filename()).append(":")
+                                .append(location.lineNumber());
+                    }
                 }
             }
+        } catch (RuntimeException e) {
+            // Application code, e.g. an overridden getParent() or hashCode(),
+            // must not turn a log message into an error
+            LoggerFactory.getLogger(StateNode.class)
+                    .debug("Failed to describe a state node", e);
+            targetInfo.append(", describing it further threw ")
+                    .append(e.getClass().getName());
         }
         return targetInfo.toString();
     }

flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java

@@ -160,6 +160,14 @@ public static class TestButton extends Component {
     public static class TestOtherButton extends Component {
     }
 
+    @Tag("button")
+    public static class BrokenToStringButton extends Component {
+        @Override
+        public String toString() {
+            throw new UnsupportedOperationException("broken toString");
+        }
+    }
+
     private Component divWithTextComponent;
     private Component parentDivComponent;
     private Component child1SpanComponent;
@@ -2124,6 +2132,23 @@ public void cannotMoveComponentsToOtherUI() {
                 ex.getMessage());
     }
 
+    @Test
+    public void cannotMoveComponentsToOtherUI_componentToStringThrows_originalErrorIsReported() {
+        final UI otherUI = createMockedUI();
+        final BrokenToStringButton button = new BrokenToStringButton();
+        otherUI.add(button);
+
+        IllegalStateException ex = assertThrows(IllegalStateException.class,
+                () -> testUI.add(button));
+        assertTrue(
+                ex.getMessage().contains(BrokenToStringButton.class.getName()),
+                ex.getMessage());
+        assertTrue(
+                ex.getMessage().contains(
+                        UnsupportedOperationException.class.getName()),
+                ex.getMessage());
+    }
+
     private void resetComponentTrackerProductionMode() throws Exception {
         Field disabled = ComponentTracker.class.getDeclaredField("disabled");
         disabled.setAccessible(true);

flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java

@@ -1947,8 +1947,39 @@ void describe_nodeWithoutElementFeatures_onlyNodeIdIncluded() {
         assertEquals("node id=" + node.getId(), node.describe());
     }
 
+    @Test
+    void describe_applicationCodeThrows_failureDescribedWithDetailsSoFar() {
+        UI ui = new UI();
+        BrokenParentComponent component = new BrokenParentComponent();
+        ui.getElement().appendChild(component.getElement());
+        component.broken = true;
+
+        String description = component.getElement().getNode().describe();
+
+        assertTrue(description.contains(BrokenParentComponent.class.getName()),
+                description);
+        assertTrue(
+                description.contains(
+                        UnsupportedOperationException.class.getName()),
+                description);
+    }
+
     @Tag("div")
     private static class TestDescribedComponent
             extends com.vaadin.flow.component.Component {
     }
+
+    @Tag("div")
+    private static class BrokenParentComponent
+            extends com.vaadin.flow.component.Component {
+        private boolean broken;
+
+        @Override
+        public Optional<com.vaadin.flow.component.Component> getParent() {
+            if (broken) {
+                throw new UnsupportedOperationException("broken getParent");
+            }
+            return super.getParent();
+        }
+    }
 }