diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index 64763938..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): @@ -79,19 +80,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,43 +228,35 @@ 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: + if lat is not None: + 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: + 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( "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 navigation_mode: - _validate_navigation_mode(navigation_mode) - - if lat: - url = f"{NLDI_API_BASE_URL}/comid/position" - query_params = {"coords": f"POINT({long} {lat})"} - else: + if feature_source: + _validate_data_source(feature_source) if navigation_mode: + navigation_mode = _validate_navigation_mode(navigation_mode) if feature_source: url = f"{NLDI_API_BASE_URL}/{feature_source}/{feature_id}/navigation" else: @@ -280,19 +268,7 @@ def get_features( 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}'" - ) - else: - err_msg = ( - f"Error getting features for comid '{comid}'" - f" and data source '{data_source}'" - ) + 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 +389,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 +417,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 +462,27 @@ 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 ValueError(f"Invalid navigation mode '{navigation_mode}'") +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..ea6b0f65 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,23 @@ 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"