[3958] Prevent Stackoverflow when rendering a twice an expanded TreeItem #4015

adaussy · 2024-09-18T09:58:55Z

This commit provide a partial fix for the given bug. It only prevents rendering twice in the same tree path and expanded element because it would cause an infinite loop.

A fix still need to be implemented in the "Expand All" action to prevent stackoverfow when rendering a infinite loop with non unique ID.

Bug: #3958

Pull request template

General purpose

What is the main goal of this pull request?

Project management

Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
Have the relevant issues been added to the pull request?
Have the relevant labels been added to the issues? (area:, difficulty:, type:)
Have the relevant issues been added to the same project and milestone as the pull request?
Has the CHANGELOG.adoc been updated to reference the relevant issues?
Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

Does the title of the commit contributing the ADR start with [doc]?
Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

Are all the event handlers tested?
Are the event processor tested?
Is the business code (services) tested?
Are diagram layout changes tested?

Architecture

Are data structure classes properly separated from behavioral classes?
Are all the relevant fields final?
Is any data structure mutable? If so, please write a comment indicating why
Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

Has the Kiwi TCMS test suite been updated with tests for this contribution?

sbegaudeau · 2024-09-18T13:44:36Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

    public static final String EXPANDED = "expanded";

    public static final String ACTIVE_FILTER_IDS = "activeFilterIds";

+    private static final Logger LOGGER = LoggerFactory.getLogger(TreeRenderer.class);


private final Logger logger = ...

No need to this to be static

sbegaudeau · 2024-09-18T13:45:10Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

    public static final String EXPANDED = "expanded";

    public static final String ACTIVE_FILTER_IDS = "activeFilterIds";

