-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Fix UniqueConstraint validation with conditional fields (#9707) #10021
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
Open
majidkhazaei
wants to merge
13
commits into
encode:main
Choose a base branch
from
majidkhazaei:pr-9744
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
8d796e8
chore: unique constraints with distinct condition fields use unique t…
nefrob a2cc17a
chore: add comments and improve tests
nefrob 867e9be
test: correct assertions
nefrob ff4ced3
Add get_referenced_base_fields_from_q to compat and fix test indentation
nefrob d773cff
Fix pre-commit formatting issues (isort and flake8)
majidkhazaei e8794dc
Apply Copilot suggestions: fix heading hierarchy and use Django's Q.r…
majidkhazaei 51b7897
Fix all remaining issues: empty constraint.fields, preserve constrain…
majidkhazaei d49f7ee
Merge branch 'main' into pr-9744
auvipy 54c7472
Merge branch 'main' into pr-9744
auvipy 93e84b8
Fix isort import ordering in serializers.py
majidkhazaei cbfead3
Resolve merge conflicts with upstream/main in validators and tests
majidkhazaei 06a6057
Merge branch 'main' into pr-9744
auvipy 970392e
Refactor uniqueness validation test case
auvipy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,9 @@ | |
| from django.utils.functional import cached_property | ||
| from django.utils.translation import gettext_lazy as _ | ||
|
|
||
| from rest_framework.compat import postgres_fields | ||
| from rest_framework.compat import ( | ||
| get_referenced_base_fields_from_q, postgres_fields | ||
| ) | ||
| from rest_framework.deprecation import RemovedInDRF320Warning | ||
| from rest_framework.exceptions import ErrorDetail, ValidationError | ||
| from rest_framework.fields import get_error_detail | ||
|
|
@@ -1469,20 +1471,28 @@ def get_unique_together_constraints(self, model): | |
| """ | ||
| 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, None | ||
| yield unique_together, model._default_manager, [], None, None, None | ||
| for constraint in parent_class._meta.constraints: | ||
| if isinstance(constraint, models.UniqueConstraint) and len(constraint.fields) > 1: | ||
| if isinstance(constraint, models.UniqueConstraint): | ||
| if constraint.condition is None: | ||
| condition_fields = [] | ||
| else: | ||
| condition_fields = list(constraint.condition.referenced_base_fields) | ||
| yield ( | ||
| constraint.fields, | ||
| model._default_manager, | ||
| condition_fields, | ||
| constraint.condition, | ||
| constraint.nulls_distinct, | ||
| ) | ||
| condition_fields = list( | ||
| get_referenced_base_fields_from_q(constraint.condition) | ||
| ) | ||
|
|
||
| # Combine constraint fields and condition fields. If the union | ||
| # involves multiple fields, treat as unique-together validation | ||
| required_fields = {*constraint.fields, *condition_fields} | ||
| if constraint.fields and len(required_fields) > 1: | ||
| yield ( | ||
| constraint.fields, | ||
| model._default_manager, | ||
| condition_fields, | ||
| constraint.condition, | ||
|
Comment on lines
+1488
to
+1492
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @majidkhazaei was this resolved ? |
||
| constraint.nulls_distinct, | ||
| constraint, | ||
| ) | ||
|
|
||
| def get_uniqueness_extra_kwargs(self, field_names, declared_fields, extra_kwargs): | ||
| """ | ||
|
|
@@ -1515,7 +1525,8 @@ def get_uniqueness_extra_kwargs(self, field_names, declared_fields, extra_kwargs | |
|
|
||
| # Include each of the `unique_together` and `UniqueConstraint` field names, | ||
| # so long as all the field names are included on the serializer. | ||
| for unique_together_list, queryset, condition_fields, condition, nulls_distinct in self.get_unique_together_constraints(model): | ||
| for unique_together_list, queryset, condition_fields, condition, nulls_distinct, unused_constraint in self.get_unique_together_constraints( | ||
| model): | ||
| unique_together_list_and_condition_fields = set(unique_together_list) | set(condition_fields) | ||
| if model_fields_names.issuperset(unique_together_list_and_condition_fields): | ||
| unique_constraint_names |= unique_together_list_and_condition_fields | ||
|
|
@@ -1624,7 +1635,12 @@ def _get_constraint_violation_error_message(self, constraint): | |
|
|
||
| def get_unique_together_validators(self): | ||
| """ | ||
| Determine a default set of validators for any unique_together constraints. | ||
| Determine a default set of validators for any unique_together constraints | ||
| and UniqueConstraint objects. | ||
|
|
||
| This method now preserves the original constraint object in the yielded | ||
| data from get_unique_together_constraints() to ensure custom violation | ||
| messages and error codes are correctly propagated to the validators. | ||
| """ | ||
| # The field names we're passing though here only include fields | ||
| # which may map onto a model field. Any dotted field name lookups | ||
|
|
@@ -1648,17 +1664,11 @@ def get_unique_together_validators(self): | |
| for name, source in field_sources.items(): | ||
| source_map[source].append(name) | ||
|
|
||
| unique_constraint_by_fields = { | ||
| constraint.fields: constraint | ||
| for model_cls in (*self.Meta.model._meta.parents, self.Meta.model) | ||
| for constraint in model_cls._meta.constraints | ||
| if isinstance(constraint, models.UniqueConstraint) | ||
| } | ||
|
|
||
| # Note that we make sure to check `unique_together` both on the | ||
| # base model class, but also on any parent classes. | ||
| validators = [] | ||
| for unique_together, queryset, condition_fields, condition, nulls_distinct in self.get_unique_together_constraints(self.Meta.model): | ||
| for unique_together, queryset, condition_fields, condition, nulls_distinct, constraint in self.get_unique_together_constraints( | ||
| self.Meta.model): | ||
| # Skip if serializer does not map to all unique together sources | ||
| unique_together_and_condition_fields = set(unique_together) | set(condition_fields) | ||
| if not set(source_map).issuperset(unique_together_and_condition_fields): | ||
|
|
@@ -1682,16 +1692,17 @@ def get_unique_together_validators(self): | |
|
|
||
| field_names = tuple(source_map[f][0] for f in unique_together) | ||
|
|
||
| constraint = unique_constraint_by_fields.get(tuple(unique_together)) | ||
| # Extract custom violation message and code from the constraint if available | ||
| violation_error_message = self._get_constraint_violation_error_message(constraint) if constraint else None | ||
| violation_error_code = getattr(constraint, 'violation_error_code', None) | ||
|
|
||
| validator = UniqueTogetherValidator( | ||
| queryset=queryset, | ||
| fields=field_names, | ||
| condition_fields=tuple(source_map[f][0] for f in condition_fields), | ||
| condition=condition, | ||
| message=violation_error_message, | ||
| code=getattr(constraint, 'violation_error_code', None), | ||
| code=violation_error_code, | ||
| nulls_distinct=nulls_distinct, | ||
| ) | ||
| validators.append(validator) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,27 +203,34 @@ def __call__(self, attrs, serializer): | |
| queryset = self.filter_queryset(attrs, queryset, serializer) | ||
| queryset = self.exclude_current_instance(attrs, queryset, serializer.instance) | ||
|
|
||
| checked_names = [ | ||
| serializer.fields[field_name].source for field_name in self.fields | ||
| ] | ||
| # Combine constraint fields and condition fields to detect changes | ||
| # in either set of fields. This ensures that updates to condition | ||
| # fields also trigger revalidation. | ||
| checked_names = list({ | ||
| serializer.fields[field_name].source for field_name in self.fields | ||
| } | { | ||
| serializer.fields[field_name].source for field_name in self.condition_fields | ||
| }) | ||
|
|
||
| # Ignore validation if any field is None | ||
| if serializer.instance is None: | ||
| checked_values = [attrs[field_name] for field_name in checked_names] | ||
| checked_values = [attrs.get(field_name) for field_name in checked_names] | ||
| else: | ||
| # Ignore validation if all field values are unchanged | ||
| checked_values = [ | ||
| attrs[field_name] | ||
| attrs.get(field_name) | ||
| for field_name in checked_names | ||
| if attrs[field_name] != getattr(serializer.instance, field_name) | ||
| if attrs.get(field_name) != getattr(serializer.instance, field_name, None) | ||
|
Comment on lines
+217
to
+223
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @majidkhazaei please cross check this and other open suggestions |
||
| ] | ||
|
|
||
| condition_sources = (serializer.fields[field_name].source for field_name in self.condition_fields) | ||
| condition_kwargs = { | ||
| source: attrs[source] | ||
| source: attrs.get(source) | ||
| if source in attrs | ||
| else getattr(serializer.instance, source) | ||
| else getattr(serializer.instance, source, None) | ||
| for source in condition_sources | ||
| } | ||
|
|
||
| if checked_values: | ||
| # Skip validation for None values unless nulls_distinct is False | ||
| if self.nulls_distinct is not False and None in checked_values: | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@majidkhazaei please crosscheck this and other open suggestions
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 @auvipy!
Yes, I'm currently reviewing all the suggestions from Copilot. I'll go through them one by one and apply the necessary fixes (especially the ones regarding empty constraint.fields and condition field changes).
I'll push the updates later today and let you know once they're ready for another look.
Appreciate your patience and guidance!
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?