httr2 debug tools?

[kbvernon]

Is your feature request related to a problem? Please describe.
No specific problem, just thinking about how to help users with future debugging.

Describe the solution you'd like
I'm sure you knew this, but {httr2} has some tools like last_request() and last_response() that may be useful since they can help to inspect the request/response for clues. In fact, because you are already building on {httr2}, these can be used on {arcgislayers} request/responses directly.

Describe alternatives you've considered
You could wrap these if you want to avoid the API weirdness for the user of calling on other packages to inspect this one's classes, but maybe that's not a big issue? Or not an issue at all? Could just add something to the docs with some examples of using the {httr2} functions, assuming you think that would be worthwhile.

Additional context
Friday afternoon...

[elipousson]

Since httr2::req_perform() now takes an error_call parameter, I had previously argued in favor of adding a call or error_call parameter to more functions in order to improve error messages (especially if package developers like me end up wrapping {arcgislayers} functions in other wrapper functions and you need to figure out where an error is really coming from). I think @JosiahParry was unconvinced by my argument at the time but I still think it is a good idea.

This approach is described in the rlang documentation as well as this recent blog post by Nick Tierney: https://www.njtierney.com/post/2023/12/06/long-errors-smell/

I think there are a few different opportunities to enhance the input checks, error messages, and error handling in the package, TBH. Happy to work on this in combination with this issue or submit a separate issue with an outline of potential changes (I'm partial to the tidyverse style guide on error messages) before spending time on a PR.

[kbvernon]

Thanks, Eli! I was thinking more about users, not package developers, but the latter seems like something worth considering.

Is your suggestion that top level functions like arc_select() should have a call parameter so that functions that wrap it can have their environments reported in arc_select()'s error message? Or, are you just saying that the error helpers inside those top level functions need to do this?

[elipousson]

I'd love to see the former so I can wrap arc_select() or arc_read() in my own packages for easy error handling but that is the argument I lost last time this came up. But the latter option of error helpers inside top-level functions pass call appropriately to req_perform or any input checking functions would be a good consolation prize.

Apologies if I'm accidentally hijacking your original issue! I'd assume that last_request() or last_response() could likely be used in combination with httr2::req_error() to improve handling of API errors.

[kbvernon]

used in combination with httr2::req_error() to improve handling of API errors.

Exactly what I was thinking!

No worries about hijacking. They seem sufficiently related to me to warrant discussion in the same place.

And, actually, the docs for req_error() have some advice about error handling that might be the way to go for packages that depend on {arcgislayers}. Specifically,

Or if you want to re-throw the error with some additional context, use withCallingHandlers(), e.g.:

withCallingHandlers(
  req |> req_perform() |> resp_body_json(),
  httr2_http_404 = function(cnd) {
    rlang::abort("Couldn't find user", parent = cnd)
  }
)

If that's feasible, maybe we could just add a tutorial for package devs to the docs?

[JosiahParry]

Man, this took forever to track down! For posterity: #62 (comment)

[JosiahParry]

I'm still a bit unclear what the scope of this issue is. Do you think we can narrow it down? @kbvernon in the case of handling errors directly from a feature service, its unfortunately bit dirtier than I'd like. See arcgisutils::detect_errors(). The challenge is that all requests return 200 status regardless of an error or not.

Regarding call in the arguments for arc_select() etc.:

I think the scope of the previous exchange was much smaller. I think if we have a good example and good use case for introducing error_call into arc_open() & co we can do it. But I want to prevent introducing anything that might confuse a non-developer.

Using rlang we can accomplish this today without any modification via try_fetch() which catches the errors and handles them. You could for example precipitate them with one's own class.

my_fn <- function(url) {
  rlang::try_fetch(arcgislayers::arc_open(url), error = function(cnd) {
    rlang::abort(
      class = "my_err_class", 
      parent = cnd
      )
  })
}

my_fn_no_wrap <- function(url) {
  arcgislayers::arc_open(url)
}

my_fn("234234")
#> Error in `my_fn()`:
#> Caused by error in `httr2::req_perform()` at arcgisutils/R/utils-requests.R:44:2:
#> ! Failed to perform HTTP request.
#> Caused by error in `curl::curl_fetch_memory()`:
#> ! URL rejected: No host part in the URL
#> Backtrace:
#>      ▆
#>   1. └─global my_fn("234234")
#>   2.   ├─rlang::try_fetch(...)
#>   3.   │ ├─base::tryCatch(...)
#>   4.   │ │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>   5.   │ │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>   6.   │ │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>   7.   │ └─base::withCallingHandlers(...)
#>   8.   └─arcgislayers::arc_open(url)
#>   9.     ├─arcgisutils::compact(fetch_layer_metadata(req, token))
#>  10.     │ └─base::Filter(length, .x) at arcgisutils/R/utils.R:25:11
#>  11.     │   ├─base::unlist(lapply(x, f))
#>  12.     │   └─base::lapply(x, f)
#>  13.     └─arcgisutils::fetch_layer_metadata(req, token)
#>  14.       ├─httr2::resp_body_string(httr2::req_perform(req)) at arcgisutils/R/utils-requests.R:44:2
#>  15.       │ └─httr2:::check_response(resp)
#>  16.       │   └─httr2:::is_response(resp)
#>  17.       └─httr2::req_perform(req) at arcgisutils/R/utils-requests.R:44:2
#>  18.         └─base::tryCatch(...)
#>  19.           └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  20.             └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  21.               └─value[[3L]](cond)
my_fn_no_wrap("2342343")
#> Error in `httr2::req_perform()` at arcgisutils/R/utils-requests.R:44:2:
#> ! Failed to perform HTTP request.
#> Caused by error in `curl::curl_fetch_memory()`:
#> ! URL rejected: No host part in the URL
#> Backtrace:
#>      ▆
#>   1. └─global my_fn_no_wrap("2342343")
#>   2.   └─arcgislayers::arc_open(url)
#>   3.     ├─arcgisutils::compact(fetch_layer_metadata(req, token))
#>   4.     │ └─base::Filter(length, .x) at arcgisutils/R/utils.R:25:11
#>   5.     │   ├─base::unlist(lapply(x, f))
#>   6.     │   └─base::lapply(x, f)
#>   7.     └─arcgisutils::fetch_layer_metadata(req, token)
#>   8.       ├─httr2::resp_body_string(httr2::req_perform(req)) at arcgisutils/R/utils-requests.R:44:2
#>   9.       │ └─httr2:::check_response(resp)
#>  10.       │   └─httr2:::is_response(resp)
#>  11.       └─httr2::req_perform(req) at arcgisutils/R/utils-requests.R:44:2
#>  12.         └─base::tryCatch(...)
#>  13.           └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  14.             └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  15.               └─value[[3L]](cond)

^{Created on 2024-01-23 with reprex v2.0.2}

[kbvernon]

I'm still a bit unclear what the scope of this issue is. Do you think we can narrow it down?

I think my original thought here (keep in mind it was a Friday afternoon...) was that last_request() and last_response() will return all the query parameters that you might provide through arc_select() and related, which (I think, maybe?) could be useful for debugging code, especially because it shows what the parameters look like as sanitized URL strings.

Just to add to this, one of the things that came out of our Discord discussion the other day was that ESRI doesn't necessarily do the "sanitizing" (is that the right word?) the same way as {httr2}.

[JosiahParry]

1. didn't know about those. Thats awesome! Thank you!
2. does httr2 not suffice here?

re "sanitizing", on the whole, not really for feature services. I'm sure there's some legacy reason for it and the Esri equivalent of "for compatibility with S." Other newer services (e.g. knowledge graph) probably don't have the same issues.

Here's an example using httr2 directly. This is a very handy dandy tool!

# FeatureServer
furl <- paste0(
  "https://services3.arcgis.com/ZvidGQkLaDJxRSJ2/arcgis/rest/services/",
  "PLACES_LocalData_for_BetterHealth/FeatureServer"
)

arcgislayers::arc_open(furl)
#> <FeatureServer <5 layers, 0 tables>>
#> CRS: 3785
#> Capabilities: Query,Extract
#>   0: PlacePoints (esriGeometryPoint)
#>   1: PlaceBoundaries (esriGeometryPolygon)
#>   2: Counties (esriGeometryPolygon)
#>   3: Tracts (esriGeometryPolygon)
#>   4: ZCTAs (esriGeometryPolygon)
httr2::last_request()
#> <httr2_request>
#> GET
#> https://services3.arcgis.com/ZvidGQkLaDJxRSJ2/arcgis/rest/services/PLACES_LocalData_for_BetterHealth/FeatureServer?f=json
#> Body: empty
httr2::last_response()
#> <httr2_response>
#> GET
#> https://services3.arcgis.com/ZvidGQkLaDJxRSJ2/arcgis/rest/services/PLACES_LocalData_for_BetterHealth/FeatureServer?f=json
#> Status: 200 OK
#> Content-Type: application/json
#> Body: In memory (9309 bytes)

^{Created on 2024-01-23 with reprex v2.0.2}

[kbvernon]

does httr2 not suffice here?

Maybe? That was one of the questions I posed to you initially. Personally, the {httr2} functions in that context look utterly mysterious and borderline magical. But does that (potential) mark on the API warrant wrapping the {httr2} functions or are we still in the realm of additional tutorial content?

[elipousson]

This function looks super messy but here is one possible approach to catching errors:

• Error if the response has a named error element
• Error if the body fails a test for a valid type (works because a bad request typically returns an HTML body instead of a JSON)

#' @noRd
arc_request <- function(
    base_url,
    valid_types = "application/json"
) {
  req <- httr2::request(
    base_url
  ) 
  
  req <- httr2::req_user_agent(
    req = req,
    "arcgislayers (https://r.esri.com/arcgislayers/)"
    )

  req <- httr2::req_error(
    req,
    is_error = function(resp) {
      if (rlang::has_name(resp, "error")) {
        return(TRUE)
      }
      
      rlang::try_fetch(
        httr2::resp_check_content_type(
          resp,
          valid_types = valid_types
          ),
        error = function(cnd) {
          TRUE
          }
        )
    },
    body = function(resp) {
      if (rlang::has_name(resp, "error")) {
        return(httr2::resp_body_json(resp)[["error"]])
      }
      
      rlang::try_fetch(
        httr2::resp_check_content_type(
          resp,
          valid_types = valid_types
        ),
        error = function(cnd) {
          cnd[["body"]]
        }
      )
      
    }
  )
    
  req
}

I'm not sure if or how to integrate last_error() or last_request() although it may be helpful to suggest use of those functions if we suspect the failure is due to the request and the user could de-bug by reviewing that request.

[JosiahParry]

Thinking this through, it might not be that big of a deal for the most part because we're using req_perform_parallel() in which case errors are never actually thrown but instead are returned as NULL 🤔

EDIT: thinking this through even further it would be really bad to process the json more than one time. It's a very expensive operation! In this example we process the json two times for each request before we actually process it AGAIN (third time) to convert into sf object.

I wonder if the best solution is to let this crummy errors propagate but we check for error field in the parsed results in parse_esri_json() and handle the issue THERE instead

[kbvernon]

Seems like the error should be handled right away rather than waiting for all the requests to get performed, especially if the same error is going to happen multiple times or even every time. Right now you have on_error = "continue" in req_perform_parallel(). Perhaps this should be set to "stop"?
https://httr2.r-lib.org/reference/req_perform_parallel.html

[JosiahParry]

To be clear, the responses from our requests will never return an error. The way that we can make it return an error is to use req_error(). Doing so requires us to parse the response and determine if it is an error. If we take this approach we will have to process each json response twice. Thats a lot. Because everything returns a 200 code we won't ever encounter a 400/500 code response. I'm pretty opposed to doubling the processing time to check for errors during run time.

This PR adjusts the parse_esri_json() function to report an error if it is present and then return an empty data.frame R-ArcGIS/arcgisutils#22.

The "solution" will be to use a rust-base implementation. I've got a WIP for that, but it requires geoarrow which is not released on CRAN yet.

[kbvernon]

Because everything returns a 200 code we won't ever encounter a 400/500 code response.

I see that point was made very clearly before, but I didn't pick up on it. Sorry about that. Out of curiosity, is this an AGOL thing?

I'm pretty opposed to doubling the processing time to check for errors during run time.

Yeah, that makes sense. Seems like the best option now is just to process the whole thing after performing the requests, then informing the user about what requests failed and why. I see that you use warn() instead of abort() in that PR. That's probably the right way to go in this context, even if it's not preferable.

The "solution" will be to use a rust-base implementation.

Ha. Naturally.

[elipousson]

I think one very common source of errors is issues with the input URL. Maybe we could add more detailed input URL checks rather than checking for an error in the response?

The esri2sf package has some URL checking functionality (that I expanded and revised after forking the original package) which could be worth looking at for arcgislayers or arcgisutils: https://github.com/elipousson/esri2sf/blob/master/R/esriUrl.R

[JosiahParry]

I've converted this to a discussion because I don't think there's a clear issue that we're trying to solve with it quite yet.

[kbvernon]

We should probably form a committee to investigate what the issue is, too.