Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[1301] Add support for inter model references #1303

Merged
merged 5 commits into from
Oct 5, 2022
Merged

[1301] Add support for inter model references #1303

merged 5 commits into from
Oct 5, 2022

Conversation

lfasani
Copy link
Contributor

@lfasani lfasani commented Jul 22, 2022 •
edited
Loading

ticket #1300 and #1301

@sbegaudeau sbegaudeau linked an issue Jul 25, 2022 that may be closed by this pull request
Add support for inter model references #1301
Closed
@sbegaudeau sbegaudeau changed the title Lfa/enh/intermodel [1301] Add support for inter model references Jul 25, 2022
@sbegaudeau sbegaudeau added this to the 2022.9.0 milestone Jul 25, 2022
@sbegaudeau sbegaudeau linked an issue Jul 25, 2022 that may be closed by this pull request
Rename SiriusWebJSONResourceFactoryImpl to JSONResourceFactoryImpl #1300
Closed
@sbegaudeau sbegaudeau force-pushed the lfa/enh/intermodel branch 2 times, most recently from 808999e to 04af067 Compare September 5, 2022 13:08
pcdavid
pcdavid reviewed Sep 6, 2022
View reviewed changes
CHANGELOG.adoc Outdated Show resolved Hide resolved
@sbegaudeau
Copy link
Member

I don't have any issue with the content of this specific pull request (apart from the minor issues detected by @pcdavid) but the Sirius Web side of this pull request still need too much work to be part of 2022.9.0. It will thus be rescheduled for 2022.11.0.

@sbegaudeau sbegaudeau modified the milestones: 2022.9.0, 2022.11.0 Sep 8, 2022
@lfasani lfasani force-pushed the lfa/enh/intermodel branch 2 times, most recently from a4dbaf1 to 688eee5 Compare September 13, 2022 12:30
@lfasani lfasani self-assigned this Sep 13, 2022
@lfasani lfasani force-pushed the lfa/enh/intermodel branch 2 times, most recently from 1c21955 to 79636c6 Compare September 15, 2022 14:40
@lfasani lfasani mentioned this pull request Sep 19, 2022
Closed
27 tasks
pcdavid
pcdavid requested changes Sep 21, 2022
View reviewed changes
Copy link
Member

@pcdavid pcdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not actually tested yet in practice, but I publish my remarks anyway. More may come after testing.

CHANGELOG.adoc Show resolved Hide resolved
CHANGELOG.adoc Show resolved Hide resolved
CHANGELOG.adoc Outdated Show resolved Hide resolved
doc/adrs/069_add_support_for_intermodel_reference.adoc Outdated Show resolved Hide resolved
doc/adrs/069_add_support_for_intermodel_reference.adoc Outdated Show resolved Hide resolved
...ain/java/org/eclipse/sirius/components/emf/services/EditingContextCrossReferenceAdapter.java Outdated Show resolved Hide resolved
...ain/java/org/eclipse/sirius/components/emf/services/EditingContextCrossReferenceAdapter.java Outdated Show resolved Hide resolved
...ain/java/org/eclipse/sirius/components/emf/services/EditingContextCrossReferenceAdapter.java Outdated Show resolved Hide resolved
...ain/java/org/eclipse/sirius/components/emf/services/EditingContextCrossReferenceAdapter.java Outdated
Collection<Setting> settings = this.getInverseReferences(referencedObject);

if (settings != null) {
Collection<Setting> nonContainmentReferences = this.getNonContainmentReferences(settings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method (removeProxyFromInverseReference) will be called a lot. It should be as efficient as possible. It should do the iteration and test on the references itself instead of calling getNonContainmentReferences which allocates a new ArrayList every time.
Maybe it should even avoid calling getInverseReferences (which does quite a lot of work, some which may not be needed in our case) and use ECrossReferenceAdapter.inverseCrossReferencer directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to ECrossReferenceAdapter.inverseCrossReferencer directly but it fails for the EOpposite features.
It is actually, the aim of getNonContainmentReferences to manage the EOpposite

...ain/java/org/eclipse/sirius/components/emf/services/EditingContextCrossReferenceAdapter.java Show resolved Hide resolved
@lfasani lfasani requested a review from pcdavid October 4, 2022 09:43
@lfasani
[releng] Switch to EMFJson 2.3.2-SNAPSHOT
a1f4277 
Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>
@lfasani
[1300] Rename SiriusWebJSONResourceFactoryImpl to JSONResourceFactory
f0334af 
Bug: #1300
Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>
pcdavid
pcdavid reviewed Oct 5, 2022
View reviewed changes
CHANGELOG.adoc Outdated Show resolved Hide resolved
@lfasani
[1301] Add support for intermodel references
208c26c 
The referenced object concrete type is now serialized in JSON resource
JSON resources should now be created with the right scheme

Bug: #1301
Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>
@lfasani
[1301] Add a cross reference adapter to clean the inverse references
f3a08e0 
This adapter specializes ECrossReferenceAdapter in case of deletion of
an EObject or a Resource in order to:
* clean the proxies of the inverse references
* clean the ECrossReferenceAdapter itself

Bug: #1301
Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>
@lfasani lfasani force-pushed the lfa/enh/intermodel branch 3 times, most recently from a465a59 to bc4002c Compare October 5, 2022 13:42
pcdavid
pcdavid reviewed Oct 5, 2022
View reviewed changes
...ices/src/main/java/org/eclipse/sirius/web/services/documents/UploadDocumentEventHandler.java
Comment on lines +200 to +221
boolean eObjectcontainsAProxy = eObject.eClass().getEAllReferences().stream()
.filter(ref -> !ref.isContainment() && eObject.eIsSet(ref))
.filter(ref -> {
boolean containsAProxy = false;
Object value = eObject.eGet(ref);
if (ref.isMany()) {
List<?> list = (List<?>) value;
containsAProxy = list.stream()
.filter(EObject.class::isInstance)
.map(EObject.class::cast)
.filter(EObject::eIsProxy)
.findFirst()
.isPresent();
} else {
containsAProxy = ((EObject) value).eIsProxy();
}
return containsAProxy;
})
.findFirst()
.isPresent();

return eObjectcontainsAProxy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screams for a separate helper method, and just:

.filter(this::hasProxyReferences)

@lfasani
[1301] Adapt sirius-web code to support inter-model references
adbf143 
* Use a correct URI to instantiate a JSONResource. The URI should have a
scheme "sirius"
* Only resources with "sirius" scheme are persisted and displayed in the
Explorer
* This commit also ensures that, if a specific resource factory has been
registered, it will be used to instantiate the XMI resource at download
* Forbid the upload of a single document that contains proxies. The
check is not done when uploading a project.

#1301
Signed-off-by: Laurent Fasani <laurent.fasani@obeo.fr>
@gcoutable gcoutable self-requested a review October 5, 2022 16:26
@pcdavid pcdavid merged commit acacf7f into master Oct 5, 2022
@pcdavid pcdavid deleted the lfa/enh/intermodel branch October 5, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcdavid pcdavid pcdavid approved these changes

@sbegaudeau sbegaudeau Awaiting requested review from sbegaudeau sbegaudeau is a code owner

@gcoutable gcoutable Awaiting requested review from gcoutable

Assignees

@lfasani lfasani

Labels
pr: to review soon priority: high
Projects
None yet
Milestone
2022.11.0
Development

Successfully merging this pull request may close these issues.

Add support for inter model references Rename SiriusWebJSONResourceFactoryImpl to JSONResourceFactoryImpl
3 participants
@lfasani @sbegaudeau @pcdavid