Enable Validators to defer string evaluation and handle new string format #3438
Conversation
encode#3354 - Update tests to reflect new error messages provided by Django field parent classes
…e to handle new string format.
| message = self.error_messages['max_length'].format(max_length=max_length) | ||
| self.validators.append(MaxLengthValidator(max_length, message=message)) | ||
| message = self.error_messages['max_length'] | ||
| self.validators.append(MaxLengthValidator(max_length, message=message, string_format='{')) |
There was a problem hiding this comment.
I think for this to be acceptable we'd need to find a way that doesn't require passing string_format='{'
There was a problem hiding this comment.
@tomchristie move all the messages not to use the new format style but it will impact users that did customize it.
There was a problem hiding this comment.
Can we not eg. silently try both? .format() first then fallback if it fails with AttributeError or whatever?
There was a problem hiding this comment.
Need to think about this a bit more. Note that it likely won't fail in most cases:
>>> 'demo %(ex)s'.format(ex='toto')
>>>
|
ok, after discussing a bit, there might be a better workaround though still a bit tricky. |
Conflicts: rest_framework/compat.py
|
@tomchristie looking better ? If yes, I'll ask confirmation that it actually fixes the issue :) |
| self.message = kwargs.pop('message', self.message) | ||
| super(MaxLengthValidator, self).__init__(*args, **kwargs) | ||
| class MaxLengthValidator(CustomValidatorMessage, MaxLengthValidator): | ||
| pass |
There was a problem hiding this comment.
I'd rather see us keep the existing style.
At any rate this (looks like?) an unrelated change, so if you feel strongly about it and do want it in, perhaps let's discuss that in the context of a separate PR?
There was a problem hiding this comment.
I'd rather see us keep the existing style.
If by existing style you mean the if else then it's way more than just style.
The Django 1.8+ validators force the string evaluation during init which in turn breaks the deferred evaluation we are trying to setup.
I discussed that with some core dev which agreed it could be an improvement on Django. Meanwhile, it the second part of the bug will not work with django 1.8.
There was a problem hiding this comment.
Basically, Django does a condition on the provided message. The issue is that the test forces the string evaluation which voids the deferred part.
By using pop instead of testing against the parameter we workaround this
|
Updated to use six.test_type and up to date with master. |
|
Milestoning to put it at the top of the review queue. (sorry) |
|
@xordoquy Could you please tell me why this PR was closed? Our project uses a forked version of DRF because of this issue described in #3354 - and the only custom change is the one to fix that issue: |
|
looks like I was a bit heavy on my branch cleaning. |
|
@xordoquy Thanks for your response. Was there a problem with the basic approach? I'd be happy to re-submit this PR myself if not. If the fix needed more changes, I can attempt to incorporate them. |
|
@doctoryes Are you still up for submitting that PR? We're working on the 3.7 release now and it would be great to close this one off. |
|
@carltongibson Yes - I'd be happy to re-submit the PR. I'll do so sometime this week. @bmedx @macdiesel FYI |
|
@doctoryes Awesome. Thank you! When you do can I just ask you give a small context in the PR description? The trail here is both cold and convoluted. It'll make my task easier in reviewing. (Ta!) (Aiming for the release Monday 3rd... — no stress, just FYI... 🙂) |
This should fix #3354 and take over #3407.
There has been more difficulties than expected here due to several factors.
Due to a naive test during Validator instantiation any lazy translated message passed to the validator will be evaluated which will lead to the issue in #3354. This will only apply to Django 1.8+. For Django 1.7 DRF compact module was pulling the message without evaluation which is fine.
The faulty test is located here and the issue will be risen on Django bug tracker as discussed with a core dev.
This very reason lead to the removal of Django 1.8+ compatibility code which means the 4 Validators are now the same for every Django version.
Second issue was pretty painful and not that easy to solve.
Django uses the printf style formatting ("%()s") while DRF uses the new format one ("{}s").
Therefore it would lead to nowhere to let the Django's Validator evaluate the string on its own.
To work around the two formats, an additional argument is added to the DRF Validator.
Since the only case where the new format is used is through the default Field errors we now add the message to the Validator and specify it's using the "{" format - similar to what logging's formatter do, thanks @nedbat for pointing it to me.
If a user changes the
default_error_messagesit'll still be interpreted with the new format so it's invisible.If a user chooses to provide its own Validator, he would be able to use the old format as before this change.