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

Projects
None yet
4 participants
@hsilva-keep
Copy link

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
@awoods

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Jun 16, 2015

@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 keeps:master branch 2 times, most recently from 49fbc40 to a2d114c Jun 17, 2015

@hsilva-keep

This comment has been minimized.

Copy link
Author

commented Jul 7, 2015

@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();

This comment has been minimized.

Copy link
@escowles

escowles Jul 7, 2015

Contributor

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.

@@ -41,7 +41,7 @@
* @since 2014-08-11
*/
public class FedoraObjectImpl extends FedoraResourceImpl implements FedoraObject {
private final static Node binaryType = NodeFactory.createLiteral("fedora:binary");
public final static Node binaryType = NodeFactory.createLiteral("fedora:Binary");

This comment has been minimized.

Copy link
@escowles

escowles Jul 7, 2015

Contributor

Is there a reason for this to be public? If not, it should remain private.

import static org.apache.http.HttpStatus.SC_NOT_FOUND;

This comment has been minimized.

Copy link
@escowles

escowles Jul 7, 2015

Contributor

Please back out the whitespace/ordering changes on otherwise unmodified code.

@@ -16,14 +16,11 @@
package org.fcrepo.client.utils;

import static java.lang.Integer.MAX_VALUE;

This comment has been minimized.

Copy link
@escowles

escowles Jul 7, 2015

Contributor

Please back out the whitespace/ordering changes on otherwise unmodified code.

@escowles

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2015

Other than a few minor comments, 👍 on this.

@awoods

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

See @escowles' comments.

@hsilva-keep

This comment has been minimized.

Copy link
Author

commented Jul 9, 2015

Dear @awoods , @escowles

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

Cheers

@hsilva-keep hsilva-keep force-pushed the keeps:master branch from a6d4a1e to c352ef5 Jul 10, 2015

Added forceMove (which will remove the tombstone) method to be cohere…
…nt with the other methods that also manipulate the tombstone (e.g. delete and forceDelete); Reverted some unnecessary changes

@hsilva-keep hsilva-keep force-pushed the keeps:master branch from c352ef5 to 799e03c Jul 10, 2015

@hsilva-keep

This comment has been minimized.

Copy link
Author

commented Jul 10, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2015

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

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

This comment has been minimized.

Copy link
@awoods

awoods Jul 13, 2015

Member

Please add removeTombstone to the FedoraResource interface, or make the method private.

/**
* Remove tombstone located at given path
*/
public void removeTombstone(final String path) throws FedoraException {

This comment has been minimized.

Copy link
@awoods

awoods Jul 13, 2015

Member

Please add removeTombstone to the FedoraResource interface, or make the method private.

public void copy(final String destination) throws ReadOnlyException {
// TODO Auto-generated method stub
throw new NotImplemented("Method copy(final String destination) is not implemented.");
public void copy(final String destination) throws FedoraException {

This comment has been minimized.

Copy link
@awoods

awoods Jul 13, 2015

Member

Please create a new integration test minimally for your new operations: copy, delete, move (and ideally for the other public methods on this class).
This integration test will be very similar to FedoraRepositoryImplIT.

@awoods

This comment has been minimized.

Copy link
Member

commented Jul 13, 2015

Looks good, pending response to latest code review comments.

Tracking with ticket: https://jira.duraspace.org/browse/FCREPO-1632

Added unit tests for Resource.move, Resource.forceMove, Resource.copy…
…, Resource.delete and Resource.forceDelete; Added two methods (already implemented in FedoraResourceImpl) to FedoraResource interface
@awoods

This comment has been minimized.

Copy link
Member

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
You can’t perform that action at this time.