From 1dffcc0d3735460ac6b23442eccbcd8219c58be2 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Sun, 3 May 2026 14:10:00 -0500 Subject: [PATCH 1/3] nldi: fix several user-visible bugs in get_features, get_flowlines, search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- dataretrieval/nldi.py | 115 ++++++++++++++++++++++-------------------- tests/nldi_test.py | 76 ++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 56 deletions(-) diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 8cb8f0aa..39bce8fe 100644 --- a/dataretrieval/nldi.py +++ b/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( diff --git a/tests/nldi_test.py b/tests/nldi_test.py index 462a5755..9a3007c7 100644 --- a/tests/nldi_test.py +++ b/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 From f8d0a3f0b4d01a4b6f5a63755c9b745a4bf78de1 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 4 May 2026 12:58:51 -0500 Subject: [PATCH 2/3] Tidy nldi cleanup per /simplify review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per /simplify review on PR #252: - Hoist `_VALID_NAVIGATION_MODES` to the module-level constants block alongside `NLDI_API_BASE_URL`/`_CRS` rather than wedging it between two `_validate_*` helpers. - Drop the redundant `have_latlong` local in `get_features`; inline `lat is not None` directly. Hoist the duplicated `err_msg = _features_err_msg(...)` to a single call after the comid/feature-source if/else branch. - Drop the unused `mock_request_data_sources` call in `test_get_flowlines_by_comid_includes_trim_start` — the comid path doesn't hit `_validate_data_source`. Co-Authored-By: Claude Opus 4.7 (1M context) --- dataretrieval/nldi.py | 37 +++++++++++++++---------------------- tests/nldi_test.py | 1 - 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 39bce8fe..f5fd65ea 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -13,6 +13,7 @@ NLDI_API_BASE_URL = "https://api.water.usgs.gov/nldi/linked-data" _AVAILABLE_DATA_SOURCES = None _CRS = "EPSG:4326" +_VALID_NAVIGATION_MODES = ("UM", "DM", "UT", "DD") def _query_nldi(url, query_params, error_message): @@ -230,8 +231,7 @@ def get_features( if (lat is None) != (long is None): raise ValueError("Both lat and long are required") - have_latlong = lat is not None - if have_latlong: + if lat is not None: if comid is not None: raise ValueError( "Provide only one origin type - comid cannot be provided" @@ -242,6 +242,9 @@ def get_features( "Provide only one origin type - feature_source and feature_id cannot" " be provided with lat or long" ) + url = f"{NLDI_API_BASE_URL}/comid/position" + query_params = {"coords": f"POINT({long} {lat})"} + err_msg = f"Error getting features for lat '{lat}' and long '{long}'" else: if (comid is not None or data_source is not None) and navigation_mode is None: raise ValueError( @@ -254,24 +257,17 @@ def get_features( _validate_data_source(feature_source) if navigation_mode: navigation_mode = _validate_navigation_mode(navigation_mode) - - if have_latlong: - url = f"{NLDI_API_BASE_URL}/comid/position" - query_params = {"coords": f"POINT({long} {lat})"} - err_msg = f"Error getting features for lat '{lat}' and long '{long}'" - elif navigation_mode: - if feature_source: - url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}/navigation" + 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}/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: - url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}" - query_params = {} + 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) @@ -466,9 +462,6 @@ def _validate_data_source(data_source: str): raise ValueError(err_msg) -_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 ( diff --git a/tests/nldi_test.py b/tests/nldi_test.py index 9a3007c7..a115ed68 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -308,7 +308,6 @@ 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) From 0f26510012b012e0b0df1392cd5ecb000937ea1a Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 4 May 2026 13:03:23 -0500 Subject: [PATCH 3/3] Drop trivial regression tests from nldi cleanup The trim_start drop, falsy-zero (lat/long/comid), and unbalanced-quote fixes are each one-line changes whose regressions are obvious on read. The remaining three tests (search-without-nav-mode crash, ValueError vs TypeError, and lowercase normalization) cover behavior that's less self-evident from the code. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/nldi_test.py | 53 ---------------------------------------------- 1 file changed, 53 deletions(-) diff --git a/tests/nldi_test.py b/tests/nldi_test.py index a115ed68..ea6b0f65 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -302,56 +302,3 @@ def test_validate_navigation_mode_raises_value_error_for_invalid(): 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(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