doesResourceExist function #1608
Conversation
fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/services/GetResourceServiceImpl.java
Outdated
Show resolved
Hide resolved
| resource = new ContainerImpl(fedoraId, transaction, psManager, rsFactory); | ||
| } | ||
| // Commit session so it doesn't hang around. | ||
| psSession.commit(); |
There was a problem hiding this comment.
I don't think we want to commit the session if the transaction existed before this service was called, otherwise it might prematurely commit changes. Doesn't seem like we have a commitIfShortLived equivalent for PersistentStorageSession, so might have to rely on checking if transaction was null or not.
There was a problem hiding this comment.
Also, we should probably commit in a finally block, otherwise we might leak sessions when exceptions occur
There was a problem hiding this comment.
The commit here is on a persistent storage session we just created. Would that not mean nothing else has occurred in this session and should not cause trouble? I'm a little unclear about how the various sessions work so i could be missing something.
There was a problem hiding this comment.
I have moved the commit to a finally block, but as it also can throw an exception I have had to squash and log that exception. Not sure if that is an acceptable trade-off.
There was a problem hiding this comment.
Since it retrieves a persistent session using the transaction when present, it could grab an ongoing modification session. What you have here looks good to me, it failing to close the temporary session doesn't really impact that rest of the operation, so logging is fine.
fcrepo-kernel-impl/src/main/java/org/fcrepo/kernel/impl/services/GetResourceServiceImpl.java
Outdated
Show resolved
Hide resolved
...po-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/GetResourceServiceImplTest.java
Outdated
Show resolved
Hide resolved
...po-kernel-impl/src/test/java/org/fcrepo/kernel/impl/services/GetResourceServiceImplTest.java
Outdated
Show resolved
Hide resolved
|
@bbpennel Moved the single function into ResourceFactory, so you'll probably want to have another look at this. |
|
Its a little hard to judge whether or not GetResourceService would have been helpful without these being used anywhere, but i think this is good now. |
JIRA Ticket: https://jira.lyrasis.org/browse/FCREPO-3137
What does this Pull Request do?
Adds a doesResourceExist method to the ResourceFactory
How should this be tested?
Just review, I have not added this to any other code yet.
Interested parties
@fcrepo4/committers