Bugzilla – Full Text Bug Listing
| Summary: | CDODeltaNotification.getNewValue() returns a CDOIDExternal instead of the EObject from the containing XMIResource | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Esteban DUGUEPEROUX <esteban.dugueperoux> | ||||||||||||||||||
| Component: | cdo.core | Assignee: | Eike Stepper <stepper> | ||||||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||||||||||
| Severity: | normal | ||||||||||||||||||||
| Priority: | P3 | CC: | steve.monnier | ||||||||||||||||||
| Version: | 4.2 | ||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Esteban DUGUEPEROUX
Created attachment 206105 [details]
Test case
Created attachment 206106 [details]
A possible fix
In addition with the attached test case, the Adapter receives 3 Notifications, a first with a feature == null, a second with a getNewValue() == null and the third with getNewValue() == CDOIDExternal. Then it seems have issue with the NotificationBuilder because the Adapter should receives only one Notification concerning it. Also using a H2 backend we have a exception from DBStoreAccessor.getObjectType() : "throw new IllegalStateException("No type found for " + id);"
Please indicate on the bugzilla what CDO version you've created the patch against! Generally please create workspace-rooted patches. As of now the patch does not match any version. In 4.0 this may be caused by the fix to bug 363460. Can you create a new patch or point me to a public (github?) clone of the repo with a branch that contains your changes? Created attachment 206895 [details] Patch including a partial fix + test I have attached a patch (362270.patch) done with EGit relative to the git repo root done from the 4.0-maintenance branch before the fix of https://bugs.eclipse.org/bugs/show_bug.cgi?id=363460 Created attachment 207581 [details]
Test v2
With your test case I can not make the notifications provide a CDOIDExternal. Can you confirm this with the new test case version?
Next, I'd like to clarify my understanding of the test case logic and our expectations regarding CDO behaviour. If I get everything right the test executes a RemoveCommand through a transactional editing domain and at that time there's a ResourceSetListener attached which leads to a rollback on EMFTransaction level. Model-wise there should be no changes in the EObjects after execution of that command. Right?
After that you're executing another command that does nothing but rolling back the underlying CDOTransaction to the CDOSavepoint that has been set just before the (failed) RemoveCommand has been executed. I would expect to see no notifications on registered adapters because the logical model has not been changed. Right?
Hi, With my test case, the CDOIDExternal as newValue is on the second ENotification received, with your test case, I can see the second assertion failed because the Adapter has received as newValue a CDOIDExternal, then I confirm this with your test case. Yes with the EMFT rollback the removeMartinSupplierCmd should be cancelled and the CDOSavepoint rollback should reset to CLEAN state the cancelled changes then there is no changes after that. Before the removeMartinSupplierCmd execution we have obeoCompany and martinSupplier objects with CLEAN state because of "cdoResource.save(Collections.emptyMap());" , just after the removeMartinSupplierCmd execution these objects are DIRTY, after the CDOSavePoint.rollback() these objects are CLEAN, but during the CDOSavePoint.rollback() the Adapter receives two CDODeltaNotifications on the purchaseOrders feature with a Supplier as notifier, the first notification has null as newValue and the second has CDOIDExternal as newValue. Indeed here we should'nt receives notifications because the removeMartinSupplierCmd is already rollback by EMFT then the CDOSavePoint.rollback() should detects that there is only cdoState to set to clean and no notifications to sent, but in the case that EMFT has not done a rollback, the CDOSavePoint.rollback() should revert the changes of removeMartinSupplierCmd and send CDODeltaNotifications with correct feature/notifier/newValue/... Okay, I forgot to mention that I fixed another bug in CDOSavepointImpl. Without this fix a reattached object (martinSupplier gets reattached by your Rollbacker) was *not* removed from the detachedObjects map. In this case there arrived two notifications in the AssertAdapter, but both with newValue==null. I've never been able to see a CDOIDExternal there and now, with my mentioned fix in place the adapter is not notified at all and according to my previous explanation that's the expected behaviour. Another problemm with your test case is the condition feature.equals(getModel1Package().getSupplier_PurchaseOrders()) in the AssertAdapter. Clearly this check can only be true for a many-valued reference and that can never be equal to a single value(such as a CDOIDExternal). I can neither proof that this value arrives there nor can I even imagine how that should be possible ;-( If you want to test with my small fix to CDOSavepointImpl please replace the reattachedObjects field with this one:
private Map<CDOID, CDOObject> reattachedObjects = new HashMap<CDOID, CDOObject>()
{
private static final long serialVersionUID = 1L;
@Override
public CDOObject put(CDOID key, CDOObject object)
{
detachedObjects.remove(key);
return super.put(key, object);
}
};
The in comment #9 causes regressions. A fix to bug 352977 needed instead. commit 340c5fcd42756fa5e4fefb00742345fcb9e0337b Author: Eike Stepper <stepper@esc-net.de> 2011-12-09 14:14:38 Committer: Eike Stepper <stepper@esc-net.de> 2011-12-09 14:14:38 Parent: 66dca4018564b10f778ff723d363d5ee03363c44 (Fix NPE in RepositoryConfigurator if run standalone) Branches: streams/4.0-maintenance [362270] CDODeltaNotification.getNewValue() returns a CDOIDExternal instead of the EObject from the containing XMIResource https://bugs.eclipse.org/bugs/show_bug.cgi?id=362270 Port to 4.1 via bug 366180. Closing. I reopens this issue because the fix in AbstractCDOView.getObject(CDOID,boolean) should be moved in CDODeltaNotificationImpl.adapt() instead. Created attachment 225875 [details]
A JUnit test to show the regression
I have attached a JUnit test to show the regression, it occurs in legacy. Indeed a external object cannot have a associated CDOLegacyAdapter and it occurs in this test case. This occurs when CDODeltaNotificationImpl are built and that a ContentAdapter use these notifications, a indirect call to CDODeltaNotificationImpl.adapt() is done with a CDOIDExternal then CDOUtil.getCDOObject() call add a associated CDOLegacyWrapper as Adapter.
Created attachment 225876 [details]
A patch to fix the regression.
A patch to fix the regression.
The patch did not apply (partly because of the commit for bug 376620) but I was able to adjust it manually. Please don't do the following things in tests: 1) Don't access XyzFactory.eINSTANCE or XyzPackage.eINSTANCE because that prevents legacy tests. Use getXyzFactory() or getXyzPackage() instead. 2) Don't create files outside of the system temp folder. Use this instead: File localResourceFile = createTempFile("localResource", ".model1"); 3) Don't use view.getResource(), view.createResource(), etc. without getResourcePath(). The test repos will reject commits otherwise. 4) Don't call CDOUtil.setLegacyModeDefault(). Instead execute a legacy test suite such as "CDO AllTests (MEM legacy)". Thanks ;-) BTW. I had to rewrite Bugzilla_362270c_Test almost completely. Please check carefully whether it still tests what you wanted it to test! commit 00275a1321c9da35e59ec5fd4b81a215f4c2c1fe Thanks for the feedback, but there is yet a issue in AbstractCDOView.getObject(CDOID,boolean). If I call this method with a CDOIDExternal in legacy, a CDOLegacyWrapper will be added as adapter to the corresponding not shared EObject because of call to CDOUtil.getCDOObject(). A not shared EObject with a CDOLegacyWrapper as adapter can be considered as a CDOObject in some other part of code which can lead to issue like the last attached test case. I think the block about CDOIDExternal should be removed. That's probably because I couldn't apply your patch. Can you please attach a new patch with the new changes you're proposing? Created attachment 225931 [details]
a patch of AbstractCDOView
a patch of AbstractCDOView to remove the block about CDOIDExternal.
Sorry, I must have misunderstood your comment #18. I thought it's about the former change in CDODeltaNotificationImpl. Is it about a totally different thing in AbstractCDOView? I'm reluctant to apply changes in this ultra fundamental class if I don't fully understand why and don't have a test case that demos a real problem. I think you should open a new bugzilla, explain the problem in more detail and attach a simple (i.e. ideally no other frameworks involved, such as a CommandStack or EMF Transaction) test case. Created attachment 226093 [details]
A patch to the Bugzilla_362270b_Test
I have attached a patch to the Bugzilla_362270b_Test test case to show the issue in legacy, the origin of this issue was induces by the initial fix of this bugzilla.
Fixing version: 4.2 Closing |