-
Notifications
You must be signed in to change notification settings - Fork 437
only clean query parameters #2475
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
Conversation
jsl12
left a comment
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.
Using urllib to actually parse things should resolve this stuff about where the slashes are, so I don't think #2485 is necessary any more
appdaemon/utils.py
Outdated
| return result.strip("/") | ||
| parsed = parse.urlparse(str(base)) | ||
| result = parsed._replace(path=endpoint) | ||
| return parse.urlunparse(result) |
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.
I think this breaks base URLs with a path (e.g. http://supervisor/core) - it will replace /core with the endpoint path instead of appending. See #2485 for a better fix using yarl.
We need a serializer for all fields in the model.
We prefix paths like `/api/states` with a slash even though they're relative in practice, not absolute. It's simpler to always append to the path and lstrip the leading slash from the endpoint.
There is no need to "clean" the json body of a POST request to alter things like booleans - JSON already has a way to represent them. That wasn't done in 4.4.x, which had a more transparent set_state.
One possible exception is datetime, which doesn't have one true JSON representation. But we can enhance convert_json to always use isoformat.
(Also, DELETE shouldn't have a body - but it may have query params. We don't currently use kwargs with DELETE so this isn't crucial, but it's important for correctness.)
Fixes #2464