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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d7f699b
GET w/ bad Accept header now returns proper error
Dec 22, 2015
6bd534d
Cleaned up the test code
Dec 22, 2015
797a4aa
mime-type check, returning 406 on invalid type
Jan 28, 2016
2a1214f
Minor tweaks to make the code cleaner.
Jan 29, 2016
9e50a08
Revert "Back up to less strict mime-type checking"
Feb 12, 2016
a1714b3
Changed method name to make its use clearer.
Feb 12, 2016
d00cbfd
Add mime-type check for GET of binary resources.
Feb 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 35 additions & 11 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLdp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,25 @@

import static com.hp.hpl.jena.rdf.model.ResourceFactory.createResource;
import static javax.ws.rs.core.MediaType.APPLICATION_XHTML_XML;
import static javax.ws.rs.core.MediaType.APPLICATION_XML;
import static javax.ws.rs.core.MediaType.TEXT_HTML;
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.NOT_IMPLEMENTED;
import static javax.ws.rs.core.Response.created;
import static javax.ws.rs.core.Response.notAcceptable;
import static javax.ws.rs.core.Response.noContent;
import static javax.ws.rs.core.Response.ok;
import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.NOT_IMPLEMENTED;
import static javax.ws.rs.core.Response.Status.CONFLICT;
import static javax.ws.rs.core.Response.Status.UNSUPPORTED_MEDIA_TYPE;
import static javax.ws.rs.core.Response.Status.FORBIDDEN;
import static org.apache.commons.lang3.StringUtils.isBlank;
import static org.apache.http.HttpStatus.SC_BAD_REQUEST;
import static org.apache.jena.riot.WebContent.contentTypeSPARQLUpdate;
import static org.fcrepo.http.commons.domain.RDFMediaType.JSON_LD;
import static org.fcrepo.http.commons.domain.RDFMediaType.N3;
import static org.fcrepo.http.commons.domain.RDFMediaType.N3_ALT2;
import static org.fcrepo.http.commons.domain.RDFMediaType.NTRIPLES;
import static org.fcrepo.http.commons.domain.RDFMediaType.RDF_XML;
import static org.fcrepo.http.commons.domain.RDFMediaType.TURTLE;
import static org.fcrepo.http.commons.domain.RDFMediaType.TURTLE_X;
import static org.fcrepo.kernel.api.FedoraTypes.FEDORA_BINARY;
import static org.fcrepo.kernel.api.FedoraTypes.FEDORA_CONTAINER;
import static org.fcrepo.kernel.api.FedoraTypes.FEDORA_PAIRTREE;
Expand All @@ -53,6 +51,7 @@
import java.net.URLDecoder;
import java.util.Arrays;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;

import javax.annotation.PostConstruct;
Expand Down Expand Up @@ -81,9 +80,11 @@
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.Response;
import javax.ws.rs.core.UriBuilderException;
import javax.ws.rs.core.Variant;

import org.fcrepo.http.commons.domain.ContentLocation;
import org.fcrepo.http.commons.domain.PATCH;
import org.fcrepo.http.commons.domain.RDFMediaType;
import org.fcrepo.kernel.api.exception.InvalidChecksumException;
import org.fcrepo.kernel.api.exception.MalformedRdfException;
import org.fcrepo.kernel.api.exception.RepositoryRuntimeException;
Expand All @@ -102,7 +103,6 @@
import com.codahale.metrics.annotation.Timed;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Splitter;

import static com.google.common.base.Strings.nullToEmpty;

/**
Expand Down Expand Up @@ -177,6 +177,7 @@ public Response head() {

/**
* Outputs information about the supported HTTP methods, etc.
*
* @return the outputs information about the supported HTTP methods, etc.
*/
@OPTIONS
Expand All @@ -192,21 +193,44 @@ public Response options() {
* Retrieve the node profile
*
* @param rangeValue the range value
* @return triples for the specified node
* @return the binary or the triples for the specified node
* @throws IOException if IO exception occurred
*/
@GET
@Produces({TURTLE + ";qs=10", JSON_LD + ";qs=8",
N3, N3_ALT2, RDF_XML, NTRIPLES, APPLICATION_XML, TEXT_PLAIN, TURTLE_X,
TEXT_HTML, APPLICATION_XHTML_XML, "*/*"})
@Produces({TURTLE + ";qs=10", "*/*"})
public Response describe(@HeaderParam("Range") final String rangeValue) throws IOException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, reflecting on @acoburn's comment, I would like to see us move more towards increasing the use of the jaxrs machinery instead of further away.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course we want to. But this whole method borks JAX-RS (look at the return type). That's the underlying problem. If we can factor it into two methods with proper return types, we can unshoulder the work onto JAX-RS.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of Response is perfectly reasonable jax-rs. What are you suggesting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type is not "perfectly reasonable jax-rs". Correct use of JAX-RS implies narrowly typed return values with corresponding mappers and serializers, not generically and opaquely typed return values. Correct usage here would be one method with a return type of RdfStream for RDF and another with a return type of StreamingOutput for binaries, but we can't easily do that because of our funky URI structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the issue is that this endpoint can produce anything. Hence the @Produces({"*/*"}). A better use of JAX-RS would be to have one endpoint that produces content-negotiated RDF (LDP-RS with explicit Produces values) and another that produces Binary (LDP-NR) resources where any content type is permitted. The difficulty with that approach is (AFAIK) that Fedora uses the same endpoint to produce both of these resource types.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first question is,

  1. "Is it conceivable for our endpoints/URIs would take on a different structure to differentiate between rdf and non-rdf sources"?
  2. If not, is there a more useful return type here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Sure. It's always conceivable that we will change. I don't know how useful a claim that is. The real question is "how much change are you as project lead willing to countenance bringing in front of the community"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No. The whole point is that the return type is (correctly) indicating a deeper problem with the method (that @acoburn described). The return type is "correct" in the sense that it is accurate. It is "wrong" in the sense that we would prefer it to be something else, but cannot do that without factoring apart the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where to go with this now, other then maybe going back a commit or two - to what @acoburn suggested before (comment) - my original commit on this pull request. It's clear that there's a bigger design issue here that shouldn't probably be solved via this bug. One thought is to change the method name 'describe' to something like 'getResource' so it's clear (to developers) that it's fetching NonRdfResources as well as RdfResources. I did put it in the method's description, so maybe that's enough.

I don't know how to leverage more JaxRS here. I tried to see if I could break up the method into two (sub) methods, each with their own @produces here.

I predicted this wouldn't work, and it hasn't (at least not yet, I'm still testing it out). I couldn't find anything online specifically saying this wouldn't work, though it does seem unlikely.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, @bseeger. For this round, I agree that the deeper architectural issue is out of scope.

  • Let's rename the method along the lines of getResource
  • Roll back to a simpler commit
  • Assess things from there.

Let's focus on the issue described in the ticket:

When making a GET request with an incorrect mimeType, e.g. "Accept: application/turtle", fedora responds with a 500 status code. It should, instead, respond with a 400 code.

checkCacheControlHeaders(request, servletResponse, resource(), session);

LOGGER.info("GET resource '{}'", externalPath);

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

if (acceptableMediaTypes.size() > 0) {

final List<Variant> possibleVariants = new ArrayList();
if (resource() instanceof FedoraBinary) {
final String lang = null;
final String enc = null;
possibleVariants.add(new Variant(MediaType.valueOf(((FedoraBinary) resource()).getMimeType()),
lang, enc));
} else {
possibleVariants.addAll(RDFMediaType.POSSIBLE_RDF_VARIANTS);
}

final boolean match = acceptableMediaTypes.stream().anyMatch(x ->
x.isWildcardType() || possibleVariants.stream().anyMatch(t -> t.getMediaType().isCompatible(x)));

if (!match) {
LOGGER.info("Unable to produce content in the requested mime-type(s): " );
acceptableMediaTypes.stream().forEach(x -> LOGGER.info("MediaType: ", x));

return notAcceptable(possibleVariants).build();
}
}
addResourceHttpHeaders(resource());

final RdfStream rdfStream = new RdfStream().session(session)
.topic(translator().reverse().convert(resource()).asNode());
.topic(translator().reverse().convert(resource()).asNode());

return getContent(rangeValue, getChildrenLimit(), rdfStream);

Expand Down
2 changes: 1 addition & 1 deletion fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraHtmlIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public void testGetDatastreamNode() throws IOException {
final HttpGet method =
new HttpGet(serverAddress + pid + "/ds1");

method.addHeader("Accept", "text/html");
method.addHeader("Accept", "text/plain");
assertEquals(200, getStatus(method));
}

Expand Down
17 changes: 17 additions & 0 deletions fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraLdpIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import static javax.ws.rs.core.Response.Status.CONFLICT;
import static javax.ws.rs.core.Response.Status.CREATED;
import static javax.ws.rs.core.Response.Status.NO_CONTENT;
import static javax.ws.rs.core.Response.Status.NOT_ACCEPTABLE;
import static javax.ws.rs.core.Response.Status.NOT_FOUND;
import static javax.ws.rs.core.Response.Status.NOT_MODIFIED;
import static javax.ws.rs.core.Response.Status.OK;
Expand Down Expand Up @@ -402,6 +403,22 @@ public void testGetNonRDFSourceDescription() throws IOException {
}
}

/**
* Test that a 406 gets returned in the event of an invalid or unsupported
* format being requested.
*/
@Test
public void testGetRDFSourceWrongAccept() throws IOException {
final String id = getRandomUniqueId();
createObjectAndClose(id);

final HttpGet get = new HttpGet(serverAddress + id);
get.addHeader("Accept", "application/turtle");

assertEquals(NOT_ACCEPTABLE.getStatusCode(), getStatus(get));
}


@Test
public void testDeleteObject() {
final String id = getRandomUniqueId();
Expand Down
5 changes: 3 additions & 2 deletions fcrepo-http-commons/src/main/java/org/fcrepo/http/commons/domain/RDFMediaType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ public abstract class RDFMediaType extends MediaType {

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();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public static final String POSSIBLE_RDF_RESPONSE_VARIANTS_STRING[] = {
TURTLE, N3, N3_ALT2, RDF_XML, NTRIPLES, TEXT_PLAIN, APPLICATION_XML, TURTLE_X, JSON_LD };
TURTLE, N3, N3_ALT2, RDF_XML, NTRIPLES, TEXT_PLAIN, APPLICATION_XML, TURTLE_X, JSON_LD, TEXT_HTML,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad -- I was a little confused about those two, though I do understand it more now and see that they return the entire webpage. I'll take them out of the RDF Variants list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't whether you are really wrong here, @bseeger , since some endpoints using this constant can produce HTML Maybe we can break this apart into "flavors of RDF that many endpoints can produce" and "HTML flavors". Some endpoints get just the first, and some get both together.

APPLICATION_XHTML_XML};

private static MediaType typeFromString(final String type) {
return new MediaType(type.split("/")[0], type.split("/")[1]);
Expand Down