GET w/ bad Accept header now returns proper error #966

bseeger · 2015-12-22T20:29:27Z

When an invalid or unsupported format type was requested the wrong
HTTP error code was returned. Now it returns the correct one.

Resolves: https://jira.duraspace.org/browse/FCREPO-1840

ajs6f · 2015-12-22T20:30:27Z

fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLdpIT.java

+        final HttpGet get = new HttpGet(serverAddress + id);
+        get.addHeader("Accept", "application/turtle");
+
+        try (final CloseableHttpResponse response = execute(get)) {


I think there's a getStatus method in the superclass that does the closing for you.

Here's an example of what @ajs6f is suggesting: http://git.io/vEcjc

- When an invalid or unsupported format type was requested the wrong HTTP error code was returned. Now it returns the correct one. Resolves: https://jira.duraspace.org/browse/FCREPO-1840

-Stricter mime-type checking Resolves: https://jira.duraspace.org/browse/FCREPO-1840

ajs6f · 2016-01-29T15:28:08Z

fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLdp.java

    public Response describe(@HeaderParam("Range") final String rangeValue) throws IOException {
        checkCacheControlHeaders(request, servletResponse, resource(), session);

        LOGGER.info("GET resource '{}'", externalPath);
+
+        final ImmutableList<MediaType> acceptableMediaTypes =
+                new ImmutableList.Builder<MediaType>().addAll(headers.getAcceptableMediaTypes()).build();


Use the static factory method ImmutableList.builder(), and acceptableMediaTypes can just be typed List<MediaType>. Actually, do you really need to copy the value of getAcceptableMediaTypes() over at all?

No, I don't think so, but I was trying to prevent a few function calls to access that list. I could just call headers.getAcceptableMediaTypes() whenever I need the list, or just use a

final List<MediaType> acceptableMediaTypes = headers.getAcceptableMediaTypes();

Which way is preferred in Java?

Given that the result of getAcceptableMediaTypes() isn't likely to change in the course of this method, I think final List<MediaType> acceptableMediaTypes = headers.getAcceptableMediaTypes(); is fine.

Just for the record, function calls in Java are incredibly cheap (JIT). Creating short-lived objects is what you really want to avoid.

awoods · 2016-02-03T15:05:49Z

fcrepo-http-commons/src/main/java/org/fcrepo/http/commons/domain/RDFMediaType.java

@@ -67,10 +67,11 @@

    public static final List<Variant> POSSIBLE_RDF_VARIANTS = mediaTypes(
            RDF_XML_TYPE, TURTLE_TYPE, N3_TYPE, N3_ALT2_TYPE, NTRIPLES_TYPE, APPLICATION_XML_TYPE,
-            TEXT_PLAIN_TYPE, TURTLE_X_TYPE, JSON_LD_TYPE).add().build();
+            TEXT_PLAIN_TYPE, TURTLE_X_TYPE, JSON_LD_TYPE, TEXT_HTML_TYPE, APPLICATION_XHTML_XML_TYPE).add().build();


I have serious concerns about introducing arguably incorrect RDF variants: html and xhtml.

By what means could Fedora possibly be producing HTML RDF output? Are you thinking about the HTML "admin" presentation?

This reverts commit 2a1214f. This reverts commit 797a4aa.

bseeger · 2016-02-12T21:44:18Z

Reverted back to simpler mime-type checking. Changed the method name from describe(...) to getResource(...). Let me know what you think.

awoods · 2016-02-12T21:47:59Z

Thanks, @bseeger.
First question, does the PR resolve the issue described in the ticket?
Second question, does the PR introduce unexpected side effects?

bseeger · 2016-02-12T22:16:24Z

First question: Yes! it now returns a 406 if someone requests an invalid mime-type on a RdfResource
Legitimate request: $>curl -XGET -i -H"Accept: text/turtle" http://localhost:8080/fcrepo/rest/pizza/ HTTP/1.1 200 OK
Bad Request: $>curl -XGET -i -H"Accept: application/turtle" http://localhost:8080/fcrepo/rest/pizza/ HTTP/1.1 406 Not Acceptable

Second question: Yes! You can't specify the mime-type for a binary, you'll get a 406 every time you do - even if you pick the correct mime-type. And... and... I'm not sure what to do with this. I can put the */* back in the @produces line and do mime-type checking only on binary resources, but then the original bug exists again, unless I do mime-type checking on everything (skipping JAX-RS) and then we go in circles. ;)

awoods · 2016-02-12T22:21:49Z

Thanks again, @bseeger. Your summary is helpful for expectation-setting.
We don't do conneg on binaries... so it is not completely insane to respond with a 406 when it is attempted.
I have put your ticket back into "Review".

ajs6f · 2016-02-12T22:30:35Z

No, it's not insane and it may be as good as we can do. But it is weird. Are we saying that if I stick in a binary, then request it with the correct mimetype, I will still get an error?

bseeger · 2016-02-12T22:34:09Z

Yes, you will get a 406. It is weird, though unless we list all the relevant mime-types for binaries in the @produces line, we would need to do our own mime-type checking further down (or fix the larger issue of both Rdf and Non-Rdf resources being fetched via the same method).

ajs6f · 2016-02-12T22:41:55Z

Well, we definitely can't list all the relevant mimetypes, because we don't know them! :)

