Bug 432133 - Semantic model based on XML schema generated ecore and handling of DocumentRoot class
Summary: Semantic model based on XML schema generated ecore and handling of DocumentRo...
Status: CLOSED FIXED
Alias: None
Product: Sirius
Classification: Modeling
Component: Core (show other bugs)
Version: 1.0.0M6   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 3.0.0M6   Edit
Assignee: Mickael LANOE CLA
QA Contact:
URL:
Whiteboard:
Keywords: triaged
Depends on:
Blocks: 459636
  Show dependency tree
 
Reported: 2014-04-07 04:52 EDT by Stephan Kranz CLA
Modified: 2015-06-24 11:15 EDT (History)
6 users (show)

See Also:


Attachments
Sample diagram specification (7.39 KB, application/zip)
2014-04-07 05:10 EDT, Stephan Kranz CLA
no flags Details
Basicfamily ecore and sources generated from XSD schema (74.60 KB, application/zip)
2014-04-07 05:12 EDT, Stephan Kranz CLA
no flags Details
Modified DAnalysisSessionImpl.doAddSemanticResouces with rationalization (5.32 KB, text/plain)
2015-02-02 12:28 EST, John Palof CLA
pierre-charles.david: iplog+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stephan Kranz CLA 2014-04-07 04:52:55 EDT

    
Comment 1 Stephan Kranz CLA 2014-04-07 05:10:19 EDT
Created attachment 241659 [details]
Sample diagram specification
Comment 2 Stephan Kranz CLA 2014-04-07 05:11:51 EDT
The attached semantic model and sources of the basic family sample have been generated from an XSD schema. (I have exported your basicfamily.ecore file to an XSD file using the EMF export capabilities. In a second step I have removed all elements coming from the ecore namespace from the XSD file to emulate a file coming fresh from a design tool not knowing anything about ecore.) 

The EMF ecore file features an additional (EMF generated) top-level DocumentRoot Class basically representing the XML file which then contains the actual top level semantic element class. This DocumentRoot class as generated by EMF, happens to have an ExtendedMetaData attribute setting the name of the class to an empty string.

In general, creating diagrams for this semantic model following the pattern described in your family sample works out fine. However when re-staring Eclipse, I get an "Impossible to find an interpreter - Could not find a session for model element" message and all diagrams are broken. A quick workaround is, to remove and add the "Modelling" nature to my project again.

Since this is not very convenient in the long-term, I had a look into the .aird file and the Sirius code base and eventually managed to identify the cause of the problem:

There is a problematic entry in the .aird file, i.e. the xmi:type entry of the <models> element does not contain the namespace + name but the namespace only. (E.g. xmi:type="basicfamily" instead of xmi:type="basicfamily:Family"). When re-starting the Eclipse session, Sirius fails to find the model which results in the error described above.

As a second solution (to avoid the work-around), I have modified the Sirius sources in terms of accessing the root object of the semantic Resource in a couple of places.

Instead of:

final EObject root = newResource.getContents().get(0);

I am doing this:

EObject root = newResource.getContents().get(0);
EList<EAnnotation> annotations = root.eClass().getEAnnotations();
for (EAnnotation annotation : annotations)
{
EMap<String, String> details = annotation.getDetails();
if (details.containsKey("name") && details.get("name").length() == 0)
{
// Number 3 is the semantic root
EStructuralFeature eFeature = root.eClass().getEStructuralFeature(3);
root = (EObject) root.eGet(eFeature);
break;
}
}

(e.g. in DAnalysisSessionImpl.java)

