Make pure python match C: call 'jar.register()' in '__setattr__'/'_p_register' even if OID is not set.

[tonich-sh]

fixes #28

[tseaver]

@jamadden Can you have a look? This seems similar to some of the discrepancies you tracked down.

@tonich-sh this PR needs to add tests for the code being changed: the tests should fail without the changes, and pass with them.

[jamadden]

@tseaver There does appear to be a discrepancy. But it's deeper than just calling register. In the example code in #28, the C implementation sets _p_changed to True when a.name = 'test' is executed, but the Python implementation doesn't.

However, the example code is violating the specification for a data manager and so we're in undefined territory. According to IPersistent in interfaces.py, where the state transitions are documented (emphasis mine):

...
- Saved -> Changed
Sticky -> Changed
Ghost -> Changed

 This transition occurs when someone sets an attribute or sets
 _p_changed to a true value on a saved, sticky or ghost object.  When
 the transition occurs, the persistent object is required to call the
 register()  method on its data manager...

...
In all the above, __p_oid (the persistent object id) is set when p_jar first gets set.

So in my reading, having a _p_jar without also having a _p_oid is undefined behaviour and neither implementation is either correct or wrong. They both behave identically when _p_oid is set as per the documentation.

This discrepancy was not a problem in practice for ZODB or our application (in fact it was never noticed) because ZODB's Connection always does what the documentation says and sets oid and jar together (Connection.py:227, Connection.add):

       elif obj._p_jar is None:
            assert obj._p_oid is None
            oid = obj._p_oid = self.new_oid()
            obj._p_jar = self

[tseaver]

@tonich-sh can you provide a valid usecase for setting _p_jar without also setting _p_oid?

[tseaver]

@tonich-sh Before we can consider merging, the PR needs to add a test which fails without your change to persistent.py (for the pure-Python case) but passes for both cases with your change. It would probably be a straightforward mutation of the example in #28 into a testcase method in the PyPersistentTests class.

[tseaver]

@jamadden You are using the pure-Python versions most heavily of anyone: would merging this PR break anything for you?

[jamadden]

It will probably be next week before I can check.

Sent from my iPad

On Aug 10, 2016, at 12:13 PM, Tres Seaver notifications@github.com wrote:

@jamadden https://github.com/jamadden You are using the pure-Python
versions most heavily of anyone: would merging this PR break anything for
you?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABMqkhpT5bsefAQvA8J6w9lqWoM43140ks5qegalgaJpZM4FpdCw
.

[jamadden]

I haven't run all the test suites, but I don't think this change would cause us a problem.

However, without a motivating example from the OP, I also don't know how valuable it is (especially if it doesn't also deal with _p_changed). It appears to be a workaround for broken data managers, so the "correct" solution to the problem would be to fix the bad data manager. On the other hand, if this change does go in, then the documentation I quoted above should be changed to explicitly re-classify such currently-broken data managers as correct.

[jamadden]

It's been two years with no comments from the author of this PR addressing the feedback, so I'm going to assume that @tonich-sh has moved on and I will close this PR. If that's not true, feel free to reopen!