-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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 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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -170,6 +170,24 @@ class Meta: | |||||||||||||||||
unique_together = ('race_name', 'position') | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
class ConditionUniquenessTogetherModel(models.Model): | ||||||||||||||||||
""" | ||||||||||||||||||
Used to ensure that unique constraints with single fields but at least one other | ||||||||||||||||||
distinct condition field are included when checking unique_together constraints. | ||||||||||||||||||
""" | ||||||||||||||||||
race_name = models.CharField(max_length=100) | ||||||||||||||||||
position = models.IntegerField() | ||||||||||||||||||
|
||||||||||||||||||
class Meta: | ||||||||||||||||||
constraints = [ | ||||||||||||||||||
models.UniqueConstraint( | ||||||||||||||||||
name="condition_uniqueness_together_model_race_name", | ||||||||||||||||||
fields=('race_name',), | ||||||||||||||||||
condition=models.Q(position__lte=1) | ||||||||||||||||||
) | ||||||||||||||||||
] | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
class UniquenessTogetherSerializer(serializers.ModelSerializer): | ||||||||||||||||||
class Meta: | ||||||||||||||||||
model = UniquenessTogetherModel | ||||||||||||||||||
|
@@ -182,6 +200,12 @@ class Meta: | |||||||||||||||||
fields = '__all__' | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
class ConditionUniquenessTogetherSerializer(serializers.ModelSerializer): | ||||||||||||||||||
class Meta: | ||||||||||||||||||
model = ConditionUniquenessTogetherModel | ||||||||||||||||||
fields = '__all__' | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
class TestUniquenessTogetherValidation(TestCase): | ||||||||||||||||||
def setUp(self): | ||||||||||||||||||
self.instance = UniquenessTogetherModel.objects.create( | ||||||||||||||||||
|
@@ -222,6 +246,22 @@ def test_is_not_unique_together(self): | |||||||||||||||||
] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_is_not_unique_together_condition_based(self): | ||||||||||||||||||
""" | ||||||||||||||||||
Failing unique together validation should result in non field errors when a condition-based | ||||||||||||||||||
unique together constraint is violated. | ||||||||||||||||||
Comment on lines
+251
to
+252
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. Hyphenate 'non-field errors' for correctness and consistency with DRF terminology. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
""" | ||||||||||||||||||
ConditionUniquenessTogetherModel.objects.create(race_name='example', position=1) | ||||||||||||||||||
|
||||||||||||||||||
data = {'race_name': 'example', 'position': 1} | ||||||||||||||||||
serializer = ConditionUniquenessTogetherSerializer(data=data) | ||||||||||||||||||
assert not serializer.is_valid() | ||||||||||||||||||
assert serializer.errors == { | ||||||||||||||||||
'non_field_errors': [ | ||||||||||||||||||
'The fields race_name must make a unique set.' | ||||||||||||||||||
] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_is_unique_together(self): | ||||||||||||||||||
""" | ||||||||||||||||||
In a unique together validation, one field may be non-unique | ||||||||||||||||||
|
@@ -235,6 +275,21 @@ def test_is_unique_together(self): | |||||||||||||||||
'position': 2 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_unique_together_condition_based(self): | ||||||||||||||||||
""" | ||||||||||||||||||
In a unique together validation, one field may be non-unique | ||||||||||||||||||
so long as the set as a whole is unique. | ||||||||||||||||||
""" | ||||||||||||||||||
ConditionUniquenessTogetherModel.objects.create(race_name='example', position=1) | ||||||||||||||||||
|
||||||||||||||||||
data = {'race_name': 'other', 'position': 1} | ||||||||||||||||||
serializer = ConditionUniquenessTogetherSerializer(data=data) | ||||||||||||||||||
assert serializer.is_valid() | ||||||||||||||||||
assert serializer.validated_data == { | ||||||||||||||||||
'race_name': 'other', | ||||||||||||||||||
'position': 1 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_updated_instance_excluded_from_unique_together(self): | ||||||||||||||||||
""" | ||||||||||||||||||
When performing an update, the existing instance does not count | ||||||||||||||||||
|
@@ -248,6 +303,21 @@ def test_updated_instance_excluded_from_unique_together(self): | |||||||||||||||||
'position': 1 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_updated_instance_excluded_from_unique_together_condition_based(self): | ||||||||||||||||||
""" | ||||||||||||||||||
When performing an update, the existing instance does not count | ||||||||||||||||||
as a match against uniqueness. | ||||||||||||||||||
""" | ||||||||||||||||||
ConditionUniquenessTogetherModel.objects.create(race_name='example', position=1) | ||||||||||||||||||
|
||||||||||||||||||
data = {'race_name': 'example', 'position': 0} | ||||||||||||||||||
serializer = ConditionUniquenessTogetherSerializer(self.instance, data=data) | ||||||||||||||||||
Comment on lines
+311
to
+314
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. The serializer under test is passed an instance of UniquenessTogetherModel (self.instance) instead of ConditionUniquenessTogetherModel. This will cause the update validation to target the wrong model. Capture and pass the instance of ConditionUniquenessTogetherModel created in this test.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||
assert serializer.is_valid() | ||||||||||||||||||
assert serializer.validated_data == { | ||||||||||||||||||
'race_name': 'example', | ||||||||||||||||||
'position': 0 | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
def test_unique_together_is_required(self): | ||||||||||||||||||
""" | ||||||||||||||||||
In a unique together validation, all fields are required. | ||||||||||||||||||
|
@@ -699,20 +769,17 @@ class Meta: | |||||||||||||||||
def test_single_field_uniq_validators(self): | ||||||||||||||||||
""" | ||||||||||||||||||
UniqueConstraint with single field must be transformed into | ||||||||||||||||||
field's UniqueValidator | ||||||||||||||||||
field's UniqueValidator if no distinct condition fields exist (else UniqueTogetherValidator) | ||||||||||||||||||
""" | ||||||||||||||||||
# Django 5 includes Max and Min values validators for IntegerField | ||||||||||||||||||
extra_validators_qty = 2 if django_version[0] >= 5 else 0 | ||||||||||||||||||
serializer = UniqueConstraintSerializer() | ||||||||||||||||||
assert len(serializer.validators) == 2 | ||||||||||||||||||
assert len(serializer.validators) == 4 | ||||||||||||||||||
validators = serializer.fields['global_id'].validators | ||||||||||||||||||
assert len(validators) == 1 + extra_validators_qty | ||||||||||||||||||
assert validators[0].queryset == UniqueConstraintModel.objects | ||||||||||||||||||
|
||||||||||||||||||
validators = serializer.fields['fancy_conditions'].validators | ||||||||||||||||||
assert len(validators) == 2 + extra_validators_qty | ||||||||||||||||||
ids_in_qs = {frozenset(v.queryset.values_list(flat=True)) for v in validators if hasattr(v, "queryset")} | ||||||||||||||||||
assert ids_in_qs == {frozenset([1]), frozenset([3])} | ||||||||||||||||||
assert ids_in_qs == {frozenset([1, 2, 3])} | ||||||||||||||||||
|
||||||||||||||||||
def test_nullable_unique_constraint_fields_are_not_required(self): | ||||||||||||||||||
serializer = UniqueConstraintNullableSerializer(data={'title': 'Bob'}) | ||||||||||||||||||
|
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?