eclipse-sirius / sirius-web

Reusable frontend and backend components for Sirius Web
https://eclipse.dev/sirius/sirius-web.html
Eclipse Public License 2.0
63 stars 45 forks source link

ViewRepresentationDescriptionSearchService does not look into reused nodes #2392

Open pcdavid opened 10 months ago

pcdavid commented 10 months ago

When looking for a node by id (ViewRepresentationDescriptionSearchService.findNodeDescriptionById(DiagramDescription, String)), we only search in:

Stream.concat(node.getBorderNodesDescriptions().stream(), node.getChildrenDescriptions().stream())

This ignores reused (sub, border) nodes.

In "normal" cases, where we only reuse nodes defined from the same DiagramDescription, it does not cause any issue because the recursive walk on the DiagramDescription's content will end-up finding the element.

In a case where the reused node is defined in another DiagramDescription, findNodeDescriptionById can return Optional.empty(). Clicking a such a node (which is correctly rendered) then causes an NPE:

Caused by: java.lang.NullPointerException: null
    at java.base/java.util.Objects.requireNonNull(Objects.java:208) ~[na:na]
    at org.eclipse.sirius.components.collaborative.diagrams.dto.GetPaletteSuccessPayload.<init>(GetPaletteSuccessPayload.java:30) ~[classes/:na]
    at org.eclipse.sirius.components.collaborative.diagrams.handlers.GetPaletteEventHandler.handle(GetPaletteEventHandler.java:117) ~[classes/:na]

GetPaletteEventHandler is not ready to handle the case where findDiagramElementDescription is empty. In that case it leaves palette to null, and GetPaletteSuccessPayload rejects it.

Note that (I think) we can not "simply" do:

List<NodeDescription> nodeDescriptionChildren = Stream.concat(
        Stream.concat(node.getBorderNodesDescriptions().stream(), node.getChildrenDescriptions().stream()),
        Stream.concat(node.getReusedBorderNodeDescriptions().stream(), node.getReusedChildNodeDescriptions().stream())).toList();

This could cause infinite loops if e.g. A reuses B and B reuses A (even indirectly).

pcdavid commented 6 months ago

Note that DropNodeCompatibiliyProvider.forEachNodeDescription(DiagramDescription, Consumer<NodeDescription>) tries to work around the issue for its use case.