[4372] Use semantic_data_id as editing context id #4397

mcharfadi · 2025-01-09T09:01:50Z

Bug: #4372

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?

...ava/org/eclipse/sirius/web/application/project/services/TemplateBasedProjectInitializer.java

sbegaudeau

I've stopped the review halfway because there is a small mis-understanding. I don't need you to remove the coupling between the editing context id and the project id to replace it by the exact same kind of coupling with the semantic data id.

I need you to make sure that downstream applications can couple the editing context id with something else and we should use the semantic data id as an example. I don't want to see the semantic data id being taken as an hardcoded dependency everywhere.

I first need a pull request making sure that the id of the project is now a string instead of an uuid and just that. After that, I need to be able to leverage that. For this second part, here's another acceptance criteria to ensure that your code works:

I need a piece of code in the project sirius-web-papaya which ensure that if the project id received by the backend is composed of UUID_OF_A_PAPAYA_PROJECT+demo-papaya so for example something like this:

http://localhost:5173/projects/f23c3cf0-7244-4892-8a58-82d40bfd1aa7+demo-papaya/edit/d4aa7077-707a-4cfb-a36f-1abdc46219e4

Then a new document with some Papaya content is added in the editing context. This document would not be saved in the editing context once it is persisted. I want to contribute a special IEditingContextLoader for Papaya that can change how data are loaded and a special IEditingContextSaver which changes how data are saved and I want to control how we are finding the editing context id from the project id.

I'm not even 100% sure that we need to change any of the code that you have modified to add this dependency to ISemanticDataSearchService.

sbegaudeau · 2025-01-13T10:17:08Z

...rius/web/application/editingcontext/controllers/ProjectCurrentEditingContextDataFetcher.java

