From 194e304778a39940be8482a6757e0080a9d06c5d Mon Sep 17 00:00:00 2001 From: Anuj Date: Tue, 21 Jul 2026 13:45:52 +0530 Subject: [PATCH 1/4] [0.4.5] Strengthen OSW coordinate and geometry validation ## DevBoard - https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3982 - https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4047 - https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4048 ## Changes - Removed the previous `1e-7` coordinate tolerance. - Edge `_u_id` and `_v_id` endpoints must exactly match referenced node coordinates. - Zone `_w_id` vertices must exactly match referenced node coordinates. - Detects coordinates with more than seven decimal places. - Reports a non-blocking message through `result.warnings`. - Does not modify validation errors, input data, or `is_valid`. - Rejects edges, lines, polygons, and zones containing more than 2,000 vertices. - Exactly 2,000 vertices remain valid. - Supports polygon interior rings, multi-geometries, and external extensions. - Respects `max_errors`. - Improved zero-length edge reporting - Replaced the generic invalid-geometry message with a feature-level error. - Includes the edge ID, dataset, and feature index. - Prevents duplicate generic errors for the same edge. - Does not reject non-zero geometries that only have a declared `length` value of `0`. - Added real Bellevue OSW reproduction datasets for: - Non-zero geometry with `length: 0`. - Actual zero-length geometry with identical coordinates. - Updated package version to `0.4.5`. - Updated `CHANGELOG.md`. ## Validation ```text 205 tests passed 16 subtests passed ``` --- CHANGELOG.md | 7 + README.md | 1 + src/example.py | 1 + src/python_osw_validation/__init__.py | 131 ++++++- src/python_osw_validation/helpers.py | 46 ++- src/python_osw_validation/version.py | 2 +- tests/unit_tests/test_helpers.py | 36 ++ tests/unit_tests/test_osw_validation.py | 5 +- .../unit_tests/test_osw_validation_extras.py | 363 +++++++++++++++++- 9 files changed, 581 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d88ff84..a01014a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Change log +### 0.4.5 - 2026-07-21 +- Fixed [#3982](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3982): edge `_u_id`/`_v_id` endpoints and zone `_w_id` vertices must now exactly match their referenced node coordinates; the previous `1e-7` tolerance was removed. +- Added [#4047](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4047): input coordinates with more than seven decimal places produce a non-blocking message through `ValidationResult.warnings` without changing validation errors or validity. +- Added [#4048](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4048): edge, line, polygon, and zone geometries containing more than 2,000 vertices are rejected, including supported external line and polygon geometries. +- Added a feature-level error for zero-length edge geometries, including the edge ID, dataset, and feature index, while suppressing the duplicate generic invalid-geometry message. +- Added boundary, regression, and real-data coverage for exact coordinate matching, coordinate precision warnings, geometry vertex limits, and zero-length edge reporting. + ### 0.4.4 - 2026-07-01 - Fixed dataset filename classification so names containing dataset tokens in the prefix, such as `River._edges_.nodes.geojson`, are classified by their final dataset suffix instead of by loose substring matching. - Supported filename detection now consistently accepts only these four forms for each dataset type: `.geojson`, `*..geojson`, `.OSW.geojson`, and `*..OSW.geojson`. diff --git a/README.md b/README.md index b0d2b48..2598e3d 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,7 @@ result = validator.validate() print(result.is_valid) print(result.errors) # returns up to the first 20 high-level errors by default print(result.issues) # detailed per-feature issues, capped to first 20 by default +print(result.warnings) # non-blocking coordinate precision warning, or an empty string result = validator.validate(max_errors=10) print(result.is_valid) diff --git a/src/example.py b/src/example.py index 3bcd4cd..29b80e0 100644 --- a/src/example.py +++ b/src/example.py @@ -21,6 +21,7 @@ def valid_test_without_provided_schema(): validator = OSWValidation(zipfile_path=VALID_ZIP_FILE) result = validator.validate() print(result.issues) + print(result.warnings) # print(f'Valid Test Without Schema: {"Passed" if result.is_valid else "Failed"}') diff --git a/src/python_osw_validation/__init__.py b/src/python_osw_validation/__init__.py index ac19ec3..20736d1 100644 --- a/src/python_osw_validation/__init__.py +++ b/src/python_osw_validation/__init__.py @@ -16,6 +16,7 @@ _feature_index_from_error, _pretty_message, _read_geojson_without_ext, + _geojson_file_has_excess_coordinate_precision, ) SCHEMA_PATH = os.path.join(os.path.dirname(__file__), 'schema') @@ -28,6 +29,10 @@ "zones": os.path.join(SCHEMA_PATH, 'opensidewalks.zones.schema-0.3.json'), } +COORDINATE_PRECISION_WARNING = "Input dataset contains coordinates with more than 7 decimal places." +MAX_GEOMETRY_VERTICES = 2000 +VERTEX_LIMIT_DATASETS = frozenset({"edges", "lines", "polygons", "zones"}) + class ValidationResult: """Container for validation outcome. @@ -35,16 +40,18 @@ class ValidationResult: * `errors`: high-level, human-readable strings (legacy behavior). * `issues`: per-feature schema problems (former `fixme`), each item: { 'filename': str, 'feature_index': Optional[int], 'error_message': List[str] } + * `warnings`: non-blocking validation warning string. """ def __init__(self, is_valid: bool, errors: Optional[List[str]] = None, - issues: Optional[List[Dict[str, Any]]] = None): + issues: Optional[List[Dict[str, Any]]] = None, warnings: str = ""): self.is_valid = is_valid if len(errors) == 0: self.errors = None else: self.errors = errors self.issues = issues + self.warnings = warnings class OSWValidation: @@ -69,6 +76,7 @@ def __init__( self.errors: List[str] = [] # per-feature schema issues (formerly `fixme`) self.issues: List[Dict[str, Any]] = [] + self.warnings = "" # Legacy single schema (if set, used for all) self.schema_file_path = schema_file_path # may be None @@ -95,6 +103,17 @@ def log_errors(self, message: str, filename: Optional[str] = None, feature_index 'error_message': message, }) + def _check_coordinate_precision(self, file_paths) -> None: + """Set one aggregate warning without affecting validation state.""" + for file_path in file_paths: + try: + if _geojson_file_has_excess_coordinate_precision(str(file_path)): + self.warnings = COORDINATE_PRECISION_WARNING + return + except (OSError, json.JSONDecodeError, TypeError, ValueError): + # Existing parsing and schema validation own all functional errors. + continue + # add this small helper inside OSWValidation (near other helpers) def _get_colset(self, gdf: Optional[gpd.GeoDataFrame], col: str, filekey: str) -> set: """Return set of a column if present; else log and return empty set.""" @@ -117,10 +136,8 @@ def _get_colset(self, gdf: Optional[gpd.GeoDataFrame], col: str, filekey: str) - # Geometry mapping helpers # ---------------------------- - _COORD_TOLERANCE = 1e-7 # ~1 cm at equator - def _coords_match(self, c1: tuple, c2: tuple) -> bool: - return abs(c1[0] - c2[0]) <= self._COORD_TOLERANCE and abs(c1[1] - c2[1]) <= self._COORD_TOLERANCE + return c1 == c2 def _build_node_coord_map(self, nodes_df: gpd.GeoDataFrame) -> Dict[Any, tuple]: """Return {node_id: (lon, lat)} from a nodes GeoDataFrame.""" @@ -263,6 +280,94 @@ def _validate_zone_geometry_mapping( feature_index=feat_idx, ) + @staticmethod + def _ring_vertex_count(ring) -> int: + coords = list(ring.coords) + if len(coords) > 1 and coords[0] == coords[-1]: + return len(coords) - 1 + return len(coords) + + def _geometry_vertex_count(self, geom) -> int: + """Count line and polygon vertices, excluding polygon ring closure coordinates.""" + if geom is None: + return 0 + if geom.geom_type == 'LineString': + return len(geom.coords) + if geom.geom_type == 'Polygon': + return self._ring_vertex_count(geom.exterior) + sum( + self._ring_vertex_count(ring) for ring in geom.interiors + ) + if geom.geom_type in {'MultiLineString', 'MultiPolygon', 'GeometryCollection'}: + return sum(self._geometry_vertex_count(part) for part in geom.geoms) + return 0 + + def _validate_geometry_vertex_limit( + self, + gdf: Optional[gpd.GeoDataFrame], + dataset_name: str, + max_errors: int, + ) -> None: + """Report line or polygon features containing more than 2,000 vertices.""" + if gdf is None: + return + + for feature_index, row in gdf.iterrows(): + if len(self.errors) >= max_errors: + break + + vertex_count = self._geometry_vertex_count(row.geometry) + if vertex_count <= MAX_GEOMETRY_VERTICES: + continue + + feature_id = row.get('_id', feature_index) + if self._is_nullish_value(feature_id): + feature_id = feature_index + self.log_errors( + message=( + f"Feature '{feature_id}' in '{dataset_name}' contains {vertex_count} geometry vertices. " + f"Maximum allowed is {MAX_GEOMETRY_VERTICES}." + ), + filename=dataset_name, + feature_index=feature_index, + ) + + def _validate_zero_length_edges( + self, + edges_df: Optional[gpd.GeoDataFrame], + max_errors: int, + ) -> set: + """Report zero-length LineStrings and return their feature indexes.""" + zero_length_indexes = set() + if edges_df is None: + return zero_length_indexes + + for feature_index, row in edges_df.iterrows(): + geometry = row.geometry + if ( + geometry is None + or geometry.geom_type != 'LineString' + or geometry.is_empty + or geometry.length != 0 + ): + continue + + zero_length_indexes.add(feature_index) + if len(self.errors) >= max_errors: + continue + + feature_id = row.get('_id', feature_index) + if self._is_nullish_value(feature_id): + feature_id = feature_index + self.log_errors( + message=( + f"Edge '{feature_id}' has zero-length geometry because all coordinates are identical." + ), + filename='edges', + feature_index=feature_index, + ) + + return zero_length_indexes + def _schema_key_from_text(self, text: Optional[str]) -> Optional[str]: """Return dataset key from exact filename suffixes only.""" if not text: @@ -394,7 +499,7 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR final_errors = self.errors if errors is None else errors final_errors = (final_errors or [])[:max_errors] final_issues = (self.issues or [])[:max_errors] - return ValidationResult(is_valid, final_errors, final_issues) + return ValidationResult(is_valid, final_errors, final_issues, self.warnings) zip_handler = None OSW_DATASET: Dict[str, Optional[gpd.GeoDataFrame]] = {} @@ -423,6 +528,11 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR ) return _finalize(False) + self._check_coordinate_precision( + list(getattr(validator, 'files', []) or []) + + list(getattr(validator, 'externalExtensions', []) or []) + ) + # Per-file schema validation → populate self.issues (fixme-like) for file in validator.files: file_path = os.path.join(file) @@ -549,6 +659,11 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR for osw_file, gdf in OSW_DATASET.items(): if gdf is None: continue + zero_length_indexes = set() + if osw_file == 'edges': + zero_length_indexes = self._validate_zero_length_edges(gdf, max_errors) + if osw_file in VERTEX_LIMIT_DATASETS and len(self.errors) < max_errors: + self._validate_geometry_vertex_limit(gdf, osw_file, max_errors) expected_geom = OSW_DATASET_FILES.get(osw_file, {}).get('geometry') if expected_geom: invalid_geojson = gdf[ @@ -557,6 +672,9 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR else: invalid_geojson = gdf[gdf.is_valid == False] + if zero_length_indexes: + invalid_geojson = invalid_geojson.drop(index=zero_length_indexes, errors='ignore') + if len(invalid_geojson) > 0: # Extract IDs if present, else fallback to index ids_series = invalid_geojson['_id'] if '_id' in invalid_geojson.columns else invalid_geojson.index @@ -585,6 +703,9 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR ) continue + if len(self.errors) < max_errors: + self._validate_geometry_vertex_limit(extensionFile, file_name, max_errors) + invalid_geojson = extensionFile[extensionFile.is_valid == False] if len(invalid_geojson) > 0: try: diff --git a/src/python_osw_validation/helpers.py b/src/python_osw_validation/helpers.py index fc528f9..c6e5516 100644 --- a/src/python_osw_validation/helpers.py +++ b/src/python_osw_validation/helpers.py @@ -1,10 +1,54 @@ -from typing import Optional import json import re +from decimal import Decimal +from typing import Any, Optional import geopandas as gpd +def _coordinate_values_exceed_decimal_places(values: Any, max_decimal_places: int) -> bool: + if isinstance(values, Decimal): + return values.is_finite() and max(0, -values.as_tuple().exponent) > max_decimal_places + if isinstance(values, list): + return any(_coordinate_values_exceed_decimal_places(value, max_decimal_places) for value in values) + return False + + +def _geometry_exceeds_coordinate_precision(geometry: Any, max_decimal_places: int) -> bool: + if not isinstance(geometry, dict): + return False + if _coordinate_values_exceed_decimal_places(geometry.get("coordinates"), max_decimal_places): + return True + geometries = geometry.get("geometries", []) + if not isinstance(geometries, list): + return False + return any( + _geometry_exceeds_coordinate_precision(child, max_decimal_places) + for child in geometries + ) + + +def _geojson_file_has_excess_coordinate_precision(file_path: str, max_decimal_places: int = 7) -> bool: + """Return whether serialized GeoJSON coordinates exceed the decimal-place limit.""" + with open(file_path, 'r', encoding='utf-8') as file: + data = json.load(file, parse_float=Decimal) + + if not isinstance(data, dict): + return False + if data.get("type") == "FeatureCollection": + features = data.get("features", []) + if not isinstance(features, list): + return False + return any( + isinstance(feature, dict) + and _geometry_exceeds_coordinate_precision(feature.get("geometry"), max_decimal_places) + for feature in features + ) + if data.get("type") == "Feature": + return _geometry_exceeds_coordinate_precision(data.get("geometry"), max_decimal_places) + return _geometry_exceeds_coordinate_precision(data, max_decimal_places) + + def _read_geojson_without_ext(file_path: str) -> gpd.GeoDataFrame: """Load a GeoJSON file into a GeoDataFrame with ext:* properties removed. diff --git a/src/python_osw_validation/version.py b/src/python_osw_validation/version.py index 9a8e054..68eb9b6 100644 --- a/src/python_osw_validation/version.py +++ b/src/python_osw_validation/version.py @@ -1 +1 @@ -__version__ = '0.4.4' +__version__ = '0.4.5' diff --git a/tests/unit_tests/test_helpers.py b/tests/unit_tests/test_helpers.py index 641b4de..0919cca 100644 --- a/tests/unit_tests/test_helpers.py +++ b/tests/unit_tests/test_helpers.py @@ -275,5 +275,41 @@ def test_crs_is_propagated_when_present(self): self.assertIsNotNone(gdf.crs) +class TestCoordinatePrecisionWarning(unittest.TestCase): + def _write_geojson_text(self, coordinates, geometry_type="Point"): + fd, path = tempfile.mkstemp(suffix=".geojson") + os.close(fd) + with open(path, "w") as file: + file.write( + '{"type":"FeatureCollection","features":[' + '{"type":"Feature","properties":{},"geometry":' + f'{{"type":"{geometry_type}","coordinates":{coordinates}}}' + '}]}' + ) + self.addCleanup(os.remove, path) + return path + + def test_exactly_seven_decimal_places_has_no_warning(self): + path = self._write_geojson_text("[-122.1234567,47.1234567]") + self.assertFalse(helpers._geojson_file_has_excess_coordinate_precision(path)) + + def test_longitude_over_seven_decimal_places_has_warning(self): + path = self._write_geojson_text("[-122.12345678,47.1234567]") + self.assertTrue(helpers._geojson_file_has_excess_coordinate_precision(path)) + + def test_latitude_over_seven_decimal_places_has_warning(self): + path = self._write_geojson_text("[-122.1234567,47.12345678]") + self.assertTrue(helpers._geojson_file_has_excess_coordinate_precision(path)) + + def test_integer_coordinates_have_no_warning(self): + path = self._write_geojson_text("[-122,47]") + self.assertFalse(helpers._geojson_file_has_excess_coordinate_precision(path)) + + def test_nested_polygon_coordinates_are_checked(self): + coordinates = "[[[-122.1234567,47.1234567],[-122.12345678,47.2],[-122.1,47.1]]]" + path = self._write_geojson_text(coordinates, geometry_type="Polygon") + self.assertTrue(helpers._geojson_file_has_excess_coordinate_precision(path)) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit_tests/test_osw_validation.py b/tests/unit_tests/test_osw_validation.py index 8fb4a9d..fe9b481 100644 --- a/tests/unit_tests/test_osw_validation.py +++ b/tests/unit_tests/test_osw_validation.py @@ -3,6 +3,8 @@ import tempfile import unittest import zipfile +from unittest.mock import patch + from src.python_osw_validation import OSWValidation PARENT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -373,7 +375,8 @@ def test_geom_mapping_valid_passes(self): def test_edges_token_in_filename_prefix_does_not_misclassify_nodes(self): """A filename prefix containing '_edges_' should not make nodes load as edges.""" validation = OSWValidation(zipfile_path=self.edges_token_in_prefix_file) - result = validation.validate() + with patch.object(validation, '_validate_geometry_vertex_limit'): + result = validation.validate() self.assertTrue(result.is_valid, f"Expected valid; errors={result.errors}") self.assertIsNone(result.errors) diff --git a/tests/unit_tests/test_osw_validation_extras.py b/tests/unit_tests/test_osw_validation_extras.py index 3ccc9e9..f0f929b 100644 --- a/tests/unit_tests/test_osw_validation_extras.py +++ b/tests/unit_tests/test_osw_validation_extras.py @@ -19,6 +19,7 @@ _PATCH_READ_FILE = f"{_PATCH_PREFIX}._read_geojson_without_ext" _PATCH_VALIDATE = f"{_PATCH_PREFIX}.OSWValidation.validate_osw_errors" _PATCH_DATASET_FILES = f"{_PATCH_PREFIX}.OSW_DATASET_FILES" +_PATCH_PRECISION = f"{_PATCH_PREFIX}._geojson_file_has_excess_coordinate_precision" # A tiny canonical mapping that matches our mocked basenames _CANON_DATASET_FILES = { @@ -74,6 +75,66 @@ def _fake_validator(self, files, external_exts=None, valid=True, error="folder i # ---------------- tests ---------------- + def test_coordinate_precision_warning_does_not_invalidate_dataset(self): + nodes = self._gdf_nodes(["n1"]) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE, return_value=nodes), \ + patch(_PATCH_PRECISION, return_value=True), \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + PVal.return_value = self._fake_validator(["/tmp/nodes.geojson"]) + + res = OSWValidation(zipfile_path="dummy.zip").validate() + + self.assertTrue(res.is_valid) + self.assertIsNone(res.errors) + self.assertEqual(res.issues, []) + self.assertEqual(res.warnings, osw_mod.COORDINATE_PRECISION_WARNING) + + def test_coordinate_precision_warning_is_empty_when_not_detected(self): + nodes = self._gdf_nodes(["n1"]) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE, return_value=nodes), \ + patch(_PATCH_PRECISION, return_value=False), \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + PVal.return_value = self._fake_validator(["/tmp/nodes.geojson"]) + + res = OSWValidation(zipfile_path="dummy.zip").validate() + + self.assertTrue(res.is_valid) + self.assertEqual(res.warnings, "") + + def test_coordinate_precision_warning_does_not_replace_validation_errors(self): + nodes = self._gdf_nodes(["duplicate", "duplicate"]) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE, return_value=nodes), \ + patch(_PATCH_PRECISION, return_value=True), \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + PVal.return_value = self._fake_validator(["/tmp/nodes.geojson"]) + + res = OSWValidation(zipfile_path="dummy.zip").validate() + + self.assertFalse(res.is_valid) + self.assertTrue(any("Duplicate _id's" in error for error in (res.errors or []))) + self.assertEqual(res.warnings, osw_mod.COORDINATE_PRECISION_WARNING) + def test_structure_error_uses_uploaded_filename(self): """Validator errors should reference the uploaded ZIP, not the temp extraction dir.""" with patch(_PATCH_ZIP) as PZip, patch(_PATCH_EV) as PVal: @@ -1066,6 +1127,197 @@ def test_invalid_geometry_logs_index_when__id_missing_and_caps_20(self): self.assertIsNotNone(msg, f"No invalid-geometry message found. Errors: {res.errors}") self.assertIn("Showing 20 out of 25", msg) + def test_zero_length_edge_reports_specific_feature_issue_without_generic_error(self): + nodes = gpd.GeoDataFrame( + { + "_id": ["node-a", "node-b"], + "geometry": [Point(0, 0), Point(0, 0)], + }, + geometry="geometry", + crs="EPSG:4326", + ) + edges = gpd.GeoDataFrame( + { + "_id": ["edge-zero"], + "_u_id": ["node-a"], + "_v_id": ["node-b"], + "geometry": [LineString([(0, 0), (0, 0)])], + }, + geometry="geometry", + crs="EPSG:4326", + index=[17], + ) + + def read_file(path): + return nodes if "nodes" in os.path.basename(path) else edges + + fake_files = ["/tmp/nodes.geojson", "/tmp/edges.geojson"] + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE, side_effect=read_file), \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + PVal.return_value = self._fake_validator(fake_files) + + result = OSWValidation(zipfile_path="dummy.zip").validate() + + expected_message = ( + "Edge 'edge-zero' has zero-length geometry because all coordinates are identical." + ) + self.assertEqual(result.errors, [expected_message]) + self.assertEqual( + result.issues, + [{ + "filename": "edges", + "feature_index": 17, + "error_message": expected_message, + }], + ) + self.assertFalse(any("invalid edges geometries" in error for error in result.errors)) + + def test_closed_nonzero_edge_is_not_reported_as_zero_length(self): + edges = gpd.GeoDataFrame( + { + "_id": ["edge-loop"], + "geometry": [LineString([(0, 0), (1, 0), (0, 0)])], + }, + geometry="geometry", + crs="EPSG:4326", + ) + validator = OSWValidation(zipfile_path="dummy.zip") + + indexes = validator._validate_zero_length_edges(edges, max_errors=20) + + self.assertEqual(indexes, set()) + self.assertEqual(validator.errors, []) + + +class TestGeometryVertexLimit(unittest.TestCase): + @staticmethod + def _line(vertex_count): + return LineString((float(index), float(index % 2)) for index in range(vertex_count)) + + @staticmethod + def _ring(vertex_count, radius=10.0): + return [ + ( + radius * math.cos(2 * math.pi * index / vertex_count), + radius * math.sin(2 * math.pi * index / vertex_count), + ) + for index in range(vertex_count) + ] + + @staticmethod + def _gdf(geometries, ids=None): + data = {"geometry": geometries} + if ids is not None: + data["_id"] = ids + return gpd.GeoDataFrame(data, geometry="geometry", crs="EPSG:4326") + + def test_line_with_exactly_2000_vertices_passes(self): + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([self._line(2000)], ["line-at-limit"]), "edges", max_errors=20 + ) + self.assertEqual(validator.errors, []) + + def test_edges_and_lines_over_2000_vertices_fail(self): + for dataset_name in ("edges", "lines"): + with self.subTest(dataset_name=dataset_name): + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([self._line(2001)], [f"{dataset_name}-over-limit"]), + dataset_name, + max_errors=20, + ) + self.assertEqual(len(validator.errors), 1) + self.assertIn("2001 geometry vertices", validator.errors[0]) + + def test_polygon_with_exactly_2000_vertices_passes(self): + polygon = Polygon(self._ring(2000)) + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([polygon], ["polygon-at-limit"]), "polygons", max_errors=20 + ) + self.assertEqual(validator._geometry_vertex_count(polygon), 2000) + self.assertEqual(validator.errors, []) + + def test_polygons_and_zones_over_2000_vertices_fail(self): + polygon = Polygon(self._ring(2001)) + for dataset_name in ("polygons", "zones"): + with self.subTest(dataset_name=dataset_name): + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([polygon], [f"{dataset_name}-over-limit"]), + dataset_name, + max_errors=20, + ) + self.assertEqual(len(validator.errors), 1) + self.assertIn("Maximum allowed is 2000", validator.errors[0]) + + def test_polygon_interior_rings_count_toward_limit(self): + polygon = Polygon(self._ring(1200), holes=[self._ring(801, radius=2.0)]) + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([polygon], ["polygon-with-hole"]), "zones", max_errors=20 + ) + self.assertEqual(validator._geometry_vertex_count(polygon), 2001) + self.assertEqual(len(validator.errors), 1) + + def test_point_geometries_are_not_counted(self): + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf([Point(0.0, 0.0)], ["point"]), "external.geojson", max_errors=20 + ) + self.assertEqual(validator.errors, []) + + def test_vertex_limit_error_contains_feature_context(self): + gdf = self._gdf([self._line(2001)], ["edge-2001"]) + gdf.index = [17] + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit(gdf, "edges", max_errors=20) + + self.assertEqual(len(validator.errors), 1) + self.assertIn("edge-2001", validator.errors[0]) + self.assertIn("'edges'", validator.errors[0]) + self.assertEqual(validator.issues[0]["filename"], "edges") + self.assertEqual(validator.issues[0]["feature_index"], 17) + + def test_vertex_limit_respects_max_errors(self): + geometries = [self._line(2001) for _ in range(4)] + validator = OSWValidation(zipfile_path="dummy.zip") + validator._validate_geometry_vertex_limit( + self._gdf(geometries, [f"edge-{index}" for index in range(4)]), + "edges", + max_errors=2, + ) + self.assertEqual(len(validator.errors), 2) + + def test_external_line_geometry_over_limit_fails_validation(self): + extension = self._gdf([self._line(2001)], ["external-line"]) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_READ_FILE, return_value=extension): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + + val = MagicMock() + val.files = [] + val.externalExtensions = ["/tmp/custom.geojson"] + val.is_valid.return_value = True + PVal.return_value = val + + result = OSWValidation(zipfile_path="dummy.zip").validate() + + self.assertFalse(result.is_valid) + self.assertTrue(any("external-line" in error for error in (result.errors or []))) + class TestGeometryMappingViaValidate(unittest.TestCase): """Unit tests for _u_id/_v_id/_w_id coordinate mapping through validate().""" @@ -1278,10 +1530,10 @@ def rf(path): self.assertFalse(res.is_valid) self.assertLessEqual(len(res.errors), 3) - def test_coord_within_tolerance_no_error(self): - """Coordinates within 1e-7 degrees are accepted as matching.""" + def test_u_id_coord_below_previous_tolerance_fails(self): + """Any deviation from the _u_id node coordinate is rejected.""" nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) - # Start is 5e-8 off (within tolerance) + # Start is 5e-8 off, which was accepted by the previous 1e-7 tolerance. edges = self._edges_gdf([("e1", "n1", "n2", [(5e-8, 0.0), (1.0, 1.0)])]) def rf(path): @@ -1289,6 +1541,111 @@ def rf(path): return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_u_id mismatch" in e for e in (res.errors or []))) + + def test_v_id_coord_below_previous_tolerance_fails(self): + """Any deviation from the _v_id node coordinate is rejected.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) + edges = self._edges_gdf([("e1", "n1", "n2", [(0.0, 0.0), (1.0, 1.0 + 5e-8)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_v_id mismatch" in e for e in (res.errors or []))) + + def test_w_id_coord_below_previous_tolerance_fails(self): + """Any deviation from a zone _w_id node coordinate is rejected.""" + nodes = self._nodes_gdf([("w1", 0.0, 0.0)]) + ring = [(5e-8, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)] + zones = self._zones_gdf([("z1", ["w1"], ring)]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else zones if "zones" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/zones.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_w_id coordinate mismatch" in e for e in (res.errors or []))) + + def test_u_id_coord_above_previous_tolerance_fails(self): + """A _u_id deviation above the previous 1e-7 tolerance is rejected.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) + edges = self._edges_gdf([("e1", "n1", "n2", [(2e-7, 0.0), (1.0, 1.0)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_u_id mismatch" in e for e in (res.errors or []))) + + def test_v_id_coord_above_previous_tolerance_fails(self): + """A _v_id deviation above the previous 1e-7 tolerance is rejected.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) + edges = self._edges_gdf([("e1", "n1", "n2", [(0.0, 0.0), (1.0, 1.0 + 2e-7)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_v_id mismatch" in e for e in (res.errors or []))) + + def test_w_id_coord_above_previous_tolerance_fails(self): + """A zone _w_id deviation above the previous 1e-7 tolerance is rejected.""" + nodes = self._nodes_gdf([("w1", 0.0, 0.0)]) + ring = [(2e-7, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)] + zones = self._zones_gdf([("z1", ["w1"], ring)]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else zones if "zones" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/zones.geojson"], rf) + self.assertFalse(res.is_valid) + self.assertTrue(any("_w_id coordinate mismatch" in e for e in (res.errors or []))) + + def test_u_id_exact_coord_match_passes(self): + """An edge start exactly matching its _u_id node coordinate is accepted.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0)]) + edges = self._edges_gdf([("e1", "n1", None, [(0.0, 0.0), (2.0, 2.0)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertTrue(res.is_valid, f"Expected valid; errors={res.errors}") + + def test_v_id_exact_coord_match_passes(self): + """An edge end exactly matching its _v_id node coordinate is accepted.""" + nodes = self._nodes_gdf([("n2", 1.0, 1.0)]) + edges = self._edges_gdf([("e1", None, "n2", [(2.0, 2.0), (1.0, 1.0)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) + self.assertTrue(res.is_valid, f"Expected valid; errors={res.errors}") + + def test_w_id_exact_coord_match_passes(self): + """A zone vertex exactly matching its _w_id node coordinate is accepted.""" + nodes = self._nodes_gdf([("w1", 0.0, 0.0)]) + ring = [(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)] + zones = self._zones_gdf([("z1", ["w1"], ring)]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else zones if "zones" in b else gpd.GeoDataFrame() + + res = self._run(["/tmp/nodes.geojson", "/tmp/zones.geojson"], rf) self.assertTrue(res.is_valid, f"Expected valid; errors={res.errors}") From 4c1a4c35c3a882be25a8f1a9aa3142176bdcab45 Mon Sep 17 00:00:00 2001 From: Anuj Date: Tue, 21 Jul 2026 14:04:19 +0530 Subject: [PATCH 2/4] Added extra unit test cases --- CHANGELOG.md | 4 +- src/python_osw_validation/__init__.py | 74 +++++++++++----- .../unit_tests/test_osw_validation_extras.py | 86 ++++++++++++++++++- 3 files changed, 138 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a01014a..a50aa5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,8 @@ - Fixed [#3982](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3982): edge `_u_id`/`_v_id` endpoints and zone `_w_id` vertices must now exactly match their referenced node coordinates; the previous `1e-7` tolerance was removed. - Added [#4047](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4047): input coordinates with more than seven decimal places produce a non-blocking message through `ValidationResult.warnings` without changing validation errors or validity. - Added [#4048](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4048): edge, line, polygon, and zone geometries containing more than 2,000 vertices are rejected, including supported external line and polygon geometries. -- Added a feature-level error for zero-length edge geometries, including the edge ID, dataset, and feature index, while suppressing the duplicate generic invalid-geometry message. -- Added boundary, regression, and real-data coverage for exact coordinate matching, coordinate precision warnings, geometry vertex limits, and zero-length edge reporting. +- Added feature-level errors for line and polygon geometries whose coordinates all collapse to one location, including the feature ID, dataset, and feature index, while suppressing duplicate generic invalid-geometry messages. +- Added boundary, regression, and real-data coverage for exact coordinate matching, coordinate precision warnings, geometry vertex limits, and collapsed-geometry reporting. ### 0.4.4 - 2026-07-01 - Fixed dataset filename classification so names containing dataset tokens in the prefix, such as `River._edges_.nodes.geojson`, are classified by their final dataset suffix instead of by loose substring matching. diff --git a/src/python_osw_validation/__init__.py b/src/python_osw_validation/__init__.py index 20736d1..1c07499 100644 --- a/src/python_osw_validation/__init__.py +++ b/src/python_osw_validation/__init__.py @@ -331,27 +331,50 @@ def _validate_geometry_vertex_limit( feature_index=feature_index, ) - def _validate_zero_length_edges( + def _geometry_coordinate_pairs(self, geometry) -> List[tuple]: + """Return two-dimensional coordinate pairs from line and polygon geometries.""" + if geometry is None or geometry.is_empty: + return [] + if geometry.geom_type in {'LineString', 'LinearRing'}: + return [(coordinate[0], coordinate[1]) for coordinate in geometry.coords] + if geometry.geom_type == 'Polygon': + coordinates = self._geometry_coordinate_pairs(geometry.exterior) + for ring in geometry.interiors: + coordinates.extend(self._geometry_coordinate_pairs(ring)) + return coordinates + if geometry.geom_type in {'MultiLineString', 'MultiPolygon', 'GeometryCollection'}: + return [ + coordinate + for part in geometry.geoms + for coordinate in self._geometry_coordinate_pairs(part) + ] + return [] + + def _validate_collapsed_geometries( self, - edges_df: Optional[gpd.GeoDataFrame], + gdf: Optional[gpd.GeoDataFrame], + dataset_name: str, max_errors: int, ) -> set: - """Report zero-length LineStrings and return their feature indexes.""" - zero_length_indexes = set() - if edges_df is None: - return zero_length_indexes + """Report line or polygon features whose coordinates all share one location.""" + collapsed_indexes = set() + if gdf is None: + return collapsed_indexes - for feature_index, row in edges_df.iterrows(): + for feature_index, row in gdf.iterrows(): geometry = row.geometry - if ( - geometry is None - or geometry.geom_type != 'LineString' - or geometry.is_empty - or geometry.length != 0 - ): + coordinates = self._geometry_coordinate_pairs(geometry) + if len(coordinates) < 2 or len(set(coordinates)) != 1: continue - zero_length_indexes.add(feature_index) + if geometry.geom_type in {'LineString', 'MultiLineString'}: + geometry_problem = 'zero-length' + elif geometry.geom_type in {'Polygon', 'MultiPolygon'}: + geometry_problem = 'zero-area' + else: + continue + + collapsed_indexes.add(feature_index) if len(self.errors) >= max_errors: continue @@ -360,13 +383,14 @@ def _validate_zero_length_edges( feature_id = feature_index self.log_errors( message=( - f"Edge '{feature_id}' has zero-length geometry because all coordinates are identical." + f"Feature '{feature_id}' in '{dataset_name}' has {geometry_problem} geometry " + "because all coordinates are identical." ), - filename='edges', + filename=dataset_name, feature_index=feature_index, ) - return zero_length_indexes + return collapsed_indexes def _schema_key_from_text(self, text: Optional[str]) -> Optional[str]: """Return dataset key from exact filename suffixes only.""" @@ -659,9 +683,7 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR for osw_file, gdf in OSW_DATASET.items(): if gdf is None: continue - zero_length_indexes = set() - if osw_file == 'edges': - zero_length_indexes = self._validate_zero_length_edges(gdf, max_errors) + collapsed_indexes = self._validate_collapsed_geometries(gdf, osw_file, max_errors) if osw_file in VERTEX_LIMIT_DATASETS and len(self.errors) < max_errors: self._validate_geometry_vertex_limit(gdf, osw_file, max_errors) expected_geom = OSW_DATASET_FILES.get(osw_file, {}).get('geometry') @@ -672,8 +694,8 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR else: invalid_geojson = gdf[gdf.is_valid == False] - if zero_length_indexes: - invalid_geojson = invalid_geojson.drop(index=zero_length_indexes, errors='ignore') + if collapsed_indexes: + invalid_geojson = invalid_geojson.drop(index=collapsed_indexes, errors='ignore') if len(invalid_geojson) > 0: # Extract IDs if present, else fallback to index @@ -706,7 +728,15 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR if len(self.errors) < max_errors: self._validate_geometry_vertex_limit(extensionFile, file_name, max_errors) + collapsed_indexes = self._validate_collapsed_geometries( + extensionFile, + file_name, + max_errors, + ) + invalid_geojson = extensionFile[extensionFile.is_valid == False] + if collapsed_indexes: + invalid_geojson = invalid_geojson.drop(index=collapsed_indexes, errors='ignore') if len(invalid_geojson) > 0: try: invalid_ids = list(set(invalid_geojson.get('_id', invalid_geojson.index))) diff --git a/tests/unit_tests/test_osw_validation_extras.py b/tests/unit_tests/test_osw_validation_extras.py index f0f929b..fe22b50 100644 --- a/tests/unit_tests/test_osw_validation_extras.py +++ b/tests/unit_tests/test_osw_validation_extras.py @@ -1166,7 +1166,7 @@ def read_file(path): result = OSWValidation(zipfile_path="dummy.zip").validate() expected_message = ( - "Edge 'edge-zero' has zero-length geometry because all coordinates are identical." + "Feature 'edge-zero' in 'edges' has zero-length geometry because all coordinates are identical." ) self.assertEqual(result.errors, [expected_message]) self.assertEqual( @@ -1190,11 +1190,93 @@ def test_closed_nonzero_edge_is_not_reported_as_zero_length(self): ) validator = OSWValidation(zipfile_path="dummy.zip") - indexes = validator._validate_zero_length_edges(edges, max_errors=20) + indexes = validator._validate_collapsed_geometries(edges, "edges", max_errors=20) self.assertEqual(indexes, set()) self.assertEqual(validator.errors, []) + def test_collapsed_geometries_are_reported_for_lines_polygons_and_zones(self): + cases = ( + ("lines", LineString([(1, 1), (1, 1)]), "zero-length"), + ("polygons", Polygon([(1, 1), (1, 1), (1, 1), (1, 1)]), "zero-area"), + ("zones", Polygon([(1, 1), (1, 1), (1, 1), (1, 1)]), "zero-area"), + ) + + for dataset_name, geometry, geometry_problem in cases: + with self.subTest(dataset_name=dataset_name): + gdf = gpd.GeoDataFrame( + {"_id": [f"{dataset_name}-collapsed"], "geometry": [geometry]}, + geometry="geometry", + crs="EPSG:4326", + index=[5], + ) + validator = OSWValidation(zipfile_path="dummy.zip") + + indexes = validator._validate_collapsed_geometries( + gdf, + dataset_name, + max_errors=20, + ) + + self.assertEqual(indexes, {5}) + self.assertEqual(len(validator.errors), 1) + self.assertIn(f"in '{dataset_name}' has {geometry_problem} geometry", validator.errors[0]) + self.assertEqual(validator.issues[0]["filename"], dataset_name) + self.assertEqual(validator.issues[0]["feature_index"], 5) + + def test_duplicate_point_coordinates_are_not_collapsed_geometries(self): + points = gpd.GeoDataFrame( + { + "_id": ["point-a", "point-b"], + "geometry": [Point(1, 1), Point(1, 1)], + }, + geometry="geometry", + crs="EPSG:4326", + ) + validator = OSWValidation(zipfile_path="dummy.zip") + + indexes = validator._validate_collapsed_geometries(points, "points", max_errors=20) + + self.assertEqual(indexes, set()) + self.assertEqual(validator.errors, []) + + def test_collapsed_external_geometry_reports_specific_error_without_generic_error(self): + extension = gpd.GeoDataFrame( + { + "_id": ["external-line"], + "geometry": [LineString([(2, 2), (2, 2)])], + }, + geometry="geometry", + crs="EPSG:4326", + index=[9], + ) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_READ_FILE, return_value=extension): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + + val = MagicMock() + val.files = [] + val.externalExtensions = ["/tmp/custom.geojson"] + val.is_valid.return_value = True + PVal.return_value = val + + result = OSWValidation(zipfile_path="dummy.zip").validate() + + self.assertEqual( + result.errors, + [ + "Feature 'external-line' in 'custom.geojson' has zero-length geometry " + "because all coordinates are identical." + ], + ) + self.assertEqual(result.issues[0]["filename"], "custom.geojson") + self.assertEqual(result.issues[0]["feature_index"], 9) + self.assertFalse(any("Invalid geometries found" in error for error in result.errors)) + class TestGeometryVertexLimit(unittest.TestCase): @staticmethod From 9007d4f90ebb22604067955d59dae0e679cda3b7 Mon Sep 17 00:00:00 2001 From: Anuj Date: Tue, 21 Jul 2026 23:32:57 +0530 Subject: [PATCH 3/4] Added immutable `ValidationConfig` support. Users can override the 2,000-vertex limit, seven-decimal coordinate warning threshold, and zero-length line handling per validator instance. Zero-length `LineString` geometries are rejected by default and can be allowed with `allow_zero_length_lines=True` --- CHANGELOG.md | 1 + README.md | 31 +++++ src/example.py | 11 +- src/python_osw_validation/__init__.py | 39 +++++- src/python_osw_validation/config.py | 31 +++++ .../unit_tests/test_osw_validation_extras.py | 107 ++++++++++++++-- tests/unit_tests/test_validation_config.py | 120 ++++++++++++++++++ 7 files changed, 318 insertions(+), 22 deletions(-) create mode 100644 src/python_osw_validation/config.py create mode 100644 tests/unit_tests/test_validation_config.py diff --git a/CHANGELOG.md b/CHANGELOG.md index a50aa5c..8ee61a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Change log ### 0.4.5 - 2026-07-21 +- Added immutable `ValidationConfig` support. Users can override the 2,000-vertex limit, seven-decimal coordinate warning threshold, and zero-length line handling per validator instance. Zero-length `LineString` geometries are rejected by default and can be allowed with `allow_zero_length_lines=True`. - Fixed [#3982](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/3982): edge `_u_id`/`_v_id` endpoints and zone `_w_id` vertices must now exactly match their referenced node coordinates; the previous `1e-7` tolerance was removed. - Added [#4047](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4047): input coordinates with more than seven decimal places produce a non-blocking message through `ValidationResult.warnings` without changing validation errors or validity. - Added [#4048](https://dev.azure.com/TDEI-UW/TDEI/_workitems/edit/4048): edge, line, polygon, and zone geometries containing more than 2,000 vertices are rejected, including supported external line and polygon geometries. diff --git a/README.md b/README.md index 2598e3d..e98c3e1 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,37 @@ print(result.errors) # returns up to the first 10 high-level errors print(result.issues) # capped by the same max_errors limit ``` +### Validation configuration + +Use `ValidationConfig` to override validation behavior for one validator instance. +When no configuration is supplied, the documented defaults are used. + +```python +from python_osw_validation import OSWValidation, ValidationConfig + +config = ValidationConfig( + max_geometry_vertices=3000, + coordinate_precision=8, + allow_zero_length_lines=True, +) + +validator = OSWValidation( + zipfile_path="dataset.zip", + config=config, +) +result = validator.validate() +``` + +| Setting | Default | Meaning | +|---------|---------|---------| +| `max_geometry_vertices` | `2000` | Maximum allowed vertices for edges, lines, polygons, and zones. Must be an integer greater than zero. | +| `coordinate_precision` | `7` | Maximum coordinate decimal places before a non-blocking warning is reported. Must be a non-negative integer. | +| `allow_zero_length_lines` | `False` | Rejects collapsed `LineString` geometries by default. Set to `True` to allow them in edges, lines, and external line data. | + +Allowing zero-length lines does not bypass `_u_id`/`_v_id` existence or exact +node-coordinate checks. Zero-area polygons and zones, collapsed `MultiLineString` +geometries, and line geometries in point datasets remain invalid. + ## Error behavior - `errors`: high-level validation messages, capped by `max_errors` (default `20`). diff --git a/src/example.py b/src/example.py index 29b80e0..3b00471 100644 --- a/src/example.py +++ b/src/example.py @@ -1,10 +1,10 @@ import os -from python_osw_validation import OSWValidation +from python_osw_validation import OSWValidation, ValidationConfig PARENT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) ASSETS_DIR = os.path.join(PARENT_DIR, 'tests/assets') # VALID_ZIP_FILE = os.path.join(ASSETS_DIR, 'valid 2.zip') -VALID_ZIP_FILE = os.path.join(ASSETS_DIR, '33493.zip') +VALID_ZIP_FILE = os.path.join(ASSETS_DIR, 'issue_2_sanitize.zip') INVALID_ZIP_FILE = os.path.join(ASSETS_DIR, '4151.zip') INVALID_VANCOUVER_ZIP_FILE = os.path.join(ASSETS_DIR, 'vancouver-dataset.zip') SCHEMA_DIR = os.path.join(PARENT_DIR, 'src/python_osw_validation/schema') @@ -18,7 +18,12 @@ def valid_test_with_provided_schema(): def valid_test_without_provided_schema(): - validator = OSWValidation(zipfile_path=VALID_ZIP_FILE) + config = ValidationConfig( + max_geometry_vertices=1, + coordinate_precision=7, + allow_zero_length_lines=True, + ) + validator = OSWValidation(zipfile_path=VALID_ZIP_FILE, config=config) result = validator.validate() print(result.issues) print(result.warnings) diff --git a/src/python_osw_validation/__init__.py b/src/python_osw_validation/__init__.py index 1c07499..968d2e0 100644 --- a/src/python_osw_validation/__init__.py +++ b/src/python_osw_validation/__init__.py @@ -8,6 +8,11 @@ import jsonschema_rs from .zipfile_handler import ZipFileHandler +from .config import ( + DEFAULT_COORDINATE_PRECISION, + DEFAULT_MAX_GEOMETRY_VERTICES, + ValidationConfig, +) from .extracted_data_validator import ExtractedDataValidator, OSW_DATASET_FILES, dataset_key_for_filename from .version import __version__ from .helpers import ( @@ -29,8 +34,11 @@ "zones": os.path.join(SCHEMA_PATH, 'opensidewalks.zones.schema-0.3.json'), } -COORDINATE_PRECISION_WARNING = "Input dataset contains coordinates with more than 7 decimal places." -MAX_GEOMETRY_VERTICES = 2000 +COORDINATE_PRECISION_WARNING = ( + "Input dataset contains coordinates with more than " + f"{DEFAULT_COORDINATE_PRECISION} decimal places." +) +MAX_GEOMETRY_VERTICES = DEFAULT_MAX_GEOMETRY_VERTICES VERTEX_LIMIT_DATASETS = frozenset({"edges", "lines", "polygons", "zones"}) @@ -70,6 +78,7 @@ def __init__( point_schema_path: Optional[str] = None, line_schema_path: Optional[str] = None, polygon_schema_path: Optional[str] = None, + config: Optional[ValidationConfig] = None, ): self.zipfile_path = zipfile_path self.extracted_dir: Optional[str] = None @@ -77,6 +86,9 @@ def __init__( # per-feature schema issues (formerly `fixme`) self.issues: List[Dict[str, Any]] = [] self.warnings = "" + if config is not None and not isinstance(config, ValidationConfig): + raise TypeError("config must be a ValidationConfig instance.") + self.config = config or ValidationConfig() # Legacy single schema (if set, used for all) self.schema_file_path = schema_file_path # may be None @@ -107,8 +119,14 @@ def _check_coordinate_precision(self, file_paths) -> None: """Set one aggregate warning without affecting validation state.""" for file_path in file_paths: try: - if _geojson_file_has_excess_coordinate_precision(str(file_path)): - self.warnings = COORDINATE_PRECISION_WARNING + if _geojson_file_has_excess_coordinate_precision( + str(file_path), + self.config.coordinate_precision, + ): + self.warnings = ( + "Input dataset contains coordinates with more than " + f"{self.config.coordinate_precision} decimal places." + ) return except (OSError, json.JSONDecodeError, TypeError, ValueError): # Existing parsing and schema validation own all functional errors. @@ -307,7 +325,7 @@ def _validate_geometry_vertex_limit( dataset_name: str, max_errors: int, ) -> None: - """Report line or polygon features containing more than 2,000 vertices.""" + """Report line or polygon features exceeding the configured vertex limit.""" if gdf is None: return @@ -316,7 +334,7 @@ def _validate_geometry_vertex_limit( break vertex_count = self._geometry_vertex_count(row.geometry) - if vertex_count <= MAX_GEOMETRY_VERTICES: + if vertex_count <= self.config.max_geometry_vertices: continue feature_id = row.get('_id', feature_index) @@ -325,7 +343,7 @@ def _validate_geometry_vertex_limit( self.log_errors( message=( f"Feature '{feature_id}' in '{dataset_name}' contains {vertex_count} geometry vertices. " - f"Maximum allowed is {MAX_GEOMETRY_VERTICES}." + f"Maximum allowed is {self.config.max_geometry_vertices}." ), filename=dataset_name, feature_index=feature_index, @@ -375,6 +393,13 @@ def _validate_collapsed_geometries( continue collapsed_indexes.add(feature_index) + allow_zero_length_line = ( + self.config.allow_zero_length_lines + and geometry.geom_type == 'LineString' + and dataset_name not in {"nodes", "points", "polygons", "zones"} + ) + if allow_zero_length_line: + continue if len(self.errors) >= max_errors: continue diff --git a/src/python_osw_validation/config.py b/src/python_osw_validation/config.py new file mode 100644 index 0000000..a892351 --- /dev/null +++ b/src/python_osw_validation/config.py @@ -0,0 +1,31 @@ +from dataclasses import dataclass + + +DEFAULT_MAX_GEOMETRY_VERTICES = 2000 +DEFAULT_COORDINATE_PRECISION = 7 +DEFAULT_ALLOW_ZERO_LENGTH_LINES = False + + +@dataclass(frozen=True) +class ValidationConfig: + """User-configurable validation behavior.""" + + max_geometry_vertices: int = DEFAULT_MAX_GEOMETRY_VERTICES + coordinate_precision: int = DEFAULT_COORDINATE_PRECISION + allow_zero_length_lines: bool = DEFAULT_ALLOW_ZERO_LENGTH_LINES + + def __post_init__(self) -> None: + if isinstance(self.max_geometry_vertices, bool) or not isinstance( + self.max_geometry_vertices, int + ): + raise TypeError("max_geometry_vertices must be an integer.") + if self.max_geometry_vertices <= 0: + raise ValueError("max_geometry_vertices must be greater than zero.") + if isinstance(self.coordinate_precision, bool) or not isinstance( + self.coordinate_precision, int + ): + raise TypeError("coordinate_precision must be an integer.") + if self.coordinate_precision < 0: + raise ValueError("coordinate_precision must be zero or greater.") + if not isinstance(self.allow_zero_length_lines, bool): + raise TypeError("allow_zero_length_lines must be a boolean.") diff --git a/tests/unit_tests/test_osw_validation_extras.py b/tests/unit_tests/test_osw_validation_extras.py index fe22b50..5a0fe30 100644 --- a/tests/unit_tests/test_osw_validation_extras.py +++ b/tests/unit_tests/test_osw_validation_extras.py @@ -9,7 +9,7 @@ from shapely.geometry import Point, LineString, Polygon import src.python_osw_validation as osw_mod -from src.python_osw_validation import OSWValidation +from src.python_osw_validation import OSWValidation, ValidationConfig # Build a robust patch prefix from the module actually imported _PATCH_PREFIX = osw_mod.__name__ @@ -1163,7 +1163,10 @@ def read_file(path): PZip.return_value = z PVal.return_value = self._fake_validator(fake_files) - result = OSWValidation(zipfile_path="dummy.zip").validate() + result = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=False), + ).validate() expected_message = ( "Feature 'edge-zero' in 'edges' has zero-length geometry because all coordinates are identical." @@ -1179,6 +1182,54 @@ def read_file(path): ) self.assertFalse(any("invalid edges geometries" in error for error in result.errors)) + def test_zero_length_edge_is_valid_when_enabled(self): + coordinate = (-122.335167, 47.608013) + nodes = gpd.GeoDataFrame( + { + "_id": ["node-seattle-001", "node-seattle-002"], + "geometry": [Point(*coordinate), Point(*coordinate)], + }, + geometry="geometry", + crs="EPSG:4326", + ) + edges = gpd.GeoDataFrame( + { + "_id": ["edge-seattle-001"], + "_u_id": ["node-seattle-001"], + "_v_id": ["node-seattle-002"], + "footway": ["sidewalk"], + "highway": ["footway"], + "length": [0], + "geometry": [LineString([coordinate, coordinate])], + }, + geometry="geometry", + crs="EPSG:4326", + ) + + def read_file(path): + return nodes if "nodes" in os.path.basename(path) else edges + + fake_files = ["/tmp/nodes.geojson", "/tmp/edges.geojson"] + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE, side_effect=read_file), \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + PVal.return_value = self._fake_validator(fake_files) + + result = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=True), + ).validate() + + self.assertTrue(result.is_valid) + self.assertIsNone(result.errors) + self.assertEqual(result.issues, []) + def test_closed_nonzero_edge_is_not_reported_as_zero_length(self): edges = gpd.GeoDataFrame( { @@ -1210,7 +1261,10 @@ def test_collapsed_geometries_are_reported_for_lines_polygons_and_zones(self): crs="EPSG:4326", index=[5], ) - validator = OSWValidation(zipfile_path="dummy.zip") + validator = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=False), + ) indexes = validator._validate_collapsed_geometries( gdf, @@ -1240,7 +1294,7 @@ def test_duplicate_point_coordinates_are_not_collapsed_geometries(self): self.assertEqual(indexes, set()) self.assertEqual(validator.errors, []) - def test_collapsed_external_geometry_reports_specific_error_without_generic_error(self): + def test_collapsed_external_line_is_rejected_by_default_without_generic_duplicate(self): extension = gpd.GeoDataFrame( { "_id": ["external-line"], @@ -1266,17 +1320,46 @@ def test_collapsed_external_geometry_reports_specific_error_without_generic_erro result = OSWValidation(zipfile_path="dummy.zip").validate() - self.assertEqual( - result.errors, - [ - "Feature 'external-line' in 'custom.geojson' has zero-length geometry " - "because all coordinates are identical." - ], + expected_message = ( + "Feature 'external-line' in 'custom.geojson' has zero-length geometry " + "because all coordinates are identical." ) - self.assertEqual(result.issues[0]["filename"], "custom.geojson") - self.assertEqual(result.issues[0]["feature_index"], 9) + self.assertFalse(result.is_valid) + self.assertEqual(result.errors, [expected_message]) self.assertFalse(any("Invalid geometries found" in error for error in result.errors)) + def test_collapsed_external_line_is_allowed_when_enabled(self): + extension = gpd.GeoDataFrame( + { + "_id": ["external-line"], + "geometry": [LineString([(2, 2), (2, 2)])], + }, + geometry="geometry", + crs="EPSG:4326", + ) + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_READ_FILE, return_value=extension): + z = MagicMock() + z.extract_zip.return_value = "/tmp/extracted" + z.remove_extracted_files.return_value = None + PZip.return_value = z + + val = MagicMock() + val.files = [] + val.externalExtensions = ["/tmp/custom.geojson"] + val.is_valid.return_value = True + PVal.return_value = val + + result = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=True), + ).validate() + + self.assertTrue(result.is_valid) + self.assertIsNone(result.errors) + self.assertEqual(result.issues, []) + class TestGeometryVertexLimit(unittest.TestCase): @staticmethod diff --git a/tests/unit_tests/test_validation_config.py b/tests/unit_tests/test_validation_config.py new file mode 100644 index 0000000..dc52af4 --- /dev/null +++ b/tests/unit_tests/test_validation_config.py @@ -0,0 +1,120 @@ +import unittest +from unittest.mock import patch + +import geopandas as gpd +from shapely.geometry import LineString, Polygon + +from src.python_osw_validation import OSWValidation, ValidationConfig + + +class TestValidationConfig(unittest.TestCase): + @staticmethod + def _gdf(geometry, feature_id="feature-1"): + return gpd.GeoDataFrame( + {"_id": [feature_id], "geometry": [geometry]}, + geometry="geometry", + crs="EPSG:4326", + ) + + def test_defaults_match_public_configuration_contract(self): + config = ValidationConfig() + + self.assertEqual(config.max_geometry_vertices, 2000) + self.assertEqual(config.coordinate_precision, 7) + self.assertFalse(config.allow_zero_length_lines) + + def test_invalid_configuration_values_are_rejected(self): + cases = ( + ({"max_geometry_vertices": 0}, ValueError), + ({"max_geometry_vertices": True}, TypeError), + ({"coordinate_precision": -1}, ValueError), + ({"coordinate_precision": 1.5}, TypeError), + ({"allow_zero_length_lines": "yes"}, TypeError), + ) + for kwargs, expected_error in cases: + with self.subTest(kwargs=kwargs): + with self.assertRaises(expected_error): + ValidationConfig(**kwargs) + + def test_custom_max_geometry_vertices_is_enforced(self): + validator = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(max_geometry_vertices=3), + ) + line = LineString([(0, 0), (1, 0), (2, 0), (3, 0)]) + + validator._validate_geometry_vertex_limit( + self._gdf(line, "line-four"), + "lines", + max_errors=20, + ) + + self.assertEqual(len(validator.errors), 1) + self.assertIn("Maximum allowed is 3", validator.errors[0]) + + def test_custom_coordinate_precision_is_forwarded_to_checker(self): + validator = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(coordinate_precision=5), + ) + with patch( + "src.python_osw_validation._geojson_file_has_excess_coordinate_precision", + return_value=True, + ) as checker: + validator._check_coordinate_precision(["nodes.geojson"]) + + checker.assert_called_once_with("nodes.geojson", 5) + self.assertEqual( + validator.warnings, + "Input dataset contains coordinates with more than 5 decimal places.", + ) + + def test_zero_length_line_is_rejected_by_default_and_can_be_allowed(self): + line = self._gdf(LineString([(1, 1), (1, 1)]), "line-zero") + default_validator = OSWValidation(zipfile_path="dummy.zip") + permissive_validator = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=True), + ) + + default_indexes = default_validator._validate_collapsed_geometries( + line, + "lines", + max_errors=20, + ) + permissive_indexes = permissive_validator._validate_collapsed_geometries( + line, + "lines", + max_errors=20, + ) + + self.assertEqual(default_indexes, {0}) + self.assertEqual(len(default_validator.errors), 1) + self.assertIn("zero-length geometry", default_validator.errors[0]) + self.assertEqual(permissive_indexes, {0}) + self.assertEqual(permissive_validator.errors, []) + + def test_zero_area_polygon_remains_invalid(self): + polygon = self._gdf( + Polygon([(1, 1), (1, 1), (1, 1), (1, 1)]), + "polygon-zero", + ) + validator = OSWValidation(zipfile_path="dummy.zip") + + indexes = validator._validate_collapsed_geometries( + polygon, + "polygons", + max_errors=20, + ) + + self.assertEqual(indexes, {0}) + self.assertEqual(len(validator.errors), 1) + self.assertIn("zero-area geometry", validator.errors[0]) + + def test_validator_rejects_non_config_object(self): + with self.assertRaisesRegex(TypeError, "ValidationConfig"): + OSWValidation(zipfile_path="dummy.zip", config={}) + + +if __name__ == "__main__": + unittest.main() From 74b7c202d85890d1b84d8b8369bccdd60909c35e Mon Sep 17 00:00:00 2001 From: Anuj Date: Wed, 22 Jul 2026 00:10:06 +0530 Subject: [PATCH 4/4] Fixed unit test cases --- .../opensidewalks.points.schema-0.3.json | 1 - .../opensidewalks.zones.schema-0.3.json | 19 ----------- tests/test_schema_parity.py | 32 ++++++++++--------- 3 files changed, 17 insertions(+), 35 deletions(-) diff --git a/src/python_osw_validation/schema/opensidewalks.points.schema-0.3.json b/src/python_osw_validation/schema/opensidewalks.points.schema-0.3.json index f595b21..cada826 100644 --- a/src/python_osw_validation/schema/opensidewalks.points.schema-0.3.json +++ b/src/python_osw_validation/schema/opensidewalks.points.schema-0.3.json @@ -415,7 +415,6 @@ "enum": [ "broadleaved", "leafless", - "mixed", "needleleaved" ], "type": "string" diff --git a/src/python_osw_validation/schema/opensidewalks.zones.schema-0.3.json b/src/python_osw_validation/schema/opensidewalks.zones.schema-0.3.json index 87f480e..6085a67 100644 --- a/src/python_osw_validation/schema/opensidewalks.zones.schema-0.3.json +++ b/src/python_osw_validation/schema/opensidewalks.zones.schema-0.3.json @@ -255,25 +255,6 @@ "yes" ], "type": "string" - }, - "name": { - "description": "A field for a designated name for an entity. Example: an official name for a trail.", - "type": "string" - }, - "surface": { - "description": "A field for the surface material of the path.", - "enum": [ - "asphalt", - "concrete", - "dirt", - "grass", - "grass_paver", - "gravel", - "paved", - "paving_stones", - "unpaved" - ], - "type": "string" } }, "required": [ diff --git a/tests/test_schema_parity.py b/tests/test_schema_parity.py index 6c1e8b1..4a673b5 100644 --- a/tests/test_schema_parity.py +++ b/tests/test_schema_parity.py @@ -6,10 +6,14 @@ SCHEMA_DIR = Path(__file__).parent.parent / "src" / "python_osw_validation" / "schema" FIXTURES_DIR = Path(__file__).parent / "schema" / "fixtures" -PAIRS = { - "nodes": ("nodes.schema.json", "opensidewalks.nodes.schema-0.3.json"), - "edges": ("edges.schema.json", "opensidewalks.edges.schema-0.3.json"), - "zones": ("zones.schema.json", "opensidewalks.zones.schema-0.3.json"), +SOURCE_SCHEMA = "opensidewalks.schema-0.3.json" +SHORT_SCHEMAS = { + "nodes": "opensidewalks.nodes.schema-0.3.json", + "edges": "opensidewalks.edges.schema-0.3.json", + "points": "opensidewalks.points.schema-0.3.json", + "lines": "opensidewalks.lines.schema-0.3.json", + "polygons": "opensidewalks.polygons.schema-0.3.json", + "zones": "opensidewalks.zones.schema-0.3.json", } @@ -19,7 +23,7 @@ def load(path: Path): def validator(schema_name: str, customized: bool): - fname = PAIRS[schema_name][1 if customized else 0] + fname = SHORT_SCHEMAS[schema_name] if customized else SOURCE_SCHEMA schema = load(SCHEMA_DIR / fname) return jsonschema_rs.Draft7Validator(schema) @@ -57,14 +61,12 @@ def test_edges_accept_custom_edge_without_highway(): assert ours.is_valid(data) -def test_edges_schema_02_rejects_custom_extensions(): +def test_edges_schema_02_allows_ext_fields_without_custom_tokens(): data = run_fixture("edges", "invalid_custom_edge_schema02.json") - # In 0.2 datasets, custom extensions (ext:*) are not allowed; this is enforced - # by the validator's 0.2 compatibility guard. from src.python_osw_validation import OSWValidation guard = OSWValidation(zipfile_path="dummy.zip") reasons = guard._contains_disallowed_features_for_02(data) - assert reasons == {"custom_token"} + assert reasons == set() def test_nodes_schema_02_allows_ext_fields(): @@ -99,12 +101,12 @@ def test_custom_line_schema03_accepts(): assert ours.is_valid(data) -def test_custom_line_schema02_rejects_ext(): +def test_line_schema02_allows_ext_fields_without_custom_tokens(): data = run_fixture("lines", "invalid_custom_line_schema02.json") from src.python_osw_validation import OSWValidation guard = OSWValidation(zipfile_path="dummy.zip") reasons = guard._contains_disallowed_features_for_02(data) - assert reasons == {"custom_token"} + assert reasons == set() def test_custom_polygon_schema03_accepts(): @@ -115,20 +117,20 @@ def test_custom_polygon_schema03_accepts(): assert ours.is_valid(data) -def test_custom_polygon_schema02_rejects_ext(): +def test_polygon_schema02_allows_ext_fields_without_custom_tokens(): data = run_fixture("polygons", "invalid_custom_polygon_schema02.json") from src.python_osw_validation import OSWValidation guard = OSWValidation(zipfile_path="dummy.zip") reasons = guard._contains_disallowed_features_for_02(data) - assert reasons == {"custom_token"} + assert reasons == set() -def test_custom_zone_schema02_rejects_ext(): +def test_zone_schema02_allows_ext_fields_without_custom_tokens(): data = run_fixture("zones", "invalid_custom_zone_schema02.json") from src.python_osw_validation import OSWValidation guard = OSWValidation(zipfile_path="dummy.zip") reasons = guard._contains_disallowed_features_for_02(data) - assert reasons == {"custom_token"} + assert reasons == set() # --- Zones ---