Skip to content
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

Enable Validators to defer string evaluation and handle new string format #3438

Closed
wants to merge 8 commits into from

Conversation

xordoquy
Copy link
Collaborator

@xordoquy xordoquy commented Sep 23, 2015

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_messages it'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.

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='{'))
Copy link
Member

@tomchristie tomchristie Sep 23, 2015

Choose a reason for hiding this comment

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

I think for this to be acceptable we'd need to find a way that doesn't require passing string_format='{'

Copy link
Collaborator Author

@xordoquy xordoquy Sep 23, 2015

Choose a reason for hiding this comment

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

@tomchristie move all the messages not to use the new format style but it will impact users that did customize it.

Copy link
Member

@tomchristie tomchristie Sep 23, 2015

Choose a reason for hiding this comment

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

Can we not eg. silently try both? .format() first then fallback if it fails with AttributeError or whatever?

Copy link
Collaborator Author

@xordoquy xordoquy Sep 24, 2015

Choose a reason for hiding this comment

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

Need to think about this a bit more. Note that it likely won't fail in most cases:

>>> 'demo %(ex)s'.format(ex='toto')
>>>

@xordoquy
Copy link
Collaborator Author

xordoquy commented Sep 25, 2015

ok, after discussing a bit, there might be a better workaround though still a bit tricky.

@xordoquy xordoquy self-assigned this Oct 18, 2015
@xordoquy
Copy link
Collaborator Author

xordoquy commented Nov 10, 2015

@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
Copy link
Member

@tomchristie tomchristie Nov 12, 2015

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@xordoquy xordoquy Nov 12, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@xordoquy xordoquy Nov 12, 2015

Choose a reason for hiding this comment

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

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

@xordoquy
Copy link
Collaborator Author

xordoquy commented Nov 25, 2015

Updated to use six.test_type and up to date with master.

@tomchristie
Copy link
Member

tomchristie commented Nov 26, 2015

Milestoning to put it at the top of the review queue. (sorry)

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@doctoryes
Copy link
Contributor

doctoryes commented Jun 22, 2017

@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:
edx-unsupported@3c72cb5
We'd like to move back to an upstream released DRF version. Is there a way I could move a fix for this issue forward? Thanks!

@xordoquy
Copy link
Collaborator Author

xordoquy commented Jun 26, 2017

looks like I was a bit heavy on my branch cleaning.

@doctoryes
Copy link
Contributor

doctoryes commented Jun 27, 2017

@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.

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 25, 2017

@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.

@doctoryes
Copy link
Contributor

doctoryes commented Sep 25, 2017

@carltongibson Yes - I'd be happy to re-submit the PR. I'll do so sometime this week.

@bmedx @macdiesel FYI

@carltongibson
Copy link
Collaborator

carltongibson commented Sep 25, 2017

@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... 🙂)

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.

Don't call .format() on lazy-translated strings during Field.__init__
5 participants