nldi: fix several user-visible bugs in get_features, get_flowlines, search

Bundles six discrete fixes to the NLDI module:

* `search(find='flowlines')` without `navigation_mode` crashed with
  `AttributeError: 'NoneType' object has no attribute 'upper'` from inside
  `_validate_navigation_mode`. `_validate_navigation_mode` now treats
  `None` as a clean `ValueError`, returns the upper-cased value (callers
  use the normalized form), and raises `ValueError` rather than
  `TypeError` for invalid modes — these are bad values, not bad types.
  `search` also surfaces a clear error before delegating to
  `get_flowlines`.

* `get_flowlines(comid=...)` silently dropped the `trim_start` argument:
  the `comid` branch built `query_params` without `trimStart`. Move the
  shared params out of the branch.

* `get_features(lat=0.0, ...)` and `search(lat=0.0, ...)` treated the
  equator/prime-meridian as missing because the code used `if lat:` /
  `if comid:` truthiness checks. Switch to `is None` checks throughout
  so coordinates of exactly zero are accepted and `comid=0` is no longer
  conflated with "no comid".

* The error message produced by `get_features` for feature-source +
  data-source paths had an unbalanced quote
  (`"feature_id 'USGS-X, and data source ..."`). Closing quote restored
  via a small `_features_err_msg` helper that the two branches share.

* The unconditional `_validate_data_source(feature_source)` in
  `get_features` is now guarded by `if feature_source:` to avoid
  validating `None` once the cache-bug fix (#246) lands.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: thodson-usgs

dataretrieval/nldi.py

@@ -79,19 +79,14 @@ def get_flowlines(
         ...     comid=13294314, navigation_mode="UM"
         ... )
     """
-    # validate the navigation mode
-    _validate_navigation_mode(navigation_mode)
-    # validate the feature source and comid
+    navigation_mode = _validate_navigation_mode(navigation_mode)
     _validate_feature_source_comid(feature_source, feature_id, comid)
     if feature_source:
-        # validate the feature source
         _validate_data_source(feature_source)
-
         url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}/navigation"
-        query_params = {"distance": str(distance), "trimStart": str(trim_start).lower()}
     else:
         url = f"{NLDI_API_BASE_URL}/comid/{comid}/navigation"
-        query_params = {"distance": str(distance)}
+    query_params = {"distance": str(distance), "trimStart": str(trim_start).lower()}
 
     url += f"/{navigation_mode}/flowlines"
     if stop_comid is not None:
@@ -232,67 +227,52 @@ def get_features(
         >>> gdf = dataretrieval.nldi.get_features(lat=43.073051, long=-89.401230)
     """
 
-    # check only one origin is provided
-    if (lat and long is None) or (long and lat is None):
+    if (lat is None) != (long is None):
         raise ValueError("Both lat and long are required")
 
-    if lat:
-        if comid:
+    have_latlong = lat is not None
+    if have_latlong:
+        if comid is not None:
             raise ValueError(
                 "Provide only one origin type - comid cannot be provided"
                 " with lat or long"
             )
-        if feature_source or feature_id:
+        if feature_source is not None or feature_id is not None:
             raise ValueError(
                 "Provide only one origin type - feature_source and feature_id cannot"
                 " be provided with lat or long"
             )
-
-    if not lat:
-        if (comid or data_source) and navigation_mode is None:
+    else:
+        if (comid is not None or data_source is not None) and navigation_mode is None:
             raise ValueError(
                 "navigation_mode is required if comid or data_source is provided"
             )
-        # validate the feature source and comid
         _validate_feature_source_comid(feature_source, feature_id, comid)
-        # validate the data source
         if data_source:
             _validate_data_source(data_source)
-        # validate feature source
-        _validate_data_source(feature_source)
-        # validate the navigation mode
+        if feature_source:
+            _validate_data_source(feature_source)
         if navigation_mode:
-            _validate_navigation_mode(navigation_mode)
+            navigation_mode = _validate_navigation_mode(navigation_mode)
 
-    if lat:
+    if have_latlong:
         url = f"{NLDI_API_BASE_URL}/comid/position"
         query_params = {"coords": f"POINT({long} {lat})"}
-    else:
-        if navigation_mode:
-            if feature_source:
-                url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}/navigation"
-            else:
-                url = f"{NLDI_API_BASE_URL}/comid/{comid}/navigation"
-            url += f"/{navigation_mode}/{data_source}"
-            query_params = {"distance": str(distance)}
-            if stop_comid is not None:
-                query_params["stopComid"] = str(stop_comid)
-        else:
-            url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}"
-            query_params = {}
-
-    if lat:
         err_msg = f"Error getting features for lat '{lat}' and long '{long}'"
