Do not persist the context in validators #6172
Conversation
rest_framework/fields.py
Outdated
| errors = [] | ||
| for validator in self.validators: | ||
| if hasattr(validator, 'set_context'): | ||
| warnings.warn( | ||
| "Method `set_context` on validators is deprecated and will " | ||
| "no longer be called starting with 3.10. Instead derive the " |
There was a problem hiding this comment.
btw - we've formalized our deprecation policy. This should use RemovedInDRF312Warning, which is currently a subclass of PendingDeprecationWarning. Deprecation classes can be found in rest_framework/__init__.py
| """Base class for validators that need a context during evaluation. | ||
|
|
||
| In extension to regular validators their `__call__` method must not only | ||
| accept a value, but also an instance of a serializer. |
There was a problem hiding this comment.
I think it should say "serializer field" here?
There was a problem hiding this comment.
I'm not sure. Depends on the validator, I guess. I think it's mostly relevant for validators used on serializers since fields are deepcopied.
| @@ -47,7 +47,7 @@ If set, this gives the default value that will be used for the field if no input | |||
|
|
|||
| The `default` is not applied during partial update operations. In the partial update case only fields that are provided in the incoming data will have a validated value returned. | |||
|
|
|||
| May be set to a function or other callable, in which case the value will be evaluated each time it is used. When called, it will receive no arguments. If the callable has a `set_context` method, that will be called each time before getting the value with the field instance as only argument. This works the same way as for [validators](validators.md#using-set_context). | |||
| May be set to a function or other callable, in which case the value will be evaluated each time it is used. When called, it will receive no arguments. If the callable has a `set_context` method, that will be called each time before getting the value with the field instance as only argument. | |||
There was a problem hiding this comment.
Sounds like changing default to use the same mechanism would be a good idea as well.
There was a problem hiding this comment.
Might be not relevant due to the deepcloning. But I'm not sure.
There was a problem hiding this comment.
I think even if it's not strictly required for defaults, we should still adopt it for consistency.
michael-k
force-pushed
the
race-condition-2
branch
from
7a775a4 to
c0dfaa1
Compare
|
@bluetech Thanks for the review! I've rebased my branch to fix the merge conflicts and included some of your suggestions. |
There was a problem hiding this comment.
I still need to review this further (when it's not the end of the day), but one thing that pops out at me is testing. There isn't much change to external API surface, but the following would be great:
- A test that demonstrates the race condition (should fail/pass pre/post changes).
Incorporate a variation of Test that validator's context is not used by another serializer #5761? - A test that ensures backwards compatibility w/
set_context.
| field it is being used with as additional context. You can do so by using | ||
| `rest_framework.validators.ContextBasedValidator` as a base class for the | ||
| validator. The `__call__` method will then be called with the `serializer_field` | ||
| or `serializer` as an additional argument. |
There was a problem hiding this comment.
ContextBasedValidator is potentially confusing, as the serializer context is an entirely different concept. Possibly FieldContextValidator or FieldBasedValidator?
|
@tomchristie I'm inclined to punt this to the 3.11 release. The short version is that I think the deprecation path could be improved, or it should be removed. Right now, the warning is sufficient for users who write their own validators that have a Also, there are a few outstanding changes, like using the proper deprecation exception classes. |
|
I'm ok with this not landing in 3.10. Haven't had the time to update this PR and incorporate the suggested changes. |
|
Thanks @michael-k. I think this is a great start, but I also think there's some non-obvious things we'll need to consider. Hoping we can get this in for 3.11! |
|
This is looking great! I think rather than a different class we could either...
|
|
Fixed in #7062 |
This fixes #5760 and supersedes #5762. It does however not include the failing test from #5761 as this makes no longer sense.
Description
Please see #5760 for a detailed description of the race condition. In short: When two serializers of the same type run their validators in parallel, the context might leak from one validator to the other (because the validator instances are identical).
Before validators could have a
set_contextmethod that persisted things like the instance of a django model on the validator. This PR deprecates this feature.Instead it adds a new base class
ContextBasedValidatorfor class based validators. Instances of this class will get the context (serializer or serialzer_field) as an additional argument. This context can be used during validation without the need to persist it on the instance.Background of the implementation
This was basically proposed by @rpkilby in #5762 (comment) in a similar form:
__call__accepts the serializer[_field] instead. This ensures that the full context is still available if needed.This approach also prevents
deepcopyas preferred by @tomchristie:Source: #5762 (comment)
Backwards compatibility
set_contextis still called by fields/serializers to give authors of custom validators time to adjust to the new behavior. (The concrete release for the removal might need adjustment.)self.instance,self.field_name, …). Custom validators based on those classes need to be adjusted immediately without any deprecation warning.