Skip to content

fix: don't request id as a property (omit it) for id/geometry-only OGC calls#906

Draft
thodson-usgs wants to merge 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix/ogc-properties-id-400
Draft

fix: don't request id as a property (omit it) for id/geometry-only OGC calls#906
thodson-usgs wants to merge 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix/ogc-properties-id-400

Conversation

@thodson-usgs

Copy link
Copy Markdown
Contributor

Problem

read_waterdata_*(properties = "<id column>") (or only id + geometry) errors:

read_waterdata_daily(monitoring_location_id = "USGS-05427718",
                     parameter_code = "00060", time = "2025-01-01/..",
                     properties = "daily_id")
#> Error: Invalid properties: id

switch_properties_id() correctly strips the feature id and geometry from the wire properties (the feature id is always returned and renamed downstream; geometry is controlled by skipGeometry). But when the request was only the id and/or geometry, the list became empty and it fell back to properties <- "id". That fallback no longer works: the USGS OGC API now rejects id as a selectable property on several collections (e.g. daily, continuous), and id also fails the available-properties check in construct_api_requests().

This is the same upstream API change that affected the Python dataretrieval package; the R package's normal multi-property path already drops id correctly, so only the id/geometry-only fallback is affected.

Fix

Fall back to NA_character_ instead of "id", which omits the properties filter entirely. The feature id still comes back regardless, and rejigger_cols() subsets the result to the requested column — so id/geometry-only requests now succeed on every collection.

Verified live:

  • daily / continuous (reject id): previously errored, now return a valid result (the synthetic daily_id/continuous_id is dropped for these services by design, as before).
  • monitoring-locations (accepts id, keeps the id column): returns the id column, unchanged from current behavior.

The only cost is that an id/geometry-only request now fetches all columns and subsets client-side (the API can no longer return just id); this is a rare request and avoids the error.

Tests

Adds a switch_properties_id() unit test (no network) covering the id/geometry-only and mixed cases — it asserts the fallback omits properties (NA) rather than producing "id".

🤖 Generated with Claude Code

…try-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 <noreply@anthropic.com>
@thodson-usgs

Copy link
Copy Markdown
Contributor Author

@ldecicco-USGS ,
this bug appeared on the python side. Haven't really dug into, nor the relevance to R.

@ldecicco-USGS

Copy link
Copy Markdown
Collaborator

There's a conversation about it going on with the API developers. They pushed out an update last night and I think it wasn't intentional that "id" now isn't allowed in the properties query - but it does sound like the official pygeoapi platform doesn't include "id" in the property list. So this is probably a permanent change, but not 100% confirmed.

dataRetrieval had already dropped "id" from the property list query because it seemed redundant - you always get an id back whether you ask for it or not. So, it wouldn't be a terrible idea to do this fix (both packages would act the same).

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.

3 participants