Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

hsilva-keep
Copy link

  • 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

@awoods
Copy link
Member

awoods commented Jun 2, 2015

@hsilva-keep, this is great. In order to move this code into the codebase, please submit a CLA to legal@duraspace.org.
https://wiki.duraspace.org/display/DSP/Contributor+License+Agreements

@hsilva-keep
Copy link
Author

@awoods
The build has errors but we will fix them soon. Also, just to let you know, we've already submitted the Contributor License Agreement.

@hsilva-keep hsilva-keep force-pushed the master branch 2 times, most recently from 49fbc40 to a2d114c Compare June 17, 2015 09:50
@hsilva-keep
Copy link
Author

@awoods
Anything else is needed to do the merge (as the build already succeeded and the merge can be done)?


if (status.getStatusCode() == HttpStatus.SC_CREATED) { // Created
LOGGER.debug("resource successfully moved from " + path + " to " + destination, uri);
removeTombstone();
Copy link
Contributor

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.

@escowles
Copy link
Contributor

escowles commented Jul 7, 2015

Other than a few minor comments, 👍 on this.

@awoods
Copy link
Member

awoods commented Jul 9, 2015

See @escowles' comments.

@hsilva-keep
Copy link
Author

Dear @awoods , @escowles

My apologies but I didn't had the time, yet, to "react" to the comments. I'll do that tomorrow.

Cheers

…nt with the other methods that also manipulate the tombstone (e.g. delete and forceDelete); Reverted some unnecessary changes
@hsilva-keep
Copy link
Author

@escowles
With commit 799e03c I've...

  1. Reverted the code changes that were unnecessary;
  2. Followed your suggestion of, as it was already being done with the delete/forceDelete, implementing a forceMove method and removing, from the move method, that task of deleting the tombstone.

So, please take another look at the code changes.

@escowles
Copy link
Contributor

👍 — thanks for the update, this now looks good to me.

/**
* Remove tombstone (for the current path)
*/
public void removeTombstone() throws FedoraException {
Copy link
Member

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.

@awoods
Copy link
Member

awoods commented Jul 13, 2015

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
@awoods
Copy link
Member

awoods commented Jul 14, 2015

Resolved with: 6f485d8

@awoods awoods closed this Jul 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants