launchpad-dev team mailing list archive

[Robert Collins <robert.collins@xxxxxxxxxxxxx>]

On Wed, Dec 22, 2010 at 7:15 PM, Martin Pool <mbp@xxxxxxxxxxxxx> wrote:
>> * Explicit actions whenever possibly expensive operations will occur.
>> * Multiple object / object graph operations.
>
> +1
>
> In your description of layering, is there supposed to be another layer
> that describes how model objects map to a store?  Is this direct from
> model object to a particular store, or do you map to something
> abstracted across stores, or both?

the backend defines that; its a sub-layer of the persistence layer,
and all the storm definitions and so on will end up in our storm
backend.

> In the examples it would be nice to see a worked example of defining a
> new model class.  For example if we wanted to track (uh I'm lost for
> examples because Launchpad tracks so many things) but let's say a
> person's plane tickets, we would add:
>
>  a PlaneTicket model object?

Yes.

>  a mapping from that to psql?  is this inline with the model or separate?

A Storm class

>  the SQL DDL to add it to the db? or is this inferred?

Manual, as we do today - unchanged.

>  a query class for tickets?  or is there a sufficient generic default?

There will be something highly magical, but curried or not we'll want
a lp.$thing.persistence.planeticket.PlaneTicket class - that defines
the needed data for any PlaneTicket method calls.

>>    bug = Bug(launchbag.user, launchbag.context)
>
> I guess user is there just as 'filed by this user'?

Yeah- the launchpad carries current request state around. Its
'context' in a DI environment.

>>     transaction.add(user, bug)
>
> I realize it's near-pseudocode but I think it's better not to pass
> lists of arbitrary things as *args.

mmm, I can go either way. Sometimes its lovely, sometimes not. I'm not
committed to this spelling.

>    query = tranasction.make_query()
>    query.projects.filter = ByName('bzr')
>    return query.projects.bugs.execute()
>
> It seems odd that traversing the 'projects' attribute on the query
> makes it be a query about projects, if that's what the example is
> supposed to convey.  Also unfortunate to need to declare a new class
> ByName?  This style seems to make it hard to select, say, tasks that
> touch both bzr and launchpad, even though you could say that easily
> enough in sql.

query.projects.filter = Or(ByName('bzr'), ByName('launchpad'))

There is more about this on LEP/PersistenceLayer.

> What's returned?  An iterable of projects?

A list of bug model objects.

> If we want good control over when the db is touched it seems useful to
> be able to piggyback together queries that return similar data used by
> different bits of code.  (This may not always actually be faster but
> perhaps it's worth allowing for doing it.)  So that seems to mean its
> useful to let the result a query pull back many objects, and then to
> get just a particular aspect of them, and therefore it should give
> back something richer than just an iterable.

Yes, and no. Returning just an object graph is sufficient: if we need
unrelated data, then its not related. If its related, there are object
links we can follow to get it.

> It seems like to describe a query you need to describe what parts of
> the object graph you wish to be able to traverse in the result.  You
> need to know ahead of time, because we want to avoid people going back
> to the db to ask for just a bit more and just a bit more.  This
> delineation is in two dimensions, at least: which relation attributes
> you want to traverse, and which values of them are wanted, if not all.

Yes, thats what this does.

> It may be worth explicitly mentioning (if true) that db access only
> happens during .execute(), .commit(), and perhaps a few other
> operations.  You never get lazy query results back.

Indeed! I'll add that.

>    def getConjoinedMaster(bugtask_collection_query, *args, **kwargs):
>        """Expand bug_collection_query to permit getConjoinedMaster calls.
>
>        See `lp.bugs.model.bugtask.BugTask.getConjoinedMaster` for the
>        arguments accepted by this call.
>        """
>        # We need to access the bugtasks and their series
>        bugtask_collection_query.distroseries
>        # and their product series
>        bugtask_collection_query.productseries
>        # and the distribution and its currentseries
>        bugtask_collection_query.distribution.currentseries
>        # and we access products too
>        bugtask_collection_query.product.development_focus
>
> what happens to the starargs? Are they somehow magically read out?

They are ignored in this method; a more sophisticated planner method
may only extend the query depending on the arguments.

> It looks like you're saying merely touching an attribute causes it to
> be brought in.  That's kind of cool, but perhaps too magical.  I
> wonder too if it will cause trouble you want to specify
>
>  bug.assignee == Person
>  bug.reporter == Person
>
> Is that an 'and' or an 'or'?  You might reasonably want either.

Generally we don't have a Person to specify at this stage, except in
very rare cases. For the planner we're mainly expanding the graph
rather than constraining it. However when we do need to exclude, the
filter attribute should be sufficient.

> It seems one key thing about queries is that
>
>    query = transaction.make_query()
>    query.bugs.tasks.project.ByName('bzr')
>    query.bugs.execute()
>
> I wonder should that be query.execute().bugs?  And say
> query.bugs.tasks.project.where('name', Equals, 'bzr').

That would couple model-mapping objects to the query graph - this
conflates separate concerns and I think would work poorly. Consider
'assertEquals' for an analogous issue you're well familiar with :)

Thanks for the feedback, it is useful.

-Rob
 -