chore: correctly validate unique constraints with distinct condition fields #9744
Conversation
| for unique_together in parent_class._meta.unique_together: | ||
| yield unique_together, model._default_manager, [], None | ||
| for constraint in parent_class._meta.constraints: | ||
| if isinstance(constraint, models.UniqueConstraint) and len(constraint.fields) > 1: |
There was a problem hiding this comment.
do this need to be removed?
There was a problem hiding this comment.
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.
Shouldn't there be a distinction made between condition fields and constraint fields here?
There was a problem hiding this comment.
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.
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 or learn how to get started.
| else set() | ||
| ) | ||
| if len(field_set | condition_fields) == 1: | ||
| yield UniqueValidator( |
There was a problem hiding this comment.
Django’s UniqueConstraint supports custom error messages and codes via violation_error_message and violation_error_code, but as far as I can see these values are not propagated to UniqueValidator.
It would be great to have the same behaviour as for UniqueTogetherValidator, but I’m not sure this should be done within the scope of this PR.
There was a problem hiding this comment.
Good point about custom error messages! That would be a nice enhancement but outside this PR's scope. Worth tracking in a separate issue.
nefrob
force-pushed
the
nefrob/unique-constraint-with-condition-fields
branch
from
32c02c8 to
e09a89a
Compare
|
Thanks for the feedback! Ready for re-review when you have a chance. |
There was a problem hiding this comment.
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.
| def test_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`. |
There was a problem hiding this comment.
There is a typo in the docstring. The backtick at the end of "when the condition applies" should be removed.
| the constrained field differs when the condition applies`. | |
| the constrained field differs when the condition applies. |
4 participants
Description
Resolves: #9707.