[3344] Add support for reloading representation from the database #3348

pcdavid · 2024-04-08T14:29:26Z

Bug: #3344
Signed-off-by: Pierre-Charles David pierre-charles.david@obeo.fr

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?

pcdavid · 2024-04-12T13:56:40Z

...a/org/eclipse/sirius/components/collaborative/diagrams/dto/DiagramRefreshedEventPayload.java

@@ -29,6 +29,8 @@ public record DiagramRefreshedEventPayload(UUID id, Diagram diagram, String caus

    public static final String CAUSE_LAYOUT = "layout";

+    public static final String CAUSE_RELOAD = "reload";


There's a bug here: the new value is not added in the GraphQL Schema and not handled by the front (in DiagramRenderer.tsx). I'm investigating how exactly it should be handled.

sbegaudeau

I thought we agreed on the way to move forward on this issue by starting on #3303, then #3304 and finally #3305. Why choose instead seven different ways to reload the state?

sbegaudeau · 2024-04-29T09:12:37Z

...ius/components/collaborative/formdescriptioneditors/FormDescriptionEditorEventProcessor.java

+        } else if (changeDescription.getKind().equals(ChangeKind.RELOAD_REPRESENTATION) && changeDescription.getSourceId().equals(this.formDescriptionEditorContext.getFormDescriptionEditor().getId())) {
+            Optional<FormDescriptionEditor> reloadedFormDescriptionEditor = this.representationSearchService.findById(this.editingContext, this.formDescriptionEditorContext.getFormDescriptionEditor().getId(), FormDescriptionEditor.class);
+            if (reloadedFormDescriptionEditor.isPresent()) {
+                this.formDescriptionEditorContext.update(reloadedFormDescriptionEditor.get());
+                this.formDescriptionEditorEventFlux.formDescriptionEditorRefreshed(changeDescription.getInput(), reloadedFormDescriptionEditor.get());
+            }


I took upon myself to add tests last week to make this PR go through the continuous integration without issues and there something I don't understand here because I can't succeed in making this code work. The data structure persisted in the database is not the same as the one sent back to the frontend, the refresh is needed to compute the whole structure since it is not persisted. See these two PRs that you've reviewed before (#1484 and #1963) for additional details.

Without a new refresh and unless I've missed something, I think this will only reload an empty representation. I suspect that the same would be true for at least for the form representation which behave the same way, see CreateFormEventHandler#handle which does not store the real content of the form in the database. Have you ever seen this code work for forms or form description editors?

I'll push in another commit the tests that I've started to add, could you make the form description editor test succeed and contribute a XxxxLifecycleControllerTests for each representation kind (I've successfully done the Gantt one) to ensure that the mechanism works please.

Should be fixed. I've added a small test for portals to increase the coverage. There's still a coverage issue with the Hierarchy representation, but it's not clear how to properly test it or of its even relevant.

Bug: #3344 Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr> Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

sbegaudeau · 2024-05-13T09:41:22Z

...rts/src/main/java/org/eclipse/sirius/components/collaborative/charts/HierarchyEventFlux.java

@@ -55,7 +55,9 @@ public void hierarchyRefreshed(IInput input, Hierarchy newHierarchy) {
    }

    public Flux<IPayload> getFlux(IInput input) {
-        var initialRefresh = Mono.fromCallable(() -> new HierarchyRefreshedEventPayload(input.id(), this.currentHierarchy));
+        var initialRefresh = Mono.fromCallable(() -> {


Is there anything that should change here?

My mistake. I had changed the formatting to put a breakpoint on the inside of the lambda and forgot to revert.

.../org/eclipse/sirius/web/application/controllers/portal/PortalControllerIntegrationTests.java

sbegaudeau · 2024-05-13T09:45:18Z

.../org/eclipse/sirius/web/application/controllers/portal/PortalControllerIntegrationTests.java

+        var input = new CreateRepresentationInput(UUID.randomUUID(), TestIdentifiers.ECORE_SAMPLE_PROJECT.toString(), PortalDescriptionProvider.DESCRIPTION_ID, TestIdentifiers.EPACKAGE_OBJECT.toString(), "Sample Portal");
+        var flux = this.givenCreatedPortalSubscription.createAndSubscribe(input);
+
+        Consumer<PortalRefreshedEventPayload> initialPortalContentConsumer = payload -> Optional.of(payload)


I'm not sure what this test adds, why not test the feature that you have added?

sbegaudeau · 2024-05-13T09:46:23Z

...ipse/sirius/web/application/controllers/hierarchy/SubscriptionHierarchyEventDataFetcher.java

+ * @author pcdavid
+ */
+@SubscriptionDataFetcher(type = "Subscription", field = "hierarchyEvent")
+public class SubscriptionHierarchyEventDataFetcher implements IDataFetcherWithFieldCoordinates<Publisher<IPayload>> {


Why is that in a test folder?

We don't have a sirius-components-charts-graphql module yet and it felt overkill to create one just to host this single class only used in a test, especially as we do not have actual UI to create hierarchy representations.

...g/eclipse/sirius/web/application/controllers/hierarchy/HierarchyEventSubscriptionRunner.java

sbegaudeau · 2024-05-13T09:51:08Z

...cation/controllers/formdescriptioneditors/FormDescriptionEditorLifecycleControllerTests.java

+            var deleteWidgetInput = new DeleteWidgetInput(
+                    UUID.randomUUID(),
+                    StudioIdentifiers.SAMPLE_STUDIO_PROJECT.toString(),
+                    formDescriptionEditorId.get(),
+                    newWidgetId.get());
+            var result = this.deleteWidgetMutationRunner.run(deleteWidgetInput);
+            String typename = JsonPath.read(result, "$.data.deleteWidget.__typename");
+            assertThat(typename).isEqualTo(SuccessPayload.class.getSimpleName());
+
+            TestTransaction.flagForCommit();
+            TestTransaction.end();
+            TestTransaction.start();


This will trigger a refresh of the form description editor by itself, wouldn't that make the test "work" even without the reload after that? Unless I missed something, written this way the test does not prove that the feature works.

The test should probably change to show the feature working

sbegaudeau · 2024-05-13T09:54:59Z

...orms/src/main/java/org/eclipse/sirius/components/collaborative/forms/FormEventProcessor.java

+        } else if (changeDescription.getKind().equals(ChangeKind.RELOAD_REPRESENTATION) && changeDescription.getSourceId().equals(this.currentForm.get().getId())) {
+            Optional<Form> reloadedForm = this.representationSearchService.findById(this.editingContext, this.currentForm.get().getId(), Form.class);
+            if (reloadedForm.isPresent()) {
+                this.currentForm.set(reloadedForm.get());
+                this.emitNewForm(changeDescription);
+            }
+        }
+    }


Would that really work for forms? Wouldn't that reload the empty form from the database and send it back as is to the frontend making everything go blank?

Could you add the relevant tests for all representations to prove that your contribution is working please?

Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

pcdavid requested review from AxelRICHARD, gcoutable and sbegaudeau as code owners April 8, 2024 14:29

pcdavid added pr: to review soon priority: high labels Apr 8, 2024

pcdavid added this to the 2024.5.0 milestone Apr 8, 2024

pcdavid linked an issue Apr 8, 2024 that may be closed by this pull request

Add support for reloading representation from the database #3344

Open

pcdavid force-pushed the pcd/enh/reload-representation branch 3 times, most recently from 0250015 to db47907 Compare April 11, 2024 07:28

pcdavid commented Apr 12, 2024

View reviewed changes

sbegaudeau requested changes Apr 14, 2024

View reviewed changes

pcdavid force-pushed the pcd/enh/reload-representation branch from db47907 to d0f6a38 Compare April 15, 2024 13:44

pcdavid force-pushed the pcd/enh/reload-representation branch from d0f6a38 to b618808 Compare April 24, 2024 12:16

sbegaudeau force-pushed the pcd/enh/reload-representation branch from b618808 to 9cff301 Compare April 26, 2024 06:51

sbegaudeau requested changes Apr 29, 2024

View reviewed changes

[3344] Add support for reloading representation from the database

d6deaa8

Bug: #3344 Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

pcdavid force-pushed the pcd/enh/reload-representation branch 3 times, most recently from 16e8bd8 to 6a1fb7e Compare May 2, 2024 14:34

[test] Add additional tests

8e726e2

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr> Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

pcdavid force-pushed the pcd/enh/reload-representation branch from 6a1fb7e to 8e726e2 Compare May 6, 2024 13:06

sbegaudeau requested changes May 13, 2024

View reviewed changes

[test] Complete the reloading tests

8c1aec8

Signed-off-by: Pierre-Charles David <pierre-charles.david@obeo.fr>

pcdavid force-pushed the pcd/enh/reload-representation branch from 5823ed2 to 8c1aec8 Compare May 21, 2024 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3344] Add support for reloading representation from the database #3348

[3344] Add support for reloading representation from the database #3348

pcdavid commented Apr 8, 2024 •

edited

pcdavid Apr 12, 2024

sbegaudeau left a comment

sbegaudeau Apr 29, 2024

pcdavid Apr 29, 2024

pcdavid May 3, 2024

sbegaudeau May 13, 2024

pcdavid May 15, 2024

sbegaudeau May 13, 2024

sbegaudeau May 13, 2024

pcdavid May 15, 2024

sbegaudeau May 13, 2024

sbegaudeau May 13, 2024

sbegaudeau May 13, 2024

		@@ -29,6 +29,8 @@ public record DiagramRefreshedEventPayload(UUID id, Diagram diagram, String caus

		public static final String CAUSE_LAYOUT = "layout";

		public static final String CAUSE_RELOAD = "reload";

[3344] Add support for reloading representation from the database #3348

Are you sure you want to change the base?

[3344] Add support for reloading representation from the database #3348

Conversation

pcdavid commented Apr 8, 2024 • edited

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

sbegaudeau left a 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

pcdavid commented Apr 8, 2024 •

edited