Done and committed. Cheers,
Les On Thu, Jul 31, 2008 at 11:01 AM, Jeremy Haile <[EMAIL PROTECTED]> wrote: > Sounds good. > > > > On Jul 31, 2008, at 10:57 AM, Les Hazlewood wrote: > > Yep, that's what I'm thinking. >> >> I'll commit the adjustments in just a few minutes so you can see if it >> works >> for you or not... >> >> On Thu, Jul 31, 2008 at 10:46 AM, Jeremy Haile <[EMAIL PROTECTED]> >> wrote: >> >> I don't have a problem adding it in now. So would onStart() receive a >>> mutable copy, but onStop/onExpiration receives an immutable copy? >>> >>> >>> >>> >>> On Jul 31, 2008, at 10:42 AM, Les Hazlewood wrote: >>> >>> Should it be immutable? If so, we could provide a wrapper >>> >>>> >>>>> 'ImmutableSession' implementation that throws exceptions on any method >>>>>> call >>>>>> that tries to alter state and delegates to the wrapped Session when >>>>>> just >>>>>> reading state, like getAttribute(). The wrapper would only be used >>>>>> during >>>>>> listener notification, and the session would be 'unwrapped' for any >>>>>> remaining processing after listener notification.... >>>>>> >>>>>> >>>>>> My thoughts exactly. We'd need to look closely at the session to see >>>>> what >>>>> should be mutable vs immutable. But it's something that >>>>> security-conscious >>>>> framework should think about. Immutability is good in many cases. >>>>> >>>>> Is this something you think we should do now? Is it something that we >>>>> >>>>> should even enforce? >>>>>> >>>>>> >>>>>> Maybe for 1.0, not for 0.9.0. >>>>> >>>>> >>>> >>>> Actually, I don't see any problems with adding this even today unless >>>> you >>>> have objections - it is very unintrusive and wouldn't take any time at >>>> all. >>>> The only methods that I can see that would throw an exception are: >>>> >>>> setTimeout(long); >>>> touch(); >>>> stop(); >>>> setAttribute(key,value); >>>> removeAttribute(key); >>>> >>>> Everything else is read-only already. >>>> >>>> In fact, I would prefer that it is in now, to minimize any surprise >>>> later. >>>> I'm not sure anyone is even using the SessionListener construct yet, so >>>> it'd >>>> be nice to get this in before that might happen.... >>>> >>>> >>>> >>>> I have no idea what the best practice would be, but for some reason I >>>> >>>>> >>>>> really >>>>>> like the sound of the default not allowing modification. It seems >>>>>> more >>>>>> logical to me, because those notifications are supposed to indicate >>>>>> that >>>>>> the >>>>>> session is in an invalid state and probably shouldn't be messed with >>>>>> anymore. >>>>>> >>>>>> >>>>>> Yeah - I like the idea of certain parts of the session being >>>>> immutable >>>>> always - and other parts of the session being wrapped in an immutable >>>>> implementation for listener events, etc. >>>>> >>>>> We do need to think about things like when someone wants to use a >>>>> session >>>>> listener to set an attribute when a session is created. >>>>> >>>>> That may be valid - so this does require some more thought as to what >>>>> >>>> >>>> should be immutable and when it should be immutable. >>>>> >>>>> >>>> >>>> I definitely think its ok for a listener to do whatever they want to the >>>> session upon creation. But I do think the wrapper concept should be in >>>> place today, and we can tweak exactly what the behavior may do in the >>>> future >>>> - just starting off by throwing exceptions for the above methods. That >>>> ok? >>>> >>>> >>>> On Jul 30, 2008, at 11:32 AM, Les Hazlewood wrote: >>>> >>>>> >>>>> >>>>>> >>>>>> So in my review of the code in prep for RC1 last night, I found >>>>>>> something >>>>>>> >>>>>>> I'd never really thought about before: >>>>>>> >>>>>>>> >>>>>>>> Should the Session *Interface* have the two methods >>>>>>>> getStopTimestamp() >>>>>>>> and >>>>>>>> isExpired() at all? >>>>>>>> >>>>>>>> I remember those existing in the implementation specifically to >>>>>>>> handle >>>>>>>> the >>>>>>>> case where Sessions were persistent beyond their user lifetime - >>>>>>>> e.g. >>>>>>>> in >>>>>>>> environments that do user activity reporting, they could organize >>>>>>>> user >>>>>>>> events based on sessions, etc. But the framework couldn't use >>>>>>>> stopped >>>>>>>> or >>>>>>>> expired sessions anymore. >>>>>>>> >>>>>>>> It seems as if implementation methods bubbled their way up into the >>>>>>>> interface. >>>>>>>> >>>>>>>> Would a user ever need to call getStopTimestamp() or isExpired()? >>>>>>>> >>>>>>>> In our web support for example, these methods would never return >>>>>>>> anything >>>>>>>> useful: getStopTimestamp() would _always_ return null and >>>>>>>> isExpired() >>>>>>>> would >>>>>>>> _always_ return false, because of our WebSessionManager logic: >>>>>>>> >>>>>>>> When a Subject is being constructed for the incoming Request, the >>>>>>>> WebSessionManager it tries to acquire the user's corresponding >>>>>>>> session >>>>>>>> (if >>>>>>>> it exists). If it does exist, but that session is invalid (that is, >>>>>>>> stopped >>>>>>>> or expired), it suppresses resulting InvalidSessionException (prints >>>>>>>> out >>>>>>>> a >>>>>>>> trace message really) and returns null to the caller. The caller >>>>>>>> views >>>>>>>> this >>>>>>>> as an indication that the session doesn't exist, and it must create >>>>>>>> a >>>>>>>> brand >>>>>>>> new one if it needs to use a Session. >>>>>>>> >>>>>>>> Because of this logic, those two methods are probably meaningless. >>>>>>>> I >>>>>>>> also >>>>>>>> think that our logic to ignore invalid sessions and create a brand >>>>>>>> new >>>>>>>> one >>>>>>>> when necessary is the right way to go - it is how web containers >>>>>>>> work, >>>>>>>> and >>>>>>>> everyone is pretty much happy with that behavior. >>>>>>>> >>>>>>>> Do you guys agree? Do you see a need to retain them in the >>>>>>>> interface? >>>>>>>> (They >>>>>>>> would stay in the implementation because the implementation needs to >>>>>>>> use >>>>>>>> them to determine if in fact it is invalid or not). >>>>>>>> >>>>>>>> I'm just looking for your thoughts if you have any... >>>>>>>> >>>>>>>> Les >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>> >>> >
