`.destfile` unexpectedly written even on error

[arcresu]

When .destfile is provided, gh will write to it even if the API returned an error:

library(gh)

# trying to download a file from a github repo that unexpectedly doesn't exist
> gh("/repos/r-lib/gh/contents/DOES_NOT_EXIST", .destfile="gh_destfile", .accept = "application/vnd.github.v3.raw")
Error in `gh_process_response()`:
! GitHub API error (404): Not Found
✖ URL not found:
  <https://api.github.com/repos/r-lib/gh/contents/DOES_NOT_EXIST>
ℹ Read more at
  <https://docs.github.com/rest/reference/repos#get-repository-content>

> readLines(file("gh_destfile"), warn=FALSE)
[1] "{\"message\":\"Not Found\",\"documentation_url\":\"https://docs.github.com/rest/reference/repos#get-repository-content\"}

In the absence of .destfile, when the request succeeds (e.g. change DOES_NOT_EXIST to README.md), the contents of the file are printed out. When it fails there's an error message, but nothing else is printed. However when .destfile is given, that file is written to whether or not the request succeeded. In the error case, the JSON-encoded error is put in the file even when .accept specified that we were expecting to write something other than JSON.

It's a bit of a surprising behaviour, and it's the underlying cause of a bug in usethis (r-lib/usethis#1774). Granted, usethis could have been handling the error more carefully.

I think it would be less surprising if the .destfile were only created and written when the API request succeeded. All data from the API error already appears in the R error, so we don't lose any information.

[gaborcsardi]

What version of gh did you use?

[arcresu]

I get the same behaviour on both 1.4.0 from CRAN and the latest git HEAD.

The relevant code I think is: https://github.com/r-lib/gh/blob/26c50f300fbee64472a4c1d9311035a599de4c94/R/gh.R#L286-L289

It writes the request to the output file first and only checks the response status afterwards.

[jennybc]

I went further back in time, i.e. to gh v1.3.1 and v1.3.0, and the behaviour was the same (.destfile is written even on error). So I don't think this is from the httr2 refactoring.

[gaborcsardi]

I guess this is a side effect of how HTTP works. Even if the status code is an error, the response has a body, and the HTTP client that handles .destfile will write it out.

So I guess we should delete it on error?

That can also lead some weird consequences, e.g. if you want to overwrite a file, and there is an error, then it will be deleted.

[gaborcsardi]

We could write the response to a temporary file and then rename it on success and delete it on failure.