From 9c96cf7faeb404f4486007b581363e9f9fe546c3 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 12 Jun 2026 13:22:55 -0500 Subject: [PATCH] fix: omit `properties` filter instead of requesting "id" for id/geometry-only calls `switch_properties_id()` strips the feature id and `geometry` from the wire `properties` (the feature id is always returned and renamed downstream; geometry is controlled by `skipGeometry`). When a user requested only the id and/or geometry, the list became empty and the function fell back to `properties <- "id"`. That fallback now fails: several collections (e.g. `daily`, `continuous`) reject `id` as a selectable property, and it also fails the available-properties check in `construct_api_requests()`, so e.g. `read_waterdata_daily(properties = "daily_id")` errors with `Invalid properties: id`. Fall back to `NA_character_` instead, which omits the `properties` filter entirely. The feature id still comes back and `rejigger_cols()` subsets the result to the requested column, so id/geometry-only requests succeed on every collection (verified live against daily, continuous, and monitoring-locations). Collections that still accept `id` (e.g. monitoring-locations) are unchanged. Adds a unit test for `switch_properties_id()` covering the id/geometry-only and mixed cases. Co-Authored-By: Claude Fable 5 --- R/get_ogc_data.R | 11 ++++++++--- tests/testthat/tests_general.R | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/R/get_ogc_data.R b/R/get_ogc_data.R index 89ebb6306..d78519fd6 100644 --- a/R/get_ogc_data.R +++ b/R/get_ogc_data.R @@ -171,9 +171,14 @@ switch_properties_id <- function(properties, id) { } if (length(properties) == 0) { - # If a user requested only id and/or geometry, properties would now be empty - # geometry is taken care of with skipGeometry - properties <- "id" + # If a user requested only id and/or geometry, properties is now empty. + # The feature "id" always comes back (and geometry is handled by + # skipGeometry), so omit the properties filter entirely rather than + # requesting "id": several collections (e.g. daily, continuous) now + # reject "id" as a selectable property, and it also fails the + # available-properties check below. rejigger_cols() still subsets the + # result to the requested id column. + properties <- NA_character_ } } diff --git a/tests/testthat/tests_general.R b/tests/testthat/tests_general.R index 25d272e57..f0830ffa9 100644 --- a/tests/testthat/tests_general.R +++ b/tests/testthat/tests_general.R @@ -648,3 +648,35 @@ test_that("format_dates", { "2025-10-01/2026-02-02" ) }) + +test_that("switch_properties_id drops id and geometry from the wire properties", { + # The feature "id" always comes back (renamed to the service id + # downstream) and several collections (e.g. daily, continuous) now reject + # "id" as a selectable property, so it must never be sent. A request for + # only id and/or geometry must omit the properties filter (NA), not fall + # back to "id". + expect_true(all(is.na( + dataRetrieval:::switch_properties_id("daily_id", id = "daily_id") + ))) + expect_true(all(is.na( + dataRetrieval:::switch_properties_id("id", id = "daily_id") + ))) + expect_true(all(is.na( + dataRetrieval:::switch_properties_id(c("daily_id", "geometry"), id = "daily_id") + ))) + + # Mixed requests: the id alias and geometry are dropped, real properties kept. + expect_equal( + dataRetrieval:::switch_properties_id(c("daily_id", "value"), id = "daily_id"), + "value" + ) + expect_equal( + dataRetrieval:::switch_properties_id(c("id", "value", "geometry"), id = "daily_id"), + "value" + ) + + # No properties requested -> unchanged (NA passes through). + expect_true(all(is.na( + dataRetrieval:::switch_properties_id(NA, id = "daily_id") + ))) +})