Bug 361049

Comment 1 Martin Fluegge 2011-10-15 08:25:46 EDT

Created attachment 205249 [details]
Test v1

Attached a patch to reprocude the problem.

Comment 2 Eike Stepper 2011-10-18 12:18:25 EDT

(In reply to comment #0)
> Given the following URI: cdo://repo1/test1" the following call should create the
> resource and return it:
> 
> Resource resource =
> resourceSet.getResource(URI.createURI("cdo://repo1/test1"), true);

With createResource() you could expect that behaviour. 
With getResource() you must expect an exception if it doesn't exist, yet.

Comment 3 Martin Fluegge 2011-10-18 13:27:55 EDT

The java doc from ResourceSetImpl

"A resource set is expected to implement the following strategy in order to resolve the given URI to a resource. First it uses it's URI converter to normalize the URI and then to compare it with the normalized URI of each resource; if it finds a match, that resource becomes the result. Failing that, it delegates to allow the URI to be resolved elsewhere. For example, the package registry is used to resolve the namespace URI of a package to the static instance of that package. So the important point is that an arbitrary implementation may resolve the URI to any resource, not necessarily to one contained by this particular resource set. If the delegation step fails to provide a result, and if loadOnDemand is true, a resource is created and that resource becomes the result. *If loadOnDemand is true and the result resource is not loaded, it will be loaded before it is returned.*"

With the current implementation we prevent the ResourceSet implementation from working correctly. From my point of view this is a serious issue. 

That's why I re-opened the bug. I think there is a bit more discussion or input need before closing this ;)

Comment 4 Eike Stepper 2011-10-18 16:02:10 EDT

Bummer! I should have read the docs. Since when I started to use EMF I've wondered what "loadOnDemand" means for getResource() and I've always ignored it because I cuold not make a sense of it. BTW. nobody has complained in 8 years of CDO!

But you seem to be right, we should fallback to createResource(uri) if getResource(uri, false) would return null.

And we should ask EMF why the parameter is not called "createOnDemand".

Comment 5 Ed Merks 2011-10-19 00:26:21 EDT

If the resource set contains a resource that's already created but not yet loaded then it will be loaded during this process.  Also keep in mind that creating a resource doesn't load it, so while you might argue to call it createAndLoadOnDemand or loadOrCreateAndLoadOnDemand, you can't really argue it should just be called createOnDemand...

Comment 6 Eike Stepper 2011-10-19 00:54:51 EDT

Oh my goodness! That beast should have been made a bit field or a number of boolean parameters ;-(

Comment 7 Ed Merks 2011-10-19 01:06:26 EDT

Martin,

I think you're misreading this last sentence:

  If loadOnDemand is true and the result resource is not loaded, 
  it will be loaded before it is returned.

This last step will (should) fail if the resource doesn't actually exist in the backing store.   Of course the resource will exist in the resource set at this point, but it will have failed to load and the getErrors() will contain the reason(s) why.

Comment 8 Eike Stepper 2011-10-19 01:12:35 EDT

Ed, thanks for the clarification!

Martin, with this information I think we can close this bug.

Comment 9 Martin Fluegge 2011-10-20 02:58:29 EDT

Hi guys,

I must admit that I somehow mixed some things because the creation process of a CDOResource was not 100% clear to me. 

Now I have learned that the CDOResourceFactoryImpl only creates an in-memory instance of a CDOResource which has no counterpart on the server site. I wrongly assummed that this should happens while the resource is created and thus was confused about the exception. Now the exception makes sense, since the resource in not yet created on the server site and the server cannot load it from the store. 

After discussion with Eike we came to the conclusion that it would be better to create the new CDOResource’s counterpart on the server site while the CDOResource is created. Thus the loading would not fail, because the resource already exists. As a result porting code from existing resource implementations to CDO would be easier for customer. 

Because this bug started with some confusion, Eike asked me to open a new bug for tracking the change of the creation process. 

Since his wish is my command, I filed Bug 361501 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=361501) ;)

Comment 10 Eike Stepper 2011-10-20 05:11:51 EDT

(In reply to comment #9)
> After discussion with Eike we came to the conclusion that it would be better to
> create the new CDOResource’s counterpart on the server site while the
> CDOResource is created.

I did emphasize on the opposite! To make it clear again:

- createResource() is absolutely okay as it is.

- getResource(uri, true) is currently not working as documented in the ResorceSet contract. Instead of *creating* a new *in-memory* resource and *not* loading it we're throwing an exception. *this* we may decide to change (although nobody ever complained about it).

Comment 11 Ed Merks 2011-10-20 05:33:38 EDT

Not sure about the last comment...

getResource(URI, true)

should look for a resource already in the resource set, call it x if it's there.
Failing that it should create a new resource call x.
In either case, if that resource x isn't loaded, there should be an attempt to load it, which should fail if there is no underlying resource in the backing store.

getResource(URI, true) should not create a new resource in the backing store.  Only resource.save should do that.  So creating a new resource involves creating a new in-memory one (using ResourceSet.create or a factory and adding it), populating it, and then calling save.

Comment 12 Eike Stepper 2012-09-21 07:16:25 EDT

Closing.