Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
chore: correctly validate unique constraints with distinct condition fields#9744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
chore: correctly validate unique constraints with distinct condition fields#9744
Conversation
| forunique_togetherinparent_class._meta.unique_together: | ||
| yieldunique_together,model._default_manager, [],None | ||
| forconstraintinparent_class._meta.constraints: | ||
| ifisinstance(constraint,models.UniqueConstraint)andlen(constraint.fields)>1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do this need to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We're now including the condition fields as part of the applicable fields that contribute to this count below. If we kept this here the condition fields would not be considered, which we need for this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't there be a distinction made between condition fields and constraint fields here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could split out the logic for clarity to require >1 constraint fields or 1 constraint field and 1+ condition fields distinct from the constraint field. Practically that would be the same however.
If you want to split out unique together constraints with a single constraint field and distinct condition fields into a new method, that would require a larger refactor of the existing constraint validation code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR updates DRF’s validation to correctly treat single-field UniqueConstraint entries with additional condition fields as unique-together validations rather than per-field UniqueValidators.
- Extend unique-together detection to include UniqueConstraint with one field when the condition references other fields.
- Adjust field-level UniqueValidator generation to only apply when no additional condition fields are referenced.
- Add tests covering condition-based unique-together behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_validators.py | Adds a model, serializer, and tests for condition-based unique-together validation; updates expectations for single-field UniqueConstraint behavior. |
| rest_framework/utils/field_mapping.py | Imports helper and changes logic to only create UniqueValidator when the union of fields and condition fields is a single field. |
| rest_framework/serializers.py | Updates unique-together constraint discovery to include single-field UniqueConstraints when conditions reference additional fields. |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| elseset() | ||
| ) | ||
| iflen(field_set|condition_fields)==1: | ||
| yieldUniqueValidator( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Django’sUniqueConstraint supports custom error messages and codes viaviolation_error_message andviolation_error_code, but as far as I can see these values are not propagated toUniqueValidator.
It would be great to have the same behaviour as forUniqueTogetherValidator, but I’m not sure this should be done within the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point about custom error messages! That would be a nice enhancement but outside this PR's scope. Worth tracking in a separate issue.
32c02c8 toe09a89aComparenefrob commentedDec 12, 2025
Thanks for the feedback! Ready for re-review when you have a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡Add Copilot custom instructions for smarter, more guided reviews.Learn how to get started.
| deftest_is_unique_together_condition_based(self): | ||
| """ | ||
| In a condition-based unique together validation, data is valid when | ||
| the constrained field differs when the condition applies`. |
CopilotAIDec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is a typo in the docstring. The backtick at the end of "when the condition applies" should be removed.
| theconstrainedfielddifferswhentheconditionapplies`. | |
| theconstrainedfielddifferswhentheconditionapplies. |
Description
Resolves:#9707.