@@ -30,10 +31,17 @@
 */
 @QueryDataFetcher(type = "Project", field = "currentEditingContext")
 public class ProjectCurrentEditingContextDataFetcher implements IDataFetcherWithFieldCoordinates<DataFetcherResult<String>> {
+
+    private final ISemanticDataSearchService semanticDataSearchService;


Please use the IEditingContextApplicationService here and leverage the ISemanticDataSearchService inside, since it's an application service it will help you not to forget using the @Transactionnal annotation which we need to have some proper transaction management.

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

sbegaudeau · 2025-01-13T10:23:04Z

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

@@ -74,7 +81,9 @@ public void persist(ICause cause, IEditingContext editingContext) {
        if (editingContext instanceof IEMFEditingContext emfEditingContext) {
            var applyMigrationParticipants = this.migrationParticipantPredicates.stream().anyMatch(predicate -> predicate.test(emfEditingContext));
            new UUIDParser().parse(editingContext.getId())
-                    .map(AggregateReference::<Project, UUID>to)
+                    .flatMap(this.semanticDataSearchService::findById)


While it should work I suspect that the whole if block will end up being extracted in an IEditingContextSaver

sbegaudeau · 2025-01-13T10:24:59Z

.../org/eclipse/sirius/web/application/editingcontext/services/EditingContextSearchService.java

+        var optionalUUID = new UUIDParser().parse(editingContextId);
+        if (optionalUUID.isPresent()) {
+            var optionalSemanticData = this.semanticDataSearchService.findById(optionalUUID.get());
+            if (optionalSemanticData.isPresent() && optionalSemanticData.get().getId() != null) {
+                return this.projectSearchService.findById(optionalSemanticData.get().getProject().getId())
+                        .map(project -> this.toEditingContext(project, optionalSemanticData.get().getId().toString()));
+            }
+        }
+        return Optional.empty();


I suspect that this is where a generic API will be needed

sbegaudeau · 2025-01-13T10:27:54Z

...rc/main/java/org/eclipse/sirius/web/application/object/controllers/ObjectRestController.java

@@ -50,53 +52,68 @@ public class ObjectRestController {

    private final IEditingContextDispatcher editingContextDispatcher;

-    public ObjectRestController(IEditingContextDispatcher editingContextDispatcher) {
+    private final ISemanticDataSearchService semanticDataSearchService;


You will need to use an application service here to leverage the @Transactionnal annotation (probably the same IEditingContextApplicationService?)

sbegaudeau · 2025-01-13T10:29:25Z

...eclipse/sirius/web/application/project/data/versioning/controllers/CommitRestController.java

@@ -56,18 +58,25 @@ public class CommitRestController {

    private final IEditingContextDispatcher editingContextDispatcher;

-    public CommitRestController(IEditingContextDispatcher editingContextDispatcher) {
+    private final ISemanticDataSearchService semanticDataSearchService;


Same comment

sbegaudeau · 2025-01-13T10:30:52Z

.../web/application/project/data/versioning/services/DefaultProjectDataVersiongRestService.java

@@ -48,9 +52,12 @@ public class DefaultProjectDataVersiongRestService implements IDefaultProjectDat

    private final IObjectService objectService;

-    public DefaultProjectDataVersiongRestService(IDefaultObjectRestService defaultObjectRestService, IObjectService objectService) {
+    private final ISemanticDataSearchService semanticDataSearchService;


Same comment

sbegaudeau · 2025-01-14T10:08:35Z

...ava/org/eclipse/sirius/web/application/project/services/TemplateBasedProjectInitializer.java

        this.projectSearchService = Objects.requireNonNull(projectSearchService);
        this.projectMapper = Objects.requireNonNull(projectMapper);
        this.editingContextSearchService = Objects.requireNonNull(editingContextSearchService);
        this.editingContextPersistenceService = Objects.requireNonNull(editingContextPersistenceService);
        this.projectTemplateInitializers = Objects.requireNonNull(projectTemplateInitializers);
+        this.semanticDataSearchService = semanticDataSearchService;


require non null (same comment at various other locations)

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

sbegaudeau

I'll update the PR and ensure that it gets merged

...g/eclipse/sirius/web/application/document/controllers/MutationCreateDocumentDataFetcher.java

.../sirius-web-application/src/main/java/org/eclipse/sirius/web/application/dto/Identified.java

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

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

.../org/eclipse/sirius/web/application/editingcontext/services/EditingContextSearchService.java

...g/eclipse/sirius/web/application/studio/services/representations/DomainExplorerServices.java

...eclipse/sirius/web/application/controllers/documents/DocumentControllerIntegrationTests.java

...rius-web/backend/sirius-web/src/test/java/org/eclipse/sirius/web/data/StudioIdentifiers.java

sbegaudeau · 2025-02-07T14:26:15Z

...sirius-web/backend/sirius-web/src/test/java/org/eclipse/sirius/web/data/TestIdentifiers.java

@@ -26,7 +26,8 @@ public final class TestIdentifiers {
    public static final String UML_SAMPLE_PROJECT = "7ba7bda7-13b9-422a-838b-e45a3597e952";
    public static final String SYSML_SAMPLE_PROJECT = "4164c661-e0cb-4071-b25d-8516440bb8e8";
    public static final UUID SYSML_IMAGE = UUID.fromString("ff37f0eb-effb-4c57-b17f-76bc7ea64f5b");
-    public static final String ECORE_SAMPLE_PROJECT = "99d336a2-3049-439a-8853-b104ffb22653";
+    public static final String ECORE_SAMPLE_PROJECT = "cb133bf0-d7aa-4a83-a277-0972919dd46a";
+    public static final String ECORE_SAMPLE_PROJECT_ID = "99d336a2-3049-439a-8853-b104ffb22653";


There's no reason to rename the existing identifier to use _ID, it is an identifier, that's the goal of the class. Use another name if it's relevant but right now I don't know how anyone could tell the difference between the identifier ECORE_SAMPLE_PROJECT and ECORE_SAMPLE_PROJECT_ID.

sbegaudeau · 2025-02-07T14:28:49Z

CHANGELOG.adoc

@@ -44,6 +44,8 @@ See `PapayaExtensionRegistry.tsx` for an example of how rendering a project subt
 It is not an API breaking change but a breaking change from the end-user point of view.
 - https://github.com/eclipse-sirius/sirius-web/issues/4500[#4500] [sirius-web] Remove the tight coupling between `SemanticData` and `Project` by removing `SemanticData#project` along with the column `project_id` from the `semantic_data` table
 - https://github.com/eclipse-sirius/sirius-web/issues/4501[#4501] [browser] `IReferenceWidgetRootCandidateSearchProvider` API has been renamed to `IModelBrowserRootCandidateSearchProvider` and moved to the `sirius-components-collaborative-browser` module.
+- https://github.com/eclipse-sirius/sirius-web/issues/4372[#4372] [sirius-web] The identifier used for editingContextId is now semanticDataId instead of the projectId.


Since this will be a MASSIVE change for all downstream application which may be using editingContextId as the projectId some additional information will be necessary to explain them how to proceed with this breaking change.

mcharfadi added pr: to review soon priority: high labels Jan 9, 2025

mcharfadi added this to the 2025.2.0 milestone Jan 9, 2025

mcharfadi requested a review from sbegaudeau as a code owner January 9, 2025 09:01

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

Lower the coupling between project and editing context #4372

Open

mcharfadi force-pushed the mch/enh/editing_context_id branch 25 times, most recently from fd7d267 to 2ad3d10 Compare January 10, 2025 13:46

mcharfadi force-pushed the mch/enh/editing_context_id branch 11 times, most recently from 8d9fe9a to c354ee1 Compare January 14, 2025 09:45

mcharfadi commented Jan 14, 2025

View reviewed changes

...ava/org/eclipse/sirius/web/application/project/services/TemplateBasedProjectInitializer.java Outdated Show resolved Hide resolved

mcharfadi force-pushed the mch/enh/editing_context_id branch 3 times, most recently from 92b25f9 to 37a7bb4 Compare January 14, 2025 10:21

sbegaudeau requested changes Jan 14, 2025

View reviewed changes

mcharfadi added pr: to review later priority: low and removed pr: to review soon priority: high labels Jan 29, 2025

mcharfadi force-pushed the mch/enh/editing_context_id branch 7 times, most recently from b708dde to b82c6aa Compare February 6, 2025 16:37

[4372] Use semantic_data_id as editing context id

ac67887

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

mcharfadi force-pushed the mch/enh/editing_context_id branch from b82c6aa to ac67887 Compare February 7, 2025 07:48

sbegaudeau requested changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4372] Use semantic_data_id as editing context id #4397

[4372] Use semantic_data_id as editing context id #4397

mcharfadi commented Jan 9, 2025 •

edited

Loading

sbegaudeau left a comment

sbegaudeau Jan 13, 2025

sbegaudeau Jan 13, 2025

sbegaudeau Jan 13, 2025

sbegaudeau Jan 13, 2025

sbegaudeau Jan 13, 2025

sbegaudeau Jan 13, 2025

sbegaudeau Jan 14, 2025

sbegaudeau left a comment

sbegaudeau Feb 7, 2025

sbegaudeau Feb 7, 2025

[4372] Use semantic_data_id as editing context id #4397

Are you sure you want to change the base?

[4372] Use semantic_data_id as editing context id #4397

Conversation

mcharfadi commented Jan 9, 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?

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

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

mcharfadi commented Jan 9, 2025 •

edited

Loading