Skip to content

Fix for #3863: validators are not thread safe #3870

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

Closed
wants to merge 1 commit into from

Conversation

skyjur
Copy link

@skyjur skyjur commented Jan 25, 2016

Closes #3863

@xordoquy
Copy link
Contributor

@skyjur did you manage to get a test that illustrate the issue ?

@AbstractVisions
Copy link

Two things about this PR:

  1. Use deepcopy inside run_validators not enough good idea by performance reasons.
    in case of set processing (like ListField, or serializing list of items). It would be better to insert a similar code in the Field.__deepcopy__ (rest_framework/fields.py), for example:
    def __deepcopy__(self, memo):
        """
        When cloning fields we instantiate using the arguments it was
        originally created with, rather than copying the complete state.
        """
        args = copy.deepcopy(self._args)
        kwargs = dict(self._kwargs)
        # Bit ugly, but we need to special case 'validators' as Django's
        # RegexValidator does not support deepcopy.
        # We treat validator callables as immutable objects.
        # See https://github.com/tomchristie/django-rest-framework/issues/1954
        validators = kwargs.pop('validators', None)
        kwargs = copy.deepcopy(kwargs)
        if validators is not None:
            kwargs['validators'] = [
                copy.deepcopy(vld) if hasattr(vld, 'set_context') else vld for vld in validators
            ] 
        return self.__class__(*args, **kwargs)
  1. note that - not each object can be cloned by copy.deepcopy without raise errors (for example - object, containing Regexp) and this is widespread practice for Django Validators

@AbstractVisions
Copy link

And things about design:
it isn't good to mix data preparation of Serializer instance and validation processing...
It is desirable that process of validation (and making representation or internal value) would be the simply and fast for for repeated using (for exapmle - processing of a list of similar data).
Therefore it is better to do it at different stages, namely it is desirable to realize cloning of Validator object only once at the preparation stage.

Preparation stage of instance of Serializer class comes to an end with a call and caching Serializer.get_fields(), which cloning ( use deepcopy) _declared_fields of Serializer's class

@lovelydinosaur lovelydinosaur added this to the 3.4.5 Release milestone Aug 10, 2016
@lovelydinosaur lovelydinosaur modified the milestones: 3.4.7 Release, 3.4.8 Release Sep 21, 2016
@lovelydinosaur
Copy link
Member

Closing this in favour of the #3713 issue. We could use deepcopy, but seems like a different solution is probably called for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants