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

Fcrepo 1555 #810

Closed
wants to merge 26 commits into from

Conversation

Projects
None yet
8 participants
@robyj
Copy link
Contributor

commented Jun 3, 2015

@@ -1597,7 +1598,7 @@ public void testRepeatedPut() throws Exception {

final HttpPut secondPut = new HttpPut(serverAddress + pid);
secondPut.setHeader("Content-Type", "text/turtle");
assertEquals(400, getStatus(secondPut));
assertEquals(409, getStatus(secondPut));

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jun 3, 2015

Contributor

Maybe use a named constant here?

@@ -39,6 +39,10 @@
public Response toResponse(final MalformedRdfException e) {
final Link link = Link.fromUri(getConstraintUri(e)).rel(CONSTRAINED_BY.getURI()).build();
final String msg = e.getMessage();

if (msg.indexOf("given RDF is out-of-date") != -1) {

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jun 3, 2015

Contributor

String.contains() might read better here.

This comment has been minimized.

Copy link
@awoods

awoods Jun 3, 2015

Member

This ExceptionMapper does not apply to the scenario described in the ticket. Instead, ServerManagedPropertyExceptionMapper is used... but does not appear to require any updates.

@@ -31,6 +31,7 @@
import static java.util.regex.Pattern.compile;
import static javax.ws.rs.core.MediaType.TEXT_PLAIN;
import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
//import static javax.ws.rs.core.Response.Status.CONFLICT;

This comment has been minimized.

Copy link
@awoods

awoods Jun 3, 2015

Member

Remove instead of comment-out.

+ predicate.toString()
+ " on node "
+ node.getPath());
//} else {

This comment has been minimized.

Copy link
@awoods

awoods Jun 3, 2015

Member

Remove instead of comment-out.

This comment has been minimized.

Copy link
@robyj

robyj Jun 4, 2015

Author Contributor

Modifications have been made and pushed

+ " to node "
+ node.getPath());
throw new ServerManagedPropertyException("Cannot update the container as the given RDF is "
+ "out-of-date based on the value of its fedora:lastModified triple. Predicate is "

This comment has been minimized.

Copy link
@awoods

awoods Jun 3, 2015

Member

We should not hardcode fedora:lastModifed into this error message since the exception can be thrown for any server-managed property.
Maybe the message should be:

Could not remove triple containing server-managed predicate "
                    + predicate.toString()
                    + " to node "
                    + node.getPath()
@awoods

This comment has been minimized.

If JcrRdfTools.removeProperty() is throwing a ServerManagedPropertyException, why is any update needed for MalformedRdfExceptionMapper?

This comment has been minimized.

Copy link
Owner

replied Jun 4, 2015

To catch the string and return a 409 code instead of a 400 code.

This comment has been minimized.

Copy link

replied Jun 4, 2015

My point is, MalformedRdfExceptionMapper never interacts with ServerManagedPropertyExceptions.

This comment has been minimized.

Copy link
Owner

replied Jun 4, 2015

That is an interesting question. Why are the other code being routed that way also?

+ node.getPath());
throw new ServerManagedPropertyException("Could not remove triple containing server-managed "
+ "predicate "
+ predicate.toString()

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jun 4, 2015

Contributor

Unnecessary toString().

@@ -39,6 +39,10 @@
public Response toResponse(final MalformedRdfException e) {
final Link link = Link.fromUri(getConstraintUri(e)).rel(CONSTRAINED_BY.getURI()).build();
final String msg = e.getMessage();

if (msg.contains("server-managed predicate")) {

This comment has been minimized.

Copy link
@ajs6f

ajs6f Jun 4, 2015

Contributor

Not necessarily in this PR, but sometime, we should factor this string out into a constant in the appropriate class.

ajs6f and others added some commits Jun 4, 2015

Merge pull request #809 from acoburn/fcrepo-1563
Refactor guava functional classes into Java8 idiom
Merge pull request #805 from yinlinchen/fcrepo-1394
https://jira.duraspace.org/browse/FCREPO-1394

Replace fedora:Blanknode with fedora:Skolem in the code and in the on…

escowles and others added some commits Jun 4, 2015

robyj
Using awoods work on non-wrapping ServerManagedPropertyExceptions, ma…
…de changes for fcrepo-1555

Added integration test that should be accurate.

@robyj robyj closed this Jun 5, 2015

robyj
Merge branch 'fcrepo-1555' of github.com:robyj/fcrepo4 into fcrepo-1555
Conflicts:
	fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLdpIT.java

@robyj robyj deleted the robyj:fcrepo-1555 branch Jun 6, 2015

@robyj robyj restored the robyj:fcrepo-1555 branch Jun 6, 2015

@awoods

This comment has been minimized.

Copy link
Member

commented Jun 6, 2015

@robyj, why did you close this PR? It is very close to completion...

@robyj robyj reopened this Jun 7, 2015

@robyj

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2015

I had technical issues on friday and messed up my local branches, thinking if I closed the pull request, it might help me re-enable/re-create my local branch. I re-constructed the branch and the modifcations and pushed it up so hopefully its still good. I ran a build and it passed the tests.

Merge pull request #1 from awoods/fcrepo-1555
Suggestion for not wrapping ServerManagedPropertyExceptions

@barmintor barmintor added the wontfix label Mar 30, 2016

@barmintor

This comment has been minimized.

Copy link
Member

commented Mar 30, 2016

This PR appears to be inactive or obsolete, is that correct? I am marking it inactive in advance of the 4.5.1 codefreeze.

@barmintor barmintor added inactive and removed wontfix labels Mar 30, 2016

@robyj

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2016

Hi, Jared asked me to rebase the code into the latest master branch, which i'm still trying to find time to do

Thanks
Jon

@dbernstein dbernstein closed this Jan 2, 2019

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.