Validate monitoring_location_id format in waterdata functions (#229)

  Validate and normalize monitoring_location_id (and other multi-value filters) at the boundary

  Closes #188.

  Catches a class of bug that previously produced silent zero-result responses
  or confusing JSONDecodeErrors when callers passed malformed
  `monitoring_location_id` values to the WaterData OGC API getters.

  Behavior changes (every public waterdata function accepting
  `monitoring_location_id`):

  - `monitoring_location_id` is now validated client-side. Non-string,
    non-iterable inputs (`int`, `dict`, …) raise `TypeError`; strings that
    don't match the AGENCY-ID hyphen-separated form (e.g. `"USGS-01646500"`)
    raise `ValueError`; non-string elements inside an iterable raise
    `TypeError`. A clear "Expected 'AGENCY-ID' format, e.g.
    'USGS-01646500'" hint is appended in every error.
  - Other multi-value string filters (`parameter_code`, `statistic_id`,
    `agency_code`, etc.) now accept any non-string iterable of strings —
    `list`, `tuple`, `pandas.Series`, `pandas.Index`, `numpy.ndarray`, and
    generators are all materialized to `list[str]` before the URL is built.
    A bare single string is still accepted unchanged.
  - `properties` additionally accepts a single string and wraps it into a
    one-element list, since `",".join(...)` would otherwise iterate it as
    characters.
  - `list[int]` filters on `get_peaks` (`water_year`, `year`, `month`,
    `day`, `peak_since`) and `list[float]` on `get_combined_metadata`
    (`thresholds`) pass through untouched.
  - `_format_api_dates` now rejects `Mapping` inputs (which previously
    silently materialized as the keys list) and `None` is short-circuited
    up front.

  Public API:
  - Type annotations widened to `str | Iterable[str] | None` (or
    `str | list[str] | None` where only a list is meaningful) across all
    affected functions. Numpydoc parameter descriptions updated from "list
    of strings" to "iterable of strings" to match.
  - Coverage verified on all 15 public functions that accept
    `monitoring_location_id`: 12 in `waterdata/api.py` (via centralized
    `_get_args`), `get_ratings` in `waterdata/ratings.py` (direct call),
    and `get_nearest_continuous` in `waterdata/nearest.py` (transitively
    via `get_continuous`).

  Internals:
  - `_normalize_str_iterable` — one O(N) walk validates element types and
    materializes to `list`. Generic, used by every string-filter param.
  - `_check_monitoring_location_id` — composes `_normalize_str_iterable`
    with per-element `_check_id_format` (regex `[^-\s]+-[^-\s]+`, fullmatch).
  - `_get_args` — single dispatch point that runs the right normalizer per
    param name. New `get_*` functions inherit validation automatically.
  - `_DATE_RANGE_PARAMS` — shared constant covering `time`, `datetime`,
    `last_modified`, `begin`, `begin_utc`, `end`, `end_utc`; used by both
    `_construct_api_requests`'s date formatting and `_get_args`'s bypass.

  Tests:
  - Live-verified against the USGS OGC + STAC APIs with ~70 stress cases
    covering every iterable shape, every public function with mloc, and
    every rejection path.
  - 100+ unit tests including regressions for: int-list parameter rejection,
    Series-of-IDs materialization, Mapping rejection on date inputs,
    AGENCY-ID format edge cases (trailing/leading hyphen, embedded comma,
    whitespace, multi-hyphen).

Author: thodson-usgs

dataretrieval/waterdata/api.py

@@ -5,6 +5,7 @@
 
 from __future__ import annotations
 
+from collections.abc import Iterable
 from typing import Literal, get_args
 
 import pandas as pd
@@ -18,8 +19,8 @@
 
 def get_nearest_continuous(
     targets,
-    monitoring_location_id: str | list[str] | None = None,
-    parameter_code: str | list[str] | None = None,
+    monitoring_location_id: str | Iterable[str] | None = None,
+    parameter_code: str | Iterable[str] | None = None,
     *,
     window: str | pd.Timedelta = "PT7M30S",
     on_tie: OnTie = "first",
@@ -44,9 +45,9 @@ def get_nearest_continuous(
         Target timestamps. Naive datetimes are treated as UTC. Accepts a
         list, ``pandas.Series``, ``pandas.DatetimeIndex``, ``numpy.ndarray``,
         or anything ``pandas.to_datetime`` consumes.
-    monitoring_location_id : string or list of strings, optional
+    monitoring_location_id : string or iterable of strings, optional
         Forwarded to ``get_continuous``.
-    parameter_code : string or list of strings, optional
+    parameter_code : string or iterable of strings, optional
         Forwarded to ``get_continuous``.
     window : string or ``pandas.Timedelta``, default ``"PT7M30S"``
         Half-window around each target, as an ISO 8601 duration

dataretrieval/waterdata/nearest.py

@@ -22,7 +22,13 @@
 
 from dataretrieval.rdb import extract_rdb_comment, read_rdb
 
-from .utils import _DURATION_RE, BASE_URL, _default_headers, _format_api_dates
+from .utils import (
+    _DURATION_RE,
+    BASE_URL,
+    _check_monitoring_location_id,
+    _default_headers,
+    _format_api_dates,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -33,7 +39,7 @@
 
 
 def get_ratings(
-    monitoring_location_id: str | list[str] | None = None,
+    monitoring_location_id: str | Iterable[str] | None = None,
     file_type: RATING_FILE_TYPE | list[RATING_FILE_TYPE] = "exsa",
     file_path: str | None = None,
     time: str | list[str] | None = None,
@@ -62,7 +68,7 @@ def get_ratings(
 
     Parameters
     ----------
-    monitoring_location_id : string or list of strings, optional
+    monitoring_location_id : string or iterable of strings, optional
         One or more identifiers in ``AGENCY-ID`` form (e.g.
         ``"USGS-01104475"``). If omitted, the spatial / temporal filters
         determine the result set.
@@ -142,6 +148,7 @@ def get_ratings(
         ... )
 
     """
+    monitoring_location_id = _check_monitoring_location_id(monitoring_location_id)
     file_types = _as_list(file_type)
     invalid = [ft for ft in file_types if ft not in _VALID_FILE_TYPES]
     if invalid:

dataretrieval/waterdata/ratings.py

@@ -4,6 +4,7 @@
 import logging
 import os
 import re
+from collections.abc import Iterable, Mapping
 from datetime import datetime
 from typing import Any, get_args
 from zoneinfo import ZoneInfo
@@ -143,6 +144,15 @@ def _switch_properties_id(properties: list[str] | None, id_name: str, service: s
 # admits time-only forms like ``PT36H``.
 _DURATION_RE = re.compile(r"^[Pp]T?\d")
 
+# OGC API parameters that carry a date/datetime value (single string,
+# two-element range, or interval/duration string) rather than a multi-value
+# string list. Used by ``_construct_api_requests`` to keep them out of the
+# POST/CQL2 multi-value path and to route them through ``_format_api_dates``,
+# and by ``_NO_NORMALIZE_PARAMS`` to bypass string-iterable normalization.
+_DATE_RANGE_PARAMS = frozenset(
+    {"datetime", "last_modified", "begin", "begin_utc", "end", "end_utc", "time"}
+)
+
 
 def _parse_datetime(value: str) -> datetime | None:
     """Parse a single datetime string against the supported formats.
@@ -223,12 +233,24 @@ def _format_api_dates(
     converted from that offset to UTC; naive inputs are interpreted in the
     local time zone for backwards compatibility.
     """
+    if datetime_input is None:
+        return None
     # Get timezone
     local_timezone = datetime.now().astimezone().tzinfo
 
     # Convert single string to list for uniform processing
     if isinstance(datetime_input, str):
         datetime_input = [datetime_input]
+    elif isinstance(datetime_input, Mapping):
+        # `list(mapping)` returns keys, which silently accepts the wrong shape.
+        raise TypeError(
+            f"date input must be a string or sequence of strings, "
+            f"not {type(datetime_input).__name__}."
+        )
+    elif not isinstance(datetime_input, (list, tuple)):
+        # Materialize any other iterable (pandas.Series, numpy.ndarray,
+        # generator, ...) so the len()/subscript operations below work.
+        datetime_input = list(datetime_input)
 
     # Check for null or all NA and return None
     if all(pd.isna(dt) or dt == "" or dt is None for dt in datetime_input):
@@ -429,14 +451,11 @@ def _construct_api_requests(
     """
     service_url = f"{OGC_API_URL}/collections/{service}/items"
 
-    # Single parameters can only have one value
-    single_params = {"datetime", "last_modified", "begin", "end", "time"}
-
     # Identify which parameters should be included in the POST content body
     post_params = {
         k: v
         for k, v in kwargs.items()
-        if k not in single_params and isinstance(v, (list, tuple)) and len(v) > 1
+        if k not in _DATE_RANGE_PARAMS and isinstance(v, (list, tuple)) and len(v) > 1
     }
 
     # Everything else goes into the params dictionary for the URL
@@ -452,15 +471,13 @@ def _construct_api_requests(
     POST = bool(post_params)
 
     # Convert dates to ISO08601 format
-    time_periods = {"last_modified", "datetime", "time", "begin", "end"}
-    for i in time_periods:
+    for i in _DATE_RANGE_PARAMS:
         if i in params:
             dates = service == "daily" and i != "last_modified"
             params[i] = _format_api_dates(params[i], date=dates)
 
-    # String together bbox elements from a list to a comma-separated string,
-    # and string together properties if provided
-    if bbox:
+    # `len()` instead of truthiness: a numpy ndarray would raise on `if bbox:`.
+    if bbox is not None and len(bbox) > 0:
         params["bbox"] = ",".join(map(str, bbox))
     if properties:
         params["properties"] = ",".join(properties)
@@ -1168,6 +1185,129 @@ def _check_profiles(
         )
 
 
+_MONITORING_LOCATION_ID_RE = re.compile(r"[^-\s]+-[^-\s]+")
+
+
+# Iterable-shaped params that ``_get_args`` must NOT push through
+# ``_normalize_str_iterable`` (scalar non-string knobs are caught by runtime
+# type, so only iterables with special handling need to be named here):
+#   - date-range params may contain ``pd.NaT``/None or interval strings
+#   - ``bbox``/``boundingBox`` are ``list[float]``, sometimes ``numpy.ndarray``
+#   - ``get_peaks``'s int-valued filters (``water_year`` etc.) are ``list[int]``
+#   - ``get_combined_metadata``'s ``thresholds`` is ``list[float]``
+_NO_NORMALIZE_PARAMS = _DATE_RANGE_PARAMS | {
+    "bbox",
+    "boundingBox",
+    "water_year",
+    "year",
+    "month",
+    "day",
+    "peak_since",
+    "thresholds",
+}
+
+
+def _normalize_str_iterable(
+    value: str | Iterable[str] | None,
+    param_name: str = "value",
+) -> str | list[str] | None:
+    """Validate that ``value`` is None, a string, or an iterable of strings.
+
+    Non-string iterables (``list``, ``tuple``, ``pandas.Series``,
+    ``pandas.Index``, ``numpy.ndarray``, generators) are materialized to a
+    ``list`` so downstream code that branches on ``isinstance(v, (list,
+    tuple))`` keeps working. ``Mapping`` types are rejected because
+    iterating a mapping yields keys, not values.
+
+    Parameters
+    ----------
+    value : None, str, or iterable of str
+    param_name : str, optional
+        Used in error messages. Defaults to ``"value"``.
+
+    Returns
+    -------
+    None, str, or list of str
+
+    Raises
+    ------
+    TypeError
+        If the input isn't ``None``, ``str``, or a non-``Mapping``
+        iterable; or if any iterable element isn't a string.
+    """
+    if value is None:
+        return None
+    if isinstance(value, str):
+        return value
+    if isinstance(value, Mapping) or not isinstance(value, Iterable):
+        raise TypeError(
+            f"{param_name} must be a string or iterable of strings, "
+            f"not {type(value).__name__} (got {value!r})."
+        )
+    values: list[str] = []
+    for v in value:
+        if not isinstance(v, str):
+            raise TypeError(
+                f"{param_name} elements must be strings, "
+                f"not {type(v).__name__} (got {v!r})."
+            )
+        values.append(v)
+    return values
+
+
+def _check_monitoring_location_id(
+    monitoring_location_id: str | Iterable[str] | None,
+) -> str | list[str] | None:
+    """Validate and normalize a ``monitoring_location_id`` value.
+
+    Combines :func:`_normalize_str_iterable` with the AGENCY-ID format
+    check that is unique to ``monitoring_location_id`` (the OGC spec
+    requires a hyphen separator, e.g. ``USGS-01646500``).
+
+    Parameters
+    ----------
+    monitoring_location_id : None, str, or iterable of str
+        See :func:`_normalize_str_iterable`. Each string is additionally
+        required to match the AGENCY-ID hyphen-separated format.
+
+    Returns
+    -------
+    None, str, or list of str
+
+    Raises
+    ------
+    TypeError
+        If the input isn't ``None``, ``str``, or a non-``Mapping``
+        iterable; or if any iterable element isn't a string.
+    ValueError
+        If any identifier doesn't contain a hyphen separator
+        (per the OGC API spec: AGENCY-ID format, e.g. ``USGS-01646500``).
+    """
+    try:
+        value = _normalize_str_iterable(
+            monitoring_location_id, "monitoring_location_id"
+        )
+    except TypeError as exc:
+        # Re-raise with the AGENCY-ID hint the generic helper doesn't carry.
+        raise TypeError(
+            f"{exc} Expected 'AGENCY-ID' format, e.g., 'USGS-01646500'."
+        ) from None
+    if value is None:
+        return None
+    for item in (value,) if isinstance(value, str) else value:
+        _check_id_format(item)
+    return value
+
+
+def _check_id_format(value: str) -> None:
+    """Raise ``ValueError`` if ``value`` is not in ``AGENCY-ID`` format."""
+    if not _MONITORING_LOCATION_ID_RE.fullmatch(value):
+        raise ValueError(
+            f"Invalid monitoring_location_id: {value!r}. "
+            f"Expected 'AGENCY-ID' format, e.g., 'USGS-01646500'."
+        )
+
+
 def _get_args(
     local_vars: dict[str, Any], exclude: set[str] | None = None
 ) -> dict[str, Any]:
@@ -1194,6 +1334,21 @@ def _get_args(
     if exclude:
         to_exclude.update(exclude)
 
-    return {
-        k: v for k, v in local_vars.items() if k not in to_exclude and v is not None
-    }
+    args: dict[str, Any] = {}
+    for k, v in local_vars.items():
+        if k in to_exclude or v is None:
+            continue
+        if k == "monitoring_location_id":
+            args[k] = _check_monitoring_location_id(v)
+        elif k == "properties":
+            # `",".join(properties)` would iterate a bare string as characters.
+            args[k] = [v] if isinstance(v, str) else _normalize_str_iterable(v, k)
+        elif (
+            k in _NO_NORMALIZE_PARAMS
+            or isinstance(v, str)
+            or not isinstance(v, Iterable)
+        ):
+            args[k] = v
+        else:
+            args[k] = _normalize_str_iterable(v, k)
+    return args