Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Fix unique_together validation with source#9482
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix unique_together validation with source#9482
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
hey thanks for the patch! whats your take on this comment#9442 (comment) ?
| @@ -159,17 +159,18 @@ 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 | |||
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.
Since fields is a dict it is more readable to iterate over values, also it should be a little faster.
| serializer.fields[field_name].sourceforfield_nameinself.fields | |
| field.sourceforfieldinself.fields.values() |
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@sevdog thanks for the comment, but I believeself.fields is a list of strings, not dict.
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.
Yeah, sorry I was considering to be in an other class/file.
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
518eb22 intoencode:masterUh oh!
There was an error while loading.Please reload this page.
@encode/maintainers One of the most important aspects for REST framework right now is a maintenance team who consistently push back on pull requests. (See pinned discussion#9130, and PR#9392) In my view the churn and security issues associated with the 3.15 release and subsequent follow-ups clearly demonstrate that we need to draw a clear line here and stop accepting code pull requests, withabsolutely the only exceptions being CVE'ed security issues and changes in Django versions don't pass against our CI. |
It a was a fix for a regression.... |
mredaelli commentedAug 13, 2024
Fantastic! Thanks. Would it be possible to have a release with this fix? |
NoeAzarGit commentedSep 26, 2024
Are there any plans to release this fix? |
mredaelli commentedOct 16, 2024
Sorry to bump again, but this is preventing us from upgrading and thus fixing a security advisory. Can you please do a release? |
upgrade `django-rest-framework` to `3.16.0`The reverted commit is purely an optimization which unfortunately breaks authentik, specifically Blueprints. It adds `getattr(serializer.instance, field)` to a validator. If `field` is a `RelatedObject`, that invocation queries the database.When authentik creates objects using Blueprints, it doesn't place related objects into the database before the validator tries to get them from there, so with the reverted commit, it produces `RelatedObjectDoesNotExist`.Perhaps a long-term solution is to revise how Blueprints work, or perhaps it is to change upstream. But in the meantime, Django 5.0 support ended and upgrading to Django 5.1 requires an upgrade of `django-rest-framework` to `3.16.0`, hence this workaround.See-encode/django-rest-framework#9154-encode/django-rest-framework#9358-encode/django-rest-framework#9482-encode/django-rest-framework#9483
Thisfixed#9442
Fields with specified sources should now be handled correctly.
The unique together queryset with condition is another separated issue, which should be addressed in#9360