[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?

mcharfadi · 2025-01-14T10:06:00Z

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

    private final TransactionTemplate transactionTemplate;

-    private final ApplicationEventPublisher applicationEventPublisher;
+    //private final ApplicationEventPublisher applicationEventPublisher;


No sure if I should adopt a Parameters object or move some things around

Or I could remove ApplicationEventPublisher & TransactionTemplate

No sure if I should adopt a Parameters object or move some things around

I have not looked into this in details but there are some very specific places where we had to do that because we can't use Spring dependency injection but if the question arises anywhere else the answer is always no. This "parameters" object pattern is something we don't want to propagate.

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

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.

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

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

@@ -63,7 +62,7 @@ public EditingContextLoader(ISemanticDataSearchService semanticDataSearchService
    public void load(EditingContext editingContext, UUID projectId) {
        this.editingContextProcessors.forEach(processor -> processor.preProcess(editingContext));

-        this.semanticDataSearchService.findByProject(AggregateReference.to(projectId))
+        this.semanticDataSearchService.findById(UUID.fromString(editingContext.getId()))


Is the projectId parameter still revenant now? Shouldn't we have something to select a relevant loader? We will see with a test.

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)

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 12 times, most recently from 8d9fe9a to c354ee1 Compare January 14, 2025 09:45

mcharfadi commented Jan 14, 2025

View reviewed changes

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

[4372] Use semantic_data_id as editing context id

37a7bb4

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

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

sbegaudeau requested changes Jan 14, 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

mcharfadi Jan 14, 2025

mcharfadi Jan 14, 2025

sbegaudeau Jan 14, 2025

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 13, 2025

sbegaudeau Jan 14, 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?

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

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 9, 2025 •

edited

Loading