fix(conform): never write NaN coordinates to the output GeoJSON

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.

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

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."

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'')

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):

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,

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
+}

openaddr/tests/data/us-pa-bradford-count-only.json

@@ -0,0 +1 @@
+{"count": 3}

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
+}