Skip to content

fix: urlencode id part of path #1606

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Pro
Copy link

@Pro Pro commented Sep 22, 2021

The tag_name part of the path must be urlencoded.

Found this issue when trying to update a release which has a slash in its title, e.g., "1.2.3/testing"

See also:
https://docs.gitlab.com/ee/api/releases/index.html#update-a-release

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2021

Codecov Report

Merging #1606 (6944bc7) into master (227607c) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
+ Coverage   91.59%   91.85%   +0.25%     
==========================================
  Files          74       74              
  Lines        4272     4295      +23     
==========================================
+ Hits         3913     3945      +32     
+ Misses        359      350       -9     
Flag Coverage Δ
cli_func_v4 81.86% <100.00%> (+0.26%) ⬆️
py_func_v4 80.88% <100.00%> (+0.26%) ⬆️
unit 83.67% <100.00%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gitlab/mixins.py 91.14% <100.00%> (+0.02%) ⬆️
gitlab/v4/objects/container_registry.py 83.33% <0.00%> (ø)
gitlab/v4/objects/users.py 98.43% <0.00%> (+0.03%) ⬆️
gitlab/v4/objects/merge_request_approvals.py 93.05% <0.00%> (+0.19%) ⬆️
gitlab/client.py 89.79% <0.00%> (+0.93%) ⬆️
gitlab/v4/objects/branches.py 100.00% <0.00%> (+26.47%) ⬆️

@Pro
Copy link
Author

Pro commented Oct 8, 2021

@nejch this PR should be ready for review/merge

@nejch
Copy link
Member

nejch commented Oct 8, 2021

Hi @Pro, thanks for this PR. Just a few notes on this:

@nejch nejch assigned Pro Oct 8, 2021
@Pro
Copy link
Author

Pro commented Oct 8, 2021

I squashed all the commits. Currently I cannot invest more time into writing tests, maybe in a few weeks when there is more time, sorry.

@nejch
Copy link
Member

nejch commented Nov 7, 2021

@Pro this might need a rebase now as we've switched to f-strings.

@nejch
Copy link
Member

nejch commented Jan 13, 2022

Hi @Pro! This was part of a much larger URL-encoding issue and this PR provides a specific solution in update().

I believe we have now solved the underlying issue in a more general way that solves it for all endpoints and ensures a string is only ever URL-encoded once: #1819.

So I'll close this PR as I think the issue is fixed and we have tests for this now. But thanks again for your contribution, it showed us how widespread the URL-encoding problem was. Feel free to report any more issues or PRs if you find anything else :)

@nejch nejch closed this Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants