-
Notifications
You must be signed in to change notification settings - Fork 129
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
GET w/ bad Accept header now returns proper error #966
Changes from 3 commits
d7f699b
6bd534d
797a4aa
2a1214f
9e50a08
a1714b3
d00cbfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -102,7 +103,7 @@ | |
import com.codahale.metrics.annotation.Timed; | ||
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.base.Splitter; | ||
|
||
import com.google.common.collect.ImmutableList; | ||
import static com.google.common.base.Strings.nullToEmpty; | ||
|
||
/** | ||
|
@@ -177,6 +178,7 @@ public Response head() { | |
|
||
/** | ||
* Outputs information about the supported HTTP methods, etc. | ||
* | ||
* @return the outputs information about the supported HTTP methods, etc. | ||
*/ | ||
@OPTIONS | ||
|
@@ -192,21 +194,45 @@ 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 { | ||
checkCacheControlHeaders(request, servletResponse, resource(), session); | ||
|
||
LOGGER.info("GET resource '{}'", externalPath); | ||
|
||
final ImmutableList<MediaType> acceptableMediaTypes = | ||
new ImmutableList.Builder<MediaType>().addAll(headers.getAcceptableMediaTypes()).build(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the static factory method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think so, but I was trying to prevent a few function calls to access that list. I could just call final List<MediaType> acceptableMediaTypes = headers.getAcceptableMediaTypes(); Which way is preferred in Java? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that the result of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for the record, function calls in Java are incredibly cheap (JIT). Creating short-lived objects is what you really want to avoid. |
||
|
||
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(x.toString())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use a method reference here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool! I didn't know about that. I'll change it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, it doesn't appear as if I can do this here, since I'd be using an instanceMethod that doesn't take a MediaType as it's argument. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you are right. I mistakenly thought that there was a
|
||
|
||
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); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 ofStreamingOutput
for binaries, but we can't easily do that because of our funky URI structure.There was a problem hiding this comment.
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 explicitProduces
values) and another that producesBinary
(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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first question is,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
getResource
Let's focus on the issue described in the ticket: