kombu
kombu copied to clipboard
[second try] Fixed error when `query` dict has keyname equal to `virtual_host` on `kombu.utils.url.parse_url`
Due to the request for help in https://github.com/celery/celery/issues/5180 I added a test for the change proposed by @mavriq in #911
To confess, I don't really know whether this makes sense at all. I just figured out what must have gone wrong so the PR was opened in the first place, wrote a test for this and am submitting the PR now. How is it possible that virtual_host
is part of the query string?
Feel free to discard this if it doesn't make any sense. I just wanted to help unblocking the release of celery 4.3 because we want to migrate to Python 3.7 😎
Codecov Report
Merging #991 into master will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #991 +/- ##
=======================================
Coverage 88.64% 88.64%
=======================================
Files 63 63
Lines 6542 6544 +2
Branches 781 781
=======================================
+ Hits 5799 5801 +2
Misses 660 660
Partials 83 83
Impacted Files | Coverage Δ | |
---|---|---|
kombu/utils/url.py | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 2dff21c...27cff8f. Read the comment docs.
How is it possible that virtual_host is part of the query string?
I think your question is valid, as seen in the documentation, the URL path corresponds to the virtual host. Unless there is some conflict with the available query parameters of any of the Transports, this is not a bug as far as I understand.
@mavriq perhaps you would like to give some more feedback on the above?
Since @mavriq didn't answer to the questions in his original PR which is 6 months old and also didn't show any activity on GitHub since October '18, I don't expect an answer back and propose to just throw this away.
I guess query parameters that interfere with anything parsed from the url and hence overwrite the parsed values should just not be allowed. I imagine this to be causing bugs that are pretty hard to trace down. Maybe one could think about converting the error message to a one that's more informational (e.g. Query parameters must not contain keys 'hostname', 'port', ...
).
Anyhow, I don't think this should be blocking a release of kombu because it doesn't seem to be a bug