kombu icon indicating copy to clipboard operation
kombu copied to clipboard

[second try] Fixed error when `query` dict has keyname equal to `virtual_host` on `kombu.utils.url.parse_url`

Open larsrinn opened this issue 6 years ago • 3 comments

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 😎

larsrinn avatar Jan 16 '19 18:01 larsrinn

Codecov Report

Merging #991 into master will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

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

codecov[bot] avatar Jan 16 '19 18:01 codecov[bot]

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?

georgepsarakis avatar Jan 17 '19 05:01 georgepsarakis

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

larsrinn avatar Jan 20 '19 16:01 larsrinn