New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
doesResourceExist function #1608
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service should be helpful, I seem to recall we needed doesResourceExist
a few times now.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should probably commit in a finally block, otherwise we might leak sessions when exceptions occur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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