Return value from update()
Russell Keith-Magee
[9967] introduced a failure into the test suite, highlighting either
an error in the documentation, or a potentially nasty problem that
requires fixing.
According to the docs[1] :
"The update() method is applied instantly and doesn't return anything"
Except... it does.
Further complicating matters: the value that is returned varies
slightly depending on the database. Postgres and SQLite return the
number of rows that were potentially affected - i.e., the number of
rows that match the selection criteria. MySQL, however, returns the
number of rows that are actually modified. This means that if no
columns are changed as a result of an update, a successful MySQL
update call will return a value different that returned by SQLite and
Postgres.
This is the cause of the failure introduced by [9967] - this changeset
introduces a test [2] to model_inheritance_regress that makes 2
successive calls to update a model attribute to the same value - under
MySQL, the second call isn't a modification, so it returns 0, not 1.
So - which one is correct? Documentation or implementation? And which
one can we change without violating backwards compatibility?
Personally, I'm inclined to go with the documentation being correct -
we already have a backend inconsistency, and I can't imagine the
situation getting any better if non-SQL backends are added to the mix.
However, I am also sensitive to the fact that someone might have a use
case for a return value coming from update().
Opinions?
[1] http://docs.djangoproject.com/en/dev/topics/db/queries/#updating-multiple-objects-at-once
[2] http://code.djangoproject.com/changeset/9967#file2
Yours,
Russ Magee %-)
Malcolm Tredinnick
On Sat, 2009-03-14 at 14:10 +0900, Russell Keith-Magee wrote:
> Hi all,
>
> [9967] introduced a failure into the test suite, highlighting either
> an error in the documentation, or a potentially nasty problem that
> requires fixing.
>
> According to the docs[1] :
>
> "The update() method is applied instantly and doesn't return anything"
>
> Except... it does.
>
> Further complicating matters: the value that is returned varies
> slightly depending on the database. Postgres and SQLite return the
> number of rows that were potentially affected - i.e., the number of
> rows that match the selection criteria. MySQL, however, returns the
> number of rows that are actually modified. This means that if no
> columns are changed as a result of an update, a successful MySQL
> update call will return a value different that returned by SQLite and
> Postgres.
Aah .. I knew that one, once. I had completely forgotten about that.
Wow.. deja vu. Thanks for working it out. :-)
>
> This is the cause of the failure introduced by [9967] - this changeset
> introduces a test [2] to model_inheritance_regress that makes 2
> successive calls to update a model attribute to the same value - under
> MySQL, the second call isn't a modification, so it returns 0, not 1.
>
> So - which one is correct? Documentation or implementation? And which
> one can we change without violating backwards compatibility?
The documentation is wrong here. I hadn't realised we even documented
that, so I didn't check when I added that, way back when. We've been
returning something from update() since before 1.0, so it's a docs bug,
rather than a backwards-incompat change.
The reasoning here is this: We need the update return value for
Model.save(force_update=True). It's the only way to tell if a row was
actually updated. An update query that alters zero rows still succeeds,
but it is in wrong in that particular siutuation.
The test in question, that is failing, could be converted into a failing
force_update example, for example. We need to fix the MySQL handling
there and work out how to tell if *any* rows are updated. Actually,
that's what we really need there: a way to tell if any rows are updated.
The actual number is pretty irrelevant for our purposes. So I'd be happy
documenting that it returns True/False if we can get that indication out
of the update.
I understand what you're saying here, though, so it might not be easy.
I'll think about it a bit, but I'd like to make that return value
meaningful on all backends.
[...]
> However, I am also sensitive to the fact that someone might have a use
> case for a return value coming from update().
Yeah, the guy who added it does. (hi there!) :-)
Regards,
Malcolm
Malcolm Tredinnick
[...]
>
> I understand what you're saying here, though, so it might not be easy.
> I'll think about it a bit, but I'd like to make that return value
> meaningful on all backends.
Just about to run out the door and I'll be offline until tomorrow, but I
wanted to throw out this idea: I'd be happy saying "wontfix" for now on
MySQL and we can conditionally avoid that test there.
This is solved by adding "work out which columns changed" support to
models, which I was intending to look at in the 1.2 timeframe. I might
make time to bump that up to 1.1 (or we just say it's a known issue in
1.1 and will be fixed thereafter). Once we have that, if no columns have
changed, force_update doesn't necessarily have to do anything (it's not
quite that clear, but that's the idea -- see below), so we're fine and
if it does have to do something, MySQL shouldn't return zero rows
changed, so we have error detection back again.
So a do nothing approach is survivable here for now. But, again, I'll
keep thinking.
Note: one reason this problem is potentially tricky is because it can
only be optional behaviour. There's a time-race involved: load Model A,
data changes, try to save Model A unchanged and it should reset the
data, but won't because it looks like nothing has changed. So we can't
turn it on unconditionally as some use-cases will fail because of it. I
don't want to turn this thread into an incremental update thread, but
that's why this is something that's still open.
Regards,
Malcolm