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

Inconsistent handling of file_path arg in convenience functions wrt percent-encoding #111

Closed
salim-b opened this issue Jan 17, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@salim-b
Copy link

salim-b commented Jan 17, 2024

Consider the following reprex:

gitlabr::set_gitlab_connection(gitlab_con = gitlabr::gl_project_connection(gitlab_url = "https://gitlab.com",
                                                                           project = 19126559L,
                                                                           private_token = Sys.getenv("GITLAB_COM_TOKEN")))    
path_rel <- "inst/rstudio/addins.dcf"

# `gl_get_file()` fails without percent-encoded `file_path`
try(
  gitlabr::gl_get_file(file_path = path_rel,
                       ref = "master")
)
#> Error in http_error_or_content(.) : Not Found (HTTP 404).

gitlabr::gl_get_file(file_path = xml2::url_escape(path_rel),
                     ref = "master")
#> [1] "Name: Process R Markdown package\nDescription: Executes `pkgpurl::process_pkg()`.\nBinding: process_pkg\nInteractive: true\n\nName: Load R Markdown package\nDescription: Executes `pkgpurl::load_pkg()`.\nBinding: load_pkg\nInteractive: true\n\nName: Purl `Rmd/`\nDescription: Executes `pkgpurl::purl_rmd()`.\nBinding: purl_rmd\nInteractive: false\n\nName: Lint R Markdown package\nDescription: Executes `pkgpurl::lint_rmd()`.\nBinding: lint_rmd\nInteractive: true\n\nName: Run `*.nopurl.Rmd` files\nDescription: Executes `pkgpurl::run_nopurl_rmd()`.\nBinding: run_nopurl_rmd\nInteractive: true\n"

# `gl_file_exists()` OTOH doesn't work properly with percent-encoded `file_path`
gitlabr::gl_file_exists(file_path = xml2::url_escape(path_rel),
                        ref = "master")
#> [1] FALSE

gitlabr::gl_file_exists(file_path = path_rel,
                        ref = "master")
#> [1] TRUE

Created on 2024-01-17 with reprex v2.1.0

As can be seen in the example above, gl_get_file() requires the file_path argument to be percent-encoded while gl_file_exists() requires the opposite. This is confusing. I think gitlabr should instead expect the file_path / path arguments to be unescaped, i.e. literal, in all functions and do the necessary escaping internally. This would be more convenient and less error-prone for users.

@statnmap
Copy link
Member

Thanks for reporting.
This is what we started to fix in this PR : #106 , and I hardly find time to finish this big step...

@statnmap statnmap added the bug Something isn't working label Jan 17, 2024
@salim-b
Copy link
Author

salim-b commented Jan 18, 2024

Note that this problem also causes gitlabr::gl_push_file() to fail updating existing files in subfolders since it uses gl_file_exists() internally

exists <- gl_file_exists(project = project, file_path, ref = branch, ...)

and thus wrongly concludes an already existing file wouldn't exist and makes a POST instead of a PUT request.

( gl_push_file() requires file_path to be already percent-encoded (same as gl_get_file()), while gl_file_exists() requires the opposite.)

statnmap added a commit that referenced this issue May 14, 2024
Appropriate handling of files in subfolders using URLdecode and URLencode when needed.

fix issue #111
@statnmap
Copy link
Member

That should be good now in the dev version.
I treated this one in PR: #106
And added some unit tests to verify.
Thank you for your exploration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants