Hi Santiago,
Comments below
On 20/05/13 14:23, Santiago Pericas-Geertsen wrote:
>
> On May 20, 2013, at 7:22 AM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
>
>> Hi Santiago,
>>
>> I've done some more analysis. In my original example, I see a non-match per the JAX-RS 1.1 spec, so I'm doing a minor update, now
>>
>> On 19/05/13 14:53, Santiago Pericas-Geertsen wrote:
>>> Hi Sergey,
>>>
>>> Let us focus on what the JAX-RS 1.1 spec states before we discuss the best way to handle your example. Please run the JAX-RS 1.1 algorithm on your example and find the values for E, R_match and M in step 2. I'd like to understand why it works in your analysis.
>> Sure, though I can not promise the proper use of mathematical notations :-), so we have
>>
>> GET /sub
>>
>> and
>>
>> *** Example 1 ***
>>
>> @Path("/")
>> public class Resource {
>> @POST
>> @Path("{id}")
>> public void post();
>>
>> @Path("sub")
>> public Resource locator() {
>> }
>>
>> @GET
>> public void get();
>> }
>> }
>>
>> now, we use the JAX-RS 1.1 matching algorithm [1], starting from 3.7.2, path to match: "sub".
>>
>> 3.7.2.a : we have "sub", move to 3.7.2.b
>> 3.7.2.b : C = Resource.class, E={}
>> 3.7.2.c.i : add post to E, E= { R("{id}") }
>> 3.7.2.c.ii : add locator to E, E= { R("sub") }
>>
>> we now have
>>
>> E= { R("{id}"), R("sub" }
>>
>> 3.7.2.d: we keep E= { R("{id}"), R("sub" }, as both match ("sub")
>> 3.7.2.e: E is non empty, we are OK
>>
>> 3.7.2.f:
>> "Sort E using the number of literal characters in each member as the primary key (descending order), the number of capturing groups as a secondary key (descending order), the number of capturing groups with non-default regular expressions (i.e. not ‘([^ /]+?)’) as the tertiary key (descending order), and the source of each member as quaternary key sorting those derived from Tmethod ahead of those derived from Tlocator"
>>
>> R("sub") comes first (3 literal chars vs 0)
>>
>> 3.7.2.g-h: do not lose Resource.locator, so we repeat the process with Resource acting as the locator and we get Resource.get selected.
>>
>> This is where 2.0 spec produces a backward-incompatible text change for sure, in 2.0, 3.7.2.h actually loses Resource.locator, do you see it ?
>
> Yes, I thought the original example was the other way around, but I agree with this one. I think this was an unfortunate copy/paste error in the definition of M (from earlier in step 2). It should be,
>
> M = { subresource methods of all classes in C′ where R(T_method) = R_match (excluding sub-resource locators) }
>
> which should result in M = \empty and no jump to step 3.
>
> Make sense?
Sorry, not yet, how will it make Resource.locator selected ?
>
>> Next, I'm really surprised (shocked I would say) that if we have
>>
>> *** Example 2 ***
>>
>> @Path("/")
>> public class Resource {
>> @POST
>> @Path("sub")
>> public void post();
>>
>> @Path("{id}")
>> public Resource locator() {
>> }
>>
>> @GET
>> public void get();
>> }
>> }
>>
>> then we have a non-match per the spec !
>>
>> CXF adds resource methods to the list of candidates only if they match HTTP verb/Accept/Content-Type, and also simply adds the matching locators and then it does 3.7.2.f (referring to JAX-RS 1.1 text, or 3.7.2.h in 2.0), so it will always have a match, no need to backtrack.
>>
>> See we have nearly identical examples above, one works, another one does not.
>>
>> Summary:
>>
>> 1. 2.0 text introduces a backward incompatible text change leading to the ***Example 1*** not working in 2.0 any longer. IMHO this has to be fixed for 2.1
>
> Yes, see explanation of typo above. There's a filtering condition in set missing.
>
>>
>> 2. A little variation in the method path values (***Example 2***) leads to a spec-compliant non-match. Proposal:
>> 2.1 Do not enforce such non-matches at the TCK level as it is wrong that an available Locator method is ignored with the current Resource class having no matching methods in the first place, I accept ***Example 2*** will correctly per the spec return 40x, but it is totally unexpected IMHO.
>>
>> 2.2 Update the matching algorithm to add resource methods to a list of candidates only if they pass HTTP Verb + All Media Type checks - IMHO this will make the spec better: and it is safe, because as it stands, in ***Example 2*** Resource.get method can never be invoked
>
> I tend to agree that this can be improved, but let's keep the issue you found and the improvements as separate issues. Please file the JIRAs.
>
I've created two issues,
https://java.net/jira/browse/JAX_RS_SPEC-405
(regression in 2.0, as shown in Example1 below)
and
https://java.net/jira/browse/JAX_RS_SPEC-406
(Improvement to get Example2 working)
Thanks, Sergey
> -- Santiago
>
>>> On May 17, 2013, at 4:42 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
>>>
>>>> Hi Santiago
>>>> On 17/05/13 18:15, Santiago Pericas-Geertsen wrote:
>>>>> Bill, Sergey,
>>>>>
>>>>> I can see the issue that you're reporting. However, and I don't have a copy of the old spec handy, I believe HTTP method filtering was also done in step 3 before, so why did it work before?
>>>>>
>>>>> The general idea of the algorithm has always been to avoid backtracking; I think it would be possible to construct a more involved example in which you'd need to backtrack more than one step to match something.
>>>>>
>>>>> Could it be that your implementation always supported backtracking but the TCKs didn't test that before?
>>>>>
>>>> No, we've never supported back-tracking. And as I tried to clarify, JAX-RS 1.1 did not even require it and CXF simply followed it to the letter.
>>>>
>>>> When we have
>>>>
>>>> @Path("/")
>>>> public class Resource {
>>>> @POST
>>>> @Path("sub")
>>>> public void post();
>>>>
>>>> @Path("{id}")
>>>> public Resource locator() {
>>>> }
>>>>
>>>> @GET
>>>> public void get();
>>>> }
>>>>
>>>> and
>>>>
>>>> GET /sub
>>>>
>>>> returning 404 is simply not the option IMHO.
>>>>
>>>> JAX-RS 1.1 adds two candidates, post() & locator() and the text clearly makes locator() win and hence get() selected. No backtracking, simple comparison or check: post() has a non-matching method, it is out
>>>>
>>>> Likewise if we have
>>>>
>>>> @Path("/")
>>>> public class Resource {
>>>> @GET
>>>> @Path("{id}")
>>>> public void get();
>>>>
>>>> @Path("{id}")
>>>> public Resource locator() {
>>>> }
>>>>
>>>> @GET
>>>> public void getSub();
>>>> }
>>>>
>>>> we have the same 2 candidates, but get() wins because locator() loses due to the text preferring Tmethod to Tlocator.
>>>>
>>>> Whichever way you go, there's absolutely no need for the backtracking. The only case where backtracking might be required id when two locators have exactly the same Path, but 2.0 correctly recommends to report an error in this case.
>>>>
>>>> FYI, in 1.1, originally sublocators were winning in the latter case, and I recall myself when the decision was made to make sure that get() in the 2nd example wins, but the bottom line is that both get() and locator() has always been two candidates to be selected from.
>>>>
>>>> We have tests on the above and the documentation on this case.
>>>> It is backward-incompatible spec change, please check 1.1 text.
>>>>
>>>> We have to keep 1.1 behavior, we can not break the existing code with 2.0
>>>>
>>>> Thanks, Sergey
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> -- Santiago
>>>>>
>>>>> On May 17, 2013, at 8:59 AM, Bill Burke <bburke_at_redhat.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/17/2013 4:35 AM, Sergey Beryozkin wrote:
>>>>>>> Hi Bill
>>>>>>> On 17/05/13 04:41, Bill Burke wrote:
>>>>>>>> Let me give you an example:
>>>>>>>>
>>>>>>>> Given this request:
>>>>>>>>
>>>>>>>> OPTIONS /sub
>>>>>>>>
>>>>>>>> This will match:
>>>>>>>>
>>>>>>>> @Path("/")
>>>>>>>> class Resource {
>>>>>>>>
>>>>>>>> @Path("{id}")
>>>>>>>> public Locator locator() { return new Locator();}
>>>>>>>>
>>>>>>>> }
>>>>>>>>
>>>>>>>> class Locator {
>>>>>>>>
>>>>>>>> @OPTIONS
>>>>>>>> public String options() { return "OK"; }
>>>>>>>> }
>>>>>>>>
>>>>>>>> But this won't match:
>>>>>>>>
>>>>>>>> @Path("/")
>>>>>>>> class Resource {
>>>>>>>>
>>>>>>>> @GET
>>>>>>>> @Path("sub")
>>>>>>>> public String get() { return "OK"; }
>>>>>>>>
>>>>>>>> @Path("{id}")
>>>>>>>> public Locator locator() { return new Locator();}
>>>>>>>>
>>>>>>>> }
>>>>>>>>
>>>>>>> Seems to me it will too. @GET method is selected only if no @OPTIONS is
>>>>>>> available and in this case it is available.
>>>>>>>
>>>>>>
>>>>>> Check out the spec. It won't match because "sub" is matched in 2(h). 2(h) says that if there are any non-locator matches, then go to 3, then 3(a) will abort the request.
>>>>>>
>>>>>> I ran into this very problem when testing against the TCK. The TCK assumes there is no loopback to Locators and one of the tests fails. I'm able to fix this problem quite easily, but, I have some users that reported this as a "bug". They will now "regress".
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Bill Burke
>>>>>> JBoss, a division of Red Hat
>>>>>> http://bill.burkecentral.com
>>>>>
>