-    elif feature_source:
-        err_msg = (
-            f"Error getting features for feature source '{feature_source}'"
-            f" and feature_id '{feature_id}, and data source '{data_source}'"
-        )
+    elif navigation_mode:
+        if feature_source:
+            url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}/navigation"
+        else:
+            url = f"{NLDI_API_BASE_URL}/comid/{comid}/navigation"
+        url += f"/{navigation_mode}/{data_source}"
+        query_params = {"distance": str(distance)}
+        if stop_comid is not None:
+            query_params["stopComid"] = str(stop_comid)
+        err_msg = _features_err_msg(feature_source, feature_id, comid, data_source)
     else:
-        err_msg = (
-            f"Error getting features for comid '{comid}'"
-            f" and data source '{data_source}'"
-        )
+        url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}"
+        query_params = {}
+        err_msg = _features_err_msg(feature_source, feature_id, comid, data_source)
 
     feature_collection = _query_nldi(url, query_params, err_msg)
     if as_json:
@@ -413,28 +393,26 @@ def search(
         ... )
 
     """
-    if (lat and long is None) or (long and lat is None):
+    if (lat is None) != (long is None):
         raise ValueError("Both lat and long are required")
 
-    # validate find
     find = find.lower()
     if find not in ("basin", "flowlines", "features"):
         raise ValueError(
             f"Invalid value for find: {find} - allowed values are:"
             f" 'basin', 'flowlines', or 'features'"
         )
-    if lat and find != "features":
+    if lat is not None and find != "features":
         raise ValueError(
             f"Invalid value for find: {find} - lat/long is to get features not {find}"
         )
-    if comid and find == "basin":
+    if comid is not None and find == "basin":
         raise ValueError(
             "Invalid value for find: basin - comid is to get features"
             " or flowlines not basin"
         )
 
-    if lat:
-        # get features by hydrologic location
+    if lat is not None:
         return get_features(lat=lat, long=long, as_json=True)
 
     if find == "basin":
@@ -443,6 +421,11 @@ def search(
         )
 
     if find == "flowlines":
+        if navigation_mode is None:
+            raise ValueError(
+                "navigation_mode is required for find='flowlines';"
+                f" allowed values are {_VALID_NAVIGATION_MODES}"
+            )
         return get_flowlines(
             navigation_mode=navigation_mode,
             distance=distance,
@@ -483,10 +466,30 @@ def _validate_data_source(data_source: str):
             raise ValueError(err_msg)
 
 
-def _validate_navigation_mode(navigation_mode: str):
-    navigation_mode = navigation_mode.upper()
-    if navigation_mode not in ("UM", "DM", "UT", "DD"):
-        raise TypeError(f"Invalid navigation mode '{navigation_mode}'")
+_VALID_NAVIGATION_MODES = ("UM", "DM", "UT", "DD")
+
+
+def _features_err_msg(feature_source, feature_id, comid, data_source) -> str:
+    if feature_source is not None:
+        return (
+            f"Error getting features for feature source '{feature_source}'"
+            f" and feature_id '{feature_id}', and data source '{data_source}'"
+        )
+    return f"Error getting features for comid '{comid}' and data source '{data_source}'"
+
+
+def _validate_navigation_mode(navigation_mode: str | None) -> str:
+    if navigation_mode is None:
+        raise ValueError(
+            f"navigation_mode is required; allowed values are {_VALID_NAVIGATION_MODES}"
+        )
+    normalized = navigation_mode.upper()
+    if normalized not in _VALID_NAVIGATION_MODES:
+        raise ValueError(
+            f"Invalid navigation mode '{navigation_mode}';"
+            f" allowed values are {_VALID_NAVIGATION_MODES}"
+        )
+    return normalized
 
 
 def _validate_feature_source_comid(

tests/nldi_test.py

@@ -1,7 +1,9 @@
+import pytest
 from geopandas import GeoDataFrame
 
 from dataretrieval.nldi import (
     NLDI_API_BASE_URL,
+    _validate_navigation_mode,
     get_basin,
     get_features,
     get_flowlines,
@@ -280,3 +282,77 @@ def test_search_for_features_by_lat_long(requests_mock):
     assert search_results["features"][0]["type"] == "Feature"
     assert search_results["features"][0]["geometry"]["type"] == "LineString"
     assert len(search_results["features"][0]["geometry"]["coordinates"]) == 27
+
+
+# --- regression tests for nldi cleanup batch ---
+
+
+def test_search_flowlines_without_navigation_mode_raises_value_error():
+    """Regression: previously crashed with AttributeError on None.upper()."""
+    with pytest.raises(ValueError, match="navigation_mode is required"):
+        search(comid=13294314, find="flowlines")
+
+
+def test_validate_navigation_mode_raises_value_error_for_invalid():
+    """Regression: previously raised TypeError; should be ValueError."""
+    with pytest.raises(ValueError, match="Invalid navigation mode"):
+        _validate_navigation_mode("XX")
+
+
+def test_validate_navigation_mode_normalizes_lowercase():
+    """Regression: lowercase values used to validate but be sent unchanged."""
+    assert _validate_navigation_mode("um") == "UM"
+
+
+def test_get_flowlines_by_comid_includes_trim_start(requests_mock):
+    """Regression: trim_start was previously dropped on the comid code path."""
+    request_url = f"{NLDI_API_BASE_URL}/comid/13294314/navigation/UM/flowlines"
+    response_file_path = "tests/data/nldi_get_flowlines_by_comid.json"
+    mock_request_data_sources(requests_mock)
+    mock_request(requests_mock, request_url, response_file_path)
+
+    get_flowlines(navigation_mode="UM", comid=13294314, trim_start=True)
+
+    sent = requests_mock.request_history[-1]
+    assert sent.qs.get("trimstart") == ["true"]
+
+
+def test_get_features_with_zero_coordinates(requests_mock):
+    """Regression: lat=0.0 / long=0.0 were treated as missing (falsy bug)."""
+    request_url = f"{NLDI_API_BASE_URL}/comid/position"
+    requests_mock.get(
+        request_url,
+        json={"type": "FeatureCollection", "features": []},
+        headers={"mock_header": "value"},
+    )
+
+    gdf = get_features(lat=0.0, long=0.0, as_json=True)
+    assert isinstance(gdf, dict)
+    sent = requests_mock.request_history[-1]
+    assert "POINT(0.0 0.0)" in sent.qs.get("coords", [""])[0].upper()
+
+
+def test_get_features_lat_zero_long_missing_raises():
+    """Regression: lat=0.0 with missing long was silently accepted (falsy bug)."""
+    with pytest.raises(ValueError, match="Both lat and long are required"):
+        get_features(lat=0.0)
+
+
+def test_get_features_error_message_has_balanced_quotes(requests_mock):
+    """Regression: error message had a missing closing quote after feature_id."""
+    request_url = f"{NLDI_API_BASE_URL}/WQP/USGS-bad/navigation/UM/nwissite"
+    # Use a status code utils.query() doesn't intercept so the _query_nldi
+    # error path runs and we see its formatted message.
+    requests_mock.get(request_url, status_code=403, reason="Forbidden", json={})
+    mock_request_data_sources(requests_mock)
+
+    with pytest.raises(ValueError) as exc:
+        get_features(
+            feature_source="WQP",
+            feature_id="USGS-bad",
+            data_source="nwissite",
+            navigation_mode="UM",
+        )
+    msg = str(exc.value)
+    # Closing quote after feature_id must now be present.
+    assert "feature_id 'USGS-bad'," in msg