launchpad-dev team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

On 5 May 2011 09:17, Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx> wrote:
> On Wed, May 4, 2011 at 10:32 PM, Gavin Panella
> <gavin.panella@xxxxxxxxxxxxx> wrote:
[...]
>> Instead we're learning to write this like:
>>
>>    owners = bulk.load_related(Person, lots_of_bugs, "ownerID")
>
> I suspect this should be a Storm feature. Something like
> Bug.owner.load(lots_of_bugs)

That would be neat. +1

Want to write it? :)

>> One approach to improving this situation could be to forbid (with
>> code) the use of Reference properties that are not already cached (in
>> the store cache; this is nothing to do with @cachedproperty).
>
> Rather than forbid, we could warnings.warn(). This will break tests,
> but continue to work live for any untested code paths that sneak
> through.

I've been thinking that we could use this blocking in two modes:

- A warnings mode for development where we can see what's still using
  non-cached References (with a call stack). We could then either
  eager load, or annotate/context-manage when it's okay to deferenece.

- When the warnings are silenced the mode can be changed to raise an
  exception instead, to fence off that section of code so that it
  can't regress.

Interactions with GenerationalCache - see my message to Jeroen from
earlier - might scupper this idea; i.e. the warning mode might be as
far as we can go. Or, for production alone, we could select (via
config) the warnings-only mode, and keep exceptions-mode for
development only.

>
>> Doing this everywhere is not going to fly, at least right now -
>> there's probably way too much code that would break if we did this -
>> and sometimes we would want to permit their use - when we're just
>> looking up a single object for example.
>
> Adding a ratchet might help for the broken tests. ie. only raise that
> warning the 10th time we dereference, resetting the counter in
> DatabaseLayer.testTearDown().

We often only create one or two entries for a collection for tests, so
a count may never get high enough in the test suite. Test set-up would
also contribute to a count, possibly causing false warnings.

[...]
> Enforcing this will catch issues, but I can't see how to enforce it
> without also requiring the bulk load syntax be used for accessing a
> single item. I suspect this would be an awful lot of fallout in the
> current code base, and a lot of typing to fix as I don't think we can
> use a script to migrate the code.

The Reference.unblocked() context manager is for accessing a single
item.

Gavin.