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

[FIX] web_planner: test should not assume url parameter order #30138

Closed
wants to merge 1 commit into
base: 11.0
from

Conversation

Projects
None yet
7 participants
@ap-wtioit
Copy link
Contributor

ap-wtioit commented Jan 11, 2019

Description of the issue/feature this PR addresses:
test_web_planner throws errors like this on some environments:
AssertionError:
'/web#action=135&view_type=list' != '/web#view_type=list&action=135'

Current behavior before PR:
the urls in web_planner are created with a dict for the parameters to
create the urls. dicts are unorderd up to python 3.5 (see
https://www.python.org/dev/peps/pep-0468/)

Desired behavior after PR is merged:
As the urls /web#action=135&view_type=list and
/web#view_type=list&action=135 are equivalent we now test if the
parameters are the equal in the url regardless of their order.

Info @wt-io-it

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the seen 🙂 label Jan 11, 2019

@wtaferner

This comment has been minimized.

Copy link
Contributor

wtaferner commented Jan 11, 2019

@xmo-odoo @nim-odoo
May I ask you to confirm and merge this?

@jpp-odoo
FYI as you have written these tests

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Jan 11, 2019

@jpp-odoo please review

@jpp-odoo

This comment has been minimized.

Copy link
Contributor

jpp-odoo commented Jan 11, 2019

@wtaferner @ap-wtioit
The runbot is red, it fails in the test you just change.
See: http://runbot.odoo.com/runbot/build/426058

@ap-wtioit

This comment has been minimized.

Copy link
Contributor

ap-wtioit commented Jan 11, 2019

@jpp-odoo sorry about that, i'll check the python / werkzeug version compatiblity (it was working on our runbot)

@ap-wtioit

This comment has been minimized.

Copy link
Contributor

ap-wtioit commented Jan 11, 2019

it seems:

from werkzeug import urls
dict(urls.url_decode('a=b&c=d'))

gives {'a': 'b', 'c': 'd'} in python 3.7
vs {'c': ['d'], 'a': ['b']} in python 3.5 and python 3.6

seems to be related to pallets/werkzeug#1379

@ap-wtioit ap-wtioit force-pushed the ap-wtioit:11.0-fix_test_web_planner_parameter_order_in_asserted_urls branch from e3778e9 to 0960daa Jan 11, 2019

@d-fence

This comment has been minimized.

Copy link
Contributor

d-fence commented Jan 11, 2019

Result from your test on the runbot build of this PR
`
Python 3.6.7 (default, Oct 22 2018, 11:32:17)
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.

from werkzeug import urls
dict(urls.url_decode('a=b&c=d'))
{'a': 'b', 'c': 'd'}

`

@ap-wtioit

This comment has been minimized.

Copy link
Contributor

ap-wtioit commented Jan 11, 2019

@jpp-odoo, @d-fence i updated the merge request to user .to_dict(flat=False) of werkzeugs MultiDict

i tested with docker:

docker run --rm -it python:3.5 bash -c 'pip install werkzeug; python --version; python -c "from werkzeug import urls; print(urls.url_decode(\"a=b&a=f&c=d\").to_dict(flat=False))"'
docker run --rm -it python:3.6 bash -c 'pip install werkzeug; python --version; python -c "from werkzeug import urls; print(urls.url_decode(\"a=b&a=f&c=d\").to_dict(flat=False))"'
docker run --rm -it python:3.7 bash -c 'pip install werkzeug; python --version; python -c "from werkzeug import urls; print(urls.url_decode(\"a=b&a=f&c=d\").to_dict(flat=False))"'

and then locally with odoo and runbot

the behaviour seems to be changed in 3.6.7:

 docker run --rm -it python:3.6.6 bash -c 'pip install werkzeug; python --version; python -c "from werkzeug import urls; print(dict(urls.url_decode(\"a=b&a=f&c=d\")))"'
 docker run --rm -it python:3.6.7 bash -c 'pip install werkzeug; python --version; python -c "from werkzeug import urls; print(dict(urls.url_decode(\"a=b&a=f&c=d\")))"'
Python 3.6.6
{'a': ['b', 'f'], 'c': ['d']}
Python 3.6.7
{'a': 'b', 'c': 'd'}

@ap-wtioit ap-wtioit force-pushed the ap-wtioit:11.0-fix_test_web_planner_parameter_order_in_asserted_urls branch from 0960daa to 268804b Jan 11, 2019

"/web#view_type=list&action=%s" % action.id,
)
backend_url = Planner.prepare_backend_url(self, "web_planner.test_00")
parameters = urls.url_decode(backend_url[backend_url.index('#') + 1:]).to_dict(flat=False)

