Closed mayfield closed 10 years ago
Okay, multiple issues, all of them bugs, and not what I intended. :( for my mistakes, but :) for you catching them :D
For the first issue, passing an argument into the Lua piece to delete the hash itself is kosher, I'd take a pull request and tests.
For the deletion resurrection, the fix is simply to replace session.add(self)
with session.forget(self)
. That was the original intent (as per the session object documentation), and it passes the current test suite... so I'd call it good. If someone was relying on obj.delete(); session.commit()
resurrecting the object, their code is buggy, and I don't feel bad if it breaks.
Please submit your pull request for the first part, and if you want to change one more line, the second part too. Either way I'll try to review, merge, maybe add a test or two, then cut a release as soon as I can.
Thank you yet again for the bug report :)
Hi Josiah,
I've been debugging a regression in our use of rom with new builds (USE_LUA=True) and in my efforts to write tests for said regression I've run into a couple things I'm not comfortable with. The heart of the first issue is that with the lua writer, deletions of models are not always purged from the redis DB. The _apply_changes() method relies on column value changes to instigate a HDEL ...
In the case of a related column whose value is set to None (after having previously been set) we never inform the lua writer that it needs to call HDEL for that attribute. As a result the hash entry for this model will remain in redis with a key/value pair of this attr and any others with the same conditions. When not using lua an explicit DEL method is called on the hash bucket itself, ensuring that the DB is cleaned up.
I have a patch for this that I'd like to provide which passes the modality of the apply_changes to the lua writer so it can also perform the DEL when appropriate. Ie. I pass is_delete=bool to the lua writer so it knows if it should call DEL.
Second issue - In attempting to be a good TDD citizen I wrote tests for the above scenario. It's a little strange to reproduce the issue in a test case however because the session layer has some unexpected impacts during model deletion. In our (cradlepoint) production code we turn off sessions so it's relatively obvious what is happening but with sessions enabled (default), things like model deletion have to be purged from the session cache by calling rollback(); This is rather surprising and even a bit magical. For example..
The reason the first call fails is that Model.delete() surprisingly calls session.add(self) as its last line; I can't decide if that's intentional or not. I suspect it is based on the optimism of the session cache (ie. using weak refs until the bitter end (eg. rollback())).
The second call fails because the session.commit() will clear it's 'known' object list (for that thread) but not the weakref dictionary, 'wknown'. The only way to clear the weakref session cache is by calling session.rollback(). My opinion is that the weakref table should just go away altogether. It makes the model's life cycle indeterminate unless you follow some non-obvious patterns (magic/must-read-source-to-understand). I.e. model.delete() followed by session.rollback() reads as though I rolled the delete back, when in fact I persisted it for subsequent callers.
I'll happily provide pull-requests for both issues including tests (py2k and py3k) if you're amenable. Let me know how this strikes you.