gitlabr icon indicating copy to clipboard operation
gitlabr copied to clipboard

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

Open salim-b opened this issue 1 year ago • 2 comments

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.

salim-b avatar Jan 17 '24 13:01 salim-b

Thanks for reporting.
This is what we started to fix in this PR : https://github.com/ThinkR-open/gitlabr/pull/106 , and I hardly find time to finish this big step...

statnmap avatar Jan 17 '24 14:01 statnmap

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

https://github.com/ThinkR-open/gitlabr/blob/bd31578710289034fe03bd654bd0899a669625f1/R/files.R#L127

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.)

salim-b avatar Jan 18 '24 03:01 salim-b

That should be good now in the dev version. I treated this one in PR: https://github.com/ThinkR-open/gitlabr/pull/106 And added some unit tests to verify.
Thank you for your exploration

statnmap avatar May 14 '24 15:05 statnmap