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

Handle substitution errors with no parameters to ValidationError #9313

Closed
wants to merge 8 commits into from

Conversation

jctanner
Copy link

@jctanner jctanner commented Mar 19, 2024

% substituation requires all keys to be matched during substitution, so if a ValidationError happens to include a %(foo)s style variable not met by the parameters, it will throw a KeyError. In Field.run_validators there is also an accumulation of errors that are wrapped by a final ValidationError which can then throw TypeError if any of the sub-errors contain replaceable substrings.

This is more complicated than it first seemed. #9295 has a dateformat string throwing a ValueError, which won't ever work with foo % bar so this patch needs more testing and edge case handling.

This patch implements a new function to handle the string substitution and related errors in a unified way that returns the original string if something fails.

should fix #9295

…alidationError messages.

"%" substitution requires all keys to be matched during substitution, so if a ValidationError happens
to include a %(foo)s style variable not met by parameters, it will throw a KeyError. In Field.run_validators
there is also an accumulation of errors that are wrapped by a final ValidationError which can then
throw TypeError if any of the sub-errors contain replaceable substrings.

This patch implements a subclassed dict which simply returns the key's name for any missing keys. The end
result for the logic in exceptions.py is that the final message is an exact copy of the original message
with only found parameters replaced and the rest left untouched.

Signed-off-by: James Tanner <[email protected]>
Signed-off-by: James Tanner <[email protected]>
@jctanner jctanner marked this pull request as draft March 19, 2024 19:39
@jctanner jctanner marked this pull request as ready for review March 19, 2024 19:54
Signed-off-by: James Tanner <[email protected]>
Signed-off-by: James Tanner <[email protected]>
Signed-off-by: James Tanner <[email protected]>
@auvipy auvipy self-requested a review March 20, 2024 08:12
Copy link

@AlanCoding AlanCoding left a comment

Choose a reason for hiding this comment

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

Catching these errors will unblock us from being able to upgrade. In the cases where the format errors, it is almost always the case that params is empty dict, so raising the error with the detail matches the expectation.

@tomchristie
Copy link
Member

I'm not yet convinced by this... I'd suggest we instead revert #8863 as a first pass.
(Preventing breaking changes > introducing new functionality.)

The equivalent case in Django uses if error.params before using formatting, a revised attempt at #8863 which does the same ensuring that there's no chance of a regression when params are not used would be a good idea.

@auvipy
Copy link
Member

auvipy commented Mar 21, 2024

@tomchristie #9326

@jctanner
Copy link
Author

This is obsolete now that #9326 is merged, which I fully support. If someone re-attempts the feature in the future, please feel free to copy/paste the unit tests I made in this PR.

@jctanner jctanner closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3.15.0 - bug in rendering % characters from ValidationError
4 participants