+    private static final Logger LOGGER = LoggerFactory.getLogger(TreeRenderer.class);
+
+    private static final StyledStringFragment ITEM_RENDERING_LOOP_FRAGMENT = new StyledStringFragment("<Item rendering loop> ", StyledStringFragmentStyle.newDefaultStyledStringFragmentStyle()


No need to make this a constant, just use it where you need it

I hesitated so ok

sbegaudeau · 2024-09-18T13:48:14Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

+     * @return <code>true</code> if a loop is detected
+     */
+    private boolean loopDetected(VariableManager treeItemVariableManager, String id) {
+        // @formatter:off


Remove those useless // @formatter:off, there are other instances of this in this file

sbegaudeau · 2024-09-18T13:52:27Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

            childrenTreeItems.add(this.renderTreeItem(childVariableManager));
        }
-        return TreeItem.newTreeItem(id)
+        return TreeItem.newTreeItem(id).kind(kind).label(label).editable(editable).deletable(deletable).selectable(selectable).iconURL(iconURL).children(childrenTreeItems).hasChildren(hasChildren)


Please put that on multiple lines

sbegaudeau · 2024-09-18T13:54:20Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

+        fragments.add(0, ITEM_RENDERING_LOOP_FRAGMENT);
+        StyledString newLabel = new StyledString(fragments);
+        List<String> newIcons = new ArrayList<>(iconURL);
+        newIcons.add("/icons/svg/warning-overlay.svg");


Do not contribute images like that, it can't work in all use cases. Consumers of Sirius Components may not use Sirius Web and as such the path on which they are hosting their images may be different.

So I just keep the prefix warning an its enough for the user to be aware of the problem.

That is fine by me

sbegaudeau · 2024-09-18T13:55:39Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

+                .expanded(expanded).build();
+    }
+
+    private TreeItem renderWarningTreeItem(String id, String kind, StyledString label, List<String> iconURL, VariableManager mng) {


iconURL -> iconURLs?
-mng -> variableManager

sbegaudeau · 2024-09-18T13:56:39Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

        for (Object rootElement : rootElements) {
            VariableManager rootElementVariableManager = this.variableManager.createChild();
            rootElementVariableManager.put(VariableManager.SELF, rootElement);
+            rootElementVariableManager.put(INDEX, index++);


Is that index really necessary and do you really have to put this in the variable manager? It will be propagated to everybody.

Yes see unit test org.eclipse.sirius.components.trees.renderer.TreeRendererTests.checkAvoidStackOverFlow() line 142.
My purpose is to create unique id for those virtual elements.

In that test I have "child1" that want to display twice an item representing "Root" (for some dark reason I guess).

If I want my specifier to be able to create a unique ID it need both:

The id of all my ancestors

My index in my TreeItem parent

If do only have ancestor I am not able to create a stable unique id

sbegaudeau · 2024-09-18T13:57:38Z

packages/trees/backend/sirius-components-trees/src/main/resources/icons/svg/warning-overlay.svg

@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


If you really want to contribute an image, use one from MUI

No I'm going to do what you say and simply delete this icon

sbegaudeau · 2024-09-18T14:02:29Z

...omponents-trees/src/main/java/org/eclipse/sirius/components/trees/renderer/TreeRenderer.java

+        String path = idPath.stream().map(Object::toString).collect(joining("::")).toString() + "#" + mng.get(INDEX, Integer.class);
+
+        // This would need to be encoded to avoid getting a problem with id containing #
+        String virtualId = "siriuscomponent://treelooprendering?itemId=" + id + "&path=" + path;


Why change that much the original id?

Same reason describe here #4015 (comment)

If I want to be "sure" to have unique id I needs to have the path of the current item.

Maybe I could generate an UUID from that string it could be simpler?

sbegaudeau · 2024-09-18T14:05:23Z

...ents-trees/src/test/java/org/eclipse/sirius/components/trees/renderer/TreeRendererTests.java

+ *
+ * @author Arthur Daussy
+ */
+public class TreeRendererTests {


Please prefer an integration test to validate the whole process

Please don't make me do that 😢

Unit testing is life...

It this particular case it allowed me to easily create the "StackOverflow" use case and use a TDD approach. It even help me to fix to issue. It help me to detect the need of the index to give the capacity for the specifier to create truly unique id.

Could you explain why you prefer integration test of unit test please?

adaussy · 2024-09-20T13:59:10Z

In order to avoid too long id I translate the possible long path into a UUID

String path =  idPath.stream().map(Object::toString).collect(joining("::")).toString() + "#" + variableManager.get(INDEX, Integer.class);
String virtualId = "siriuscomponent://treelooprendering?renderedItemId=" + id + "&itemItem=" + UUID.nameUUIDFromBytes(path.getBytes());

I hope that is fine by you

This commit provide a partial fix for the given bug. It only prevents rendering twice in the same tree path and expanded element because it would cause an infinite loop. A fix still need to be implemented in the "Expand All" action to prevent stackoverfow when rendering a infinite loop with non unique ID. Bug: eclipse-sirius#3958 Signed-off-by: Arthur Daussy <arthur.daussy@obeo.fr>

adaussy · 2024-09-20T14:27:10Z

New rendering:

adaussy requested a review from pcdavid as a code owner September 18, 2024 09:58

sbegaudeau added pr: to review soon priority: high labels Sep 18, 2024

sbegaudeau added this to the 2024.11.0 milestone Sep 18, 2024

sbegaudeau linked an issue Sep 18, 2024 that may be closed by this pull request

Possible stack overflow when invoking "Expand All" on a domain #3958

Open

sbegaudeau requested changes Sep 18, 2024

View reviewed changes

sbegaudeau reviewed Sep 18, 2024

View reviewed changes

adaussy force-pushed the ada/fix/i-3958 branch from 25495ff to 2b10030 Compare September 20, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3958] Prevent Stackoverflow when rendering a twice an expanded TreeItem #4015

[3958] Prevent Stackoverflow when rendering a twice an expanded TreeItem #4015

adaussy commented Sep 18, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

adaussy Sep 19, 2024

sbegaudeau Sep 18, 2024

adaussy Sep 19, 2024

adaussy commented Sep 20, 2024 •

edited

Loading

adaussy commented Sep 20, 2024

		@@ -0,0 +1,43 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>

[3958] Prevent Stackoverflow when rendering a twice an expanded TreeItem #4015

Are you sure you want to change the base?

[3958] Prevent Stackoverflow when rendering a twice an expanded TreeItem #4015

Conversation

adaussy commented Sep 18, 2024

Pull request template

General purpose

Project management

Architectural decision records (ADR)

Dependencies

Frontend

General purpose

Typing

Backend

General purpose

Architecture

Review

How to test this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaussy commented Sep 20, 2024 • edited Loading

adaussy commented Sep 20, 2024

adaussy commented Sep 20, 2024 •

edited

Loading