-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Support violation_error_code and violation_error_message from UniqueConstraint in UniqueTogetherValidator #9766
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 our terms of service and privacy 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?
Support violation_error_code and violation_error_message from UniqueConstraint in UniqueTogetherValidator #9766
Conversation
rest_framework/serializers.py
Outdated
for parent_class in [model] + list(model._meta.parents): | ||
for unique_together in parent_class._meta.unique_together: | ||
yield unique_together, model._default_manager, [], None | ||
yield unique_together, model._default_manager, [], None, None, None |
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.
This is a breaking change: Callers consuming the iterator will experience ValueError: too many values to unpack (expected 4)
.
For backwards compatibility, the old iterator structure should be returned, unless the new functionality is requested (such as via a default-False
argument).
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.
Thanks for the review. I agree it was a critical change — I’ve restored the original signature and took a different approach.
What's the use case for that? (Why not use model-level default?) |
…ture Extracted error message logic to a separate method. fix: conditionally include violation_error_code for Django >= 5.0 fix(validators): use custom error message and code from model constraints
24e69af
to
c54c658
Compare
Returning the constraint’s default message would change the output and might break users relying on the validator’s default message—for example, in tests or UI rendering. I consider that the current behaviour should be maintained. |
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.
Looks good overall, just minor questions!
rest_framework/serializers.py
Outdated
source_map[source].append(name) | ||
|
||
unique_constraint_by_fields = { | ||
constraint.fields: constraint for constraint in self.Meta.model._meta.constraints |
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.
This loop is over self.Meta.model._meta.constraints
while the loop in get_unique_together_constraints()
is over [model] + list(model._meta.parents)
. Is this difference intended?
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.
Fixed, thanks.
To be honest, I have doubts about the necessity of collecting constraints from parent models, as this contradicts the expected behavior of the framework.
-
When inheriting from an abstract model, the Meta class is fully inherited, including constraints. And we have a possibility to overload it. Django handles this correctly, and no additional parent traversal is required.
-
When inheriting from a non-abstract model (multi-table inheritance), a separate table is created with its own Meta class and set of constraints (if specified). The parent model’s constraints apply only to the parent model and should not be considered during validation of the child model.
Thus, collecting constraints from parent models is not only redundant, but may also lead to incorrect logic.
There’s also an open question: if we are traversing parents at all, why only direct parents (_meta.parents) and not the full inheritance tree (_meta.all_parents)? This seems inconsistent.
For now, I’m preserving the current (get_unique_together_constraints
-like) behavior to avoid unnecessary deviation, but I believe this deserves further discussion and potential improvement.
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.
I agree with this assessment, both that the current traversal approach may need revision, but also that this PR should continue to adhere to the current approach. @browniebroke FYI
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.
lgtm! Let's wait for @browniebroke's comment on the open conversation and then we're done!
Hi @browniebroke! Just a quick ping in case this got lost — would be great to get your review when you have time. |
Thanks for the patience and pinging again, I indeed missed that. Will try to take a look this week |
That looks good to me. Will have to wait a bit for merging at we are working out the next version to release (3.16.2 or 3.17) and when. Thanks for your patience |
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 adds support for custom violation error codes and messages from Django model-level UniqueConstraint definitions to be passed through to UniqueTogetherValidator in Django REST Framework serializers.
- Enhanced UniqueTogetherValidator to accept and use custom error codes and messages
- Updated serializer logic to extract custom violation messages and codes from UniqueConstraint objects
- Added comprehensive test coverage for both custom and default error handling scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
rest_framework/validators.py | Enhanced UniqueTogetherValidator with code parameter support and updated error handling |
rest_framework/serializers.py | Added constraint lookup logic and violation message extraction to pass custom codes/messages to validators |
tests/test_validators.py | Added test model and test cases to verify custom and default error message/code behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
unique_constraint_by_fields = { | ||
constraint.fields: constraint | ||
for model_cls in (self.Meta.model, *self.Meta.model._meta.parents) | ||
for constraint in model_cls._meta.constraints | ||
if isinstance(constraint, models.UniqueConstraint) | ||
} |
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.
The dictionary comprehension will overwrite constraints with identical field combinations from different model classes. This could result in losing custom error messages/codes if a parent class constraint is overwritten by a child class constraint with the same fields but different error configuration.
Copilot uses AI. Check for mistakes.
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.
@s-aleshin can you please cross check this suggestion?
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.
Hi! As mentioned in an earlier comment, iterating over parent models here might not be necessary — constraints are usually defined per model and not inherited unless explicitly copied.
That said, if we keep the current logic, we should switch the order in line 1611:
for model_cls in (*self.Meta.model._meta.parents, self.Meta.model)
Unfortunately, I can’t fix it right now — currently on vacation without a laptop. Will be able to fix in a week.
constraints = [ | ||
models.UniqueConstraint( | ||
fields=("username", "company_id"), | ||
name="unique_username_company_custom_msg", | ||
violation_error_message="Username must be unique within a company.", | ||
**(dict(violation_error_code="duplicate_username") if django_version[0] >= 5 else {}), |
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.
[nitpick] The inline conditional dictionary unpacking makes the code harder to read. Consider extracting this into a separate variable or using a cleaner approach for version-dependent constraint arguments.
constraints = [ | |
models.UniqueConstraint( | |
fields=("username", "company_id"), | |
name="unique_username_company_custom_msg", | |
violation_error_message="Username must be unique within a company.", | |
**(dict(violation_error_code="duplicate_username") if django_version[0] >= 5 else {}), | |
violation_error_code_kwargs = ( | |
dict(violation_error_code="duplicate_username") | |
if django_version[0] >= 5 else {} | |
) | |
constraints = [ | |
models.UniqueConstraint( | |
fields=("username", "company_id"), | |
name="unique_username_company_custom_msg", | |
violation_error_message="Username must be unique within a company.", | |
**violation_error_code_kwargs, |
Copilot uses AI. Check for mistakes.
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.
and this too
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.
I believe it’s not possible to specify violation_error_code_kwargs
in the Meta class, since it’s not a recognized attribute of Django’s Model.Meta — this would likely raise an error.
All possible Meta options are listed in the documentation:
https://docs.djangoproject.com/en/5.2/ref/models/options/#model-meta-options
Added support for retrieving custom violation_error_code (for django 5+) and violation_error_message from Django model-level UniqueConstraint definitions and passing them to UniqueTogetherValidator.
refs #9714 and #9352
The update ensures that:
• If violation_error_message or violation_error_code are explicitly defined on the model constraint, they are forwarded to the corresponding validator.
• If the constraint uses the default message/code, they are not passed, allowing the validator to use its own defaults.
Additionally, tests have been added to cover both scenarios:
• When a custom error message/code is used.
• When defaults are applied.
Note: The current structure of
get_unique_together_constraints()
may benefit from refactoring (e.g., returning a named structure for clarity and extensibility). This is outside the scope of this PR, but I may explore it in a future contribution.