Opened 13 years ago

Last modified 19 months ago

#15130 assigned Bug

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

Reported by: Tetsuya Morimoto Owned by: François Granade
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: multi-db
Cc: botondus@…, Carlos Palol, Menno Manheim 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 Tetsuya Morimoto 13 years ago.
_default_manager "using" multi-db
15130-tests.diff (2.4 KB ) - added by Ramiro Morales 13 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 Tetsuya Morimoto 13 years ago.
Tests (as an additon to the Django test suite) that occur ValidationError
15130.1.diff (4.4 KB ) - added by Ramiro Morales 13 years ago.
Patch fifing this issue by t2y plus fixed tests
15130.2.diff (4.5 KB ) - added by Ramiro Morales 13 years ago.
New version of the patch with fixes suggested by Alex
15130.3-tests.diff (10.0 KB ) - added by Ramiro Morales 13 years ago.
New tests

Download all attachments as: .zip

Change History (37)

by Tetsuya Morimoto, 13 years ago

_default_manager "using" multi-db

comment:1 by Adam Nelson, 13 years ago

milestone: 1.3

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

in reply to:  1 comment:2 by Karen Tracey, 13 years ago

milestone: 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 by Adam Nelson, 13 years ago

@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 by Ramiro Morales, 13 years ago

Triage Stage: UnreviewedAccepted

by Ramiro Morales, 13 years ago

Attachment: 15130-tests.diff added

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

comment:5 by Ramiro Morales, 13 years ago

Resolution: worksforme
Status: newclosed
Summary: Model.validate_unique method is not considered with multi-dbModel.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 by Tetsuya Morimoto, 13 years ago

Has patch: unset
Resolution: worksforme
Status: closedreopened

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 by Tetsuya Morimoto, 13 years ago

Has patch: set

by Tetsuya Morimoto, 13 years ago

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

comment:8 by Ramiro Morales, 13 years ago

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

by Ramiro Morales, 13 years ago

Attachment: 15130.1.diff added

Patch fifing this issue by t2y plus fixed tests

comment:9 by Ramiro Morales, 13 years ago

Needs tests: unset

by Ramiro Morales, 13 years ago

Attachment: 15130.2.diff added

New version of the patch with fixes suggested by Alex

in reply to:  9 comment:10 by Tetsuya Morimoto, 13 years ago

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 by Ramiro Morales, 13 years ago

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.

by Ramiro Morales, 13 years ago

Attachment: 15130.3-tests.diff added

New tests

comment:12 by Ramiro Morales, 13 years ago

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 by Béres Botond, 13 years ago

Cc: botondus@… added

comment:14 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: Bug

comment:15 by Ramiro Morales, 13 years ago

Component: ORM aggregationDatabase layer (models, ORM)
Easy pickings: unset

comment:16 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:13 by Tim Graham, 9 years ago

#20301 is a duplicate.

comment:14 by Tim Graham, 8 years ago

#25514 is another duplicate.

comment:15 by Anssi Kääriäinen, 8 years ago

Hmmh, I think the core problem is that .validate() doesn't take a using argument, while some of the checks are dependent on the used database. I don't think we can fix this without adding the using argument.

If we do that, then ModelForms should likely also have .is_valid(using=...) keyword argument. The next question is should the using argument be used for ModelChoiceField queryset checks, too? I say yes, with the exception that if the queryset has an explicit using argument, then honor that.

We can start with adding using argument to .validate() only, and then continue to further changes if wanted.

Any opinions if we should go forward with this?

comment:16 by Tim Graham, 8 years ago

Did you see the solution proposed in attachment:15130.2.diff? That seems on the right track to me. I don't see why the programmer should have to specify the database when we already have database routers to figure out where the model will be saved?

comment:17 by Anssi Kääriäinen, 8 years ago

Yes, that would be a step in the right direction. But using the write router is just a guess. There is an explicit way to tell which database to use when saving, so there should also be an explicit way to tell Django which database to use when validating.

The solution I see as the right one is:

  1) Use explicitly given database for validation
  2) Use write router's database for validation
  3) Use the default database for validation

And currently, if I'm not mistake, we have this:

  1) Use read router's database for validation
  2) Use the default database for validation
(here 2 is actually implemented by the router for reading)

So, just changing Django to use write router instead of read router as done in the attached patch would be an improvement, but not a complete solution.

comment:18 by Anssi Kääriäinen, 8 years ago

As an addition - maybe we don't even want the complete solution. Because if we add using to model.validate(), we kind of have to add it to ModelForm.is_clean(), too. And then we have to decide what we do with ModelChoiceField(Author.objects.all()), shouldn't it use the specified database for validation, too?

But it seems usages where you need to validate the same model against multiple databases really aren't that common. So, how much effort should we spend on fixing this, instead of for example trying to add composite foreign key support?

comment:19 by Michel Perez, 7 years ago

Is this already fixed?, I'm still having this problem using Django==1.10.3 and Python 3.5

in reply to:  19 comment:20 by Simon Charette, 7 years ago

Replying to Michel Perez:

Is this already fixed?, I'm still having this problem using Django==1.10.3 and Python 3.5

This has not been fixed yet.

comment:21 by Carlos Palol, 5 years ago

Cc: Carlos Palol added

comment:22 by Menno Manheim, 4 years ago

Cc: Menno Manheim added

comment:23 by François Granade, 20 months ago

I got badly bitten by this bug in #33912, and I think that there are two issues, related but not the same, here -

1) the current behavior (validating against the default database for object that live in another database) does not make sense
2) it should be be possible to define against what DB the validation happens.

To me, the first issue is a very serious bug, and the second a useful feature.

What about fixing the first one first ? For that, the patch 15130.2.diff above would work.

What would be needed for this patch to be accepted ? I can clean it up

comment:24 by François Granade, 19 months ago

Proposing a PR

comment:25 by François Granade, 19 months ago

Owner: set to François Granade
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top