On Fri, Jun 19, 2009 at 12:30 PM, Vassil Dichev <[email protected]> wrote:
> > I've looked at the pool stuff. Vassil did a great job with it. I made a > > Thank you! > > > very minor performance enhancement (using Set instead of List which means > > O(log n) rather than O(n) for access checking). > > Good idea. I have some concerns about making a new query for each pool > a user can write to just to get its name, but it might be early to > optimize, and we can certainly do it later on. > > > Rock and roll and if it's got the votes (which I think it does), I say > roll > > it on into trunk. > > Great, I will proceed with the merge right away. > > @David, I want your advice on one more issue. There are many > occurrences in ESME where a find* method is called on Message. These > calls would retrieve also messages from pools the user is not supposed > to see. What do you think is a better solution: > > -I'm toying with the idea of overriding findMapDb (since all calls to > findAll and findMap eventually call it). I'm trying to insert an SQL > fragment, which would restrict results only to the allowed pools. This > is a single point of control, so should be easier to maintain. OTOH it > could be less transparent and harder to debug if you don't expect it. It's easier than that... all models have an "addlQueryParams" instance variable. As a wrapper, define it to limit access to the current pool. If you've got questions on how to do it, ping me and we can work together on it. > > > -the standard solution of changing all instances of findAll/findMap is > more easy to understand, but harder to maintain- one must make sure no > find* call is forgotten. > > Overall, I'm leaning towards overriding the single findMapDb. It's > similar to aspects: it might be more difficult to debug and reason > about, but it's cleaner and separates the concern. The find* methods > are part of the model API and potential new ESME committers could > forget to restrict the call to only return messages from appropriate > pools. > > Everyone: relax, this will not delay merging, we will resolve the > issue afterwards. > > Cheers, > Vassil > -- Lift, the simply functional web framework http://liftweb.net Beginning Scala http://www.apress.com/book/view/1430219890 Follow me: http://twitter.com/dpp Git some: http://github.com/dpp