which works-out fine.
Comment 3 Stephan Kranz CLA 2014-04-07 05:12:44 EDT
Created attachment 241660 [details]
Basicfamily ecore and sources generated from XSD schema
Comment 4 Florian Barbin CLA 2014-04-10 10:27:04 EDT
See also the forum topic: http://www.eclipse.org/forums/index.php/t/674125/
Comment 5 Pierre-Charles David CLA 2014-04-22 04:35:31 EDT
Moving to M7 to at least try to better understand what happens exactly.
Comment 6 Pierre-Charles David CLA 2014-04-23 09:07:25 EDT
Steps to reproduce:
1. In your dev environment (Eclipse with Sirius installed or with the sources in the workspace), import the basicfamily metamodel plugins (see attachment).
2. Start a runtime, and in its workspace, import the sample diagram specification (see attachement).
3. Open "/XsdFamily.design/description/XsdFamily.odesign" as text and remove the invalid "<metamodels" line, otherwise the VSM will fail to load properly.
4. Create a new modeling project, and copy/paste /SampleFamily/example.basicfamily into it. Note that Sirius correctly detects it as a model, loads it, and allows to expand its content directly from the Model Explorer.
5. In this new modeling project, right-click on the project itself, select choose "Viewpoint Selection". Note that the "basicFamily" viewpoint is proposed, which means the example.basicfamily model is correctly loaded in the session and registered as a semantic model.
6. Enable the basicFamily viewpoint, and create a diagram on the Family element (inside the DocumentRoot). For now, everything works as expected. Save the session and close the project.
7. Re-open the project. The representation.aird file is correctly loaded and can be expanded; the diagram we just created is visible, but trying to open it fails with NPEs => KO. Also note that the example.basicfamily is not expandable: it was not loaded as a semantic model in the session => KO.
8. Right-click on the "Project Dependencies" item inside the Modeling Project; select "Add Model", the "Browse Workspace" and choose the example.basicfamily inside the project. It is now loaded correctly as a semantic model, the corresponding diagram can be opened, we are back into a working state.
Comment 7 Pierre-Charles David CLA 2014-04-23 09:18:49 EDT
In the scenario above, after saving the session at step 6, opening the representations.aird shows that the link from the session towards the semantic model has been saved like this:

  <models xmi:type="basicfamily" href="example.basicfamily#/"/>

In a normal case, the xmi:type should reference a fully qualified type, but here it only indicates a package name.

Debugging the session initialization shows that the example.basicfamily is actually loaded inside the session's ResourceSet, but the Session's DAnalysis.models reference, which should normally point to the root element of each semantic model (here we have only one), is empty. In this situation, the example.basicfamily Resource has as content a DocumentRoot, which itself contains the Family (as can be seen in the model explorer).

Note that manually changing the line above into

  <models xmi:type="basicfamily:DocumentRoot" href="example.basicfamily#/"/>

then closing and reopening the project (and thus the session), does not work, *but*, changing it into

  <models xmi:type="basicfamily:Family" href="example.basicfamily#/"/>

fixes the issue.

From this and the scenario above (where everything works fine as long as the semantic model is added explicitly inside the session, not indirectly by following the <models/> reference), everything seems to point to a serialization issue. One possible lead would be that when referencing XSD-based semantic models, we need to apply some specific options when saving the aird itself to produce the current xmi:type attribute.
Comment 8 Pierre-Charles David CLA 2014-04-23 09:43:37 EDT
The problematic xmi:type attribute is generated in org.eclipse.emf.ecore.xmi.impl.XMISaveImpl.saveTypeAttribute(EClass) by the call to org.eclipse.emf.ecore.xmi.XMLHelper.getQName(EClass eClass), with the DocumentRoot EClass as argument. More precisely, XMLHelperImpl.getQName(EClass c) returns just "basicfamily" as a qualified name, in part because its xmlMap field (of type org.eclipse.emf.ecore.xmi.XMLResource.XMLMap) is empty. The only implementation I see of that interface is org.eclipse.emf.ecore.xmi.impl.XMLMapImpl, which happens to have and XSD2ECORE field, so maybe we need an instance of that type available to correctly save references to XSD-based metamodels. Still digging.
Comment 9 Pierre-Charles David CLA 2014-04-23 10:28:08 EDT
Moving out of M7. From the analysis, it does not look like a "quick fix" will be enough. We'll try to look into this again for 1.0.
Comment 10 Pierre-Charles David CLA 2014-05-23 08:42:58 EDT
I really hoped we would have enough time before 1.0.0 to look into this, but with rc1 already done and a few other important issues to fix before the release, this will be postponed for the next release.
Comment 11 Cedric Gava CLA 2014-12-17 15:28:37 EST
Hello

