From 34a3ab3b03cdd7380353bea4d8532c3a255ef066 Mon Sep 17 00:00:00 2001 From: Mathias Hansen Date: Fri, 28 Aug 2026 16:12:50 +0200 Subject: [PATCH] fix(conform): never write NaN coordinates to the output GeoJSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ESRI services report a null point geometry as the string "NaN" rather than omitting the geometry. Bradford County PA (source 1078) has 67 such records, and they reached the addresses output as "coordinates": [NaN, NaN] — invalid GeoJSON per RFC 7946, which made the whole 39,638-feature file unparseable by Go's encoding/json. Two guards were meant to catch this and neither did: - openaddr/cache.py walked traverse(geom), which yields the geometry dict itself rather than its coordinates, so the isnan() test never ran on a coordinate. It also only recognized float NaN, not the string ESRI sends. - openaddr/conform.py compared the WKT against the literal "POINT (nan nan)", which was Shapely 1.x's spelling. Shapely 2 / GEOS 3.13 writes "POINT (NaN NaN)", so the comparison stopped matching. Skip the feature at download time, matching how every other geometry-less ESRI feature is already handled, and fall back to a null geometry further down the pipeline for the non-ESRI paths. json.dumps now runs with allow_nan=False so this class of bug fails loudly here instead of silently downstream. test_single_ny_orange asserted a golden row hash derived from the cache file's md5 fingerprint; dropping the bogus row changes that fingerprint, so the hash moves and the test now also asserts the feature count and the absence of NaN. Claude-Session: https://claude.ai/code/session_01JWLFBWRfhWhLUqFFqbPCGm --- README.md | 6 ++ openaddr/cache.py | 11 +++- openaddr/conform.py | 10 ++- openaddr/tests/__init__.py | 10 ++- openaddr/tests/cache.py | 41 ++++++++++++ openaddr/tests/conform.py | 55 +++++++++++++++- openaddr/tests/data/us-pa-bradford-0.json | 63 +++++++++++++++++++ .../tests/data/us-pa-bradford-count-only.json | 1 + .../tests/data/us-pa-bradford-metadata.json | 47 ++++++++++++++ 9 files changed, 236 insertions(+), 8 deletions(-) create mode 100644 openaddr/tests/data/us-pa-bradford-0.json create mode 100644 openaddr/tests/data/us-pa-bradford-count-only.json create mode 100644 openaddr/tests/data/us-pa-bradford-metadata.json diff --git a/README.md b/README.md index 5437249c..73b281b4 100644 --- a/README.md +++ b/README.md @@ -77,3 +77,9 @@ diverge from upstream and must be preserved across upstream merges: conformed output row, so named-but-unnumbered properties stay individually addressable. It is a geocodio-specific addition and is not part of the upstream OpenAddresses schema. +- **Non-finite geometry is never written out** — ESRI services report a null + point geometry as the string `"NaN"`, which reached the output as + `"coordinates": [NaN, NaN]`. Invalid GeoJSON per RFC 7946, and unparseable by + strict decoders. `openaddr/cache.py` now skips those features on download and + `openaddr/conform.py` treats any non-finite WKT coordinate as no geometry, + with `allow_nan=False` on the output writer as a backstop. diff --git a/openaddr/cache.py b/openaddr/cache.py index 939ba5f6..c577aadb 100644 --- a/openaddr/cache.py +++ b/openaddr/cache.py @@ -47,6 +47,13 @@ def traverse(item): else: yield item +def coordinate_is_usable(value): + "Test a single coordinate for a finite number; ESRI reports null geometry as the string \"NaN\"" + try: + return math.isfinite(float(value)) + except (TypeError, ValueError): + return False + def request(method, url, **kwargs): if urlparse(url).scheme == 'ftp': if method != 'GET': @@ -440,8 +447,8 @@ def download(self, source_urls, workdir, source_config): if not geom: raise TypeError("No geometry parsed") - if any((isinstance(g, float) and math.isnan(g)) for g in traverse(geom)): - raise TypeError("Geometry has NaN coordinates") + if any(not coordinate_is_usable(c) for c in traverse(geom.get('coordinates'))): + raise TypeError("Geometry has non-finite coordinates") shp = shape(geom) row[GEOM_FIELDNAME] = shp.wkt diff --git a/openaddr/conform.py b/openaddr/conform.py index 48ca46f4..993834e7 100644 --- a/openaddr/conform.py +++ b/openaddr/conform.py @@ -46,6 +46,10 @@ def gdal_error_handler(err_class, err_num, err_msg): # We add columns to the extracted CSV with our own data with these names. GEOM_FIELDNAME = 'oa:geom' +# WKT coordinates that json.dumps would write as NaN or Infinity, which no strict +# GeoJSON parser will accept. ESRI services report a null point geometry this way. +NONFINITE_COORDINATE_PATTERN = re.compile(r'\b(nan|inf(inity)?)\b', re.IGNORECASE) + ADDRESSES_SCHEMA = [ 'hash', 'number', 'street', 'unit', 'building_name', 'city', 'district', 'region', 'postcode', 'id', 'accuracy' ] BUILDINGS_SCHEMA = [ 'hash', 'height', 'levels'] PARCELS_SCHEMA = [ 'hash', 'pid' ] @@ -897,7 +901,7 @@ def row_extract_and_reproject(source_config, source_row, disable_centroids=False if source_row.get(GEOM_FIELDNAME.replace('GEOM', 'geom')) is not None: del out_row[GEOM_FIELDNAME.replace('GEOM', 'geom')] - if source_geom == "POINT (nan nan)": + if source_geom is not None and NONFINITE_COORDINATE_PATTERN.search(source_geom): out_row[GEOM_FIELDNAME] = None return out_row @@ -1251,7 +1255,7 @@ def row_convert_to_out(source_config, row): "Convert a row from the source schema to OpenAddresses output schema" geom = row.get(GEOM_FIELDNAME, None) - if geom == "POINT EMPTY" or geom == '': + if geom == "POINT EMPTY" or geom == '' or (geom is not None and NONFINITE_COORDINATE_PATTERN.search(geom)): geom = None output = { @@ -1328,7 +1332,7 @@ def transform_to_out_geojson(source_config, extract_path, dest_path): # For every row in the extract for extract_row in reader: out_row = row_transform_and_convert(source_config, extract_row) - dest_fp.write(json.dumps(out_row) + '\n') + dest_fp.write(json.dumps(out_row, allow_nan=False) + '\n') def conform_cli(source_config, source_path, dest_path, disable_centroids=False): "Command line entry point for conforming a downloaded source to an output CSV." diff --git a/openaddr/tests/__init__.py b/openaddr/tests/__init__.py index e00127d6..5dfecc58 100644 --- a/openaddr/tests/__init__.py +++ b/openaddr/tests/__init__.py @@ -1072,10 +1072,16 @@ def test_single_ny_orange(self): output_path = join(dirname(state_path), state["processed"]) with open(output_path, encoding='utf8') as input: - rows = list(map(json.loads, list(input))) + contents = input.read() + self.assertNotIn('NaN', contents) + + rows = list(map(json.loads, contents.splitlines())) + + # One of the nine cached features has a NaN geometry and is skipped + self.assertEqual(len(rows), 8) self.assertEqual(rows[0]['properties']['id'], u'') self.assertEqual(rows[0]['properties']['number'], u'434') - self.assertEqual(rows[0]['properties']['hash'], u'8a72112f6b1404d8') + self.assertEqual(rows[0]['properties']['hash'], u'a0127261b0619522') self.assertEqual(rows[0]['properties']['city'], u'MONROE') self.assertEqual(rows[0]['geometry']['coordinates'], [-74.1926686, 41.3187728]) self.assertEqual(rows[0]['properties']['street'], u'') diff --git a/openaddr/tests/cache.py b/openaddr/tests/cache.py index ad2d947e..5944a347 100644 --- a/openaddr/tests/cache.py +++ b/openaddr/tests/cache.py @@ -100,6 +100,21 @@ def response_content(self, url, request): if request.method == 'POST' and body_qs.get('resultOffset') == ['0']: local_path = join(data_dirname, 'us-al-cullman-0.json') + if (host, path) == ('bcmaps.bradfordco.org', '/arcgis/rest/services/Address_Points/MapServer/0'): + qs = parse_qs(query) + + if qs.get('f') == ['json']: + local_path = join(data_dirname, 'us-pa-bradford-metadata.json') + + if (host, path) == ('bcmaps.bradfordco.org', '/arcgis/rest/services/Address_Points/MapServer/0/query'): + qs = parse_qs(query) + body_qs = parse_qs(request.body) + + if qs.get('returnCountOnly') == ['true']: + local_path = join(data_dirname, 'us-pa-bradford-count-only.json') + if request.method == 'POST' and body_qs.get('resultOffset') == ['0']: + local_path = join(data_dirname, 'us-pa-bradford-0.json') + if local_path: type, _ = mimetypes.guess_type(local_path) with open(local_path, 'rb') as file: @@ -400,6 +415,32 @@ def test_handle_feature_server_with_lat_lon_in_conform(self): self.assertTrue('oa:geom' in all_data[0]) self.assertEqual(all_data[0]['oa:geom'], 'POINT (-86.82960553 34.18671398)') + def test_skip_esri_features_with_nan_geometry(self): + """ ESRI Caching Will Skip Features Whose Geometry Is The String "NaN" """ + task = EsriRestDownloadTask('us-pa-bradford') + c = SourceConfig(dict({ + "schema": 2, + "layers": { + "addresses": [{ + "name": "default", + "conform": { + "number": "Add_Number", + "street": "FullAddr" + } + }] + } + }), "addresses", "default") + + with httmock.HTTMock(self.response_content): + output_path = task.download(["https://bcmaps.bradfordco.org/arcgis/rest/services/Address_Points/MapServer/0"], self.workdir, c) + + with open(output_path[0], 'r') as file: + all_data = list(csv.DictReader(file)) + + self.assertEqual(len(all_data), 2) + self.assertEqual([row['FullAddr'] for row in all_data], ['148 DESMOND ST APT 202', '150 DESMOND ST']) + self.assertNotIn('nan', ' '.join(row['oa:geom'] for row in all_data).lower()) + class TestFromProtocolStringHeaders (unittest.TestCase): def test_headers_reach_url_download_task(self): diff --git a/openaddr/tests/conform.py b/openaddr/tests/conform.py index 15cfc580..fc943488 100644 --- a/openaddr/tests/conform.py +++ b/openaddr/tests/conform.py @@ -26,7 +26,7 @@ row_fxn_postfixed_unit, row_fxn_remove_prefix, row_fxn_remove_postfix, row_fxn_chain, row_fxn_first_non_empty, row_fxn_constant, row_fxn_map, - row_canonicalize_unit_and_number, conform_cli, + row_canonicalize_unit_and_number, conform_cli, transform_to_out_geojson, convert_regexp_replace, normalize_ogr_filename_case, is_in, geojson_source_to_csv, ogr_source_to_csv, check_source_tests, ZipDecompressTask, DecompressionError, elaborate_filenames @@ -78,6 +78,59 @@ def test_row_convert_to_out(self): } }, r) + def test_row_convert_to_out_nonfinite_geometry(self): + "ESRI sources report a null point geometry as NaN, which isn't valid GeoJSON" + d = SourceConfig(dict({ + "schema": 2, + "layers": { + "addresses": [{ + "name": "default", + "conform": { "street": "s", "number": "n" } + }] + } + }), "addresses", "default") + + for wkt in ("POINT (NaN NaN)", "POINT (nan nan)", "POINT Z (nan nan nan)"): + r = row_convert_to_out(d, {"s": "DESMOND ST", "n": "148", GEOM_FIELDNAME: wkt}) + self.assertIsNone(r["geometry"], wkt) + + def test_transform_to_out_geojson_skips_nonfinite_geometry(self): + "A NaN coordinate must never reach the output GeoJSON" + d = SourceConfig(dict({ + "schema": 2, + "layers": { + "addresses": [{ + "name": "default", + "conform": { "street": "s", "number": "n" } + }] + } + }), "addresses", "default") + + workdir = tempfile.mkdtemp(prefix='testConform-') + try: + extract_path = os.path.join(workdir, 'extract.csv') + dest_path = os.path.join(workdir, 'out.geojson') + + with open(extract_path, 'w', encoding='utf-8') as file: + writer = csv.DictWriter(file, fieldnames=['s', 'n', GEOM_FIELDNAME]) + writer.writeheader() + writer.writerow({'s': 'DESMOND ST', 'n': '148', GEOM_FIELDNAME: 'POINT (NaN NaN)'}) + writer.writerow({'s': 'DESMOND ST', 'n': '150', GEOM_FIELDNAME: 'POINT (-76.51522 41.97925)'}) + + transform_to_out_geojson(d, extract_path, dest_path) + + with open(dest_path, 'r', encoding='utf-8') as file: + contents = file.read() + + self.assertNotIn('NaN', contents) + rows = [json.loads(line) for line in contents.splitlines()] + finally: + shutil.rmtree(workdir) + + self.assertEqual(len(rows), 2) + self.assertIsNone(rows[0]['geometry']) + self.assertEqual(rows[1]['geometry'], {'type': 'Point', 'coordinates': [-76.51522, 41.97925]}) + def test_row_merge(self): d = SourceConfig(dict({ "schema": 2, diff --git a/openaddr/tests/data/us-pa-bradford-0.json b/openaddr/tests/data/us-pa-bradford-0.json new file mode 100644 index 00000000..81151554 --- /dev/null +++ b/openaddr/tests/data/us-pa-bradford-0.json @@ -0,0 +1,63 @@ +{ + "objectIdFieldName": "OBJECTID", + "globalIdFieldName": "", + "geometryType": "esriGeometryPoint", + "spatialReference": { + "wkid": 4326, + "latestWkid": 4326 + }, + "fields": [ + { + "name": "OBJECTID", + "alias": "OBJECTID", + "type": "esriFieldTypeOID" + }, + { + "name": "Add_Number", + "alias": "Add_Number", + "type": "esriFieldTypeInteger" + }, + { + "name": "FullAddr", + "alias": "FullAddr", + "type": "esriFieldTypeString", + "length": 254 + } + ], + "features": [ + { + "attributes": { + "OBJECTID": 18, + "Add_Number": 148, + "FullAddr": "148 DESMOND ST APT 206" + }, + "geometry": { + "x": "NaN", + "y": "NaN" + } + }, + { + "attributes": { + "OBJECTID": 33828, + "Add_Number": 148, + "FullAddr": "148 DESMOND ST APT 202" + }, + "geometry": { + "x": -76.51535, + "y": 41.97911 + } + }, + { + "attributes": { + "OBJECTID": 33840, + "Add_Number": 150, + "FullAddr": "150 DESMOND ST" + }, + "geometry": { + "x": -76.51522, + "y": 41.97925 + } + } + ], + "exceededTransferLimit": false +} \ No newline at end of file diff --git a/openaddr/tests/data/us-pa-bradford-count-only.json b/openaddr/tests/data/us-pa-bradford-count-only.json new file mode 100644 index 00000000..ef96c462 --- /dev/null +++ b/openaddr/tests/data/us-pa-bradford-count-only.json @@ -0,0 +1 @@ +{"count": 3} \ No newline at end of file diff --git a/openaddr/tests/data/us-pa-bradford-metadata.json b/openaddr/tests/data/us-pa-bradford-metadata.json new file mode 100644 index 00000000..7a18c4de --- /dev/null +++ b/openaddr/tests/data/us-pa-bradford-metadata.json @@ -0,0 +1,47 @@ +{ + "currentVersion": 10.51, + "id": 0, + "name": "Addresses", + "type": "Feature Layer", + "geometryType": "esriGeometryPoint", + "objectIdField": "OBJECTID", + "displayField": "FullAddr", + "advancedQueryCapabilities": { + "supportsPagination": true, + "supportsStatistics": true, + "supportsOrderBy": true, + "supportsDistinct": true + }, + "supportsStatistics": true, + "supportsAdvancedQueries": true, + "extent": { + "xmin": -76.6, + "ymin": 41.6, + "xmax": -76.2, + "ymax": 41.9, + "spatialReference": { + "wkid": 4326, + "latestWkid": 4326 + } + }, + "fields": [ + { + "name": "OBJECTID", + "alias": "OBJECTID", + "type": "esriFieldTypeOID" + }, + { + "name": "Add_Number", + "alias": "Add_Number", + "type": "esriFieldTypeInteger" + }, + { + "name": "FullAddr", + "alias": "FullAddr", + "type": "esriFieldTypeString", + "length": 254 + } + ], + "capabilities": "Map,Query,Data", + "maxRecordCount": 1000 +} \ No newline at end of file