ropensci/osmdata

[FEATURE] Add checks to `opq()` to provide useful error messages for `NA` bbox values

elipousson opened this issue · 1 comments

elipousson commented

I was teaching osmdata to a group of students this evening and discovered that opq() and osmdata_sf() both allow NA bbox values without kicking back an error. The error message from httr2 is not informative and I can't imagine a situation in which you'd want to allow a query to pass a NA bbox value without at least a warning if not an error.

Here is a reprex showing one option for a simple check to address this issue:

library(osmdata)
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright

data <- getbb("Baltimore city, MD") %>% 
  opq() %>% 
  add_osm_feature(key = "tourism", value = "artwork") %>%
  osmdata_sf()
#> Error in `resp_abort()`:
#> ! HTTP 400 Bad Request.

#> Backtrace:
#>     ▆
#>  1. ├─... %>% osmdata_sf()
#>  2. └─osmdata::osmdata_sf(.)
#>  3.   └─osmdata:::fill_overpass_data(obj, doc, quiet = quiet)
#>  4.     └─osmdata:::overpass_query(...)
#>  5.       └─httr2::req_perform(req)
#>  6.         └─httr2:::resp_abort(resp, error_body(req, resp))
#>  7.           └─rlang::abort(...)

opq_ext <- function(bbox = NULL, ...) {
  if (all(is.na(bbox))) {
    stop(
      "bbox is not valid"
    )
  }
  
  opq(bbox = bbox, ...)
}
  
data <- getbb("Baltimore city, MD") %>% 
  opq_ext() %>% 
  add_osm_feature(key = "tourism", value = "artwork") %>%
  osmdata_sf()
#> Error in opq_ext(.): bbox is not valid

packageVersion("osmdata")
#> [1] '0.1.10'
R.Version()$version.string
#> [1] "R version 4.2.0 Patched (2022-05-23 r82396)"

Created on 2022-11-16 with reprex v2.0.2


If you're interested in adding this feature, I'd be happy to add the change to my pull request #278.
mpadge commented

Yeah, you're absolutely right @elipousson, thanks! A PR would be great, but could you please do it as a separate one? You current #278 already contains a heap of changes that i'm hoping to find time to go through early next week at the latest. The best place to put the bbox check would be just before the end of bbox_to_string():

osmdata/R/getbb.R

Line 75 in d37aa01

return (paste0 (bbox, collapse = ","))

That function is called by every other function just prior to actual query submission, so will catch all instances of that. It should also error on any(is.na()), and maybe a clearer message would be something like "bbox contains 'NA' values". Thanks

  • 👍1