diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee61a0..e126f56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Change log +### 0.5.0 - 2026-08-05 +- Coordinates with more than `coordinate_precision` (default seven) decimal places are now validation errors instead of a warning. Each offending feature is reported individually with its filename and feature index, and validation stops before schema checks when any are found. +- Removed `ValidationResult.warnings` and the `COORDINATE_PRECISION_WARNING` constant; the precision message is now part of `errors`/`issues`. +- Changed `allow_zero_length_lines` to default to `True`. Set it to `False` to reject collapsed `LineString` geometries. +- Confirmed there is no `_u_id != _v_id` constraint in the validator or the schemas: an edge may start and end at the same node. A self-loop with real length always passes; one collapsed to a point is governed solely by `allow_zero_length_lines`, and is reported as a geometry problem rather than a reference problem. +- Made `issues[].filename` consistent across every error: it is now always the GeoJSON filename the problem was found in (`opensidewalks.edges.geojson`), never the internal dataset key (`edges`, `zones`) or the `All` placeholder. Error text that named a dataset key now names the file too, so `Duplicate _id's found in nodes` reads `Duplicate _id's found in nodes.geojson`. +- Coordinate precision errors now tell users how to fix the data: `Reduce them to at most 7 decimal places; you can use the OSW data wizard tool to clean this up.` +- Null/NaN errors in `ext:*` properties now point at the same remedy: `... provide a valid value or remove this property. You can use the OSW data wizard tool to clean this up.` +- Added coverage confirming coordinate precision is judged on written decimal places and never on how close a value is to a shorter one. Anything with 8 or more decimal places is rejected (`48.9999999999`, `49.0000000001`, `49.00000000`); anything with 7 or fewer is accepted (`49`, `49.0`, `49.0000000`). Exponent notation is measured after normalization. + ### 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. diff --git a/README.md b/README.md index e98c3e1..a3aa5b0 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,6 @@ 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) @@ -58,7 +57,7 @@ from python_osw_validation import OSWValidation, ValidationConfig config = ValidationConfig( max_geometry_vertices=3000, coordinate_precision=8, - allow_zero_length_lines=True, + allow_zero_length_lines=False, ) validator = OSWValidation( @@ -71,8 +70,8 @@ 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. | +| `coordinate_precision` | `7` | Maximum coordinate decimal places allowed. Features exceeding it fail validation. Must be a non-negative integer. | +| `allow_zero_length_lines` | `True` | Allows collapsed `LineString` geometries in edges, lines, and external line data. Set to `False` to reject them. | Allowing zero-length lines does not bypass `_u_id`/`_v_id` existence or exact node-coordinate checks. Zero-area polygons and zones, collapsed `MultiLineString` @@ -82,8 +81,11 @@ geometries, and line geometries in point datasets remain invalid. - `errors`: high-level validation messages, capped by `max_errors` (default `20`). - `issues`: detailed per-feature validation issues, also capped by `max_errors`. +- Coordinates carrying more than `coordinate_precision` decimal places fail validation before schema checks, one error per offending feature, and the message names the fix: + - `Feature 12 in 'osw.edges.geojson' contains coordinates with more than 7 decimal places. Reduce them to at most 7 decimal places; you can use the OSW data wizard tool to clean this up.` +- `issues[].filename` is always the GeoJSON file the problem was found in, never an internal dataset key. - If actual null or numeric NaN values are found in `ext:*` extension properties, validation fails early before schema checks with actionable messages such as: - - `Invalid value at 'ext:metadata.score': nan. Null/NaN placeholders are not allowed; provide a valid value or remove this property.` + - `Invalid value at 'ext:metadata.score': nan. Null/NaN placeholders are not allowed; provide a valid value or remove this property. You can use the OSW data wizard tool to clean this up.` - For enum validation, long allowed-value lists are summarized as: - first 5 values joined by `|` - followed by `| and N more` when applicable. diff --git a/src/python_osw_validation/__init__.py b/src/python_osw_validation/__init__.py index 968d2e0..f31595f 100644 --- a/src/python_osw_validation/__init__.py +++ b/src/python_osw_validation/__init__.py @@ -21,7 +21,7 @@ _feature_index_from_error, _pretty_message, _read_geojson_without_ext, - _geojson_file_has_excess_coordinate_precision, + _geojson_features_exceeding_coordinate_precision, ) SCHEMA_PATH = os.path.join(os.path.dirname(__file__), 'schema') @@ -34,9 +34,13 @@ "zones": os.path.join(SCHEMA_PATH, 'opensidewalks.zones.schema-0.3.json'), } -COORDINATE_PRECISION_WARNING = ( - "Input dataset contains coordinates with more than " - f"{DEFAULT_COORDINATE_PRECISION} decimal places." +COORDINATE_PRECISION_REMEDY = ( + "Reduce them to at most {limit} decimal places; " + "you can use the OSW data wizard tool to clean this up." +) +NULLISH_VALUE_REMEDY = ( + "Null/NaN placeholders are not allowed; provide a valid value or remove this " + "property. You can use the OSW data wizard tool to clean this up." ) MAX_GEOMETRY_VERTICES = DEFAULT_MAX_GEOMETRY_VERTICES VERTEX_LIMIT_DATASETS = frozenset({"edges", "lines", "polygons", "zones"}) @@ -48,18 +52,16 @@ 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, warnings: str = ""): + issues: Optional[List[Dict[str, Any]]] = None): self.is_valid = is_valid if len(errors) == 0: self.errors = None else: self.errors = errors self.issues = issues - self.warnings = warnings class OSWValidation: @@ -85,7 +87,8 @@ def __init__( self.errors: List[str] = [] # per-feature schema issues (formerly `fixme`) self.issues: List[Dict[str, Any]] = [] - self.warnings = "" + # dataset key ('edges') -> the GeoJSON filename it was read from + self.dataset_filenames: Dict[str, str] = {} if config is not None and not isinstance(config, ValidationConfig): raise TypeError("config must be a ValidationConfig instance.") self.config = config or ValidationConfig() @@ -115,30 +118,63 @@ 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.""" + def _dataset_filename(self, dataset_name: Optional[str]) -> Optional[str]: + """Resolve a dataset key to the file it was read from. + + Errors always name a real file ('opensidewalks.edges.geojson'), never the + internal dataset key ('edges'). Names that are already filenames, such as + external extensions, pass through unchanged. + """ + if dataset_name is None: + return None + return self.dataset_filenames.get(dataset_name, dataset_name) + + def _check_coordinate_precision(self, file_paths, max_errors: int = 20) -> bool: + """Log an error per feature whose coordinates exceed the precision limit. + + Returns True when every file is within the limit. + """ + is_valid = True for file_path in file_paths: + filename = os.path.basename(str(file_path)) try: - if _geojson_file_has_excess_coordinate_precision( + offending = _geojson_features_exceeding_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. continue + for feature_index in offending: + is_valid = False + location = ( + f"Feature {feature_index} in '{filename}'" + if feature_index is not None + else f"'{filename}'" + ) + self.log_errors( + message=( + f"{location} contains coordinates with more than " + f"{self.config.coordinate_precision} decimal places. " + + COORDINATE_PRECISION_REMEDY.format( + limit=self.config.coordinate_precision + ) + ), + filename=filename, + feature_index=feature_index, + ) + if len(self.errors) >= max_errors: + return False + return is_valid # 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.""" if gdf is None: return set() + filename = self._dataset_filename(filekey) if col not in gdf.columns: - self.log_errors(f"Missing required column '{col}' in {filekey}.", filekey, None) + self.log_errors(f"Missing required column '{col}' in {filename}.", filename, None) return set() try: return set(gdf[col].dropna()) @@ -147,7 +183,7 @@ def _get_colset(self, gdf: Optional[gpd.GeoDataFrame], col: str, filekey: str) - try: return set(map(str, gdf[col].dropna())) except Exception: - self.log_errors(f"Could not create set for column '{col}' in {filekey}.", filekey, None) + self.log_errors(f"Could not create set for column '{col}' in {filename}.", filename, None) return set() # ---------------------------- @@ -217,7 +253,7 @@ def _validate_edge_geometry_mapping( f"start coordinate {edge_start} does not match " f"node id '{u_id}' coordinate {node_coord} (_u_id mismatch)." ), - filename='edges', + filename=self._dataset_filename('edges'), feature_index=feat_idx, ) @@ -239,7 +275,7 @@ def _validate_edge_geometry_mapping( f"end coordinate {edge_end} does not match " f"node id '{v_id}' coordinate {node_coord} (_v_id mismatch)." ), - filename='edges', + filename=self._dataset_filename('edges'), feature_index=feat_idx, ) @@ -294,7 +330,7 @@ def _validate_zone_geometry_mapping( f"node id '{w_id}' coordinate {node_coord} is not a vertex " f"of the zone polygon geometry (_w_id coordinate mismatch)." ), - filename='zones', + filename=self._dataset_filename('zones'), feature_index=feat_idx, ) @@ -340,12 +376,13 @@ def _validate_geometry_vertex_limit( feature_id = row.get('_id', feature_index) if self._is_nullish_value(feature_id): feature_id = feature_index + filename = self._dataset_filename(dataset_name) self.log_errors( message=( - f"Feature '{feature_id}' in '{dataset_name}' contains {vertex_count} geometry vertices. " + f"Feature '{feature_id}' in '{filename}' contains {vertex_count} geometry vertices. " f"Maximum allowed is {self.config.max_geometry_vertices}." ), - filename=dataset_name, + filename=filename, feature_index=feature_index, ) @@ -406,12 +443,13 @@ def _validate_collapsed_geometries( feature_id = row.get('_id', feature_index) if self._is_nullish_value(feature_id): feature_id = feature_index + filename = self._dataset_filename(dataset_name) self.log_errors( message=( - f"Feature '{feature_id}' in '{dataset_name}' has {geometry_problem} geometry " + f"Feature '{feature_id}' in '{filename}' has {geometry_problem} geometry " "because all coordinates are identical." ), - filename=dataset_name, + filename=filename, feature_index=feature_index, ) @@ -548,7 +586,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, self.warnings) + return ValidationResult(is_valid, final_errors, final_issues) zip_handler = None OSW_DATASET: Dict[str, Optional[gpd.GeoDataFrame]] = {} @@ -577,10 +615,21 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR ) return _finalize(False) - self._check_coordinate_precision( + # Remember which file each dataset key came from, so every error can + # name a real file instead of the internal key. + self.dataset_filenames = {} + for file in getattr(validator, 'files', []) or []: + basename = os.path.basename(str(file)) + dataset_key = dataset_key_for_filename(basename) + if dataset_key: + self.dataset_filenames[dataset_key] = basename + + if not self._check_coordinate_precision( list(getattr(validator, 'files', []) or []) - + list(getattr(validator, 'externalExtensions', []) or []) - ) + + list(getattr(validator, 'externalExtensions', []) or []), + max_errors=max_errors, + ): + return _finalize(False) # Per-file schema validation → populate self.issues (fixme-like) for file in validator.files: @@ -616,14 +665,15 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR if not is_valid: total_duplicates = len(duplicates) displayed = ', '.join(map(str, duplicates[:max_errors])) + filename = self._dataset_filename(osw_file) if total_duplicates > max_errors: - message = (f"Duplicate _id's found in {osw_file}: showing first {max_errors} " + message = (f"Duplicate _id's found in {filename}: showing first {max_errors} " f"of {total_duplicates} duplicates: {displayed}") else: - message = f"Duplicate _id's found in {osw_file}: {displayed}" + message = f"Duplicate _id's found in {filename}: {displayed}" self.log_errors( message=message, - filename=osw_file, + filename=filename, feature_index=None ) @@ -646,7 +696,12 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR for item in (sub if isinstance(sub, (list, tuple)) else [sub]) ) else: - self.log_errors("Missing required column '_w_id' in zones.", 'zones', None) + zones_filename = self._dataset_filename('zones') + self.log_errors( + f"Missing required column '_w_id' in {zones_filename}.", + zones_filename, + None, + ) node_ids_zones_w = set() else: node_ids_zones_w = set() @@ -663,7 +718,7 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR message=(f"All _u_id's in edges should be part of _id's mentioned in nodes. " f"Showing {max_errors if num_unmatched > max_errors else 'all'} out of {num_unmatched} " f"unmatched _u_id's: {displayed_unmatched}"), - filename='All', + filename=self._dataset_filename('edges'), feature_index=None ) @@ -678,7 +733,7 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR message=(f"All _v_id's in edges should be part of _id's mentioned in nodes. " f"Showing {max_errors if num_unmatched > max_errors else 'all'} out of {num_unmatched} " f"unmatched _v_id's: {displayed_unmatched}"), - filename='All', + filename=self._dataset_filename('edges'), feature_index=None ) @@ -693,7 +748,7 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR message=(f"All _w_id's in zones should be part of _id's mentioned in nodes. " f"Showing {max_errors if num_unmatched > max_errors else 'all'} out of {num_unmatched} " f"unmatched _w_id's: {displayed_unmatched}"), - filename='All', + filename=self._dataset_filename('zones'), feature_index=None ) @@ -731,8 +786,9 @@ def _finalize(is_valid: bool, errors: Optional[List[str]] = None) -> ValidationR displayed_invalid = ', '.join(map(str, invalid_ids[:limit])) self.log_errors( message=(f"Showing {max_errors if num_invalid > max_errors else 'all'} out of {num_invalid} " - f"invalid {osw_file} geometries, id's of invalid geometries: {displayed_invalid}"), - filename='All', + f"invalid geometries in {self._dataset_filename(osw_file)}, " + f"id's of invalid geometries: {displayed_invalid}"), + filename=self._dataset_filename(osw_file), feature_index=None ) @@ -886,10 +942,7 @@ def validate_osw_errors(self, file_path: str, max_errors: int) -> bool: return False found_nullish = True rendered = f'"{bad_value}"' if isinstance(bad_value, str) else str(bad_value) - msg = ( - f"Invalid value at '{path}': {rendered}. " - f"Null/NaN placeholders are not allowed; provide a valid value or remove this property." - ) + msg = f"Invalid value at '{path}': {rendered}. {NULLISH_VALUE_REMEDY}" self.errors.append(f"Validation error: {msg}") self.issues.append({ "filename": filename, diff --git a/src/python_osw_validation/config.py b/src/python_osw_validation/config.py index a892351..77b012c 100644 --- a/src/python_osw_validation/config.py +++ b/src/python_osw_validation/config.py @@ -3,7 +3,7 @@ DEFAULT_MAX_GEOMETRY_VERTICES = 2000 DEFAULT_COORDINATE_PRECISION = 7 -DEFAULT_ALLOW_ZERO_LENGTH_LINES = False +DEFAULT_ALLOW_ZERO_LENGTH_LINES = True @dataclass(frozen=True) diff --git a/src/python_osw_validation/helpers.py b/src/python_osw_validation/helpers.py index c6e5516..f592323 100644 --- a/src/python_osw_validation/helpers.py +++ b/src/python_osw_validation/helpers.py @@ -1,7 +1,7 @@ import json import re from decimal import Decimal -from typing import Any, Optional +from typing import Any, List, Optional import geopandas as gpd @@ -28,25 +28,39 @@ def _geometry_exceeds_coordinate_precision(geometry: Any, max_decimal_places: in ) -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.""" +def _geojson_features_exceeding_coordinate_precision( + file_path: str, max_decimal_places: int = 7 +) -> List[Optional[int]]: + """Return feature indices whose coordinates exceed the decimal-place limit. + + A single-`Feature` or bare-geometry document has no feature index, so it is + reported as `None` when it violates the 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 + return [] if data.get("type") == "FeatureCollection": features = data.get("features", []) if not isinstance(features, list): - return False - return any( - isinstance(feature, dict) + return [] + return [ + index + for index, feature in enumerate(features) + if 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) + geometry = data.get("geometry") + else: + geometry = data + return [None] if _geometry_exceeds_coordinate_precision(geometry, max_decimal_places) else [] + + +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.""" + return bool(_geojson_features_exceeding_coordinate_precision(file_path, max_decimal_places)) def _read_geojson_without_ext(file_path: str) -> gpd.GeoDataFrame: diff --git a/src/python_osw_validation/version.py b/src/python_osw_validation/version.py index 68eb9b6..2b8877c 100644 --- a/src/python_osw_validation/version.py +++ b/src/python_osw_validation/version.py @@ -1 +1 @@ -__version__ = '0.4.5' +__version__ = '0.5.0' diff --git a/tests/unit_tests/test_helpers.py b/tests/unit_tests/test_helpers.py index 0919cca..c31adc6 100644 --- a/tests/unit_tests/test_helpers.py +++ b/tests/unit_tests/test_helpers.py @@ -275,7 +275,7 @@ def test_crs_is_propagated_when_present(self): self.assertIsNotNone(gdf.crs) -class TestCoordinatePrecisionWarning(unittest.TestCase): +class TestCoordinatePrecision(unittest.TestCase): def _write_geojson_text(self, coordinates, geometry_type="Point"): fd, path = tempfile.mkstemp(suffix=".geojson") os.close(fd) @@ -310,6 +310,67 @@ def test_nested_polygon_coordinates_are_checked(self): path = self._write_geojson_text(coordinates, geometry_type="Polygon") self.assertTrue(helpers._geojson_file_has_excess_coordinate_precision(path)) + def test_precision_is_judged_on_digits_not_on_value(self): + """A value one ten-billionth away from 49.0 is still over-precise.""" + cases = ( + ("[-122.1234567,48.9999999999]", True), + ("[-122.1234567,49.0000000001]", True), + ("[-122.1234567,49.00000000]", True), + ("[-122.1234567,49.0000000]", False), + ("[-122.1234567,49.0]", False), + ("[-122.1234567,49]", False), + ) + for coordinates, expected in cases: + with self.subTest(coordinates=coordinates): + path = self._write_geojson_text(coordinates) + self.assertEqual( + helpers._geojson_file_has_excess_coordinate_precision(path), + expected, + ) + + def test_exponent_notation_is_measured_after_normalization(self): + over_precise = self._write_geojson_text("[-122.1234567,4.90000000001e1]") + exact = self._write_geojson_text("[-122.1234567,4.9e1]") + + self.assertTrue(helpers._geojson_file_has_excess_coordinate_precision(over_precise)) + self.assertFalse(helpers._geojson_file_has_excess_coordinate_precision(exact)) + + def test_offending_feature_indices_are_reported(self): + fd, path = tempfile.mkstemp(suffix=".geojson") + os.close(fd) + with open(path, "w") as file: + file.write( + '{"type":"FeatureCollection","features":[' + '{"type":"Feature","properties":{},"geometry":' + '{"type":"Point","coordinates":[-122.1234567,47.1234567]}},' + '{"type":"Feature","properties":{},"geometry":' + '{"type":"Point","coordinates":[-122.12345678,47.1]}},' + '{"type":"Feature","properties":{},"geometry":' + '{"type":"Point","coordinates":[-122.1,47.123456789]}}' + ']}' + ) + self.addCleanup(os.remove, path) + + self.assertEqual( + helpers._geojson_features_exceeding_coordinate_precision(path), + [1, 2], + ) + + def test_single_feature_document_reports_none_index(self): + fd, path = tempfile.mkstemp(suffix=".geojson") + os.close(fd) + with open(path, "w") as file: + file.write( + '{"type":"Feature","properties":{},"geometry":' + '{"type":"Point","coordinates":[-122.12345678,47.1]}}' + ) + self.addCleanup(os.remove, path) + + self.assertEqual( + helpers._geojson_features_exceeding_coordinate_precision(path), + [None], + ) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit_tests/test_osw_validation.py b/tests/unit_tests/test_osw_validation.py index fe9b481..ede09b3 100644 --- a/tests/unit_tests/test_osw_validation.py +++ b/tests/unit_tests/test_osw_validation.py @@ -5,11 +5,15 @@ import zipfile from unittest.mock import patch -from src.python_osw_validation import OSWValidation +from src.python_osw_validation import OSWValidation, ValidationConfig PARENT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) SRC_DIR = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) ASSETS_PATH = os.path.join(PARENT_DIR, 'assets') +# Several legacy fixtures carry coordinates with more than 7 decimal places, which is +# now an error. Tests targeting unrelated checks relax that limit so the fixture still +# reaches the behavior under test. +LENIENT_PRECISION = ValidationConfig(coordinate_precision=15) SCHEMA_DIR = os.path.join(SRC_DIR, 'src/python_osw_validation/schema') SCHEMA_FILE_PATH = os.path.join(SCHEMA_DIR, 'opensidewalks.schema-0.3.json') SCHEMA_PATHS = { @@ -306,7 +310,7 @@ def test_invalid_serialization_file(self): error_message = next((err for err in result.errors if 'non-serializable' in err.lower()), None) def test_unmatched_ids_limited_to_20(self): - validation = OSWValidation(zipfile_path=self.invalid_v_id_file) + validation = OSWValidation(zipfile_path=self.invalid_v_id_file, config=LENIENT_PRECISION) result = validation.validate() # Ensure validation fails @@ -329,7 +333,7 @@ def test_unmatched_ids_limited_to_20(self): self.assertIn('Showing 20 out of', error_message) def test_task_3469_issue_payload(self): - validation = OSWValidation(zipfile_path=self.task_3469_file) + validation = OSWValidation(zipfile_path=self.task_3469_file, config=LENIENT_PRECISION) result = validation.validate(max_errors=500) self.assertFalse(result.is_valid) self.assertIsInstance(result.issues, list) @@ -338,18 +342,18 @@ def test_task_3469_issue_payload(self): self.assertIn("Invalid value at 'climb': 'null'.", flattened) def test_task_3469_string_nulls_go_through_schema_validation(self): - validation = OSWValidation(zipfile_path=self.task_3469_file) + validation = OSWValidation(zipfile_path=self.task_3469_file, config=LENIENT_PRECISION) default_result = validation.validate() self.assertFalse(default_result.is_valid) self.assertEqual(len(default_result.issues), 3) - validation = OSWValidation(zipfile_path=self.task_3469_file) + validation = OSWValidation(zipfile_path=self.task_3469_file, config=LENIENT_PRECISION) override_result = validation.validate(max_errors=500) self.assertFalse(override_result.is_valid) self.assertEqual(len(override_result.issues), 3) def test_issue_3297_issue_payload(self): - validation = OSWValidation(zipfile_path=self.issue_3297_file) + validation = OSWValidation(zipfile_path=self.issue_3297_file, config=LENIENT_PRECISION) result = validation.validate(max_errors=100) self.assertFalse(result.is_valid) self.assertEqual(len(result.issues), 3) @@ -402,7 +406,7 @@ def test_edge_u_id_coord_mismatch_issue_has_feature_index(self): None, ) self.assertIsNotNone(mismatch_issue) - self.assertEqual(mismatch_issue['filename'], 'edges') + self.assertEqual(mismatch_issue['filename'], 'opensidewalks.edges.geojson') self.assertIsNotNone(mismatch_issue['feature_index']) def test_edge_v_id_coord_mismatch_fails(self): @@ -434,7 +438,7 @@ def test_zone_w_id_coord_mismatch_issue_has_feature_index(self): None, ) self.assertIsNotNone(mismatch_issue) - self.assertEqual(mismatch_issue['filename'], 'zones') + self.assertEqual(mismatch_issue['filename'], 'opensidewalks.zones.geojson') self.assertIsNotNone(mismatch_issue['feature_index']) def test_jsonschema_rs_pin_is_0_33_0(self): diff --git a/tests/unit_tests/test_osw_validation_extras.py b/tests/unit_tests/test_osw_validation_extras.py index 5a0fe30..b3846a6 100644 --- a/tests/unit_tests/test_osw_validation_extras.py +++ b/tests/unit_tests/test_osw_validation_extras.py @@ -19,7 +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" +_PATCH_PRECISION = f"{_PATCH_PREFIX}._geojson_features_exceeding_coordinate_precision" # A tiny canonical mapping that matches our mocked basenames _CANON_DATASET_FILES = { @@ -75,13 +75,13 @@ def _fake_validator(self, files, external_exts=None, valid=True, error="folder i # ---------------- tests ---------------- - def test_coordinate_precision_warning_does_not_invalidate_dataset(self): + def test_excess_coordinate_precision_invalidates_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_PRECISION, return_value=[0]), \ patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): z = MagicMock() z.extract_zip.return_value = "/tmp/extracted" @@ -91,18 +91,25 @@ def test_coordinate_precision_warning_does_not_invalidate_dataset(self): 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) + self.assertFalse(res.is_valid) + self.assertEqual( + res.errors, + [ + "Feature 0 in 'nodes.geojson' contains coordinates with more than 7 " + "decimal places. Reduce them to at most 7 decimal places; you can use " + "the OSW data wizard tool to clean this up." + ], + ) + self.assertEqual(res.issues[0]['filename'], 'nodes.geojson') + self.assertEqual(res.issues[0]['feature_index'], 0) - def test_coordinate_precision_warning_is_empty_when_not_detected(self): + def test_no_coordinate_precision_error_when_within_limit(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_PRECISION, return_value=[]), \ patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): z = MagicMock() z.extract_zip.return_value = "/tmp/extracted" @@ -113,15 +120,15 @@ def test_coordinate_precision_warning_is_empty_when_not_detected(self): res = OSWValidation(zipfile_path="dummy.zip").validate() self.assertTrue(res.is_valid) - self.assertEqual(res.warnings, "") + self.assertIsNone(res.errors) - def test_coordinate_precision_warning_does_not_replace_validation_errors(self): + def test_coordinate_precision_error_stops_before_schema_validation(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_VALIDATE, return_value=True) as schema_validate, \ patch(_PATCH_READ_FILE, return_value=nodes), \ - patch(_PATCH_PRECISION, return_value=True), \ + patch(_PATCH_PRECISION, return_value=[1]), \ patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): z = MagicMock() z.extract_zip.return_value = "/tmp/extracted" @@ -132,8 +139,8 @@ def test_coordinate_precision_warning_does_not_replace_validation_errors(self): 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) + self.assertTrue(all("decimal places" in error for error in (res.errors or []))) + schema_validate.assert_not_called() def test_structure_error_uses_uploaded_filename(self): """Validator errors should reference the uploaded ZIP, not the temp extraction dir.""" @@ -185,13 +192,43 @@ def test_nullish_values_fail_before_schema_validation(self): self.assertEqual(len(validator.issues), 2) self.assertEqual( validator.issues[0]["error_message"], - ["Invalid value at 'ext:missing': None. Null/NaN placeholders are not allowed; provide a valid value or remove this property."], + [ + "Invalid value at 'ext:missing': None. Null/NaN placeholders are not allowed; " + "provide a valid value or remove this property. You can use the OSW " + "data wizard tool to clean this up." + ], ) self.assertEqual( validator.issues[1]["error_message"], - ["Invalid value at 'ext:metadata.score': nan. Null/NaN placeholders are not allowed; provide a valid value or remove this property."], + [ + "Invalid value at 'ext:metadata.score': nan. Null/NaN placeholders are not allowed; " + "provide a valid value or remove this property. You can use the OSW " + "data wizard tool to clean this up." + ], ) + def test_nullish_error_points_users_at_the_osw_data_wizard(self): + """Null/NaN errors come with the remedy, not just the diagnosis.""" + validator = OSWValidation(zipfile_path="dummy.zip") + geojson_data = { + "type": "FeatureCollection", + "features": [ + { + "type": "Feature", + "properties": {"_id": "e1", "ext:missing": None}, + "geometry": {"type": "LineString", "coordinates": [[0, 0], [1, 1]]}, + } + ], + } + + with patch.object(validator, "load_osw_file", return_value=geojson_data), \ + patch.object(validator, "pick_schema_for_file"): + validator.validate_osw_errors("/tmp/edges.geojson", max_errors=20) + + message = validator.errors[0] + self.assertIn("Null/NaN placeholders are not allowed", message) + self.assertIn("OSW data wizard", message) + def test_non_extension_nullish_values_are_not_upfront_nullish_values(self): validator = OSWValidation(zipfile_path="dummy.zip") @@ -637,7 +674,7 @@ def test_duplicate_ids_detection(self): res = OSWValidation(zipfile_path="dummy.zip").validate() self.assertFalse(res.is_valid) msg = next((e for e in (res.errors or []) if "Duplicate _id's found in nodes" in e), None) - self.assertEqual(msg, "Duplicate _id's found in nodes: 2") + self.assertEqual(msg, "Duplicate _id's found in nodes.geojson: 2") def test_duplicate_ids_detection_is_limited_to_20(self): """Duplicate messages cap the number of displayed IDs.""" @@ -1099,7 +1136,7 @@ def test_invalid_geometry_logs_ids_when__id_present(self): res = OSWValidation(zipfile_path="dummy.zip").validate() self.assertFalse(res.is_valid) # Expect the invalid geometry message for 'edges' - msg = next((e for e in (res.errors or []) if "invalid edges geometries" in e), None) + msg = next((e for e in (res.errors or []) if "invalid geometries in edges.geojson" in e), None) self.assertIsNotNone(msg, f"No invalid-geometry message found. Errors: {res.errors}") self.assertIn("Showing all out of 3", msg) @@ -1123,7 +1160,7 @@ def test_invalid_geometry_logs_index_when__id_missing_and_caps_20(self): res = OSWValidation(zipfile_path="dummy.zip").validate() self.assertFalse(res.is_valid, f"Expected invalid; errors={res.errors}") - msg = next((e for e in (res.errors or []) if "invalid edges geometries" in e), None) + msg = next((e for e in (res.errors or []) if "invalid geometries in edges.geojson" in e), None) self.assertIsNotNone(msg, f"No invalid-geometry message found. Errors: {res.errors}") self.assertIn("Showing 20 out of 25", msg) @@ -1169,18 +1206,19 @@ def read_file(path): ).validate() expected_message = ( - "Feature 'edge-zero' in 'edges' has zero-length geometry because all coordinates are identical." + "Feature 'edge-zero' in 'edges.geojson' has zero-length geometry " + "because all coordinates are identical." ) self.assertEqual(result.errors, [expected_message]) self.assertEqual( result.issues, [{ - "filename": "edges", + "filename": "edges.geojson", "feature_index": 17, "error_message": expected_message, }], ) - self.assertFalse(any("invalid edges geometries" in error for error in result.errors)) + self.assertFalse(any("invalid geometries in edges.geojson" in error for error in result.errors)) def test_zero_length_edge_is_valid_when_enabled(self): coordinate = (-122.335167, 47.608013) @@ -1294,7 +1332,7 @@ def test_duplicate_point_coordinates_are_not_collapsed_geometries(self): self.assertEqual(indexes, set()) self.assertEqual(validator.errors, []) - def test_collapsed_external_line_is_rejected_by_default_without_generic_duplicate(self): + def test_collapsed_external_line_is_rejected_when_disallowed_without_generic_duplicate(self): extension = gpd.GeoDataFrame( { "_id": ["external-line"], @@ -1318,7 +1356,10 @@ def test_collapsed_external_line_is_rejected_by_default_without_generic_duplicat val.is_valid.return_value = True PVal.return_value = val - result = OSWValidation(zipfile_path="dummy.zip").validate() + result = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=False), + ).validate() expected_message = ( "Feature 'external-line' in 'custom.geojson' has zero-length geometry " @@ -1599,7 +1640,7 @@ def rf(path): res = self._run(["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf) issue = next((i for i in (res.issues or []) if "_u_id mismatch" in i.get("error_message", "")), None) self.assertIsNotNone(issue) - self.assertEqual(issue["filename"], "edges") + self.assertEqual(issue["filename"], "edges.geojson") self.assertIsNotNone(issue["feature_index"]) self.assertIn("edge-xyz", issue["error_message"]) @@ -1813,6 +1854,162 @@ def rf(path): res = self._run(["/tmp/nodes.geojson", "/tmp/zones.geojson"], rf) self.assertTrue(res.is_valid, f"Expected valid; errors={res.errors}") + # ---- self-referencing edges ---- + + def test_edge_may_start_and_end_at_the_same_node(self): + """`_u_id` == `_v_id` is legal; there is no inequality check.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0)]) + edges = self._edges_gdf( + [("e1", "n1", "n1", [(0.0, 0.0), (1.0, 1.0), (0.0, 0.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_self_referencing_zero_length_edge_is_allowed_by_default(self): + """A self-loop collapsed to one point rides on allow_zero_length_lines.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0)]) + edges = self._edges_gdf([("e1", "n1", "n1", [(0.0, 0.0), (0.0, 0.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_self_referencing_edge_is_only_rejected_for_its_geometry(self): + """Opting into strict mode complains about the geometry, not about u == v.""" + nodes = self._nodes_gdf([("n1", 0.0, 0.0)]) + edges = self._edges_gdf([("e1", "n1", "n1", [(0.0, 0.0), (0.0, 0.0)])]) + + def rf(path): + b = os.path.basename(path) + return nodes if "nodes" in b else edges if "edges" in b else gpd.GeoDataFrame() + + with patch(_PATCH_ZIP) as PZip, \ + patch(_PATCH_EV) as PVal, \ + patch(_PATCH_VALIDATE, return_value=True), \ + patch(_PATCH_READ_FILE) as PRead, \ + patch(_PATCH_DATASET_FILES, _CANON_DATASET_FILES): + z, val, read_fn = self._patch_env( + ["/tmp/nodes.geojson", "/tmp/edges.geojson"], rf + ) + PZip.return_value = z + PVal.return_value = val + PRead.side_effect = read_fn + res = OSWValidation( + zipfile_path="dummy.zip", + config=ValidationConfig(allow_zero_length_lines=False), + ).validate() + + self.assertFalse(res.is_valid) + self.assertEqual(len(res.errors), 1) + self.assertIn("zero-length geometry", res.errors[0]) + self.assertNotIn("_u_id", res.errors[0]) + self.assertNotIn("_v_id", res.errors[0]) + + # ---- no tolerance at any magnitude ---- + + DEVIATIONS = ( + ("one 7-decimal step", 1e-7), + ("below the old tolerance", 5e-8), + ("a billionth", 1e-9), + ("a trillionth", 1e-12), + ) + + def test_u_id_rejects_every_deviation_magnitude(self): + """No deviation is small enough to be forgiven on an edge start.""" + for label, delta in self.DEVIATIONS: + with self.subTest(deviation=label): + nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) + edges = self._edges_gdf( + [("e1", "n1", "n2", [(delta, 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_rejects_every_deviation_magnitude(self): + """No deviation is small enough to be forgiven on an edge end.""" + for label, delta in self.DEVIATIONS: + with self.subTest(deviation=label): + 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 + delta)])] + ) + + 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_rejects_every_deviation_magnitude(self): + """No deviation is small enough to be forgiven on a zone vertex.""" + for label, delta in self.DEVIATIONS: + with self.subTest(deviation=label): + nodes = self._nodes_gdf([("w1", 0.0, 0.0)]) + ring = [(delta, 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_deviation_on_one_axis_is_enough_to_fail(self): + """Matching longitude does not excuse a latitude that is off, or vice versa.""" + cases = ( + ("longitude only", (1e-7, 0.0)), + ("latitude only", (0.0, 1e-7)), + ) + for label, (delta_lon, delta_lat) in cases: + with self.subTest(axis=label): + nodes = self._nodes_gdf([("n1", 0.0, 0.0), ("n2", 1.0, 1.0)]) + edges = self._edges_gdf( + [("e1", "n1", "n2", [(delta_lon, delta_lat), (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_only_the_off_w_id_is_reported(self): + """A zone with one exact and one deviating _w_id reports just the deviation.""" + nodes = self._nodes_gdf([("w1", 0.0, 0.0), ("w2", 1.0, 0.0)]) + ring = [(0.0, 0.0), (1.0 + 1e-7, 0.0), (1.0, 1.0), (0.0, 1.0)] + zones = self._zones_gdf([("z1", ["w1", "w2"], 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) + mismatches = [e for e in (res.errors or []) if "_w_id coordinate mismatch" in e] + self.assertEqual(len(mismatches), 1) + self.assertIn("'w2'", mismatches[0]) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit_tests/test_validation_config.py b/tests/unit_tests/test_validation_config.py index dc52af4..a97d41c 100644 --- a/tests/unit_tests/test_validation_config.py +++ b/tests/unit_tests/test_validation_config.py @@ -21,7 +21,7 @@ def test_defaults_match_public_configuration_contract(self): self.assertEqual(config.max_geometry_vertices, 2000) self.assertEqual(config.coordinate_precision, 7) - self.assertFalse(config.allow_zero_length_lines) + self.assertTrue(config.allow_zero_length_lines) def test_invalid_configuration_values_are_rejected(self): cases = ( @@ -58,23 +58,42 @@ def test_custom_coordinate_precision_is_forwarded_to_checker(self): config=ValidationConfig(coordinate_precision=5), ) with patch( - "src.python_osw_validation._geojson_file_has_excess_coordinate_precision", - return_value=True, + "src.python_osw_validation._geojson_features_exceeding_coordinate_precision", + return_value=[3], ) as checker: - validator._check_coordinate_precision(["nodes.geojson"]) + is_valid = validator._check_coordinate_precision(["nodes.geojson"]) checker.assert_called_once_with("nodes.geojson", 5) + self.assertFalse(is_valid) self.assertEqual( - validator.warnings, - "Input dataset contains coordinates with more than 5 decimal places.", + validator.errors, + [ + "Feature 3 in 'nodes.geojson' contains coordinates with more than 5 " + "decimal places. Reduce them to at most 5 decimal places; you can use " + "the OSW data wizard tool to clean this up." + ], ) - def test_zero_length_line_is_rejected_by_default_and_can_be_allowed(self): + def test_precision_error_points_users_at_the_osw_data_wizard(self): + """Over-precise coordinates come with the remedy, not just the diagnosis.""" + validator = OSWValidation(zipfile_path="dummy.zip") + with patch( + "src.python_osw_validation._geojson_features_exceeding_coordinate_precision", + return_value=[0], + ): + validator._check_coordinate_precision(["nodes.geojson"]) + + message = validator.errors[0] + self.assertIn("more than 7 decimal places", message) + self.assertIn("Reduce them to at most 7 decimal places", message) + self.assertIn("OSW data wizard", message) + + def test_zero_length_line_is_allowed_by_default_and_can_be_rejected(self): line = self._gdf(LineString([(1, 1), (1, 1)]), "line-zero") default_validator = OSWValidation(zipfile_path="dummy.zip") - permissive_validator = OSWValidation( + strict_validator = OSWValidation( zipfile_path="dummy.zip", - config=ValidationConfig(allow_zero_length_lines=True), + config=ValidationConfig(allow_zero_length_lines=False), ) default_indexes = default_validator._validate_collapsed_geometries( @@ -82,17 +101,17 @@ def test_zero_length_line_is_rejected_by_default_and_can_be_allowed(self): "lines", max_errors=20, ) - permissive_indexes = permissive_validator._validate_collapsed_geometries( + strict_indexes = strict_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, []) + self.assertEqual(default_validator.errors, []) + self.assertEqual(strict_indexes, {0}) + self.assertEqual(len(strict_validator.errors), 1) + self.assertIn("zero-length geometry", strict_validator.errors[0]) def test_zero_area_polygon_remains_invalid(self): polygon = self._gdf(