-
-
Notifications
You must be signed in to change notification settings - Fork 845
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
Raise TypeError
on invalid query params.
#2523
Conversation
tests/models/test_queryparams.py
Outdated
@@ -87,6 +87,11 @@ def test_empty_query_params(): | |||
assert str(q) == "a=" | |||
|
|||
|
|||
def test_invalid_query_params(): | |||
with pytest.raises(TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems nice to ensure that the formatted message is correct
with pytest.raises(TypeError): | |
with pytest.raises(TypeError, match=r"Expected str, int, float, bool, or None\. Got 'bytes'\."): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's neat, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I broke your line length :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤨
Co-authored-by: Michael Adkins <contact@zanie.dev>
@@ -89,7 +89,7 @@ def test_empty_query_params(): | |||
|
|||
def test_invalid_query_params(): | |||
with pytest.raises( | |||
TypeError, match=r"Expected str, int, float, bool, or None\. Got 'bytes'\." | |||
TypeError, match=r"Expected str, int, float, bool, or None. Got 'bytes'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the r
if you aren't escaping the periods — this is doing a re.search
so technically the .
will be a wildcard but 🤷♀️ it's not likely to cause you any problems. You could use re.escape
if you really wanted to be correct.
@tomchristie this broke Prefect since we were passing UUID objects — that's fine but I think we should make sure this is listed on the changelog for 0.23.2 for visibility. |
Ooofff. Okay, more care needed in the future. |
That's entirely on me - rushing things that didn't need to be rushed. Do we want to roll this back in a 0.23.3, and leave it for the upcoming 0.24.0 update? (Or do we leave it as is?) |
It'd probably be safest to make this change in 0.24.0 instead since it changes the supported interface (I'll try to keep an eye out for this in future reviews), but I don't have strong feelings about rolling it back. |
This reverts commit 4cbf13e.
FYI - This also breaks datetime -> str conversion -- would suggest to keep datetime supported and adhering to RFC |
Prompted by #2515. (Doesn't presume any particular resolution to that case.)
Closes #746 - we don't support bytes (or other unexpected types) as QueryParam values. Let's raise a clear error, rather than coercing an unexpected
str
value.Before this change...
After this change...