[Seaside-dev] session tracking refactoring: all tests green

[Julian Fitzell]

On Mon, Jul 18, 2011 at 1:40 PM, Philippe Marschall
<philippe.marschall at gmail.com> wrote:
> 2011/7/18 Julian Fitzell <jfitzell at gmail.com>:
>> On Sun, Jul 17, 2011 at 1:47 PM, Philippe Marschall
>> <philippe.marschall at gmail.com> 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...

>>> #useCookies and I also had to delete some deprecated methods on
>>> WARegistry. This bears the question do we merge it anyway or do we
>>> start with 3.1?
>>
>> I've broken #useCookies as well in my experiments. It would end up
>> being an attribute of the session request filter instead of the
>> application.
>
> That doesn't answer the question ;-)

Sorry, you're right... I didn't finish my thought. I meant to say that
this feels 3.1ish to me.

Julian