This comment has been minimized.

@xmo-odoo

xmo-odoo Jan 11, 2019

Collaborator

Seems fine to me, though I'd suggest using urls.url_parse(backend_url).fragment to extract the fragment string, instead of string munging.

Also why the flat=False bit`? That seems to only make the test file less readable, none of the tests is using multi-valued keys, why not generate a flat dict? pallets/werkzeug#1379 suggests flat=False because the issue author specifically wants multi-valued keys in the output.

This comment has been minimized.

@ap-wtioit

ap-wtioit Jan 11, 2019

Contributor

without flat=False the test would not be able to differentiate between "a=b&a=f&c=d" and "a=b&c=d".
for me the flat=False is more like the original string comparison that i replaced.

This comment has been minimized.

@ap-wtioit

ap-wtioit Jan 11, 2019

Contributor

I will update the pull request for url_parse(backen_url).fragment, thanks i didn't have this method on my radar

This comment has been minimized.

@xmo-odoo

xmo-odoo Jan 11, 2019

Collaborator

without flat=False the test would not be able to differentiate between "a=b&a=f&c=d" and "a=b&c=d".

Sure but I don't think we care, I don't think multidicts are used anywhere in Odoo.

This comment has been minimized.

@ap-wtioit

ap-wtioit Jan 11, 2019

Contributor

@xmo-odoo i will remove it if i have too, from my point of view multiple values for parameters can be a thing and "a=b&a=f" is not equal to "a=b". in version 11 at least the ExportDialog in Odoo and account_reports have <select ...multiple...> in them which would need to explicitly get all the parameter values if used in a form with action="GET". also using the MultiDict is kind of implicit if one would use request.httprequest.values.getlist(...) as i did in this merge request: #25323 that would enable/reenable/fix the selecting of multiple variant options in the shop for this url(it would only enable the product page to have a more concise selection in the shop):
http://426059-11-0-c5a3b6.runbot13.odoo.com/shop?search=&attrib=2-3&attrib=2-4&attrib=1-1&attrib=1-2
screenshot from 2019-01-11 15-40-46

Also request.httprequest.args.getlist() is used in:

attrib_list = request.httprequest.args.getlist('attrib')
attrib_values = [[int(x) for x in v.split("-")] for v in attrib_list if v]
attributes_ids = {v[0] for v in attrib_values}
attrib_set = {v[1] for v in attrib_values}

if not keep_params and not additional_params:
keep_params = ('*',)
params = additional_params.copy()
qs_keys = list(request.httprequest.args) if request else []
for keep_param in keep_params:
for param in fnmatch.filter(qs_keys, keep_param):
if param not in additional_params and param in qs_keys:
params[param] = request.httprequest.args.getlist(param)
return werkzeug.urls.url_encode(params)

And some more usages of request.httprequest.*.getlist() in Odoo

So if i have to i will remove the flat=False, but i would prefer not to.

[FIX] web_planner: test should not assume url parameter order
test_web_planner throws errors like this on some environments:
AssertionError:
'/web#action=135&view_type=list' != '/web#view_type=list&action=135'

the urls in web_planner are created with a dict for the parameters to
create the urls. dicts are unorderd up to python 3.5. (see
 https://www.python.org/dev/peps/pep-0468/)

As the urls /web#action=135&view_type=list and
/web#view_type=list&action=135 are equivalent we now test if the
parameters are the equal in the url regardless of their order.

@ap-wtioit ap-wtioit force-pushed the ap-wtioit:11.0-fix_test_web_planner_parameter_order_in_asserted_urls branch from 268804b to 3730ae4 Jan 11, 2019

@robodoo robodoo added the CI 🤖 label Jan 11, 2019

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

robodoo r+

robodoo pushed a commit that referenced this pull request Jan 15, 2019

[FIX] web_planner: test should not assume url parameter order
test_web_planner throws errors like this on some environments:
AssertionError:
'/web#action=135&view_type=list' != '/web#view_type=list&action=135'

the urls in web_planner are created with a dict for the parameters to
create the urls. dicts are unorderd up to python 3.5. (see
 https://www.python.org/dev/peps/pep-0468/)

As the urls /web#action=135&view_type=list and
/web#view_type=list&action=135 are equivalent we now test if the
parameters are the equal in the url regardless of their order.

closes #30138
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Jan 15, 2019

Merged, thanks!

@robodoo robodoo closed this Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment