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..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 @@ -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() { @@ -1233,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 */ @@ -1241,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/component/ComponentTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java index 54e09517e98..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 @@ -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); 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..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 @@ -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 getParent() { + if (broken) { + throw new UnsupportedOperationException("broken getParent"); + } + return super.getParent(); + } + } }