Community
Participate
Working Groups
Build ID: I20080617-2000 Steps To Reproduce: 1) CDO server contains an EObject, named Folder1, which in turn has a child-object, named SubFolder2. Both are stored in separate resources as Folder1 retains an uncontained reference to SubFolder2. 2) Open app and display Folder1. This inits a cdoSession and creates a transaction. It calls transaction.getResource(Folder1URI.path), finds the resource correctly and this resource is added to the resourceSet. 3) Upon display of Folder1, clicking on it asks it to display its children. This uses the FolderItemProvider.getChildren() (from model.edit project) method to attain its subfolders. getChildren() finds the resource of subFolder2 by calling (eventually) CODViewImpl.getResource(CDOID), which correctly returns the resource containing SubFolder2. However, the transaction's resourceSet still only contains Folder1's resource. It seems SubFolder2's resource exists in isolation. More information: The resource attained by getResource(CDOID) is not properly initialised on account of it does not have its URI set. This means any attempt to add it to the resourceSet fails with a NullPointerException in CDOViewSetImpl.notifyAdd(). Have attempted to patch by adding to CDOViewImpl.cleanObject(): protected void cleanObject(InternalCDOObject object, InternalCDORevision revision) { if (object instanceof CDOResourceImpl) { object.cdoInternalSetResource((CDOResourceImpl)object); CDOPathFeature pathFeature = getSession().getPackageManager().getCDOResourcePackage().getCDOResourceClass() .getCDOPathFeature(); String path = (String)revision.getValue(pathFeature); URI createURI = CDOURIUtil.createResourceURI(this, path); ((CDOResourceImpl)object).setURI(createURI); getResourceSet().getResources().add((CDOResource)object); } else <snip> } However, this causes a further exception when committing a transaction later on so not a worthwhile patch. Either way, resource should be added to resourceSet and it seems in order to successfully do that, the resource should have a valid URI.
Hi Paul, I think it's generally not a good idea to mix non-business aspects into buiness models as in your case the usage of Resource and ResourceSet in your model. Of course its left to your decision ;-) On the other hand it's certainly not a good idea to use internal implementation classes (here: CDOViewImpl). We've spent a lot of effort to provide convenient and flexible APIs as well as a proper API/impl separation. For a particular client application that uses internal code it's usually hard to judge if it's done well at least. And because it would require a deep study of this client application it's usually beyond our ability. I can only strongly recommend to use the public APIs if sufficient or to discuss with us about alternatives if the existing ones are inappropriate.
Eike, I think you misunderstand me. I am proposing to patch the cleanObject method in CDOViewImpl! Not that the patch is correct as I explained. Basically, from what I can discern, any resource obtained from a CDOSession/Transaction using CDOViewImpl.getResource(CDOID), which is what getChildren does, results in an incomplete resource, ie. a resource with no uri. Also, that same resource is never added to the transaction's resourceSet. If a resource is fetched using getResource(String path) then its added to the transaction's resourceSet and has a valid URI. This inconsistency is the problem, not what I wish to do with the resource or resourceSet. Hope that is clearer. PGR (In reply to comment #1) > Hi Paul, > > I think it's generally not a good idea to mix non-business aspects into buiness > models as in your case the usage of Resource and ResourceSet in your model. Of > course its left to your decision ;-) > > On the other hand it's certainly not a good idea to use internal implementation > classes (here: CDOViewImpl). We've spent a lot of effort to provide convenient > and flexible APIs as well as a proper API/impl separation. For a particular > client application that uses internal code it's usually hard to judge if it's > done well at least. And because it would require a deep study of this client > application it's usually beyond our ability. I can only strongly recommend to > use the public APIs if sufficient or to discuss with us about alternatives if > the existing ones are inappropriate. >
(In reply to comment #2) > I think you misunderstand me. That's not impossible. Maybe I should sleep longer ;-9 > I am proposing to patch the cleanObject method in CDOViewImpl! Not that the > patch is correct as I explained. Which patch do you refer to? > Basically, from what I can discern, any resource obtained from a > CDOSession/Transaction using CDOViewImpl.getResource(CDOID), which is what > getChildren does, results in an incomplete resource, ie. a resource with no > uri. I must ask once more: Are you using getResource(CDOID) directly? Or do you mention it because you tracked down a problem and arrived there somehow? It's possible that our tests are not complete, so is it possible that you provide a test case using AbstractCDOTest (and only public CDO API)? The cleanObject method is comparingly new (after an internal refactoring) and I've CC'ed Simon to tell his opinion. Please reopen this Bugzilla when you are able to reproduce the problem as outlined above.
> Which patch do you refer to? this one. This is directly patching the cleanObject method in CDOViewImpl. protected void cleanObject(InternalCDOObject object, InternalCDORevision revision) { if (object instanceof CDOResourceImpl) { object.cdoInternalSetResource((CDOResourceImpl)object); CDOPathFeature pathFeature = getSession().getPackageManager().getCDOResourcePackage().getCDOResourceClass() .getCDOPathFeature(); String path = (String)revision.getValue(pathFeature); URI createURI = CDOURIUtil.createResourceURI(this, path); ((CDOResourceImpl)object).setURI(createURI); getResourceSet().getResources().add((CDOResource)object); } else <snip> } > > > Basically, from what I can discern, any resource obtained from a > > CDOSession/Transaction using CDOViewImpl.getResource(CDOID), which is what > > getChildren does, results in an incomplete resource, ie. a resource with no > > uri. > > I must ask once more: Are you using getResource(CDOID) directly? > Or do you mention it because you tracked down a problem and arrived there > somehow? > > It's possible that our tests are not complete, so is it possible that you > provide a test case using AbstractCDOTest (and only public CDO API)? > > The cleanObject method is comparingly new (after an internal refactoring) and > I've CC'ed Simon to tell his opinion. > > Please reopen this Bugzilla when you are able to reproduce the problem as > outlined above. >
> Which patch do you refer to? this one. This is directly patching the cleanObject method in CDOViewImpl. protected void cleanObject(InternalCDOObject object, InternalCDORevision revision) { if (object instanceof CDOResourceImpl) { object.cdoInternalSetResource((CDOResourceImpl)object); /* I ?added code below */ CDOPathFeature pathFeature = getSession().getPackageManager().getCDOResourcePackage().getCDOResourceClass() .getCDOPathFeature(); String path = (String)revision.getValue(pathFeature); URI createURI = CDOURIUtil.createResourceURI(this, path); ((CDOResourceImpl)object).setURI(createURI); getResourceSet().getResources().add((CDOResource)object); /* Extra code ends */ } else <snip> } > I must ask once more: Are you using getResource(CDOID) directly? > Or do you mention it because you tracked down a problem and arrived there > somehow? > I never call it directly. I have arrived there from calling getChildren() on an edit adapter from my model's generated edit project. > It's possible that our tests are not complete, so is it possible that you > provide a test case using AbstractCDOTest (and only public CDO API)? Certainly can. > The cleanObject method is comparingly new (after an internal refactoring) and > I've CC'ed Simon to tell his opinion. > > Please reopen this Bugzilla when you are able to reproduce the problem as > outlined above. Certainly shall. PGR
Created attachment 113762 [details] Test case extending CDOAbstractTest that should hopefully convey the problem Hopefully demonstrates the problem I am seeing. Not been able to test it due to lack of knowledge in setting up the test suite. Any info on the latter then please can you direct me accordingly. If you find when running that the results are different to those I am expecting then let me know as soon as possible, as this may at least provide me with a base case for where my current model is going wrong. Any further info required, please let me know. Regards PGR
bug reopened upon submission of test case, as discussed. PGR
I had to fix several compile error in your test case and came up with this version: public class IncompleteCDOResource extends AbstractCDOTest { public void testIncompleteResource() throws Exception { { CDOSession session = openModel1Session(); CDOTransaction transaction = session.openTransaction(); CDOResource supplierResource = transaction.createResource("/supplierResource"); CDOResource poResource = transaction.createResource("/poResource"); msg("Creating supplier"); Supplier mySupplier = getModel1Factory().createSupplier(); supplierResource.getContents().add(mySupplier); msg("Creating purchase order"); PurchaseOrder myPurchaseOrder = getModel1Factory().createPurchaseOrder(); poResource.getContents().add(myPurchaseOrder); mySupplier.getPurchaseOrders().add(myPurchaseOrder); transaction.commit(); session.close(); } CDOSession session = openModel1Session(); CDOTransaction transaction2 = session.openTransaction(); CDOResource supplierResource2 = transaction.getResource("/supplierResource"); Supplier savedSupplier = (Supplier)supplierResource2.getContents().get(0); Model1ItemProviderAdapterFactory factory = new Model1ItemProviderAdapterFactory(); ITreeItemContentProvider provider = (ITreeItemContentProvider)factory.adapt(savedSupplier, ITreeItemContentProvider.class); for (Object obj : provider.getChildren(savedSupplier)) { if (obj instanceof PurchaseOrder) { PurchaseOrder po = (PurchaseOrder)obj; /* I believe that the po resource will be set but that URI is null */ System.out.println("po resource: " + po.eResource()); System.out.println("po resource: " + po.eResource().getURI()); } } ResourceSet resSet = transaction2.getResourceSet(); for (Resource res : resSet.getResources()) { /* I believe that only the supplied resource will be in the transaction resourceset */ System.out.println("Resource contained in resourceSet: " + res); } session.close(); } } This one has a single compile error which clearly indicates your problem: You are using the transaction *after* you have closed the underlying session. I've filed bug #248997 so that we better prevent from such misusage in the future.
Created attachment 113782 [details] Correction as patch against HEAD
My apologies Eike, being in a rush has once again caused confusion. Thanks for your help and patience so far. The line transaction.getResource("/supplierResource"); is incorrect, it should read transaction2.getResource("/supplierResource"); (Note the 2) I am not using a transaction after it is closed, that is simply a coding error on my part. This is what is going on: 1) Open a session 2) Open a transaction 3) Create a Supplier in its own resource 4) Create a Purchase Order in its own resource 5) Reference the PO in the supplier 6) Save everything to the CDO server and close transaction and session. That transaction is redundant, ignore it, can be thrown away. 7) Open a brand new session and transaction. Both have no relation to the previous session and transaction, ie. the transaction's resourceSet is new and at this point empty. 8) Load the Supplier using the new transaction. Its resource gets placed in the Transaction's resourceSet. All is fine with this one. No problem. 9) Using the Supplier's ITreeContentProvider, find all of its children. Witness the loading of the Purchase Order in its own resource, which lacks a URI and is never added to the transaction's resourceSet. This is the problem. Does this not happen here? PGR
Created attachment 113818 [details] TestCase version 0.2 Second draft version of test case submitted to codify the scenario laid out in my previous reply above. PGR
Created attachment 113834 [details] Patch v1 - incomplete
Hi Paul, when we fetch a resource from the cdoid directly from getObject(), two things are not working properly : - Uri isn't set properly since it is a transient EStore will not be notified - Resources isn't added to the resourceset. I Your patch is almost good :-) cleanObject should not change state of resources. This will be done in createObject. After I have exception since public String getResourcePath(CDOID id, long timestamp) call readRevisionByTime is called... and should not since I have exception. Eike, should these methods be only under IRevisionManager ? (right now they are under CDORevisionResolver) Should we have them without ByTime as well ? What happens if Audit isn't supported ? It throws an exception :-( Even if we have a View. /** * @since 2.0 */ public CDOID getResourceID(String path); /** * @since 2.0 */ public String getResourcePath(CDOID id); /** * @since 2.0 */ public String getResourcePathByTime(CDOID id, long timeStamp); /** * @since 2.0 */ public CDOID getResourceIDByTime(String path, long timeStamp); I can work for a complete patch tomorrow if you want ? I looked at the code (for 1.0.0 ) and it seems that we never handle that case... it is correct ? Simon
Hi Guys, Sorry Paul, as I said, I didn't want to exclude that we have a bug here but your wrong test case obfuscated the situation ;-( Simon, can you contact me via Skype so that we can dicuss this and an additional problem with removing resources?
No apologies necessary except from me. Still, I have finally now communicated the problem correctly and it seems Simon is on the way to a solution ;) Anything I can do including testing just let me know. PGR
Created attachment 114738 [details] Patch v2 - Incomplete Fixes duplicate and resource not added to the resourceset... the last one will probably changed.
I'm sorry to not give feedback on that bugs but Eike and I I've been discussing many things that are related on that bugs : What do you think of the following: (this is only to start the discussion - read until the end) - Resource that are not added explicitly to the resourceset will not be added automatically to the resourceset. This means that if the following are not called * rset.getResource * rset.createResource * rset.getObject resource will not be added to the resourceset. So If we have the following: A->B (A is a resource) If we load B and call B.eResource(). It will return A but A will not be in the resourceset... Eike, Did I forget something ? Honestly, when I discussed it with Eike I was convinced... but now... it make sense as well to add A automatically to the resourceset. It will feel bizarre that a resource will be accessible without having it in the resourceset. What do you think guys ? Simon
Hi Simon, Can you clarify at what level the explicit call to adding the resource to the resourceSet would be made. For example, this is the stacktrace from calling getChildren() from an ItemProviderAdapter: org.eclipse.emf.internal.cdo.CDOViewImpl.getResource(CDOViewImpl.java:323) org.eclipse.emf.internal.cdo.CDOViewImpl.cleanObject(CDOViewImpl.java:546) org.eclipse.emf.internal.cdo.CDOViewImpl.createObject(CDOViewImpl.java:528) org.eclipse.emf.internal.cdo.CDOViewImpl.getObject(CDOViewImpl.java:432) org.eclipse.emf.internal.cdo.CDOTransactionImpl.getObject(CDOTransactionImpl.java:270) org.eclipse.emf.internal.cdo.CDOViewImpl.convertIDToObject(CDOViewImpl.java:654) org.eclipse.emf.internal.cdo.CDOStore.get(CDOStore.java:162) org.eclipse.emf.internal.cdo.CDOObjectImpl$CDOStoreEList.delegateGet(CDOObjectImpl.java:919) org.eclipse.emf.common.util.DelegatingEList.get(DelegatingEList.java:396) org.eclipse.emf.common.util.DelegatingEList$EIterator.doNext(DelegatingEList.java:1075) org.eclipse.emf.common.util.DelegatingEList$EIterator.next(DelegatingEList.java:1062) org.eclipse.emf.edit.provider.ItemProviderAdapter.getChildren(ItemProviderAdapter.java:340) (nb. line numbers not up to date) Can I expect that the children's resources returned by getChildren() will be in the transaction's resourceSet? I mean that will the call resourceSet.add(child resource) occur somewhere in this stack below getChildren()? regards PGR
(In reply to comment #18) > Hi Simon, > > Can you clarify at what level the explicit call to adding the resource to the > resourceSet would be made. For example, this is the stacktrace from calling > getChildren() from an ItemProviderAdapter: > > org.eclipse.emf.internal.cdo.CDOViewImpl.getResource(CDOViewImpl.java:323) > org.eclipse.emf.internal.cdo.CDOViewImpl.cleanObject(CDOViewImpl.java:546) > org.eclipse.emf.internal.cdo.CDOViewImpl.createObject(CDOViewImpl.java:528) > org.eclipse.emf.internal.cdo.CDOViewImpl.getObject(CDOViewImpl.java:432) > org.eclipse.emf.internal.cdo.CDOTransactionImpl.getObject(CDOTransactionImpl.java:270) > org.eclipse.emf.internal.cdo.CDOViewImpl.convertIDToObject(CDOViewImpl.java:654) > org.eclipse.emf.internal.cdo.CDOStore.get(CDOStore.java:162) > org.eclipse.emf.internal.cdo.CDOObjectImpl$CDOStoreEList.delegateGet(CDOObjectImpl.java:919) > org.eclipse.emf.common.util.DelegatingEList.get(DelegatingEList.java:396) > org.eclipse.emf.common.util.DelegatingEList$EIterator.doNext(DelegatingEList.java:1075) > org.eclipse.emf.common.util.DelegatingEList$EIterator.next(DelegatingEList.java:1062) > org.eclipse.emf.edit.provider.ItemProviderAdapter.getChildren(ItemProviderAdapter.java:340) > (nb. line numbers not up to date) > > Can I expect that the children's resources returned by getChildren() will be in > the transaction's resourceSet? > > I mean that will the call resourceSet.add(child resource) occur somewhere in > this stack below getChildren()? > In this example, when we load an object (non resource), it will not be added automatically. (in the first scenario). But, like I said I see the needs of being added automatically. Finally I'm not sure anymore if it is a good idea!! :-) I don't see why someone would not like it to have it in the resourceset. Does someone have a case ? > regards > > PGR >
I can't give you a case against adding it to the resourceSet but can certainly give you one in favour of adding it. ECrossReferenceAdapter adapter = new ECrossReferenceAdapter(); resourceSet.eAdapters().add(adapter); If the resource is never added to the resourceSet then an ECrossReferenceAdapter cannot do its job. Thus, if one wishes to delete an EObject that is referenced by other EObjects in other resources then using this adapter to find those references will fail; thus resulting in broken href links in the model. Just for information. Regards PGR
(In reply to comment #20) > I can't give you a case against adding it to the resourceSet but can certainly > give you one in favour of adding it. > > ECrossReferenceAdapter adapter = new ECrossReferenceAdapter(); > resourceSet.eAdapters().add(adapter); > > If the resource is never added to the resourceSet then an > ECrossReferenceAdapter cannot do its job. Thus, if one wishes to delete an > EObject that is referenced by other EObjects in other resources then using this > adapter to find those references will fail; thus resulting in broken href links > in the model. > > Just for information. > > Regards > > PGR > Thank you Paul!! I knew you will raised points in favor!! :-)
Just to let you know that we do not have these problems anymore with the new way of loading/creating Resource. Eike and I are refactoring how resource are loaded/created. (in the structured_resources branch) THis bug is fixed but others stuff needs to be done. We are working hard to finish it!! Simon
Committed to HEAD
Fix available in CDO 2.0.0 I200811041537
URGENT: Attachment 3 [details] indicates iplog+. Although it looks really small we need to have Paul's confirmation here, that: - He's the author of all that code. - Contributes all of that under EPL If we can't get this confirmation in time, we need to remove that code ;-(
(In reply to comment #25) > URGENT: > > Attachment 3 [details] indicates iplog+. Although it looks really small we need to have > Paul's confirmation here, that: > - He's the author of all that code. > - Contributes all of that under EPL > > If we can't get this confirmation in time, we need to remove that code ;-( > Hi Eike, Yes I am the author of the case in attachment 3 [details] (TestCase version 0.2). Please go ahead and include it under the EPL. Cheers Paul
Thx Paul ;-)
Generally available.
