-
-
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 |
|---|---|---|
|
|
@@ -8,7 +8,9 @@ | |
| from django.db import models | ||
| from django.utils.text import capfirst | ||
|
|
||
| from rest_framework.compat import postgres_fields | ||
| from rest_framework.compat import ( | ||
| get_referenced_base_fields_from_q, postgres_fields | ||
| ) | ||
| from rest_framework.validators import UniqueValidator | ||
|
|
||
| NUMERIC_FIELD_TYPES = ( | ||
|
|
@@ -79,10 +81,18 @@ def get_unique_validators(field_name, model_field): | |
| unique_error_message = get_unique_error_message(model_field) | ||
| queryset = model_field.model._default_manager | ||
| for condition in conditions: | ||
| yield UniqueValidator( | ||
| queryset=queryset if condition is None else queryset.filter(condition), | ||
| message=unique_error_message | ||
| condition_fields = ( | ||
| get_referenced_base_fields_from_q(condition) | ||
| if condition is not None | ||
| else set() | ||
| ) | ||
| # Only use UniqueValidator if the union of field and condition fields is 1 | ||
| # (i.e. no additional fields referenced in conditions) | ||
| if len(field_set | condition_fields) == 1: | ||
| yield UniqueValidator( | ||
|
Contributor
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. Django’s
Author
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. Good point about custom error messages! That would be a nice enhancement but outside this PR's scope. Worth tracking in a separate issue. |
||
| queryset=queryset if condition is None else queryset.filter(condition), | ||
| message=unique_error_message, | ||
| ) | ||
|
|
||
|
|
||
| def get_field_kwargs(field_name, model_field): | ||
|
|
||
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?
Uh oh!
There was an error while loading. Please reload this page.
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.