Opened 5 years ago

Closed 3 years ago

Last modified 4 months ago

#30581 closed New feature (fixed)

Allow constraints to be used for validation (in Python)

Reported by: Carlton Gibson Owned by: Gagaro
Component: Database layer (models, ORM) Version: 4.0
Severity: Normal Keywords: constraints, validation
Cc: Simon Charette, Adam Johnson, Ian Foote, Fabio Caritas Barrionuevo da Luz, Gagaro Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Follow-up from #30547, where we documented how database constraints relate to validation.

Executive summary: In general they don't. Instead you get an IntegrityError on save(). Except, UniqueConstraint is able to tie-in to the existing validate_unique() logic, so you will get a ValidationError on full_clean().

It would be nice if all constraints could (in principle) be used for Python-level validation, in addition to setting up the database constraint. (We might image ModelForm, say, being able to extract correct validation logic from the model definition, and so on.)

Initial thought is that Python validators could be inferred from Q objects:

CheckConstraint(check=Q(age__gte=18), name='age_gte_18')

Looks like it maps to a simple lambda age: age > 18, for example.

More complex examples will cause issues. Discussion:

So we might be able to do some auto-generation but first goal here should (I guess) be an optional hook on BaseConstraint that if implemented can be picked up for use in validation. (Or similar.)

I'll create this as Accepted, since existing discussions imply that.

Change History (31)

comment:1 by James Timmins, 5 years ago

@carltongibson Is this something I can take on? Or do we need more design discussions?

comment:2 by Carlton Gibson, 5 years ago

Well, feel free, but read those comments. :)

If it were me, I’d come up with a half idea and then post to the DevelopersMailingList
to get input from others, before coding too much... — but yes, it just needs someone to try I’d guess.

comment:3 by James Timmins, 5 years ago

Ok, I appreciate advice! I'll put together some thoughts and get feedback before assigning to myself or writing too much code (or figuring out its beyond my knowledge level). Gracias.

Ah ok, on closer examination I see what you're saying. I didn't fully grasp the extent of the ambitions :)

Last edited 5 years ago by James Timmins (previous) (diff)

comment:4 by Simon Charette, 5 years ago

FWIW I think the validation should be pushed to the database as much as possible. Trying to emulate constraint criterias in Python will hard if not impossible to implement in a correct way for all backends.

I'm not against an overridable hook but it should default to querying the database (e.g. 'SELECT %s > %s', (age, 18)).

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:5 by Sanskar Jaiswal, 5 years ago

Owner: changed from nobody to Sanskar Jaiswal
Status: newassigned

comment:6 by Sanskar Jaiswal, 5 years ago

I went through the previous pull request comments. As far as I could understand, generating some sort of validation from a Q object seems complicated, and there would arise a lot of issues. Does anyone have any recommendations?

comment:7 by Simon Charette, 5 years ago

Cc: Simon Charette added

I suggest you have a look at the existing Model.validate_unique logic and draft an API that would allow users to define their own constraints with custom validation.

The interface should be flexible enough to implement concepts such as unique_for_date (and friends) and exclusion constraints in a way that is similar to how Field.unique=True and Model._meta.unique_together are handled right now.

I believe that we should aim for a setup where all of the current unique validation logic is deferred to UniqueConstraint by making Field.unique=True and unique_together result in UniqueConstraint.auto_created = True entries in Model._meta.constraints. These auto_created constraints entries would be ignored by migrations, just like ManyToManyField auto-created intermediary models are, and should prove that the constraint enforcing mechanism is flexible enough to cover all of the current use cases validate_unique currently has.

I order to achieve that we'll likely want to define a new Constraint.enforce (or validate) method that accepts an optional existing model instance and a set of excluded fields that would raise on violation. It would then be the responsibility of the subclass to implement or not this function (e.g. we could skip it on UniqueConstraint with a condition at first as they are more complex to implement).

I hope this gives you a better idea of a plan to tackle this issue. Happy to move the discussion to the mailing list to gather more feedback if deemed more appropriate, that's just the way I envisioned it.

comment:8 by Sanskar Jaiswal, 5 years ago

Thanks for this informative reply. I’ll be sure to look into these and come up with some sort of a plan, to be sent to the mailing list. I am quite new (this is my first ticket), so I hope you guys will bear with me if I say something stupid.

comment:9 by Sanskar Jaiswal, 5 years ago

I have gone through the issue and the related source code. I think I have a fair idea of what's causing the issue. What I dont understand is that what this ticket aims to achieve. Is there supposed to be a solution, to raise an error upon CheckConstraint? Or is it supposed to be a custom validation method in BaseConstraint, which lets users help define custom validation? What exactly is the Constraint.enforce method supposed to do? It would be nice if I could get a bit more help with some examples. I am sorry, that I am having hard time understanding this, as I am a beginner.

comment:10 by Simon Charette, 5 years ago

