Skip to content

fix: urlencode id part of path#1606

Closed
Pro wants to merge 1 commit into
python-gitlab:mainfrom
Pro:feat-urlencode
Closed

fix: urlencode id part of path#1606
Pro wants to merge 1 commit into
python-gitlab:mainfrom
Pro:feat-urlencode

Conversation

@Pro

@Pro Pro commented Sep 22, 2021

Copy link
Copy Markdown

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

codecov-commenter commented Sep 29, 2021

Copy link
Copy Markdown

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

Pro commented Oct 8, 2021

Copy link
Copy Markdown
Author

@nejch this PR should be ready for review/merge

@nejch

nejch commented Oct 8, 2021

Copy link
Copy Markdown
Member

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

@nejch nejch assigned Pro Oct 8, 2021
@Pro

Pro commented Oct 8, 2021

Copy link
Copy Markdown
Author

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

nejch commented Nov 7, 2021

Copy link
Copy Markdown
Member

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

@nejch

nejch commented Jan 13, 2022

Copy link
Copy Markdown
Member

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