Description
This issue probably needs discussion, as it may be a spec interpretation issue. Please reference JAX-RS javadoc for javax.ws.rs.ext.Providers.getContextResolver here:
Currently, the implementation does not use the '*' character in a passed-in media type as a wildcard. Rather, it matches exactly by just allowing the LinkedHashMap.get to it's object compare. Attached is a patch with tests that demonstrate the proposed function, and a fix that supports it. All existing and new tests in wink-common and wink-server passed for me with this change.
Attachments
Attachments
- patch_02.txt
- 7 kB
- Michael Rheinheimer
- patch.txt
- 6 kB
- Michael Rheinheimer
Issue Links
Activity
Hi Mike,
I looked at your patch, it's a bit problematic. In the GreedyLinkedHashMap you override only get method with a special functionality. The problem with this solution that GreedyLinkedHashMap may behave differently, when other Map methods are used. For example, what happens if keySet().contains() is invoked?
May be currently these methods are not used, but in the future any trivial change in code will cause a problem.
I prefer to think about a different solution, may be we'll need to change the MediaTypeMap implementation totally.
I promise to think about it, and everyone else is welcome to join
I actually spent some time last night thinking about this one myself. What confused me was, if it's broken for ContextResolvers, shouldn't it be broken for everything else that uses the MediaTypeMap (MessageBodyReaders and Writers for example)?
The other thing that confused me was that I thought the wildcard support would already be handled given that we're using the MediaType.equals() to do the comparison. The implementation for that method DOES account for wildcards in generic types vs. sub types. I must be missing something here though.
Yep, it's broken for everything (except ExceptionMapper that are not dependent on MediaType)
I think you're confusing equals() with isCompatible(). Equals means that the MedaiType has exact match including the parameters.
isCompatible means that the two types are compatible. E.g. / is compatible to everything, image/* is compatible with image/jpeg, image/png, etc.
Good points Michael and Nick.
The good news is that this new GreedyLinkedHashMap is a small private nested class inside another small private nested class, so the changes are containable.
I'll be glad to expand the rest of the GreedyLinkedHashMap to provide the proper LinkedHashMap overrides and expand the unittest to cover it. It sounds like I hit on the right solution, but that it was just incomplete. That sound right?
Perhaps another solution is to not override the 'get', but instead just provide another method like 'getByRegex' or 'getCompatible'. That way the intent is clear and we don't have to worry about overriding all the other LinkedHashMap methods. Given the narrow scope of these classes, I might be leaning this way. Your thoughts?
Michael E., you're right. isCompatible is more appropriate. So, couldn't we just use that do the the check? If I understand the code path correctly, looks like that would need to happen in MediaTypeMap.getProvidersByMediaType().
If I get a chance, I'll try to poke around with that later today.
Ok, I re-worked the solution to no longer use regular expressions, or override the LinkeHashMap.get method. Instead, I now iterate through the entries in the map, use MediaType.isCompatible to compare the mediaType param with the keys in the map, and collect the data in order per JAX-RS 4.3.1. Please review the patch_02.txt and see what you think.
NOTE: I did have to change two existing test methods – please be sure to see the changes and agree or disagree with my reasoning in the code comments.
I like this kind of dialog for these types of issues. Good teamwork. Thanks!
Ok, I thought about it and to tell you the truth I'm kinda confused about the scenario.
For the context resolvers the spec states the following:
Get a context resolver for a particular type of context and media type. The set of resolvers is first filtered by comparing the supplied value of mediaType with the value of each resolver's Produces, ensuring the generic type of the context resolver is assignable to the supplied value of contextType, and eliminating those that do not match. If only one resolver matches the criteria then it is returned. If more than one resolver matches then the list of matching resolvers is ordered with those with the best matching values of Produces (x/y > x/* > /) sorted first. A proxy is returned that delegates calls to ContextResolver.getContext(java.lang.Class) to each matching context resolver in order and returns the first non-null value it obtains or null if all matching context resolvers return null.
The spec states "comparing" for Media Types, it doesn't states that stars means pattern comparison like Mike did in his first patch, and it doesn't states "compatible", so I would believe that using isCompatible is incorrect.
So actually I believe that the current behavior is the correct one, but to emphasize this I'll try to give the problematic use cases. Let's consider we want to support patterns:
@Produces("text/*") class ProviderA @Produces("text/plain") class ProviderB
which provider will be returned for "text/*"?
ProviderA gives an exact match, but ProviderB will be returned...
Another question: should we support patterns like "text/plain*"?
In addition, I have check how the JAX-RS RI (Jersey) behaves on the Mike's unit test, and it also fails.
So in general, I think we should decide if the whole scenario is valid before trying to solve it
Hi Michael. I understand your concerns. In fact, the spec at 4.3.1 and the javadoc for Providers.getContextResolver makes no mention of treating * as a wildcard. If we were to treat it as such when passed into getContextResolver, and a future JAX-RS spec clarification came out saying it should be a literal string match, then our implementation and all of the customers relying on this behavior would incur some cost. It's easier to loosen the algorithm than tighten it.
So, I agree that we should explore the scenario for validity.
I haven't had any caffeine this morning but I think the scenario for this wasn't really about the ContextResolvers but more about the Providers.getMessageBodyWriter(...., MediaType mt). It could be that someone calling the providers just wants to see if there was a writer for a Class<?> type, Type genericType for any media type. It just so happened that if the interpretation of getMessageBodyWriter() were taken that way, then getContextResolvers() would have followed the same behavior.
@GET public void getSomething(@Context Providers p, @Context ServletContext s, @Context HttpHeaders headers) { Object o = // some object MediaType acceptMT = // get the Accept header media type and it happens to be */* MessageBodyWriter mbw = p.getMessageBodyWriter(o.getClass(), o.getClass(), new Annotation[0] { }, acceptMT); // get the servlet context and set Content-Type to be acceptMT (if it was wildcard i would hope they would just return) // if (mbw != null) writeTo...., etc. }
In the end, I don't think this is something to really worry about given comments in this JIRA.
I would be curious if anyone's interpretation is asking for the MediaType "widening" though (i.e. first do a specific text/plain match, then do a text/, then do */). I prefer the current behavior, but if it's really a literal match for the MediaTypes passed into the Providers interface, then that makes things a bit easier. I could see the application developer being forced to do the type widening on their own.
Regarding:
> Another question: should we support patterns like "text/plain*"?
That's an interesting point and I think we should open another JIRA for it.
>I would be curious if anyone's interpretation is asking for the MediaType "widening" though (i.e. first do a specific text/plain match, then do a text/, then do */).
Thinking about the @Produces/@Consumes sorting rules, I would guess that the current behavior is correct for the type widening.
More comments from the peanut gallery here (hopefully it submits this time without timing out).
To the issue of support "text/plain*", I don't think that should be supported. My interpretation of wildcards is that they are for the entire type or subtype. Not substrings within that.
To the broader issue of wildcard support, maybe it's a combination of the equals() and isCompatible() that need to be accounted for. I'll go back to Michael's example.
@Produces("text/*") class ProviderA @Produces("text/plain") class ProviderB
Given these two providers, let's consider two scenarios.
1. Client sends "text/plain"
In this scenario, although both providers are technically a match, the spec states that we should map to the most specific one. The behavior remains unchanged with what we have today. When you're building the list of possible providers though, you could first check for equals(). If that works, breakout immediately because you have the most specific match (provided that there are no wildcards in the inbound mediatype). If the equals() fails, check the isCompatible() to see if it at least matches based on the wildcard.
Either way, once we found the exact match for "text/plain", we just return that and be done with it.
2. Client sends "text/*"
This is what I think Mike was getting at originally. Since we may not have a specific provider for "text/*", everything that starts with "text/" is a candidate. That means both ProviderA and ProviderB show up in the list. To Michael's question about which one is then selected, I think it's ProviderB aka "text/plain".
Not go chapter-and-verse, but take a look at section 3.8 of JAX-RS. I'm interpreting step 5 to be where both are added to the list, because they're compatible. Step 7 is where "text/plain" is put ahead of "text/*", and the rest just plays out.
At least, that's my interpretation. Dustin brought up the fact that some of this is also discussed in the Accept headers section of the HTTP spec, here: http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.1
I have a feeling that we are messing two issues into the same thread:
Issue 1: Looking for a provider in the ProvidersRegistry using javax.ws.rs.ext.Providers context.
Issue 2: Selecting the right method and then the correct media type for the response, based on client's Accept header.
Please, pay attention that these are different issues and they are defined in two different places in spec:
- Issue 1 in javadocs of javax.ws.rs.ext.Providers
- Issue 2 the spec at 3.7.2.3 (b) and 3.8
In the previous comments I was always talking about the Issue 1 only and I thought that this what WINK-47 is talking about.
But I think that Nick's latest comment mainly refers to Issue 2 and not to Issue 1.
Talking about Issue 2: isCompatible() is used for selecting the correct resource method. After the correct method was selected and invoked the algorithm from 3.8 is applied. The result of the algorithm is a concrete type or an exception. This concrete type is used to search for a Provider in the ProvidersRegistry (described in spec 4.2.2 and 4.2.3). It's important to pay attention that a a concrete type is used at this point, so equals() should behave well enough.
Yes, this Jira is specific to Issue 1. The more I think about it, the more I think that the * character in the MediaType passed to the ProvidersRegistry.getContextResolver should be treated as a wildcard. I think forcing it to be a string literal match would probably go against the general understanding of *. I think that's why Nick pointed out the Accept-header section of the html spec; just to indicate that it is discussed as a wildcard. The JAX-RS spec does not explicitly say it should be treated as a wildcard, but I think this usage has been so pervasive, I can't imagine the JAX-RS spec authors intended that * be matched as a string literal.
Thoughts?
How do we come to a consensus on this? I don't want to pollute the mailing list with "vote" emails for a Jira, but if that's the right process, we can go that way. My informal vote is for patch_02.txt as the fix.
I've asked the question about this issue at the users@jsr311 mailing list, and it seems that isCompatible() should be used, when calling from the javax.ws.rs.ext.Providers context.
About the possible implementation: I think the implementation should be split into two:
- The search for a concrete type
- The search for a type containing wildcard.
The first implementation is called from the Wink's runtime (see Issue 2 in my previous comment), and it's more efficient.
The second implementation may be invoked only by the user and it will run over all providers using isCompatible method, so it would be less efficient then the first one, but... the user can only blame himself.
Ok, I've submitted a fix.
The functionality has changed for the searches that include wildcards, so one unit test needed a fix.
I've also submitted the unit test from the first Mike's patch.
Hope everyone is happy now
Integrated in Wink-Trunk-JDK15 #23 (See http://hudson.zones.apache.org/hudson/job/Wink-Trunk-JDK15/23/)
Added support for searching providers using Wildcards. See []
Sorry for the late comment here, but thanks Michael.
"The first implementation is called from the Wink's runtime (see Issue 2 in my previous comment), and it's more efficient.
The second implementation may be invoked only by the user and it will run over all providers using isCompatible method, so it would be less efficient then the first one, but... the user can only blame himself."
I think that's the best solution. No need to interrupt the main path.
I didn't explain very well. The code path I'm talking about here is the path that does media type matching to find the context resolver. Because the inbound media type parameter that is passed into getContextResolver is treated as a hard-coded string ('*' wildcards are not treated as wildcards), the matching is iffy. For example, if I have a Provider class annotated as such:
@Provider
@Produces(MediaType.APPLICATION_FORM_URLENCODED)
And then call
providers.getContextResolver(MyClass.class, new MediaType("", ""));
No match will be found.