Hello Simon,
You raised some valid concerns about caching and cross-references. I've
also thought this could be a problem, but as I didn't immediately see a
solution, I focused first on the advantages. Your detailed comments made
the potential problems clearer to me. So while at present the discussion
seems two sided, I think we need to go beyond that and come with a
better solution than the status quo.
The primary point I'd like to address with this refactoring discussion
is to find a solution for the efficient reuse of model objects, taking
into account the case when we don't necessarily have or want to
systematically create such an object. The second point, less important
but still valuable in my eyes, is to reduce the verbosity of the code
related to resources.
On 4/21/2010 1:59 AM, osimons wrote:
So, we have ended up with lots of code that create resources. We have
a lot of code that create half-baked resources that we mutate. If
these resources also could contain references to model objects of
unknown state and origin, that would in my opinion be a recipe for
disaster - certainly when developers started to use that as an entry
point to the model objects, and the resource and model somehow
persisted or was cached.
If the resource gets cached, I see that this can be a problem, and there
is also the circular reference issue...
The 'convenience' we have generally used so far has in fact been the
other way around as we have attached a Resource object to the Model in
order to have it at hand. That then causes intermingled and possibly
circular references.
We indeed already had sync issues between the model and its resource.
There must be a way to handle this better, though I don't have the
perfect solution at this point of the thinking process.
Ideally, resources and model object should share the same notion of
identity. Maybe the Resource should stay very simple, but we could
define on top of that a ResourceDescriptor which has a richer API for,
precisely, *describing* the resource. That would possibly address the
second point, about the convenience of the API. That descriptor could
keep an optional handle to the model, both would share the same Resource
for defining the identity. Descriptors should never be cached, the same
way we never cache a model object or a db connection object (oops ;-) ).
"Explicit is better than implicit" as the zen says, and I cringe each
time I offer support on IRC pointing people to
formatter.context.resource as location of this very important
information in macro code. Let 10 able coders stare at the Trac code
and see if any of them come up with formatter.context.resource.model
as the obvious starting point to access properties they need... If
something is important and needed, then pass it explicity.
In this specific example, I'm not sure how it would be better, as you
have the following relations: one formatter has one rendering context,
which corresponds to one resource. Passing all of them explicitly in
every place we currently pass a formatter would be unnecessarily verbose
and error prone. It would introduce the possibility to break the
relation by passing unrelated instances. I remember that I had this
problem for the IPermissionPolicy.check_permission method, where the
signature (action, username, resource, perm) was precisely chosen on the
ground that "explicit is better" but then, we had bugs involving the
explicit resource being different from the one in perm... So there's
also the "don't repeat yourself" principle, which I think is even more
important in this context, and the nesting makes the nature of the
relationship more obvious... (otherwise, try to answer the question
which comes first, formatter or context?) But I digress, this is
material for the API reference guide ;-)
In the context of the generic notifications (#8834), the notifications
will happen right after a change to the model object, so we have this
model object around. It would certainly make sense to propagate it to
the implementations, and only those *which know how to handle it* will
want to deal with it. I'm not talking about using those models in a
generic way, simply avoiding the need for each notifier to reinstantiate
a model object. E.g. the notifiers able to react on the "ticket" realm
will not have to create a fresh Ticket object *again*, when the instance
which triggered the notification is already available (and is more
"interesting" as well, as it has the _old values for example).
The other situation is when we only start with a resource
(IPermissionPlugin) and various permission policies want to fetch the
same model over and over (or bypass this by directly looking up the data
in the db, as instantiating a model object is costly). In that situation
it would be better to be able to create that instance only once, and
only if actually needed.
So maybe something like the ResourceDescriptor idea would work for both
cases.
Complex
objects that reference other known and unknown objects in ways that
are hard to detect, literally caused me months of sleepless nights
while trying to figure out the rare circumstance why my site went down
at irratic intervals:
http://trac-hacks.org/changeset/4366
Documentation can certainly improve this situation, one should then be
able to see which objects are referenced by what. And even with the best
API having the best documentation, it will never completely prevent the
occasional goof... sorry again for [TH4366] ;-)
Note that .db in the formatter will certainly go away in 0.13 (along
with the Formatter as we know it).
The only way these new suggestions makes sense is if the resource (and
user of the resource) could make some general assumptions about the
underlying model.
I hope what I wrote above already refutes this. It's about passing the
model around and allowing those who know how to use it to reuse it
instead of having to reinstantiate it. If in a later step we happen to
have implementations that can handle the model objects in some generic
and useful ways, why not?
Which I suppose is GenericTrac again... Personally
I'm not in favour of this, and enjoy the flexibility currently
enjoyed. As one example against the #8834 ticket (generic resource
change listening/notification), my FullBlog needs to notify when
comments are added. Comments are not resources, and they do not create
a new version of the blog post itself which is separately editable for
new versions. Still, for notifications, comments are just as important
as the resource itself.
Well, comments could certainly be seen as child resources, parented in
the blog resource.
(and I wrote the above before seeing Remy's answer to your message ;-) )
Shoe-horning all features into common model-,
manipulator- and listener infrastructure is contrary to the general
"feel" of the code base that has made Trac successful and enabled the
great variety of plugins that we actually have.
That's a misunderstanding. The GenericTrac approach is not going to take
away any possibility of variety. It's a rationalization of the currently
disparate core models, to allow to simplify them, ease their maintenance
and help spread the features from one realm to the next. Plugins would
be able to take benefit *or not* from this simplification and the new
services offered. So you'll still be able to build custom data model
objects and diverse interfaces, as usual. But if you don't have very
specialized needs, it should be easier for you to leverage the existing
infrastructure for creating new model objects and benefit from some
common utilities. Not all plugins need to start from scratch, over and
over again, or, regarding the current heterogeneity of the core classes,
have to fight their way through their low level details (i.e.
ticket/ticket_changes/ticket_custom vs. wiki vs. milestone, all
different approaches, different APIs as well as Remy noted).
I know, at one stage I actually championed accessing model through
resource, but that was in the name of "security" and not
"convenience". My idea was that the resource system could be the
gateway to accessing data in a new abstracted "business layer" that
worked regardless of entry point (commandline, request, ++) and where
security was included for free - the developer need not be concerned
with security directly as that would be gated through the resource
system.
Well, the point of a discussion like this one is to go forward, share
and "shake" ideas. Being a few years old doesn't invalidate an idea or a
proposal. Lots of the new stuff you'll find in Trac 0.12 has been first
discussed years ago as well...
Doing the permission checks in an intermediate "business" layer is
certainly a valuable goal. Beyond the data model object, we need to have
access to the "actor" which encapsulates the identity of the author of
the change (a role taken by the PermissionCache object, currently). That
would certainly be an improvement over having all that code in the
IRequestHandler implementation and rely on the web Request for the actor
role. We could also use this intermediate layer to store some state
related to the operation at hand, something we can't obviously do in a
Component which is global to the environment. This is reminiscent of the
refactoring of the vc layer started by Peter Dimov and me a few years
ago (http://trac.edgewall.org/changeset/4968 though at that time we were
not abstracting away from the Request).
-- Christian
We won't be getting there (it has been a few years since we discussed
this), and now I have a strong preference for simple and explicit.
Accessing or inferring anything about models through resources is -1
from me.
:::simon
https://www.coderesort.com
http://www.ohloh.net/accounts/osimons
--
You received this message because you are subscribed to the Google Groups "Trac
Development" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/trac-dev?hl=en.
