Bugzilla – Full Text Bug Listing
| Summary: | Mismatch between CDO_RESOURCE__RESOURCE_SET and RESOURCE__RESOURCE_SET | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Brendan <bberry> | ||||||||
| Component: | cdo.core | Assignee: | Martin Fluegge <martin.fluegge> | ||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | martin.fluegge, stepper, Thomas.M.Crockett, vroldanbet | ||||||||
| Version: | 4.0 | Flags: | stepper:
review+
|
||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Brendan
I guess the root cause is that the feature IDs in a CDOResource are different from those in a normal EMF resource. Martin, what are your experiences from Dawn? Vik, can you remember the outcome from former discussions about this? Have we already discussed the option of turning some EAttributes in CDOResourceNode (and maybe in CDOResource) into EOperations with the goal of having the same feature IDs in CDOResource that are expected by resource adapters? What do you think? > Vik, can you remember the outcome from former discussions about this? Have we
> already discussed the option of turning some EAttributes in CDOResourceNode
> (and maybe in CDOResource) into EOperations with the goal of having the same
> feature IDs in CDOResource that are expected by resource adapters? What do you
> think?
Hmm, would that impact our UI integration, like the TreeViewers?
I have never seen this exception on Dawn. Unfortunately I am currently facing another problem on closing a transaction. But this seems to be more GMF related. So it might be that I cannot see this exception because it is hidden behind my other problem. I need to do some debugging to make sure that this problem does not occur on Dawn. I can tell more if I have fixed the other stuff. Sad to say I am bit short of time these days. But I値l try to do this asap. Martin, we have had to overcome a handful of issues, mostly related to closing transactions, but as of this afternoon we are able to use CDO to store our GMF models without any obvious functional problems. Unfortunately my solution to the issue reported here is very hackish. I overrode the method "DiagramEditingDomain.broadcastUnbatched(Notification)" in the following manner:
public void broadcastUnbatched(Notification notification) {
Object notifier = notification.getNotifier();
Object feature = notification.getFeature();
if (notifier instanceof Resource && feature != null && feature.equals(EresourcePackage.Literals.CDO_RESOURCE__RESOURCE_SET)) {
notification = new ENotificationImpl((InternalEObject)notification.getNotifier(), Notification.SET, Resource.RESOURCE__RESOURCE_SET, notification.getOldValue(), notification.getNewValue());
}
super.broadcastUnbatched(notification);
}
I did so by contributing my own editing domain factory through the associated extension point. Additionally, I was forced to override "DiagramEditingDomainFactory.configure(TransactionalEditingDomain)" to install a custom "PathMapManager" in order to prevent a ClassCastException when removing resources from resource sets -- which, notably, is the same scenario as described in this bug. I will open a bug report for that and other exceptions tomorrow. I might generate patches for the fixes that we made, if there is interest, but these are VERY interim solutions.
As an aside, we are using some parts of Dawn by adding a fragment based upon the code in the "reference editor" example.
Hi Brendan, the ClassCastExeption in the Pathmanager was actually the problem I had yesterday. I also thought about changing the PathManager implementation but had not time to this yesterday evening. Glad that you solved it. This saves me a lot of time :) Cool that you are using Dawn. Every fix is certainly absolutely welcome. I am also very interest in learning how you solved your problems. Btw. Dawn also comes with an own TransactionaEditingDomain – DawnTransactionalEditingDomainImpl. We are planning to extend this (maybe with another prefix) to provide a CDO-based TED. I am not sure whether you know that we are going to release the Dawn 0.2 soon. Recently I finished the getting started for this version (http://wiki.eclipse.org/Getting_Started_with_Dawn) Dawn also comes with a generator for the fragment. I am currently on travel, so a graphical tutorial will not be finished before next week. :( But the steps to use the generator is rather simple: - right click your *.gengmf and choose "Generate Dawn GenModel" - this will create a file called *.dawngenmodel in you model folder. Currently you can use this model to influence naming conventions for the generated code - Now right click the Dawn GenModel end choose "Generate Dawn GMF fragment" If you say that you have a quite complex diagram this could be a good test for the fragment. Hi guys, sorry for the delayed answer > Vik, can you remember the outcome from former discussions about this? Have we > already discussed the option of turning some EAttributes in CDOResourceNode > (and maybe in CDOResource) into EOperations with the goal of having the same > feature IDs in CDOResource that are expected by resource adapters? What do you > think? We have some bugs opened regarding this issue: 296401: EContentAdapters no longer work on CDO resources https://bugs.eclipse.org/bugs/show_bug.cgi?id=296401 I believe the problem was introduced when the structured CDOResourceNode model was introduced. The new features introduced displaced Resource EStructuralFeatures IDs, introducing a mismatch. between Resource and CDOResource. Using EOperations would work, but not sure how would that impact when dealing when CDOResourceNode reflectively. Fortunately, EMF introduced recently reflective EOperation call (EObject.eInvoke). We could experiment and see what happens :P This issue is old and must be fixed somehow, sooner or latter, as we are compromising EMF compliance. If I get some time I'll try to modify the eresource.ecore. (In reply to comment #6) > We have some bugs opened regarding this issue: > > 296401: EContentAdapters no longer work on CDO resources > https://bugs.eclipse.org/bugs/show_bug.cgi?id=296401 I think we should extend EContentAdapter in a way that our implementation can handle both normal EMF resources and CDO resources properly. > I believe the problem was introduced when the structured CDOResourceNode model > was introduced. The new features introduced displaced Resource > EStructuralFeatures IDs, introducing a mismatch. between Resource and > CDOResource. Yes. > Using EOperations would work, What about change notification? > but not sure how would that impact when dealing > when CDOResourceNode reflectively. Fortunately, EMF introduced recently > reflective EOperation call (EObject.eInvoke). I don't see how that's related. CDOResourceXyz is generated anyway. (In reply to comment #7) > (In reply to comment #6) > > We have some bugs opened regarding this issue: > > > > 296401: EContentAdapters no longer work on CDO resources > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=296401 > > I think we should extend EContentAdapter in a way that our implementation can > handle both normal EMF resources and CDO resources properly. The problem is not the possibility or not to have a CDOContentAdapter (which, by the way, we have implemented, and should contribute somewhere in the near future :P). The problem is when integrating with other frameworks that already use EContentAdapter. In such cases we dont' have a way to hook a CDOContentAdapter there without modifying the code! > > Using EOperations would work, > > What about change notification? > Argh, we would loose those :( > > but not sure how would that impact when dealing > > when CDOResourceNode reflectively. Fortunately, EMF introduced recently > > reflective EOperation call (EObject.eInvoke). > > I don't see how that's related. CDOResourceXyz is generated anyway. Well, my point was that we have a reflective mechanism to access these operations now. No really related, but good have there if we need to look for workarounds when getting the actual values ;) Rebasing all outstanding 3.0 problem reports to version 3.0.1 Fixing wrong bug version. This same issue is still outstanding, and also causes the following exception: !ENTRY org.eclipse.emf.transaction 4 45 2010-08-11 17:39:16.529 !MESSAGE Uncaught exception during post-commit listener notifications !STACK 0 java.lang.IllegalStateException at org.eclipse.emf.common.notify.impl.NotificationImpl.getOldBooleanValue(NotificationImpl.java:883) at org.eclipse.gmf.runtime.emf.core.resources.GMFResourceModificationManager$1.matches(GMFResourceModificationManager.java:130) at org.eclipse.emf.transaction.NotificationFilter$13.matches(NotificationFilter.java:291) at org.eclipse.emf.transaction.impl.FilterManager.select(FilterManager.java:88) at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl$1.run(TransactionalEditingDomainImpl.java:775) at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.runExclusive(TransactionalEditingDomainImpl.java:328) at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.postcommit(TransactionalEditingDomainImpl.java:771) at org.eclipse.gmf.runtime.diagram.core.DiagramEditingDomainFactory$DiagramEditingDomain.postcommit(DiagramEditingDomainFactory.java:245) at com.harmonia.davinci.core.model.cdo.DaVinciDiagramEditingDomainFactory$DaVinciDiagramEditingDomain.postcommit(DaVinciDiagramEditingDomainFactory.java:232) at org.eclipse.emf.transaction.impl.TransactionalEditingDomainImpl.deactivate(TransactionalEditingDomainImpl.java:543) at org.eclipse.emf.transaction.impl.TransactionImpl.close(TransactionImpl.java:712) at org.eclipse.emf.transaction.impl.TransactionImpl.commit(TransactionImpl.java:474) at org.eclipse.emf.workspace.AbstractEMFOperation.execute(AbstractEMFOperation.java:155) at org.eclipse.core.commands.operations.DefaultOperationHistory.execute(DefaultOperationHistory.java:511) at org.eclipse.emf.workspace.impl.WorkspaceCommandStackImpl.doExecute(WorkspaceCommandStackImpl.java:208) at org.eclipse.emf.transaction.impl.AbstractTransactionalCommandStack.execute(AbstractTransactionalCommandStack.java:165) at org.eclipse.emf.transaction.impl.AbstractTransactionalCommandStack.execute(AbstractTransactionalCommandStack.java:219) at com.harmonia.davinci.navigator.job.DeleteEObjectJob.runInWorkspace(DeleteEObjectJob.java:83) at org.eclipse.core.internal.resources.InternalWorkspaceJob.run(InternalWorkspaceJob.java:38) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) I managed to temporarily avert the latter exception by providing my own EditingDomain implementation and overriding the postCommit() method as follows:
protected void postcommit(InternalTransaction tx) {
List<Notification> notifications = getValidator().getNotificationsForPostcommit(tx);
for (Notification notification : notifications) {
Object notifier = notification.getNotifier();
Object feature = notification.getFeature();
if (notifier instanceof Resource && feature != null && feature.equals(EresourcePackage.Literals.CDO_RESOURCE__RESOURCE_SET)) {
List<Notification> txNotifications = tx.getNotifications();
int indexOf = txNotifications.indexOf(notification);
notification = new ENotificationImpl((InternalEObject)notification.getNotifier(), Notification.SET, Resource.RESOURCE__RESOURCE_SET, notification.getOldValue(), notification.getNewValue());
txNotifications.set(indexOf, notification);
}
}
super.postcommit(tx);
}
Naturally this is not a very pretty fix for the long-term.
Hi Brendan, sorry that this one is still outstanding. We will address the problem as soon as we find some time for it. Just for your information: The Exception mentioned in your first post is treadted by Bug 322726. Altough the root cause belongs to this Bugzilla the exception is caused by a GMF (or better EMT Transaction) related action. If you like you could try the patch there and comment on 322726 whether this interim soultion works for you, too. (In reply to comment #13) > sorry that this one is still outstanding. We will address the problem as soon > as we find some time for it. This is not just a matter of time (although that is a big matter here because a real fix would take weeks). The root cause can only be fixed by making CDOResource no longer extend EObject and that would have a trmendous effect on all existing clients. I must admit that I'm really not sure how to deal with this issue ;-( *** Bug 296401 has been marked as a duplicate of this bug. *** Created attachment 177077 [details]
Test v1
I created a small test which reproduces the problem and attached it.
Created attachment 177085 [details]
Patch v1
I look a bit more detailed to this problem and might have found a solution for it.
Basically this problem is quite similar to multiple class inheritance, so I treated it accordingly.
In fact I did pretty much the same EMF would do. I just overrode the *eDerivedStructuralFeatureID(int, Class)* and *eBaseStructuralfeatureID(int, Class)* methods to provide a mapping between the feature id offsets.
All tests are passing so it looks quite promising that this patch solves the problem.
Sounds very promising, Martin!!! Am I supposed to review it? > Am I supposed to review it?
Actually yes. I only spend some time thinking about whether we really need the overwritten *eDerivedStructuralFeatureID(int, Class)* ...and did not come to a conclusion yet ;) (That's why I did not set the review flag)
From the functional side of view this would not make any difference, assuming that the method is always called with CDOResource a base class. But if it is ensured that in our special context it is not needed, we could just avoid the method and delegate the work to EBasicEObjectImpl. This would save at least one 'if' check.
Since retrieving the feature id is an often used feature, using *eDerivedStructuralFeatureID(int, Class)* (if it is not needed) could decrease the performance a bit.
Maybe I am to accurate on this. So, just go ahead and review the patch. Maybe we can care for performance issues later ;)
Created attachment 178672 [details]
Patch v2 - ready to be committed
I added a small fix to CDOLeagcyWrapper
Some parts of the patch v2 were already committed, so I excluded them from the commit...Apart from that: patch v2 committed to HEAD (In reply to comment #21) > Some parts of the patch v2 were already committed, so I excluded them from the > commit...Apart from that: patch v2 committed to HEAD Can you please place the text "Committed to HEAD" at least on a separate line so that it's easier to find it ;-) Worse, I wonder if you really committed to HEAD, given this is a 3.0 bug... To clarify the confusion: This bug is a 4.0 bug and we just forgot to change the version. So committing to HEAD was correct. Eike, next time I'll put the comment on an extra line, promised ;) Available in R20110608-1407 |