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
Implemented delete resource method; Several fixes and other improvements #22
Conversation
hsilva-keep
commented
Jun 2, 2015
- Implemented delete resource
- Added force delete method which also removes tombstone
- Changed exception thrown when trying to create a datastream and it already exists
- Changed binary node value typo and removed duplicated strings in unit tests by creating constants
@hsilva-keep, this is great. In order to move this code into the codebase, please submit a CLA to |
@awoods |
49fbc40
to
a2d114c
Compare
@awoods |
|
||
if (status.getStatusCode() == HttpStatus.SC_CREATED) { // Created | ||
LOGGER.debug("resource successfully moved from " + path + " to " + destination, uri); | ||
removeTombstone(); |
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.
To be consistent with the delete() behavior, I don't think the tombstone should be removed automatically. So I think we should either have a forceMove() method that moves and removes the tombstone, or maybe add an argument to both delete() and move() to control whether the tombstone is deleted or not.
Other than a few minor comments, 👍 on this. |
See @escowles' comments. |
Added delete method to FedoraResourceImpl.java
…rce and it already exists
…ready exists (from FedoraException to AlreadyExistsException)
…n unit tests by creating string constants
…ementation of HttpCopy and HttpMove (as HTTP Client doesn't have these)
…nt with the other methods that also manipulate the tombstone (e.g. delete and forceDelete); Reverted some unnecessary changes
@escowles
So, please take another look at the code changes. |
👍 — thanks for the update, this now looks good to me. |
/** | ||
* Remove tombstone (for the current path) | ||
*/ | ||
public void removeTombstone() throws FedoraException { |
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.
Please add removeTombstone
to the FedoraResource
interface, or make the method private.
Looks good, pending response to latest code review comments. Tracking with ticket: https://jira.duraspace.org/browse/FCREPO-1632 |
…, Resource.delete and Resource.forceDelete; Added two methods (already implemented in FedoraResourceImpl) to FedoraResource interface
Resolved with: 6f485d8 |