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

Make sure path has native encoding #183

Merged
merged 4 commits into from
Apr 28, 2019
Merged

Make sure path has native encoding #183

merged 4 commits into from
Apr 28, 2019

Conversation

jennybc
Copy link
Contributor

@jennybc jennybc commented Apr 27, 2019

Fixes #182

If you are on board, I can add a test and apply the same enc2native() fix to other locations where it's needed.

This fixes the reprex I posted in #182 and, when curl is installed from this PR, fixes tidyverse/googledrive#229.

library(curl)
destdir <- tempfile()
dir.create(destdir)
setwd(destdir)
res <- curl_fetch_disk("http://httpbin.org/stream/10", "äggplanta")
res$content
#> [1] "C:\\Users\\jenny\\AppData\\Local\\Temp\\RtmpSy9Rxd\\file27e87c9030d9\\äggplanta"
file.exists(res$content)
#> [1] TRUE
list.files()
#> [1] "äggplanta"

Created on 2019-04-27 by the reprex package (v0.2.1)

@jennybc jennybc changed the title Make path has native encoding Make sure path has native encoding Apr 27, 2019
@codecov-io
Copy link

codecov-io commented Apr 27, 2019

Codecov Report

Merging #183 into master will increase coverage by 1.75%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
+ Coverage   78.81%   80.56%   +1.75%     
==========================================
  Files          35       35              
  Lines        1369     1369              
==========================================
+ Hits         1079     1103      +24     
+ Misses        290      266      -24
Impacted Files Coverage Δ
R/fetch.R 56% <100%> (+24%) ⬆️
R/download.R 100% <100%> (ø) ⬆️
src/fetch.c 90.9% <0%> (+40.9%) ⬆️

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 8377465...e22ef44. Read the comment docs.

@jeroen
Copy link
Owner

jeroen commented Apr 27, 2019

Thanks! Looks good to me.

@jennybc
Copy link
Contributor Author

jennybc commented Apr 27, 2019

I will add a test then (actually, adapt one I added to readxl).

Fixes #182

Ensure native encoding for this path too
@jennybc
Copy link
Contributor Author

jennybc commented Apr 27, 2019

I went back and added the (failing) test, then re-applied the fix.

I can't tell if this PR should also be touching these loci. What do you think @jeroen?

curl/R/form.R

Line 13 in 5be4dcc

path <- normalizePath(path[1], mustWork = TRUE)

path <- normalizePath(path, mustWork = FALSE)

@jennybc
Copy link
Contributor Author

jennybc commented Apr 28, 2019

I decided it was pretty clear those other two loci also needed the same and empirically checked this by modifying a test that uses form_file. Let me know if you want tests for them. Or you'd rather make a normalize_path() shim that calls enc2native(), use it in all 4 places, and test that instead.

@jeroen
Copy link
Owner

jeroen commented Apr 28, 2019

Thanks looks good. I don't think we need extra tests. Did you empirically check this on Windows?

@jennybc
Copy link
Contributor Author

jennybc commented Apr 28, 2019

Yes. I did all of this over in Windows.

For form_file(), I empirically checked that current master does not work for a path with non-ASCII characters and this PR fixes it. I show that below, except in the opposite order, i.e. with this PR then without.

> tricky_filename <- "\u00C0\u00CB\u00D0"
> file.copy(system.file("DESCRIPTION"), tricky_filename)
[1] TRUE
> hx <- handle_setform(new_handle(),
+                      foo = "blabla",
+                      bar = charToRaw("boeboe"),
+                      iris = form_data(serialize(iris, NULL), "data/rda"),
+                      description = form_file("ÀËÐ"),
+                      logo = form_file(file.path(Sys.getenv("R_DOC_DIR"), "html/logo.jpg"), "image/jpeg")
+ )
> req <- curl_fetch_memory(httpbin("post"), handle = hx)
> expect_equal(req$status_code, 200)

> devtools::load_all(".")
Loading curl
This is libcurl version 7.64.1 with (OpenSSL/1.1.1a) Schannel 
Using test server: http://hb.opencpu.org 
> hx <- handle_setform(new_handle(),
+                      foo = "blabla",
+                      bar = charToRaw("boeboe"),
+                      iris = form_data(serialize(iris, NULL), "data/rda"),
+                      description = form_file("ÀËÐ"),
+                      logo = form_file(file.path(Sys.getenv("R_DOC_DIR"), "html/logo.jpg"), "image/jpeg")
+ )
> req <- curl_fetch_memory(httpbin("post"), handle = hx)
Error in curl_fetch_memory(httpbin("post"), handle = hx) : 
  Failed to open/read local data from file/application

@jennybc
Copy link
Contributor Author

jennybc commented Apr 28, 2019

Did you empirically check this on Windows?

Also AppVeyor seems happy. The travis failures do not seem to be related to anything I'm doing 🤷‍♀️

@jeroen jeroen merged commit ab16ac1 into jeroen:master Apr 28, 2019
@jeroen
Copy link
Owner

jeroen commented Apr 28, 2019

Thanks Jenny! Awesome the detective work 🕵

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.

Problem with UTF-8 file paths on windows Foreign character file names
3 participants