Code

Opened 3 years ago

Last modified 16 months ago

#15130 new Bug

Model.validate_unique method doesn't take in account multi-db

Reported by: t2y Owned by:
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: multi-db
Cc: botondus@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Model.validate_unique() method has 2 methods, _perform_unique_checks() and _perform_date_checks(), which use _default_manager for queryset. But, they are not considered with multi-db, that queryset is as follows.

qs = model_class._default_manager.filter(**lookup_kwargs)

I encountered a problem when ModelForm.is_valid() method is called since BaseModelForm calls full_clean() -> post_clean() -> validate_unique(). I could add/change/delete data with multi-db in AdminSite by setting arbitrary database with "using" keyword. However, the validation uniqueness check is invalid if default database has the same data.

# see also
http://docs.djangoproject.com/en/dev/topics/forms/modelforms/#the-is-valid-method-and-errors
http://docs.djangoproject.com/en/dev/ref/models/instances/#validating-objects

I only tested default table operation in AdminSite applying attached patch.
It works for me.

Attachments (6)

using_multidb_for_validate_unique.patch (1.1 KB) - added by t2y 3 years ago.
_default_manager "using" multi-db
15130-tests.diff (2.4 KB) - added by ramiro 3 years ago.
Tests (as an additon to the Djago test suite) that don't fail
15130-tests_validate_unique.diff (2.6 KB) - added by t2y 3 years ago.
Tests (as an additon to the Django test suite) that occur ValidationError
15130.1.diff (4.4 KB) - added by ramiro 3 years ago.
Patch fifing this issue by t2y plus fixed tests
15130.2.diff (4.5 KB) - added by ramiro 3 years ago.
New version of the patch with fixes suggested by Alex
15130.3-tests.diff (10.0 KB) - added by ramiro 3 years ago.
New tests

Download all attachments as: .zip

Change History (24)

Changed 3 years ago by t2y

_default_manager "using" multi-db

comment:1 follow-up: Changed 3 years ago by adamnelson

  • milestone 1.3 deleted
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

This looks like a problem on the current milestone:1.2 and therefore is too late for milestone:1.3

comment:2 in reply to: ↑ 1 Changed 3 years ago by kmtracey

  • milestone set to 1.3
  • Needs tests set

Replying to adamnelson:

This looks like a problem on the current milestone:1.2 and therefore is too late for milestone:1.3

