Handle duplicate headers in the uri module #33792

sivel · 2017-12-11T22:25:21Z

SUMMARY

...and make it easier for users to use cookies by providing a pre-built string

FIxes #33325

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

uri

ANSIBLE VERSION

devel

ADDITIONAL INFORMATION

mattclay · 2018-01-03T02:11:17Z

CI failure unrelated to PR. Restarting CI.

ansibot · 2018-03-28T16:19:04Z

cc @romeotheriault
click here for bot help

mattclay · 2018-04-09T21:33:21Z

lib/ansible/module_utils/urls.py

@@ -1049,11 +1049,36 @@ def fetch_url(module, url, data=None, headers=None, method=None,
                     url_password=password, http_agent=http_agent, force_basic_auth=force_basic_auth,
                     follow_redirects=follow_redirects, client_cert=client_cert,
                     client_key=client_key, cookies=cookies)
-        info.update(r.info())
+        # Lowercase keys, to conform to py2 behavior, so that py3 and py2 are predictable
+        info.update(dict((k.lower(), v) for k, v in r.info().items()))


This looks like a bug fix. If that's the case it should go into a separate PR, preferably along with tests.

Also, doesn't this cause potential issues with backwards compatibility?

I suppose it could be considered a bug fix.

While writing tests I noticed that py2 gives lowercased names, while py3 gives capitalization of the name from the response.

I can't see how it would cause a potential backwards compat issue. I went with the py2 default, because all of our modules should support py2. The only way it could cause a backwards compat issue, is if a module was calling fetch_url and only supported python3. If they supported both py2 and py3, then the code would have to already handle lowercase and mixed case, so it should work as expected.

This normalizes/standardizes return values from this method.

Also, in general, this whole PR is a bug fix. Largely due to difference in behavior of python3 vs python2. The goal is to normalize the functionality.

The only place not a bugfix is the addition of cookies_string. That was added to ease issues related to the original bug report of missing duplicate headers. However, due to predictability issues reconstructing the string from the headers, I would consider that part of the bugfix as well.

mattclay · 2018-04-09T21:41:06Z

lib/ansible/module_utils/urls.py

+                    _h[_k] = ', '.join((_h[_k], v))
+                else:
+                    _h[_k] = v
+            info.update(_h)


Please use more descriptive variable names for at least _h and _k, and probably k and v also. I think it's fine to use some single letter variables in list/dict comprehensions or simple loops, but once there is some additional logic I think readability is improved by using longer names.

Also, it should be fine to reassign the lowercase version of k (or whatever it ends up being) to itself since the original value isn't used anywhere else and the type hasn't changed.

mattclay · 2018-04-09T21:42:49Z

lib/ansible/module_utils/urls.py

+        # Lowercase keys, to conform to py2 behavior, so that py3 and py2 are predictable
+        info.update(dict((k.lower(), v) for k, v in r.info().items()))
+
+        # Don't be lossy, append header values for duplciate headers


Fix spelling of duplicate.

mattclay · 2018-04-09T21:44:31Z

lib/ansible/module_utils/urls.py

+                else:
+                    _h[_k] = v
+            info.update(_h)
+        except AttributeError:


What causes AttributeError to be raised?

sivel · 2018-04-09T22:25:30Z

@mattclay most comments addressed in a recent commit. I added a comment to the untouched line around concern of backwards incompat.

bersace · 2018-06-14T09:01:51Z

Hi,

Kudo @sivel. ~~Do you have some input on multiple cookies in uri modules ? Unlike what's stated in PR title, the diff seems to modify get_url module. Am I wrong ?~~ The integration test playbook is explicit. Thanks.

Regards,
Étienne

…5603)

* Handle duplicate headers, and make it easier for users to use cookies, by providing a pre-built string * Ensure proper cookie ordering, make key plural * Add note about cookie sort order * Add tests for duplicate headers and cookies_string * Extend tests, normalize headers between py2 and py3 * Add some notes in test code * Don't use AttributeError, use six.PY3. Use better names.

sivel mentioned this pull request Dec 11, 2017

uri module doesn’t handle multiple cookies with same name #33325

Closed

mkrizek removed the needs_triage Needs a first human triage before being processed. label Dec 15, 2017

sivel force-pushed the issue/33325 branch from 7656451 to 8f2aae4 Compare December 21, 2017 20:24

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Dec 21, 2017

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 31, 2017

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 3, 2018

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 11, 2018

sivel mentioned this pull request Feb 6, 2018

URI module and many cookies #17436

Closed

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018

sivel added this to the 2.7.0 milestone Mar 2, 2018

ansible deleted a comment from ansibot Mar 27, 2018

sivel force-pushed the issue/33325 branch from 5a1d66e to 0ba0aec Compare March 28, 2018 16:05

sivel force-pushed the issue/33325 branch from 207d310 to ac21a1b Compare April 9, 2018 16:29

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 9, 2018

Extend tests, normalize headers between py2 and py3

2d24ef3

sivel force-pushed the issue/33325 branch from ac21a1b to 2d24ef3 Compare April 9, 2018 17:56

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 9, 2018

Add some notes in test code

659e82f

sivel force-pushed the issue/33325 branch from 8ba589e to 659e82f Compare April 9, 2018 18:47

ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. labels Apr 9, 2018

mattclay requested changes Apr 9, 2018

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 9, 2018

Don't use AttributeError, use six.PY3. Use better names.

8ff4ad4

mattclay approved these changes Apr 9, 2018

View reviewed changes

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Apr 10, 2018

sivel merged commit 450cfa8 into ansible:devel Apr 10, 2018

sivel mentioned this pull request May 30, 2018

get_url fails to split headers correctly when extra commas are present #10902

Closed

This was referenced Sep 13, 2018

ACME: simplify code #45603

Merged

Fixing HTTPError case of fetch_url for Python 3 compatibility. #45628

Merged

felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 19, 2018

No longer necessary since ansible#33792 and ansible#45628.

a244f1b

felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 28, 2018

No longer necessary since ansible#33792 and ansible#45628. (ansible#4…

e76596a

…5603)

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle duplicate headers in the uri module #33792

Handle duplicate headers in the uri module #33792

sivel commented Dec 11, 2017

mattclay commented Jan 3, 2018

ansibot commented Mar 28, 2018

mattclay Apr 9, 2018

sivel Apr 9, 2018

sivel Apr 9, 2018 •

edited

Loading

mattclay Apr 9, 2018

mattclay Apr 9, 2018

mattclay Apr 9, 2018

sivel commented Apr 9, 2018

bersace commented Jun 14, 2018 •

edited

Loading

Handle duplicate headers in the uri module #33792

Handle duplicate headers in the uri module #33792

Conversation

sivel commented Dec 11, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

mattclay commented Jan 3, 2018

ansibot commented Mar 28, 2018

mattclay Apr 9, 2018

Choose a reason for hiding this comment

sivel Apr 9, 2018

Choose a reason for hiding this comment

sivel Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

mattclay Apr 9, 2018

Choose a reason for hiding this comment

mattclay Apr 9, 2018

Choose a reason for hiding this comment

mattclay Apr 9, 2018

Choose a reason for hiding this comment

sivel commented Apr 9, 2018

bersace commented Jun 14, 2018 • edited Loading

sivel Apr 9, 2018 •

edited

Loading

bersace commented Jun 14, 2018 •

edited

Loading