awoods · 2016-02-14T21:21:55Z

After testing this updated PR, I will add one point of clarification to the comments above:

If you GET a binary resource with an Accept header from this list, the binary is downloaded correctly, with the real content-type, not the Accept content-type.
I think this is better than the current behavior of returning a 500, but not ideal.

I would suggest adding to the current PR a simple check in "ContentExposingResource.getContent()" before line:194 for a match of the Accept header (if it exists) and either the real contentTypeString or mediaType, which ever works better. If the Accept header exists and matches, great; if not, then 406.
Reasonable?

bseeger · 2016-02-15T15:12:35Z

With the PR the way it currently is, a GET request for a Binary with an Accept header would never get that far - because the */* has been taken out of the @Produces line. If I put that back in, then the original bug is unresolved unless I go back to mime-type checking everything outside of JaxRS.

I realize this is outside the scope of the bug, but how crazy would it be to reverse the semantics for getting a binary and it's associated metadata. Ie, take out the /fcr:metadata/ and always return metadata unless they tack on a /fcr:binary/ on the end.

Meaning, if /blahbinary/ is a binary, then:
curl -XGET -v -i -H"Accept: text/plain" http://localhost:8080/fcrepo/rest/blahbinary/
Returns the metadata for that binary.
And:
curl -XGET -v -i -H"Accept: text/plain" http://localhost:8080/fcrepo/rest/blahbinary/fcr:binary
Actually gets the binary. Then we could split the GET into two functions (one for metadata, one for binaries) and clean up the api.

... I have no clue how what I'm suggesting would impact current users. Might be an absolutely terrible idea.

ajs6f · 2016-02-15T15:16:18Z

That's what we used to do, except it was fcr:content, not fcr:binary. I can't remember why we changed. @barmintor, wasn't that your idea?

awoods · 2016-02-15T15:23:42Z

@bseeger, When a GET request for a binary resource is made with an Accept header of something like text/turtle, it goes through successfully. With that in mind, please re-read: #966 (comment)

p.s. I do not think revisiting the /fcr:metadata question is on the table in the context of this PR.

bseeger · 2016-02-15T15:28:42Z

Thanks for the clarification, @awoods. I see the distinction now. Yes, I'm all for adding that mime-type checking before line:194 as described above. I can add that today.

barmintor · 2016-02-15T15:36:04Z

@ajs6f @bseeger it wasn't my idea, but I can point you to a couple of relevant posts:

a2b9358#commitcomment-7650435

https://groups.google.com/forum/#!searchin/fedora-tech/beta$20release$20notes/fedora-tech/mADFnf_1G30/i4XzmLIiq4kJ

IIRC: Our experience back in the early beta was that this issue set you chasing your own tail about various bad implementation experiences as a consequence of JAXRS & JCR, but that this approach minimized the weirdness required of the client to know about where & what to POST or DELETE to manage LDP-NRs.

cbeer · 2016-02-15T15:39:35Z

POST, DELETE, and PUT are all pretty weird for the client with fcr:content:

PUT for an LDP-NR ought to create the resource at the location given in the request. Requiring fcr:content either requires client knowledge about interacting fcrepo4, or some awkward hand-waving about when you're updating content or metadata.
DELETE for an LDP-NR cascades to the LDP-RS describing it, and having a delete request for a resource also delete its "parent" is not intuitive

ajs6f · 2016-02-15T15:47:47Z

Now I wish we had taken the high road and refused to use any magic URLs. When people create bitstreams, we could have returned the URI of the description in a header. We could have done the work of decoupling hierarchy and lifecycle. We would have been much better off in the long run.

barmintor · 2016-02-15T15:53:19Z

But we do this! When you create a bitstream, the URI of the description is
in a Link@rel="describedBy" header! This is a LDP-ism.

On Mon, Feb 15, 2016 at 10:47 AM, A. Soroka notifications@github.com
wrote:

Now I wish we had taken the high road and refused to use any magic URLs.
When people create bitstreams, we could have returned the URI of the
description in a header. We could have done the work of decoupling
hierarchy and lifecycle. We would have been much better off in the long run.

—
Reply to this email directly or view it on GitHub
#966 (comment).

awoods · 2016-02-15T15:56:42Z

May I suggest creating a GitHub issue for the fcr:metadata discussion. It is out of scope for the work that @bseeger is doing here.

ajs6f · 2016-02-15T15:58:40Z

Then why do we need fcr:metadata at all?

bseeger · 2016-02-18T19:16:00Z

The latest commit fixes the loophole where asking for a binary with Accept header containing a mime-type listed in the @produces line, but not the binary's actual mime-type, returned the resource. Now, if the mime-type of the binary does not match a mime-type requested in the Accept field, a 406 will be returned instead.

I had to introduce this change in the FedoraLdp.java file before the getContent() call because it seems you have to have the header info complete before the response builder (in getContent()) calls build(). I'm open to moving things around, this just seemed the cleanest of all the options I thought about and tried.

awoods · 2016-02-18T19:19:45Z

@bseeger, please put the JIRA ticket back into "Review" if you are ready.

awoods · 2016-02-22T16:33:28Z

Resolved with: 825d26f

ajs6f reviewed Dec 22, 2015
View reviewed changes

bseeger added 3 commits January 28, 2016 16:30

GET w/ bad Accept header now returns proper error

d7f699b

- When an invalid or unsupported format type was requested the wrong HTTP error code was returned. Now it returns the correct one. Resolves: https://jira.duraspace.org/browse/FCREPO-1840

Cleaned up the test code

6bd534d

mime-type check, returning 406 on invalid type

797a4aa

-Stricter mime-type checking Resolves: https://jira.duraspace.org/browse/FCREPO-1840

bseeger force-pushed the fcrepo-1840 branch from 555061a to 797a4aa Compare January 28, 2016 21:43

ajs6f reviewed Jan 29, 2016
View reviewed changes

Minor tweaks to make the code cleaner.

2a1214f

awoods reviewed Feb 3, 2016
View reviewed changes

bseeger added 2 commits February 12, 2016 16:16

Revert "Back up to less strict mime-type checking"

9e50a08

This reverts commit 2a1214f. This reverts commit 797a4aa.

Changed method name to make its use clearer.

a1714b3

Add mime-type check for GET of binary resources.

d00cbfd

awoods closed this Feb 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GET w/ bad Accept header now returns proper error #966

GET w/ bad Accept header now returns proper error #966

bseeger commented Dec 22, 2015

ajs6f Dec 22, 2015

acoburn Dec 22, 2015

ajs6f Jan 29, 2016

bseeger Jan 29, 2016

ajs6f Jan 29, 2016

ajs6f Jan 29, 2016

awoods Feb 3, 2016

ajs6f Feb 3, 2016

bseeger commented Feb 12, 2016

awoods commented Feb 12, 2016

bseeger commented Feb 12, 2016

awoods commented Feb 12, 2016

ajs6f commented Feb 12, 2016

bseeger commented Feb 12, 2016

ajs6f commented Feb 12, 2016

awoods commented Feb 14, 2016

bseeger commented Feb 15, 2016

ajs6f commented Feb 15, 2016

awoods commented Feb 15, 2016

bseeger commented Feb 15, 2016

barmintor commented Feb 15, 2016

cbeer commented Feb 15, 2016

ajs6f commented Feb 15, 2016

barmintor commented Feb 15, 2016

awoods commented Feb 15, 2016

ajs6f commented Feb 15, 2016

bseeger commented Feb 18, 2016

awoods commented Feb 18, 2016

awoods commented Feb 22, 2016

GET w/ bad Accept header now returns proper error #966

GET w/ bad Accept header now returns proper error #966

Conversation

bseeger commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bseeger commented Feb 12, 2016

awoods commented Feb 12, 2016

bseeger commented Feb 12, 2016

awoods commented Feb 12, 2016

ajs6f commented Feb 12, 2016

bseeger commented Feb 12, 2016

ajs6f commented Feb 12, 2016

awoods commented Feb 14, 2016

bseeger commented Feb 15, 2016

ajs6f commented Feb 15, 2016

awoods commented Feb 15, 2016

bseeger commented Feb 15, 2016

barmintor commented Feb 15, 2016

cbeer commented Feb 15, 2016

ajs6f commented Feb 15, 2016

barmintor commented Feb 15, 2016

awoods commented Feb 15, 2016

ajs6f commented Feb 15, 2016

bseeger commented Feb 18, 2016

awoods commented Feb 18, 2016

awoods commented Feb 22, 2016