[4444] Allow multiple implementation of IEditingContextSaver & IEditi… #4445

mcharfadi · 2025-01-20T12:29:31Z

…ngContextLoader

Bug : #4444
Signed-off-by: Michaël Charfadi [email protected]

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?

mcharfadi · 2025-01-20T12:37:26Z

...in/java/org/eclipse/sirius/web/application/project/controllers/ViewerProjectDataFetcher.java

@@ -40,6 +41,11 @@ public ViewerProjectDataFetcher(IProjectApplicationService projectApplicationSer
    @Override
    public ProjectDTO get(DataFetchingEnvironment environment) throws Exception {
        String projectId = environment.getArgument(PROJECT_ID_ARGUMENT);
+        var normalizedId = projectId.split("\\+");


This would probably need to use a dedicated API

…ngContextLoader Bug: #4444 Signed-off-by: Michaël Charfadi <[email protected]>

mcharfadi · 2025-01-20T13:16:36Z

Note that it's possible to persist representation by modifying the save method of RepresentationMetadataPersistenceService to get the "base" project but this is too ugly without a dedicated API.
Also we would also need to update more services that uses the projectId in order to get the representationMetaData for handling diagrams properly.

In conclusion I think we need an API to retrieve the projectId from the editingContextId or put the projectId in the editingContext to allow working with representations.

sbegaudeau · 2025-01-28T08:46:25Z

...lipse/sirius/web/application/editingcontext/controllers/ViewerEditingContextDataFetcher.java

@@ -43,9 +43,6 @@ public ViewerEditingContextDataFetcher(IEditingContextApplicationService editing
    @Override
    public DataFetcherResult<String> get(DataFetchingEnvironment environment) throws Exception {
        String editingContextId = environment.getArgument(EDITING_CONTEXT_ID_ARGUMENT);
-        if (!this.editingContextApplicationService.existsById(editingContextId)) {


We need to figure out if the editing context exists, why remove that?

The editingContext does not exist at this point we just assume that it will exist if a project exists.
So since the new editingContext does not match an existing project with this implementation, I'm not sure how to test this.

sbegaudeau · 2025-01-28T08:47:23Z

...in/java/org/eclipse/sirius/web/application/editingcontext/services/EditingContextLoader.java

    }

-    public void load(EditingContext editingContext, String projectId) {
+    public IEditingContext load(String editingContextId) {
+        var project = this.projectSearchService.findById(editingContextId);


optionalProject and test that the optional is not empty otherwise return null

sbegaudeau · 2025-01-28T08:47:57Z

...in/java/org/eclipse/sirius/web/application/editingcontext/services/EditingContextLoader.java

+        return editingContext;
+    }
+
+    public void toEditingContext(EditingContext editingContext, String projectId) {


toEditingContext? You already have an editing context just keep load

sbegaudeau · 2025-01-28T08:48:38Z

...in/java/org/eclipse/sirius/web/application/editingcontext/services/EditingContextLoader.java

@@ -73,6 +97,11 @@ public void load(EditingContext editingContext, String projectId) {
        this.editingContextProcessors.forEach(processor -> processor.postProcess(editingContext));
    }

+    @Override
+    public boolean canHandle(String projectId) {
+        return new UUIDParser().parse(projectId).isPresent() && this.projectSearchService.existsById(projectId);


Why are you parsing the UUID? We have moved away from UUID based identifier for the project id

I'm testing if the id is the id of a project and in this cas I'm keeping the current implementation

sbegaudeau · 2025-01-28T08:49:01Z

...eclipse/sirius/web/application/editingcontext/services/EditingContextPersistenceService.java

-            this.semanticDataUpdateService.updateDocuments(cause, projectId, documents, domainUris);
-        }
-
+        editingContextSavers.stream()


Just pick the first one, not all of them

sbegaudeau · 2025-01-28T08:53:21Z

...va/org/eclipse/sirius/web/application/editingcontext/services/api/IEditingContextLoader.java


+    boolean canHandle(String projectId);


Why projectId?

sbegaudeau · 2025-01-28T08:54:29Z

...n/src/main/java/org/eclipse/sirius/web/domain/boundedcontexts/semanticdata/SemanticData.java

+        public Builder id(UUID id) {
+            this.id = id;
+            return this;
+        }


Why?? Those changes are odd

sbegaudeau · 2025-01-28T08:55:17Z

...pse/sirius/web/domain/boundedcontexts/semanticdata/services/SemanticDataCreationService.java

+    @Override
+    public void add(ICause cause, UUID semanticDataId, AggregateReference<Project, String> project) {
+        var semanticData = SemanticData.newSemanticData()
+                .id(semanticDataId)
+                .project(project)
+                .documents(Set.of())
+                .build(cause);
+        this.semanticDataRepository.save(semanticData);
+    }


There's no need for that

sbegaudeau · 2025-01-28T08:57:27Z

...lipse/sirius/web/domain/boundedcontexts/semanticdata/services/SemanticDataSearchService.java

@@ -39,7 +40,12 @@ public SemanticDataSearchService(ISemanticDataRepository semanticDataRepository)

    @Override
    public Optional<SemanticData> findByProject(AggregateReference<Project, String> project) {
-        return this.semanticDataRepository.findByProjectId(project.getId());
+        return this.semanticDataRepository.findByProjectId(project.getId()).stream().findFirst();


I don't think we will need to change that but I think that you are missing some concept to capture which semantic data to use for a given project, we need to find out how to keep this information in the database.

sbegaudeau · 2025-01-28T08:58:27Z

...irius/web/domain/boundedcontexts/semanticdata/services/api/ISemanticDataCreationService.java

 /**
 * Used to create the semantic data.
 *
 * @author sbegaudeau
 */
 public interface ISemanticDataCreationService {
    void initialize(ICause cause, AggregateReference<Project, String> project);
+
+    void add(ICause cause, UUID semanticDataId, AggregateReference<Project, String> project);


Can be removed but we will probably need to update the previous method to something like create which would return the created semantic data

mcharfadi added pr: to review soon priority: low labels Jan 20, 2025

mcharfadi added this to the 2025.2.0 milestone Jan 20, 2025

mcharfadi self-assigned this Jan 20, 2025

mcharfadi linked an issue Jan 20, 2025 that may be closed by this pull request

Allow multiple implementation of IEditingContextSaver & IEditingContextLoader #4444

Open

mcharfadi commented Jan 20, 2025

View reviewed changes

mcharfadi force-pushed the mch/enh/editing_context_saver_loader branch 4 times, most recently from cac2db5 to bb225ad Compare January 20, 2025 12:59

[4444] Allow multiple implementation of IEditingContextSaver & IEditi…

2bfe4d5

…ngContextLoader Bug: #4444 Signed-off-by: Michaël Charfadi <[email protected]>

mcharfadi force-pushed the mch/enh/editing_context_saver_loader branch from bb225ad to 2bfe4d5 Compare January 20, 2025 13:09

mcharfadi marked this pull request as ready for review January 27, 2025 13:11

mcharfadi requested a review from sbegaudeau as a code owner January 27, 2025 13:11

sbegaudeau requested changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4444] Allow multiple implementation of IEditingContextSaver & IEditi… #4445

[4444] Allow multiple implementation of IEditingContextSaver & IEditi… #4445

mcharfadi commented Jan 20, 2025 •

edited

Loading

mcharfadi Jan 20, 2025

mcharfadi commented Jan 20, 2025 •

edited

Loading

sbegaudeau Jan 28, 2025

mcharfadi Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

mcharfadi Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

sbegaudeau Jan 28, 2025

[4444] Allow multiple implementation of IEditingContextSaver & IEditi… #4445

Are you sure you want to change the base?

[4444] Allow multiple implementation of IEditingContextSaver & IEditi… #4445

Conversation

mcharfadi commented Jan 20, 2025 • edited Loading

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

mcharfadi commented Jan 20, 2025 • edited Loading

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

mcharfadi commented Jan 20, 2025 •

edited

Loading

mcharfadi commented Jan 20, 2025 •

edited

Loading