I've faced the same issue in version 2.0.0, and 2.0.2.
Is this bug planned to be addressed soon ?

Best regards
Comment 12 Pierre-Charles David CLA 2014-12-29 09:59:52 EST
(In reply to Cedric Gava from comment #11)
> I've faced the same issue in version 2.0.0, and 2.0.2.
> Is this bug planned to be addressed soon ?

From what I remember of my analysis a few months ago, it looked like an issue very specific to how XSD-based models are handled by EMF. It has been postponed for now because honestly we're not very familiar with that aspect of EMF (at least I am not) and did not have the time needed to dig into this.

If you're more familiar with XSD-based models, would you be willing to propose a fix and/or collaborate with us to move on this issue?
Comment 13 Cedric Gava CLA 2015-01-07 03:16:34 EST
Hello

I'am quite unskilled regarding the development of such things, so I can only provide testing, which is, obviously not enough...
The project using this feature has not been continued, so there is no hurry.
Comment 14 John Palof CLA 2015-01-07 09:45:09 EST
I do have an active project which is impacted by this defect.  Our product has several configuration and data files defined by XSD for which I am attempting to consolidate legacy editors by use of Sirius viewpoints.  I would greatly appreciate a fix.
Comment 15 Pierre-Charles David CLA 2015-01-23 05:02:43 EST
(In reply to John Palof from comment #14)
> I do have an active project which is impacted by this defect.  Our product
> has several configuration and data files defined by XSD for which I am
> attempting to consolidate legacy editors by use of Sirius viewpoints.  I
> would greatly appreciate a fix.

Hi.

This specific issue is not in our current roadmap; we have no explicit plans to work on it in the short term. It was moved to "Target Milestone: 3.0" by default as we had started to look at it earlier and it already had a milestone. That was a mistake on my part, I'll remove the milestone to avoid confusion.

That said, this bug can move forward if you provide a fix for it (see the Contributor Guide at https://wiki.eclipse.org/Sirius/Contributor_Guide) or you contact Obeo for sponsored work. We'll add the helpwanted keyword in the meantime.
Comment 16 John Palof CLA 2015-01-23 09:27:10 EST
Thank you for the clarification.

Since my previous comment, I have found that the issue can be circumvented by specifying the metamodels in diagram's 'Metamodel' property tab.  So in my case, I have used 'Add from registry' to specify the metamodels used in my viewpoint specification.   While the aird file still has the problem documented by Pierre-Charles David in Comment#7, the file and objects load cleanly.

Given this workaround, I don't plan to pursue the issue further at this point.  I have been meaning to update this bug with this observation and your post triggered me to get it done.  Thank you.
Comment 17 John Palof CLA 2015-02-02 12:28:58 EST
Created attachment 250449 [details]
Modified DAnalysisSessionImpl.doAddSemanticResouces with rationalization

I was incorrect in stating that specifying the metamodels circumvented the problem.  Since I need this issue resolved, I did some more research.

The injection of  the 'DocumentRoot' EClass into an XSD generated ECore model is explained in https://www.eclipse.org/modeling/emf/docs/overviews/XMLSchemaToEcoreMapping.pdf.  Based on the pdf, it seems necessary that DAnalysisSessionImpl.doAddSemanticResource steps around the three canned features ('mixed', 'xMLNSPrefixMap', and 'xSISchemaLocation') of the DocumentRoot to obtain the feature representing the true EClass of the XML document rather than setting its model to the injected DocumentRoot EClass.

Similar to what Stephan Kranz offered, I have attached an updated DAnalysisImpl.doAddSemanticResources which is based on the specification found in XMLSchemaToEcoreMapping.pdf.  The code attempts to detect a 'DocumentRoot' root generated from an XSD and then steps around its three canned features to obtain the true root to be set on the analysis.

Since my knowledge of EMF and Sirius is not extensive, I am fully aware that there may be better places for a fix, but offer this in hopes that a similar or improved fix could be provided in the very near future as I have products that need to go out the door.

Thanks
Comment 18 Stephan Kranz CLA 2015-02-03 04:47:35 EST
Alternatively to modifying the Sirius sources (DAnalysisSessionImpl.java) it is also possible to override the getContents() method of your XXXResourceImpl.java file in the util package generated from the ecore. Advantage is that Sirius can be used as-is. Drawback is that you have to apply this override to each of your EMF Resource types.

So for XXXResourceImpl.java it would be:

@Override
    public EList<EObject> getContents() {
        EList<EObject> contents = super.getContents();

        if (contents.size() > 0 && contents.get(0) instanceof XXXDocument) {
            EObject document = contents.get(0);
            EStructuralFeature xxxFeature = document.eClass()
                    .getEStructuralFeature(3);
            Object object = document.eGet(xxFeature);
            if (object instanceof XXX) {
                contents.remove(0);
                contents.add(0, (XXX) object);
            }
        }
        return contents;
    }
Comment 19 John Palof CLA 2015-02-03 18:08:39 EST
Thanks for the suggestion, however I am not sure of the side-effects that might cause.

Other EMF based editor(s), such as 'Sample Reflective Ecore Model Editor' work fine as is.  I would expect changing the XXXResourceImpl.getContents() will adversely affect those?
Comment 20 Pierre-Charles David CLA 2015-02-04 05:38:55 EST
John, Stephan: thanks for digging into this!

At first glance, it looks like the approach in John's patch is the right one. It does not look like it would cause regressions in the case of non-XSD-based resources. If we merge this on master, would you be willing to test nightly builds and confirm this works for you?

John: before we can merge your code you would need to sign the Eclipse CLA (see http://wiki.eclipse.org/CLA) if that's OK with you, and optionally to convert your patch into a Gerrit PatchSet (if you want to do it but need assistance do not hesitate).
Comment 21 John Palof CLA 2015-02-04 10:05:18 EST
(In reply to Pierre-Charles David from comment #20)
> John, Stephan: thanks for digging into this!
> 
> At first glance, it looks like the approach in John's patch is the right
> one. It does not look like it would cause regressions in the case of
> non-XSD-based resources. If we merge this on master, would you be willing to
> test nightly builds and confirm this works for you?
> 
> John: before we can merge your code you would need to sign the Eclipse CLA
> (see http://wiki.eclipse.org/CLA) if that's OK with you, and optionally to
> convert your patch into a Gerrit PatchSet (if you want to do it but need
> assistance do not hesitate).

I would be fine with testing my use case on a nightly build until it is successful. I assume you are not asking for a regression test.

Yes, I will sign the CLA and attempt to convert the patch as requested (assuming most or all I need to know can be found from http://wiki.eclipse.org/CLA).
Comment 22 Pierre-Charles David CLA 2015-02-06 04:14:05 EST
(In reply to John Palof from comment #21)
> (In reply to Pierre-Charles David from comment #20)
> > John, Stephan: thanks for digging into this!
> > 
> > At first glance, it looks like the approach in John's patch is the right
> > one. It does not look like it would cause regressions in the case of
> > non-XSD-based resources. If we merge this on master, would you be willing to
> > test nightly builds and confirm this works for you?
> > 
> > John: before we can merge your code you would need to sign the Eclipse CLA
> > (see http://wiki.eclipse.org/CLA) if that's OK with you, and optionally to
> > convert your patch into a Gerrit PatchSet (if you want to do it but need
> > assistance do not hesitate).
> 
> I would be fine with testing my use case on a nightly build until it is
> successful. I assume you are not asking for a regression test.

No, just some informal manual testing to confirm the change, once integrated, actually fixes your use case. We'll rely on our automated test suites to verify it does not cause regressions.

> Yes, I will sign the CLA and attempt to convert the patch as requested
> (assuming most or all I need to know can be found from
> http://wiki.eclipse.org/CLA).

Signing the CLA is mandatory, but simple: it's just a matter of checking a box, if you agree with the terms of course. Converting the patch into a Gerrit review is more involved, but optional. See https://wiki.eclipse.org/Development_Resources/Contributing_via_Git for the details and links.
Comment 23 Stephan Kranz CLA 2015-02-06 07:17:15 EST
(In reply to Pierre-Charles David from comment #20)
> John, Stephan: thanks for digging into this!
> 
> At first glance, it looks like the approach in John's patch is the right
> one. It does not look like it would cause regressions in the case of
> non-XSD-based resources. If we merge this on master, would you be willing to
> test nightly builds and confirm this works for you?
> 
> John: before we can merge your code you would need to sign the Eclipse CLA
> (see http://wiki.eclipse.org/CLA) if that's OK with you, and optionally to
> convert your patch into a Gerrit PatchSet (if you want to do it but need
> assistance do not hesitate).

I can support testing the nightly build, too.

Indeed, applying a fix to Sirius is by far the better approach to get rid of the problem.
Comment 24 John Palof CLA 2015-02-06 18:19:12 EST
I signed the CLA but encountered difficulties creating the patch.  If the attachment to this bug is sufficient, that would be great.  If not I can try and work through my difficulties submitting the patch.
Comment 25 Pierre-Charles David CLA 2015-02-09 03:52:18 EST
(In reply to John Palof from comment #24)
> I signed the CLA but encountered difficulties creating the patch.  If the
> attachment to this bug is sufficient, that would be great.  If not I can try
> and work through my difficulties submitting the patch.

I think you current attachment should be fine. I'll try to integrate it early this week and keep you informed when nightly builds with it are available. Thanks.
Comment 26 Pierre-Charles David CLA 2015-02-10 05:58:17 EST
Pushed a Gerrit review at https://git.eclipse.org/r/#/c/41506/. If the automated tests do not identify any regression, I'll merge it.
Comment 27 John Palof CLA 2015-02-10 09:42:02 EST
Thank you. Any chance this could be put into 2.0.x?   I would like not to have to wait until 3.0 if possible.  I don't see 2.0.3 in the plans but thought I would ask.
Comment 29 Pierre-Charles David CLA 2015-02-10 10:42:32 EST
(In reply to John Palof from comment #27)
> Thank you. Any chance this could be put into 2.0.x?   I would like not to
> have to wait until 3.0 if possible.  I don't see 2.0.3 in the plans but
> thought I would ask.

Merged on master by 7b189cc2af24afe3e5fab8d98c14d62e8492f5d3 for Sirius 3.0M6.

Actually, we already release Sirius 2.0.3 (see https://wiki.eclipse.org/Sirius/2.0.3), but I had completely forgotten to add the record (fixed now: https://projects.eclipse.org/projects/modeling.sirius/releases/2.0.3).

We have a 2.0.4 bugfix release planned for this friday (https://projects.eclipse.org/projects/modeling.sirius/releases/2.0.4), but I'm not sure about including this fix in it, as we're normally freezed. I see no problem in including this in a 2.0.x in principle, but I'm just not sure we will have time to backport it and properly test it this week. We'll try, but it might need to wait for a future 2.0.5.
Comment 30 Pierre-Charles David CLA 2015-02-11 05:28:47 EST
FYI, the latest nightly builds available at http://download.eclipse.org/sirius/updates/nightly/latest/luna contain John's fix.
Comment 31 Pierre-Charles David CLA 2015-02-11 05:30:36 EST
Cloned as bug 459636 to backport on 2.0.x.
Comment 32 John Palof CLA 2015-02-11 09:27:41 EST
My use-case tested successfully with the nightly build 3.0.0.2015.02111402.

I was able to create a representation over a XSD generated eCore model, save it, close it, and re-open it without encountering the "Impossible to find an interpreter - Could not find a session for model element : ..." error which I was encountering prior to the fix.  I confirmed the model appeared as expected in the Model Explorer and properties surfaced in the Property view.

Thanks!
Comment 33 Pierre-Charles David CLA 2015-02-16 10:07:00 EST
(In reply to John Palof from comment #32)
> My use-case tested successfully with the nightly build 3.0.0.2015.02111402.
> 
> I was able to create a representation over a XSD generated eCore model, save
> it, close it, and re-open it without encountering the "Impossible to find an
> interpreter - Could not find a session for model element : ..." error which
> I was encountering prior to the fix.  I confirmed the model appeared as
> expected in the Model Explorer and properties surfaced in the Property view.

Thanks for the confirmation (and for the contribution itself!). I've created a clone to backport this into a future 2.0.5 release, which should normally happen in the next few weeks (no precise date yet).
Comment 34 Cedric Gava CLA 2015-02-17 05:03:29 EST
Thank you for the bug fix
Comment 35 Stephan Kranz CLA 2015-02-17 10:53:31 EST
I did not test the nightly build, but I have copied the changes in the DAnalysisSessionImpl class to the Sirius 3.0.0 M5 sources I am currently using and it seems to work fine (after removing the work-arounds in my EMF generated model sources).

A side effect I have noticed is that when modifying a diagram and saving it, only the aird file gets saved, but not the semantic resource, because the isModified flag of the semantic resource does not get set. I need to look deeper into this (and why it worked before), but apparently the eResource property is null for all resource content expect for the DocumentRoot element. 

For the time being I am manually setting the resource to modified when receiving a notifyChanged() call at my EContentAdapter classes.
Comment 36 Pierre-Charles David CLA 2015-02-17 11:43:59 EST
(In reply to Stephan Kranz from comment #35)
> I did not test the nightly build, but I have copied the changes in the
> DAnalysisSessionImpl class to the Sirius 3.0.0 M5 sources I am currently
> using and it seems to work fine (after removing the work-arounds in my EMF
> generated model sources).
> 
> A side effect I have noticed is that when modifying a diagram and saving it,
> only the aird file gets saved, but not the semantic resource, because the
> isModified flag of the semantic resource does not get set. I need to look
> deeper into this (and why it worked before), but apparently the eResource
> property is null for all resource content expect for the DocumentRoot
> element. 
> 
> For the time being I am manually setting the resource to modified when
> receiving a notifyChanged() call at my EContentAdapter classes.

Thanks for the feedback! I'm reopening the ticket for further investigations. I'm not sure when we will be able to look into this, so if you continue your tests, feel free to add any new insight here.
Comment 37 Stephan Kranz CLA 2015-02-17 12:29:08 EST
Problem seems to be that in my XML ecore the Transient property of the XXX reference of the XXXDocument class for the XXX class is set true (XXX being the name of the main model, e.g. Family). In the provided family example this flag is also set to true and basically prevents the XXXDocument class from being serialised.

From debugging I see that upon diagram modification Sirius collects the modifed resources using the org.eclipse.sirius.business.internal.resource.ResourceModifiedFieldUpdater class and collects those with the handleResourceChange(Resource resource, Notification notification) method. I can see this method being called multiple times for the aird resource as well as for the semantic resource. One criteria however for a resource to be added to the changedResources collection is NotificationQuery(notification).isTransientNotification() == false. This check eventually ends up in the following loop:

       while (current.eContainer() != null) {
            if (current.eContainingFeature().isTransient()) {
                return true;
            }
            current = current.eContainer();
       }
       return false

For any element in the semantic model the loop continues until the main model element (e.g. Family). There it checks against the above mentioned XXX reference of the XXXDocument which is transient indeed. Thus it returns true, fails the criteria to be added to the changedResources and thus the semantic resource is not set to be modified and does not get saved (only the aird file is).

With my previous workaround in basically removing the XXXDocument elements and adding the XXX element directly to the resource, this problem did not occur, since the XXX element does not have a further containing element, the loop stops and false is returned.

I am actually wondering, whether the others experience the same problem?
Comment 38 John Palof CLA 2015-02-17 13:31:01 EST
(In reply to Stephan Kranz from comment #37)
> ...
> I am actually wondering, whether the others experience the same problem?

I re-ran my use-case in an environment which still had the nightly build 3.0.0.2015.02111402.  However, this time rather than just confirming I could establish and reopen a viewpoint, I made a change in the diagram that should have effected the semantic resource.  Upon saving the diagram, I did NOT see the change reflected in the resource.

However, in my 2.0 based development environment (where I am doing my development and testing), which has the hand modified DAnalysisSessionImpl.doAddSemanticResource (which was attached to this defect), the resource DID change.  Seems odd it would be a 2.0 vs 3.0 thing.
Comment 39 Pierre-Charles David CLA 2015-02-18 05:04:47 EST
(In reply to John Palof from comment #38)
> (In reply to Stephan Kranz from comment #37)
> > ...
> > I am actually wondering, whether the others experience the same problem?
> 
> I re-ran my use-case in an environment which still had the nightly build
> 3.0.0.2015.02111402.  However, this time rather than just confirming I could
> establish and reopen a viewpoint, I made a change in the diagram that should
> have effected the semantic resource.  Upon saving the diagram, I did NOT see
> the change reflected in the resource.
> 
> However, in my 2.0 based development environment (where I am doing my
> development and testing), which has the hand modified
> DAnalysisSessionImpl.doAddSemanticResource (which was attached to this
> defect), the resource DID change.  Seems odd it would be a 2.0 vs 3.0 thing.

Not really surprising actually: we completely rewrote the default saving policy for 3.0 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=430724), and this includes how we determine if a resource is modified (and must be saved) or not. Actually it was rewritten for 2.0 but the new implementation was still considered too new and not enabled by default for 2.0. It was enabled early in the 3.0 cycle.

John, it looks like your patch could actually be backported in 2.0.x (with some more testing however), since it seems to work fine with the saving policy used there. For it to work in 3.0 we'll need a little more work to correctly detect changes in XSD-based semantic resources. Stephan's comment above seems to point to a good candidate where some adjustments are needed.
Comment 40 John Palof CLA 2015-02-18 19:03:14 EST
I did some more studying today on the topic guided by Stephans findings (i.e. NotificationQuery.isTransientNotification) and think that the isTransientNotification code should step around feature maps.

Here is why (referencing https://www.eclipse.org/modeling/emf/docs/overviews/XMLSchemaToEcoreMapping.pdf).

1) In section 1.5 the document explains why there is a DocumentRoot.  The documentRoot is a mixed complex type which allows maintaining comments and white space that appear before the root element. 

2) Section 3.4 specs out what a mixed complex type looks like.  That spec agrees with the DocumentRoot found in the xsd->ecore generated file.  It shows that the EReferences and EAttributes will be marked as derived and transient since there is no DocumentRoot element to hold the reference.

The conclusion I am coming to is that the transient test could be a problem when persisting data that uses feature maps and not just for XSD generated ecore models.  I think the transient check be limited to non-feature map. More on feature maps can be found at: https://www.eclipse.org/modeling/emf/docs/overviews/FeatureMap.pdf

Code similar to the following could be used in NotificationQuery. It appears to do no harm in my 2.0 environment but recall that only 3.0 appeared to have the problem of not serializing.  I have not tried in a 3.0 environment.

private boolean isContainedThroughTransientFeature(EObject obj) {
    EObject current = obj;
    while (current.eContainer() != null) {
        // Added !isFeatureMap test
        if (current.eContainingFeature().isTransient() && 
               !isFeatureMap(current.eContainer())) {
            return true;
        }
        current = current.eContainer();
    }
    return false;
}

/**
 * Return true if the EObject is a feature map as defined in section 3.5 of https://www.eclipse.org/modeling/emf/docs/overviews/XMLSchemaToEcoreMapping.pdf
 * @param obj
 * @return
 */
private boolean isFeatureMap(EObject obj){
    EAnnotation annotation = obj.eClass().getEAnnotation("http:///org/eclipse/emf/ecore/util/ExtendedMetaData");
    if (annotation!=null) {
        if (annotation.getDetails()!=null) {
            String kind = annotation.getDetails().get("kind");
            // Is it sufficient to test kind.equals("mixed")?
            if (kind!=null && kind.equals("mixed")) {
                // If not (or just to be safe), check if type is EFeatureMapEntry

                // Not exactly sure the difference between getEAllAttributes and getEAttributes.
                EList<EAttribute> attribs = obj.eClass().getEAllAttributes();  
                for (EAttribute eAttribute : attribs) {
                    if (eAttribute!=null && eAttribute.getEType().equals(EcorePackage.eINSTANCE.getEFeatureMapEntry())) {
                        return true;
                    }
                }
            }
        }
    }
    return false;
}
Comment 41 Pierre-Charles David CLA 2015-02-27 04:21:47 EST
Hi. Sorry for the lack of reaction recently, I'm busy with other stuff right now. I've not forgotten about it though, and hopefully I'll be able to reactivate this subject sometimes next week.

FWIW, I've seen NPEs with the (modified) version of the patch I've pushed to master with a different XSD-based model than the one attached here.

This part:

  if (!"mixed".equals(name) && !"xMLNSPrefixMap".equals(name) && !"xSISchemaLocation".equals(name)) {
    root = (EObject) root.eGet(eFeature);
    break;
  }

produces a null value for "root" on that model. I have not yet investigated further.
Comment 42 Pierre-Charles David CLA 2015-03-12 11:09:32 EDT
I'm reassigning the bug to Mikael who will handle the subject from now on.
Comment 43 Eclipse Genie CLA 2015-03-13 05:13:08 EDT
New Gerrit change created: https://git.eclipse.org/r/43777
Comment 45 Eclipse Genie CLA 2015-03-16 06:30:54 EDT
New Gerrit change created: https://git.eclipse.org/r/43917
Comment 46 Eclipse Genie CLA 2015-03-16 06:30:56 EDT
New Gerrit change created: https://git.eclipse.org/r/43916

WARNING: this patchset contains 46586 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 47 Eclipse Genie CLA 2015-03-16 06:30:58 EDT
New Gerrit change created: https://git.eclipse.org/r/43918
Comment 48 Eclipse Genie CLA 2015-03-16 06:57:48 EDT
WARNING: this patchset contains 46586 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 49 Eclipse Genie CLA 2015-03-17 08:58:38 EDT
WARNING: this patchset contains 46609 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 50 Eclipse Genie CLA 2015-03-17 11:01:55 EDT
WARNING: this patchset contains 46609 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 51 Eclipse Genie CLA 2015-03-18 04:52:04 EDT
WARNING: this patchset contains 46609 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 52 Eclipse Genie CLA 2015-03-19 06:47:37 EDT
WARNING: this patchset contains 46586 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 53 Eclipse Genie CLA 2015-03-19 06:47:48 EDT
New Gerrit change created: https://git.eclipse.org/r/44157
Comment 54 Eclipse Genie CLA 2015-03-19 07:19:13 EDT
WARNING: this patchset contains 46586 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 55 Eclipse Genie CLA 2015-03-19 08:33:50 EDT
WARNING: this patchset contains 46609 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 57 Eclipse Genie CLA 2015-03-19 12:04:49 EDT
Gerrit change https://git.eclipse.org/r/43916 was merged to [master].
Commit: http://git.eclipse.org/c/sirius/org.eclipse.sirius.git/commit/?id=e7d0fd6932a2b077841ba2716fe595f0fb0dbab6

WARNING: this patchset contains 46609 new lines of code and may require a Contribution Questionnaire (CQ) if the author is not a committer on the project. Please see:https://wiki.eclipse.org/Project_Management_Infrastructure/Creating_A_Contribution_Questionnaire
Comment 60 Maxime Porhel CLA 2015-03-19 12:08:35 EDT
Corrected. See Genie's commits.
Comment 61 Eclipse Genie CLA 2015-03-30 09:42:40 EDT
New Gerrit change created: https://git.eclipse.org/r/44846
Comment 63 Pierre-Charles David CLA 2015-06-24 11:15:05 EDT
Available in Sirius 3.0.0. See https://wiki.eclipse.org/Sirius/3.0.0.