Skip to content
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

Allow endpoints to have query parts. #469

Merged
merged 9 commits into from
Jan 8, 2018
Merged

Allow endpoints to have query parts. #469

merged 9 commits into from
Jan 8, 2018

Conversation

rohe
Copy link
Contributor

@rohe rohe commented Jan 1, 2018

  • Any changes relevant to users are recorded in the CHANGELOG.md.
  • The documentation has been updated, if necessary.

@@ -57,7 +57,10 @@ def get_or_post(uri, method, req, content_type=DEFAULT_POST_CONTENT_TYPE,
if method in ["GET", "DELETE"]:
_qp = req.to_urlencoded()
if _qp:
path = uri + '?' + _qp
if '?' in uri:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be safer to use urlsplit and then append to the query part of the result and the use urlunsplit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't append to the query part of a Splitresult but I've tried to something similar.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is true. This works, but it can be simplified even more.

If you move the urlunsplit from the condition you can then keep the update part and in the else branch simpli assign the _qs to _query.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point and I think I actually dealt with in the latest version of the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually not, I have commented on the appropriate line.

@selfissued
Copy link
Contributor

Is this ready to merge and deploy or are there additional changes needed?

path = urlunsplit((comp.scheme, comp.netloc, comp.path,
_query, comp.fragment))
else:
path = uri + '?' + _qp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the urlsplit/urlunsplit combo for this line as well. Since you are redoing this, it is a good idea to keep them the same.

Copy req since what don't want the update to spread outside this method.
if req.keys():
comp = urlsplit(uri)
if comp.query:
req = req.copy()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this a first statement in the function to make it clear and hopefully error prone :)

@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #469 into master will decrease coverage by 0.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #469      +/-   ##
=========================================
- Coverage   58.68%   58.2%   -0.48%     
=========================================
  Files          61      61              
  Lines       11289   11225      -64     
  Branches     1977    1907      -70     
=========================================
- Hits         6625    6534      -91     
- Misses       4126    4147      +21     
- Partials      538     544       +6
Impacted Files Coverage Δ
src/oic/oauth2/util.py 74.19% <100%> (-23.68%) ⬇️
src/oic/oauth2/message.py 68.3% <0%> (+0.14%) ⬆️

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 9378cd5...9e563d3. Read the comment docs.

@tpazderka tpazderka merged commit 1ff13be into CZ-NIC:master Jan 8, 2018
andrewkrug pushed a commit to mozilla-iam/pyoidc that referenced this pull request Jun 6, 2019
* Allow endpoints to have query parts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants