Opened 6 years ago

Closed 20 months ago

#10967 closed Bug (duplicate)

save(force_update=True) crashes with model inheritance

Reported by: lgs Owned by: nobody
Component: Database layer (models, ORM) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When there is a subclass with no fields and you try to save it with obj.save(force_update=True) it will crash with this exception:

AttributeError: 'NoneType' object has no attribute 'rowcount'

The problem is that an or condition should be an and instead. I'm talking about the check in Model.save_base that test if there are non primary fields to save. This is better explained by the patch itself.

Attachments (5)

simple-patch-in-save_base.diff (741 bytes) - added by lgs 6 years ago.
Patch that fix this error
models.py (656 bytes) - added by lgs 6 years ago.
Simple models that show this problem
10967-test.diff (1.0 KB) - added by kmtracey 5 years ago.
patch_vanschelven (2.3 KB) - added by vanschelven 4 years ago.
Patch w/ fix & tests
patch_vanschelven.2 (2.1 KB) - added by vanschelven 4 years ago.
Patch w/ tests

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by lgs

Patch that fix this error

Changed 6 years ago by lgs

Simple models that show this problem

comment:1 Changed 6 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by russellm

  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by paltman

  • Has patch set

Changed 5 years ago by kmtracey

comment:4 Changed 5 years ago by kmtracey

  • Patch needs improvement set

I've attached a diff with a test added to model_inheritance_regress. However, the proposed fix causes other tests in that testcase to fail:

Doctest: regressiontests.model_inheritance_regress.models.__test__.API_TESTS ... FAIL

======================================================================
FAIL: Doctest: regressiontests.model_inheritance_regress.models.__test__.API_TESTS
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kmt/django/trunk/django/test/_doctest.py", line 2180, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for regressiontests.model_inheritance_regress.models.__test__.API_TESTS
  File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line unknown line number, in API_TESTS

----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    Place.objects.all()
Expected:
    [<Place: Derelict lot the place>, <Place: Guido's All New House of Pasta the place>]
Got:
    [<Place: Guido's House of Pasta the place>, <Place: Main St the place>]
----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    [sorted(d.items()) for d in dicts] == [[('name', u"Guido's All New House of Pasta"), ('serves_hot_dogs', False)]]
Expected:
    True
Got:
    False
----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    [sorted(d.items()) for d in dicts] == [[('name', u"Guido's All New House of Pasta"), ('serves_gnocchi', False), ('serves_hot_dogs', False)]]
Expected:
    True
Got:
    False
----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    [sorted(d.items()) for d in dicts]
Expected:
    [[('capacity', 50), ('name', u'Derelict lot')]]
Got:
    [[('capacity', 100), ('name', u'Main St')]]
----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    [sorted(d.items()) for d in dicts] == [[('name', u"Guido's All New House of Pasta"), ('serves_gnocchi', False), ('serves_hot_dogs', False)]]
Expected:
    True
Got:
    False
----------------------------------------------------------------------
File "/home/kmt/django/trunk/tests/regressiontests/model_inheritance_regress/models.py", line ?, in regressiontests.model_inheritance_regress.models.__test__.API_TESTS
Failed example:
    Place.objects.get(pk=ident)
Expected:
    <Place: Guido's All New House of Pasta the place>
Got:
    <Place: Guido's House of Pasta the place>


----------------------------------------------------------------------
Ran 1 test in 0.566s

FAILED (failures=1)

Thus it seems the proposed fix is not quite right and a correct fix requires some more investigation.

comment:5 Changed 5 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

comment:6 Changed 4 years ago by vanschelven

Hi,

I figured I'd take a look at this problem as it was on the list for 1.3.

The offending code is indeed in django/db/models/base.py

522                     if force_update or non_pks:
523                         values = [(f, None, (raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks]
524                         rows = manager.using(using).filter(pk=pk_val)._update(values)
525                         if force_update and values and not rows:
526                             raise DatabaseError("Forced update did not affect any rows.")

For a subclass with no fields this loop is executed twice, once for the base class and once for its child. The second time values is an empty list, therefore rows is 0 and the exception is raised on line 526. I propose simply adding a check for values, and only raising an exception if values is non-empty. Patch will be attached.

Klaas

Changed 4 years ago by vanschelven

Patch w/ fix & tests

comment:7 Changed 4 years ago by vanschelven

Ok, I added the patch and tests that prove the problem exists. I'm not sure why my patch shows so badly in Trac, would gladly resubmit if someone has pointers about that.

comment:8 Changed 4 years ago by anonymous

I'd say the #HG comments at the beginning of the patch aren't recognized by trac. Maybe remove them manually ?

Changed 4 years ago by vanschelven

Patch w/ tests

comment:9 follow-up: Changed 4 years ago by vanschelven

meh... still no coloring in the patch... hope this doesn't lead to trouble applying it.

comment:10 in reply to: ↑ 9 Changed 4 years ago by kmtracey

Replying to vanschelven:

meh... still no coloring in the patch... hope this doesn't lead to trouble applying it.

Try giving the file an extension of .diff

comment:11 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:12 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 20 months ago by akaariai

  • Resolution set to duplicate
  • Status changed from new to closed

This is a duplicate of already fixed #13864.

Note: See TracTickets for help on using tickets.
Back to Top