From 2037a8d52c84a1a5404f07e30da521e5337a55a7 Mon Sep 17 00:00:00 2001 From: "totally-not-ai[bot]" <290682512+totally-not-ai[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:20:17 +0000 Subject: [PATCH 1/3] fix: keep component formatting failures from hiding state tree errors formatOwnerComponentToString calls application code, e.g. the component's toString() and, via the ComponentTracker weak maps, its hashCode(). When that code throws, its exception replaced the IllegalStateException about moving a node between state trees, hiding the actual problem. Catch runtime exceptions and describe the failure instead, logging the exception at debug level. --- .../com/vaadin/flow/internal/StateNode.java | 57 ++++++++++++------- .../vaadin/flow/component/ComponentTest.java | 24 ++++++++ 2 files changed, 61 insertions(+), 20 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java index 30a5eecd0a6..1f94afa8d19 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java +++ b/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 void doSetTree(StateTree tree) { } 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() { diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java index 54e09517e98..bd7a2e16371 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java +++ b/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,22 @@ 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().endsWith("Offending component: unavailable, " + + "describing the component of type " + + BrokenToStringButton.class.getName() + " threw " + + UnsupportedOperationException.class.getName()), + ex.getMessage()); + } + private void resetComponentTrackerProductionMode() throws Exception { Field disabled = ComponentTracker.class.getDeclaredField("disabled"); disabled.setAccessible(true); From 5a349a212671df28b013af323aab96357cac9cce Mon Sep 17 00:00:00 2001 From: "totally-not-ai[bot]" <290682512+totally-not-ai[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:30:16 +0000 Subject: [PATCH 2/3] fix: keep describe() from throwing when application code does describe() reaches into application code too: getRouteComponent walks getParent(), and the ComponentTracker lookup uses the component's hashCode(). Since the description is meant for log messages, catch runtime exceptions and append a note about the failure to the details gathered so far instead of propagating it. --- .../com/vaadin/flow/internal/StateNode.java | 74 +++++++++++-------- .../vaadin/flow/internal/StateNodeTest.java | 31 ++++++++ 2 files changed, 74 insertions(+), 31 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java index 1f94afa8d19..56f28b1f7d1 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java @@ -1250,6 +1250,9 @@ public List dumpBeforeClientResponseEntries * 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. + *

+ * 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 null */ @@ -1258,39 +1261,48 @@ public String describe() { .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 = 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 = 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(); } diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java index 3b908c13337..77267112c18 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java +++ b/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.endsWith(", describing it further threw " + + 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 getParent() { + if (broken) { + throw new UnsupportedOperationException("broken getParent"); + } + return super.getParent(); + } + } } From e5c0d277a88ab741faeca2ab0be771e8ec5bb19d Mon Sep 17 00:00:00 2001 From: "totally-not-ai[bot]" <290682512+totally-not-ai[bot]@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:42:54 +0000 Subject: [PATCH 3/3] test: assert only the essential parts of the failure descriptions Matching the full message made the tests break on any wording change, while what matters is that the description names the component class and the exception that was thrown. --- .../java/com/vaadin/flow/component/ComponentTest.java | 9 +++++---- .../java/com/vaadin/flow/internal/StateNodeTest.java | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java index bd7a2e16371..546d7b34982 100644 --- a/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java @@ -2141,10 +2141,11 @@ public void cannotMoveComponentsToOtherUI_componentToStringThrows_originalErrorI IllegalStateException ex = assertThrows(IllegalStateException.class, () -> testUI.add(button)); assertTrue( - ex.getMessage().endsWith("Offending component: unavailable, " - + "describing the component of type " - + BrokenToStringButton.class.getName() + " threw " - + UnsupportedOperationException.class.getName()), + ex.getMessage().contains(BrokenToStringButton.class.getName()), + ex.getMessage()); + assertTrue( + ex.getMessage().contains( + UnsupportedOperationException.class.getName()), ex.getMessage()); } diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java index 77267112c18..39d6d96d321 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java @@ -1959,8 +1959,8 @@ void describe_applicationCodeThrows_failureDescribedWithDetailsSoFar() { assertTrue(description.contains(BrokenParentComponent.class.getName()), description); assertTrue( - description.endsWith(", describing it further threw " - + UnsupportedOperationException.class.getName()), + description.contains( + UnsupportedOperationException.class.getName()), description); }