? We're not to the point of pushing bug-fixes out of 1.3 yet, I don't think. Features, yes, are no longer appropriate for the 1.3 milestone. But the description and the word "problem" there make this sound like a bug, not a feature. If it's been around since 1.2 it wouldn't be classified as a blocking bug, but it can still be in the milestone for 1.3. (Which does not guarantee it will get fixed for 1.3, but still it's not correct at this point to say a fix for this could not be put it for 1.3)

comment:3 Changed 3 years ago by adamnelson

@kmtracey ok, can you mark it as approved at least? It seems a bit late to be committing a patch with no tests and no approval for an issue that currently affects milestone:1.2 but I'm happy to defer if somebody wants to move it forward.

comment:4 Changed 3 years ago by ramiro

  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by ramiro

Tests (as an additon to the Djago test suite) that don't fail

comment:5 Changed 3 years ago by ramiro

  • Resolution set to worksforme
  • Status changed from new to closed
  • Summary changed from Model.validate_unique method is not considered with multi-db to Model.validate_unique method doesn't take in account multi-db

I've added tests (against trunk) that exercise the unique and unique_for_(date,month,year) field options plus the Meta.unique_together option by saving identical model instances to different databases. Saving is successful so I can't reproduce this at the ORM level.

I'm going to close this ticket as works for me. If you can describe better your use case (bonus points if they are a modification to the attached tests) your scenario, please reopen. I suspect this might be a problem in another part of the stack (admin app?) instead, or maybe I forgot to test some condition?.

comment:6 Changed 3 years ago by t2y

  • Has patch unset
  • Resolution worksforme deleted
  • Status changed from closed to reopened

Hi ramiro, thank you for reviewing and making tests code.
Though my explanation was not good, this problem occur only when a Model.validate_unique method is called. So, I fixed your test code to call validate_unique method. By running my test code, you can see ValidationError.

I think what the save method was successed makes you wonder. The reason why the save mehtod is considered with "using" parameter for multi-db.

comment:7 Changed 3 years ago by t2y

  • Has patch set

Changed 3 years ago by t2y

Tests (as an additon to the Django test suite) that occur ValidationError

comment:8 Changed 3 years ago by ramiro

Thanks for showing my tests were wrong. I didn't take in account Model.save() doesn't do validation by default.

Changed 3 years ago by ramiro

Patch fifing this issue by t2y plus fixed tests

comment:9 follow-up: Changed 3 years ago by ramiro

  • Needs tests unset

Changed 3 years ago by ramiro

New version of the patch with fixes suggested by Alex

comment:10 in reply to: ↑ 9 Changed 3 years ago by t2y

Replying to ramiro:

I tested my application(AdminSite) which found this problem originally after it applied Alex's patch. It's OK. Thank you Alex.

comment:11 Changed 3 years ago by ramiro

  • Patch needs improvement set

Russell suggested to avoid modifying ._state.db so test were changed so the fist model is saved to the 'other' DB and the second one to the 'default' DB:

class MultiDbUniqueTests(TestCase):
    multi_db = True

    def test_unique_multi_db_isolation(self):
        "Attempt to save two model instances that don't pass Field.unique checks to different DBs."
        UniqueFieldsModel(unique_charfield='Hello world', unique_integerfield=42, non_unique_field=3).save(using='other')
        b = UniqueFieldsModel(unique_charfield='Hello world', unique_integerfield=42, non_unique_field=3)
        try:
            b.full_clean()
        except ValidationError:
            self.fail("unique field validation shouldn't erroneosuly trigger when the identical model instances are on different databases.")
        else:
            b.save()
            self.assertEqual(UniqueFieldsModel.objects.using('default').count(), 1)
            self.assertEqual(UniqueFieldsModel.objects.using('other').count(), 1)

    def test_unique_together_multi_db_isolation(self):
        "Attempt to save two model instances that don't pass Meta.unique_together checks to different DBs."
        UniqueTogetherModel(cfield='Hello world', ifield=42, efield='user@example.org').save(using='other')
        b = UniqueTogetherModel(cfield='Hello world', ifield=42, efield='user@example.org')
        try:
            b.full_clean()
        except ValidationError:
            self.fail("Meta.unique_together validation shouldn't erroneosuly trigger when the identical model instances are on different databases.")
        else:
            b.save()
            self.assertEqual(UniqueTogetherModel.objects.using('default').count(), 1)
            self.assertEqual(UniqueTogetherModel.objects.using('other').count(), 1)

    def test_unique_for_x_multi_db_isolation(self):
        "Attempt to save two model instances that don't pass Field.unique_for checks to different DBs."
        today = datetime.date.today()
        now = datetime.datetime.now()
        UniqueForDateModel(start_date=today, end_date=now, count=314, order=21, name='Foo').save(using='other')
        b = UniqueForDateModel(start_date=today, end_date=now, count=314, order=21, name='Foo')
        try:
            b.full_clean()
        except ValidationError:
            self.fail("Meta.unique_for_* validation shouldn't erroneosuly trigger when the identical model instances are on different databases.")
        else:
            b.save()
            self.assertEqual(UniqueForDateModel.objects.using('default').count(), 1)
            self.assertEqual(UniqueForDateModel.objects.using('other').count(), 1)

Problem is: In this case these tests don't fail if the changes to django/db/models/base.py are undone.

So it seems there is some deeper problem here.

Changed 3 years ago by ramiro

New tests

comment:12 Changed 3 years ago by ramiro

15130.3-tests.diff contains new more granular tests (new model for tests, separated the unique_for_{date,month,year} tests). It runs ten new tests; five of them by saving first an instance to the 'default' database and then trying to save an identical instance to the 'other' DB, and the other five in the inverse order.

Theoretically all of them should fail before applying a fix for this ticket, but only the five tests with the 'default', 'other' order do.

comment:13 Changed 3 years ago by bberes

  • Cc botondus@… added

comment:14 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:15 Changed 3 years ago by ramiro

  • Component changed from ORM aggregation to Database layer (models, ORM)
  • Easy pickings unset

comment:16 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.