Bug 396735

Comment 1 Mathieu Velten 2012-12-17 06:04:39 EST

fixed on 0.8.X-EYY, to be backported.

Comment 2 Christian Damus 2013-03-15 17:42:30 EDT

The r10551 commit totally breaks the CDO integration feature:

  * the updates to the org.eclipse.papyrus.cdo.core plug-in for API refactorings
    in the Papyrus core were committed only to the cdo_kepler branch, so the
    trunk build is broken

  * the org.eclipse.papyrus.cdo.core.tests plug-in has a compilation error
    on the refactored API

  * the ReadOnlyAdapterFactory is not registered on the adapters extension
    point, leading to failure to detect any read-only conditions in anything

  * the Papyrus All CDO Tests suite runs clean on r10550; on r10551 it
    has 23 failures and errors

  * opening a repository model in the editor produces a "corrupted model"
    dialog with a very strange CDO error.  The diagram doesn't show any of
    its views and the palette has all of the MARTE tools in models that don't
    have the MARTE profile applied

The CDO integration feature is unusable until these problems are fixed.

So far, it seems that the problem is *not* the introduction of the ProxyModificationTrackingAdapter, which was my first guess (based on known issues of EContentAdapters in a CDO context).

Comment 3 Christian Damus 2013-03-15 18:14:37 EDT

The cause of the most severe problems (failure to open editors) is in the ModelSet's setting of modification tracking on its resources:

-------- 8< --------

	protected Resource setResourceOptions(Resource r) {
		if (r != null && !r.isTrackingModification()) {
			r.setTrackingModification(true);
		}
		return r;
	}

-------- >8 --------

If I block modification-tracking in the PapyrusDawnResourceImpl, then the CDO test suite runs green and opening editors doesn't present issues.

Modification-tracking in CDO is known to have issues:

    http://www.eclipse.org/forums/index.php/t/457863/

So, I'll see what I can do about that in CDO.

But, even so, we now have no less than three EContentAdapters listening for changes on all objects in every Papyrus ResourceSet:

  * the UML CacheAdapter
  * the ProxyModificationTrackingAdapter
  * the various adapters that resources use for modification-tracking

And maybe we even get GMF's CrossReferenceAdapter in the mix, too!

Comment 4 Christian Damus 2013-03-15 18:39:52 EDT

I've committed r10554 to rescue the CDO integration feature:

  * fix compilation error in test plug-in on refactored read-only API

  * add missing registration of the IReadOnlyHandler adapter for editing domain

  * block attempts to set modification tracking on CDO resources in a ModelSet

*Note* that I have not merged the r10554 to trunk, because the cdo_kepler branch has loads of other changes that have not yet been reviewed.  So, the trunk build of the CDO plug-ins is still broken on the read-only API changes.

Fixing resource modification-tracking in CDO is not an easy undertaking.  What exactly is the reason why the ModelSet sets modification tracking in its resources?  How important will it be for models in CDO repositories?

Comment 5 Mathieu Velten 2013-03-18 05:52:06 EDT

Hi,

Regarding the tracking of modification :
I need that to check in save() that all modified resources are not read only before trying to save them. This is to ensure consistent and atomic save, instead of having a half-saved model like before when we encounter a read only resource.

I introduced on my local tree is/setTrackingModification on the ModelSet which used a similar implementation of is/setTrackingModification in ResourceImpl. I will commit after M6.

Comment 6 Christian Damus 2013-03-18 07:04:48 EDT

(In reply to comment #5)
> Hi,
> 
> Regarding the tracking of modification :
> I need that to check in save() that all modified resources are not read only
> before trying to save them. This is to ensure consistent and atomic save,
> instead of having a half-saved model like before when we encounter a read
> only resource.

Thanks, Mathieu. This means that we don't need the modification tracking on CDO resources, which is good news.

CDO resources "save" by committing the transaction, which of course saves all changes made in all resources to the database. Commit has no effect on objects/resources that were not modified and read-only objects/resources aren't editable by the user anyways.


> I introduced on my local tree is/setTrackingModification on the ModelSet
> which used a similar implementation of is/setTrackingModification in
> ResourceImpl. I will commit after M6.

Are you sure? The Papyrus project plan lists M6 as the API Freeze milestone. This would change the API.

Comment 7 Camille Letavernier 2013-03-20 11:32:53 EDT

Initially posted on Bug 397758:

r10626:

I've removed the "throw exception" in one specific case. Papyrus doesn't fully support read only (It may happen that a specific action changes a read only element, although I don't know why). Thus, the following lines in ModelSet:

if(authorizeSave.isPresent() && !authorizeSave.get()) {
monitor.done();
throw new IOException("Some modified resources are read-only : the model can't be saved");
}

prevents saving the model. Instead of potential model corruption, we have systematic data loss (i.e. cannot save, and we don't even know it/why).

Especially, I have this exception when trying to save a simple model with a local profile applied. I open the model, move a class, save: OK. I create a Class, save: read-only save exception.

In some (unidentified) cases, this "throw exception" can make the editor unusable. On the other hand, it is really unlikely that we actually modified StandardL3 by creating a simple Class, so saving is probably safe.

Actions to be taken:

- Warn the user instead of throwing a silent exception
- Understand why the StandardL3 has been modified

In the specific case of applied profiles (Either local or static), it is not possible to make the resource writable (Simply because the ReadOnlyHandler doesn't permit it, even for local profiles: it doesn't make sense to modify the applied profile from the applying model).

Comment 8 Camille Letavernier 2013-03-20 11:42:23 EDT

> Especially, I have this exception when trying to save a simple model with a local profile applied. I open the model, move a class, save: OK. I create a Class, save: read-only save exception.

Note that I cannot always reproduce this case. It is systematic with this model, but works fine for other models.

Comment 9 Mathieu Velten 2013-03-20 11:48:00 EDT

can you provide the problematic model and the modification to do to trigger the problem ?

Comment 10 Camille Letavernier 2013-03-20 12:09:29 EDT

Created attachment 228720 [details]
Model to reproduce the issue

To reproduce the issue:

- Open the model
- Do no refresh the applied profile (it has been redefined)
- Move an element
- Save: OK
- Create an element
- Save: Fails (StandardProfileL3 has been modified and is read-only)

Comment 11 Mathieu Velten 2013-03-26 10:32:11 EDT

I just commited some improvements, cf comments on rev 10684 for more infos.

Regarding your issue I can't reproduce, and my changes should not have fixed it (or by accident :) ).

Comment 12 Christian Damus 2013-03-26 11:09:18 EDT

I've committed r10688 to fix the compilation error in the org.eclipse.papyrus.cdo.core.tests plug-in caused by the r10684 API refactoring.

Comment 13 Camille Letavernier 2015-01-30 07:38:31 EST

I haven't seen a save issue for a while, since the ReadOnly support has been implemented

I close the task