Sanskar Jaiswal, to be honest I think comment:7 provides a pretty good picture of what ought to be done without baking in too many implementation details.

This ticket might be bit too tricky for a first as it requires good knowledge of the ORM, model validation, and database constraints. I suggest you have a look at other ones if you don't feel comfortable handling this one right now.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:11 by Simon Charette, 5 years ago

FWIW I started exploring the above idea and published my changes which pass most of the test suite but still require a bit of polishing.

In the light of the current barrage of issues related to checks that are not UniqueConstraint aware I think the Constraint.auto_created creation by Field.unique and Model.Meta.unique_together has merits as it would allow all of the unicity checks to be performed against _meta.constraints (or the newly added total_unique_constraints) instead of combining the three APIs all over the place.

comment:12 by Adam Johnson, 5 years ago

Cc: Adam Johnson added

comment:13 by Ian Foote, 5 years ago

Cc: Ian Foote added

comment:14 by Fabio Caritas Barrionuevo da Luz, 4 years ago

Cc: Fabio Caritas Barrionuevo da Luz added

comment:15 by Gagaro, 3 years ago

Cc: Gagaro added
Owner: changed from Sanskar Jaiswal to Gagaro

This issue is being discussed on the mailing list and implementation has started.

Last edited 3 years ago by Gagaro (previous) (diff)

comment:16 by Gagaro, 3 years ago

Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Version: 2.24.0
Last edited 3 years ago by Mariusz Felisiak (previous) (diff)

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In bf524d22:

Refs #30581 -- Allowed sql.Query to be used without model.

comment:18 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 7325d291:

Refs #30581 -- Fixed DatabaseFeatures.bare_select_suffix on MySQL < 8 and MariaDB < 10.4.

comment:19 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 9d047112:

Refs #30581 -- Added Q.flatten().

comment:20 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 5d91dc8e:

Refs #30581 -- Added Q.check() hook.

comment:21 by GitHub <noreply@…>, 3 years ago

In 27b07a32:

Refs #30581 -- Moved CheckConstraint tests for conditional expressions to migrations.test_operations.

This allows avoiding warning in tests about using RawSQL in
CheckConstraints.

comment:22 by Mariusz Felisiak, 3 years ago

Needs documentation: unset

comment:23 by Mariusz Felisiak, 3 years ago

Patch needs improvement: unset

comment:24 by Mariusz Felisiak, 3 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:25 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 66710587:

Fixed #30581 -- Added support for Meta.constraints validation.

Thanks Simon Charette, Keryn Knight, and Mariusz Felisiak for reviews.

comment:26 by Carlton Gibson <carlton@…>, 2 years ago

In ce732193:

Refs #30581 -- Updated count of steps in model validation docs.

Follow-up to 667105877e6723c6985399803a364848891513cc.

comment:27 by Carlton Gibson <carlton.gibson@…>, 2 years ago

In daf83303:

[4.1.x] Refs #30581 -- Updated count of steps in model validation docs.

Follow-up to 667105877e6723c6985399803a364848891513cc.

Backport of ce7321932d07b7bf9e6be77b9865c5058d9c1e4d from main

comment:28 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 35911078:

Replaced Expression.replace_references() with .replace_expressions().

The latter allows for more generic use cases beyond the currently
limited ones constraints validation has.

Refs #28333, #30581.

comment:29 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

In 13922580:

Refs #30581 -- Made unattached UniqueConstraint(fields) validation testable.

The logic allowing UniqueConstraint(fields).validate to preserve backward
compatiblity with Model.unique_error_message failed to account for cases where
the constraint might not be attached to a model which is a common pattern
during testing.

This changes allows for arbitrary UniqueConstraint(fields) to be tested in
isolation without requiring actual models backing them up.

Co-authored-by: Mark G <mark.gensler@…>

comment:30 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

In a2791f5e:

[5.1.x] Refs #30581 -- Made unattached UniqueConstraint(fields) validation testable.

The logic allowing UniqueConstraint(fields).validate to preserve backward
compatiblity with Model.unique_error_message failed to account for cases where
the constraint might not be attached to a model which is a common pattern
during testing.

This changes allows for arbitrary UniqueConstraint(fields) to be tested in
isolation without requiring actual models backing them up.

Co-authored-by: Mark G <mark.gensler@…>

Backport of 13922580cccfb9ab2922ff4943dd39da56dfbd8c from main.

comment:31 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

In c3066982:

[5.0.x] Refs #30581 -- Made unattached UniqueConstraint(fields) validation testable.

The logic allowing UniqueConstraint(fields).validate to preserve backward
compatiblity with Model.unique_error_message failed to account for cases where
the constraint might not be attached to a model which is a common pattern
during testing.

This changes allows for arbitrary UniqueConstraint(fields) to be tested in
isolation without requiring actual models backing them up.

Co-authored-by: Mark G <mark.gensler@…>

Backport of 13922580cccfb9ab2922ff4943dd39da56dfbd8c from main.

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