session tracking refactoring: all tests green

> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
> <[hidden email]> wrote:
>> Hi
>>
>> I'm happy to report that the session tracking refactoring in the WIP
>> repository [1] is mostly done. Have a look at
>
> I'll take a look.
>
>> WAHandlerTrackingStrategy to get started. The missing things are:
>>  - more tests
>>  - class comments
>>  - WARegistryKeyHandlingTest and WAApplicationKeyHandlingTest need to
>> be refactored, there are some nonsensical tests there like two
>> sessions, one identified by a query field and one by a cookie
>
> Why is that a nonsensical test? It happens any time someone clicks on
> a link/bookmark and also has a cookie from a previous session hanging
> around...

> 2011/7/18 Julian Fitzell <[hidden email]>:
>> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
>> <[hidden email]> wrote:
>>> Hi
>>>
>>> I'm happy to report that the session tracking refactoring in the WIP
>>> repository [1] is mostly done. Have a look at
>>
>> I'll take a look.
>>
>>> WAHandlerTrackingStrategy to get started. The missing things are:
>>>  - more tests
>>>  - class comments
>>>  - WARegistryKeyHandlingTest and WAApplicationKeyHandlingTest need to
>>> be refactored, there are some nonsensical tests there like two
>>> sessions, one identified by a query field and one by a cookie
>>
>> Why is that a nonsensical test? It happens any time someone clicks on
>> a link/bookmark and also has a cookie from a previous session hanging
>> around...
>
> If the user agent accepts cookies the session key will not show up in
> the URL. Only the very first link he clicks linking to an action page
> will have it but the following render page won't. So it won't show up
> the address bar. Also we probably do the wrong thing. We should
> probably take the session from the cookie. However we currently give
> preference to the query field because document handlers are tracked
> using query fields.

>>>  - configuration with the admin UI is broken, see previous mail
>>>
>>> A small side effect is that WARegistry uses '_s' as well to track
>>> things instead of '_key'. Since it isn't used anyway I didn't bother
>>> to preserve this behavior. This obviously breaks all users of
>>
>> I don't think that should be hardcoded. First, the _s clearly implies
>> "session"; second, registries could easily be nested so having a
>> hardcoded key sucks.
>
> For the last then years only _s has ever been used. Even for document
> handlers. If you wanted something else you had to subclass
> WAApplication. With the new solution you still have to subclass, just
> the strategy instead of the application. So nothing really changed.
>
> I figured that in the hypothetical case when you wanted some other key
> subclassing a strategy class and overriding a method is preferable to
> introducing a configuration option that nobody ever uses.

> On Mon, Jul 18, 2011 at 1:40 PM, Philippe Marschall
> <[hidden email]> wrote:
>> 2011/7/18 Julian Fitzell <[hidden email]>:
>>> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
>>> <[hidden email]> wrote:
>>>> Hi
>>>>
>>>> I'm happy to report that the session tracking refactoring in the WIP
>>>> repository [1] is mostly done. Have a look at
>>>
>>> I'll take a look.
>>>
>>>> WAHandlerTrackingStrategy to get started. The missing things are:
>>>>  - more tests
>>>>  - class comments
>>>>  - WARegistryKeyHandlingTest and WAApplicationKeyHandlingTest need to
>>>> be refactored, there are some nonsensical tests there like two
>>>> sessions, one identified by a query field and one by a cookie
>>>
>>> Why is that a nonsensical test? It happens any time someone clicks on
>>> a link/bookmark and also has a cookie from a previous session hanging
>>> around...
>>
>> If the user agent accepts cookies the session key will not show up in
>> the URL. Only the very first link he clicks linking to an action page
>> will have it but the following render page won't. So it won't show up
>> the address bar. Also we probably do the wrong thing. We should
>> probably take the session from the cookie. However we currently give
>> preference to the query field because document handlers are tracked
>> using query fields.
>
> Our logic, which I still think is sound, is that *if* you pass in a
> session key on the URL you are asking to override what is in the
> cookie. I agree that in common usage that's not going to come up very
> often, but in cases where both are present in a request I still think
> priority should be given to the query field; the user has requested a
> specific resource at a specific URL that includes a session key, it
> seems weird to ignore that key if the session actually exists (and
> note that if it doesn't, we should actually maybe return a redirect
> response to a URL without the session key, which could then be handled
> using the cookie).

>>>>  - configuration with the admin UI is broken, see previous mail
>>>>
>>>> A small side effect is that WARegistry uses '_s' as well to track
>>>> things instead of '_key'. Since it isn't used anyway I didn't bother
>>>> to preserve this behavior. This obviously breaks all users of
>>>
>>> I don't think that should be hardcoded. First, the _s clearly implies
>>> "session"; second, registries could easily be nested so having a
>>> hardcoded key sucks.
>>
>> For the last then years only _s has ever been used. Even for document
>> handlers. If you wanted something else you had to subclass
>> WAApplication. With the new solution you still have to subclass, just
>> the strategy instead of the application. So nothing really changed.
>>
>> I figured that in the hypothetical case when you wanted some other key
>> subclassing a strategy class and overriding a method is preferable to
>> introducing a configuration option that nobody ever uses.
>
> I'm not suggesting we need a configuration option. An instance
> variable is just fine. Hardcoding something arbitrary like '_s',
> particularly in a new more generic class, seems really silly to me.
> Initialize it with '_s' and leave the option open...

> 2011/7/19 Julian Fitzell <[hidden email]>:
>> On Mon, Jul 18, 2011 at 1:40 PM, Philippe Marschall
>> <[hidden email]> wrote:
>>> 2011/7/18 Julian Fitzell <[hidden email]>:
>>>> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
>>>> <[hidden email]> wrote:
>>>>> Hi
>>>>>
>>>>> I'm happy to report that the session tracking refactoring in the WIP
>>>>> repository [1] is mostly done. Have a look at
>>>>
>>>> I'll take a look.
>>>>
>>>>> WAHandlerTrackingStrategy to get started. The missing things are:
>>>>>  - more tests
>>>>>  - class comments
>>>>>  - WARegistryKeyHandlingTest and WAApplicationKeyHandlingTest need to
>>>>> be refactored, there are some nonsensical tests there like two
>>>>> sessions, one identified by a query field and one by a cookie
>>>>
>>>> Why is that a nonsensical test? It happens any time someone clicks on
>>>> a link/bookmark and also has a cookie from a previous session hanging
>>>> around...
>>>
>>> If the user agent accepts cookies the session key will not show up in
>>> the URL. Only the very first link he clicks linking to an action page
>>> will have it but the following render page won't. So it won't show up
>>> the address bar. Also we probably do the wrong thing. We should
>>> probably take the session from the cookie. However we currently give
>>> preference to the query field because document handlers are tracked
>>> using query fields.
>>
>> Our logic, which I still think is sound, is that *if* you pass in a
>> session key on the URL you are asking to override what is in the
>> cookie. I agree that in common usage that's not going to come up very
>> often, but in cases where both are present in a request I still think
>> priority should be given to the query field; the user has requested a
>> specific resource at a specific URL that includes a session key, it
>> seems weird to ignore that key if the session actually exists (and
>> note that if it doesn't, we should actually maybe return a redirect
>> response to a URL without the session key, which could then be handled
>> using the cookie).
>
> The test is still there. I just moved it from registry tests to
> application tests because only sessions session support cookies and