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 >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>> >
