Skip to content

Fix race condition in validators #5762

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix race condition in validators #5762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ce30dd
Test that validator's context is not used by another serializer
michael-k Jan 22, 2018
a1322ca
Prevent validator's context to be used by another serializer
michael-k Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions rest_framework/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ def __init__(self, default):
def set_context(self, serializer_field):
self.is_update = serializer_field.parent.instance is not None
if callable(self.default) and hasattr(self.default, 'set_context') and not self.is_update:
self.default = copy.deepcopy(self.default)
self.default.set_context(serializer_field)

def __call__(self):
Expand Down Expand Up @@ -475,6 +476,7 @@ def get_default(self):
raise SkipField()
if callable(self.default):
if hasattr(self.default, 'set_context'):
self.default = copy.deepcopy(self.default)
self.default.set_context(self)
return self.default()
return self.default
Expand Down Expand Up @@ -532,6 +534,7 @@ def run_validators(self, value):
errors = []
for validator in self.validators:
if hasattr(validator, 'set_context'):
validator = copy.deepcopy(validator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If set_context means that we need a separate instance, perhaps we should be reconsidering the interface (e.g. set_context returns a new instance).

deepcopy is black magic, we should avoid it if we can.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first thought was to pass the instance to the validator when calling it, and just pass it to all functions instead of saving it on the instance (ie. as self.instance). But not all validators accept a context and using inspect to decide if the validator accepts a context seemed like a bad idea. Last, but not least, this would break all existing custom validators with set_context.

Changing the interface of set_context is also a breaking change for all such custom validators.

So maybe this is something to consider for a 4.0 release, but not for 3.8? I'd love to see a fix for this in the 3.8 release. (I've patched the one place in our code that threw the most exceptions, but not all Validators in all our microservices.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that deepcopy is a good way to fix the issue for now (I do not have an opinion about whether deepcopy is good, bad or magic). Changing the interface or the meaning of set_context is potentially a breaking change, though it would be for a very good reason.

Seeing what this bug is causing (potentially inconsistencies in the database), I believe that a quick fix should be applied.

For a long term solution, it should be considered to NOT store self.instance in the validators set_context method, but to pass it to the validator(...) method. Maybe it is possible to do both for some time and log a notification with a deprecation warning.

Copy link
Contributor Author

@michael-k michael-k Feb 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass it to the validator(...) method

Do you have any good solution how to decide whether to pass it or not, ie. a replacement for hasattr(validator, 'set_context')? It's possible to use any django validator (from the core, from drf, third party or custom) here and most of them do not accept the instance.

I could only think of using inspect (+ py2.7) or try-except.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that means all those validators would have to be changed, I guess that's not feasible at all (and, afterall, it is definately not going to happen).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible solution:

  • Refactor unique validators to inherit a base InstanceValidator
  • Change the InstanceValidator call signature to expect an instance in addition to the value.
  • Delete the set_context method handling.
  • Handle regular validators first, then instance validators second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the input. I like it and will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finally gotten around to tackle this. I've opened a new PR: #6172

validator.set_context(self)

try:
Expand Down
22 changes: 22 additions & 0 deletions tests/test_validators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,28 @@ def filter(self, **kwargs):
validator.filter_queryset(attrs=data, queryset=queryset)
assert queryset.called_with == {'race_name': 'bar', 'position': 1}

def test_validator_instances_with_context_are_not_used_twice(self):
"""
Every instance of a serializer should use unique instances of validators
when calling `set_context`.
"""
def data():
return {'race_name': 'example', 'position': 3}

def check_validators(serializer):
for validator in serializer.validators:
assert not hasattr(validator, 'instance')

serializer = UniquenessTogetherSerializer(self.instance, data=data())
check_validators(serializer)
serializer.run_validators({})
check_validators(serializer)

another_serializer = UniquenessTogetherSerializer(data=data())
check_validators(another_serializer)
another_serializer.run_validators(data())
check_validators(another_serializer)


# Tests for `UniqueForDateValidator`
# ----------------------------------
Expand Down