local_variables_are_none() check in Reference.__get__() not always appropriate

[James Henstridge]

Michael Hudson brought this problem to my attention today on IRC. Attached is a patch to add a test case.

The Reference.__get__() method includes a check to see if the local variables are None, and immediately return None if that is the case. This is intended to avoid querying the remote table when it is clear that the local key won't match any records.

However, if the reference has on_remote=True and the local variables are the local primary key, then the local variables being None could also indicate that the object is in pending add state. If the store were to be flushed and a primary key to be assigned, a remote object may be found.

This can happen if we have Reference() attributes going in both directions and the other reference was set to link the objects, as demonstrated in the attached test case.

[James Henstridge (jamesh)]

I've attached a branch that fixes this problem, but causes the test_reference_wont_touch_store_when_key_is_unset test to fail.

I am not convinced that the behaviour that test checks is desirable though, so the fix might be to remove that test.

[Gustavo Niemeyer (niemeyer)]

+1 on the branch and on disabling the bogus test.

I've just arrested Thomas for the second review and he's happy with it too, so the branch can be merged.

[Gustavo Niemeyer (niemeyer)]

The merging of this change was reverted in trunk.

As a side-effect of the introduced change, objects being constructed
could be flushed to the database inside their constructors simply by
accessing a reference which isn't set. Besides being undesired due
to the additional UPDATE queries that are executed after the CREATE,
this behavior can actually break code if the database expectations
(e.g. NOT NULL) are not entirely satisfied by the time the object
is flushed.

We have ideas for how to implement the desired behavior without
introducing this side effect, but this will require additional
research and time for the implementation.

[James Henstridge (jamesh)]

This change has been merged now, but I noticed that Gustavo noted a problem on IRC:

<niemeyer> jamesh: We've got a broken test in Landscape due to the flushing on references
<niemeyer> jamesh: The problem is that we're building certain objects in memory, and passing them around to build further objects
<niemeyer> jamesh: and the additional logic is forcing flushes in the middle of the procedure
<niemeyer> jamesh: Which means that what would be only a few CREATEs become unfinished CREATEs + UPDATEs
<niemeyer> That can potentially break up with IntegrityErrors because it's flushing mid-way through constructors :-(

It'd be good to work out what the root cause here is. My guess here is that the unset local keys here would be set to NULL by the database when the row is added, so Landscape is relying on the unset columns being treated as NULLs. Does that sound like what you're seeing, Gustavo?

[James Henstridge (jamesh)]

Actually, looks like it has been rolled back.