chore: sync fork with upstream batch-machine 10.2.0 - #1
Merged
Conversation
guess_url_file_extension() made its own GET request to sniff the Content-Type before the real download, but never carried any custom headers. On a header-gated host (e.g. one requiring Referer) that pre-flight request would fail even after headers are wired through the rest of the download path.
…nloads DownloadTask.__init__ already accepted a headers dict, but from_protocol_string() - the only production call site - never passed one through, so self.headers was always just the default User-Agent. EsriRestDownloadTask also built its EsriDumper without headers despite pyesridump already supporting extra_headers.
Wires the source-supplied 'headers' dict (added to the schema separately in openaddresses/openaddresses) into the download path. conform() deliberately does not forward headers: it re-downloads from the OA-owned cache artifact, not the contributor's host.
Covers: from_protocol_string() forwarding headers to URLDownloadTask and EsriRestDownloadTask, default User-Agent survives alongside custom headers (and can be overridden), headers reaching both the extension-guessing pre-flight request and the real download request, and EsriRestDownloadTask passing headers to EsriDumper as extra_headers.
Fill in the CHANGELOG's PR link once this is merged.
Review feedback on openaddresses/openaddresses#8306: wrapping headers in a container leaves room for query params (e.g. a token) later without a second schema migration. No source uses the flat key yet, so this costs nothing now.
Further review feedback on openaddresses/openaddresses#8306: iandees agreed 'request' is shorter and reads better than http_request_settings. Source key is now request.headers.
…-image-digest Pin gdal base image to digest
…p-headers Send custom HTTP request headers when downloading a source
…rce_to_csv output
…json-nonuniform-properties Fix geojson_source_to_csv crash on non-uniform feature properties
…regexp-replace Fix regexp replace to return empty string when pattern does not match
…multisurface Linearize curve geometries before exporting to WKT
…7-kml-support Add KML support to conform pipeline
…lat-lon-flip Fix lat/lon axis flip for sources with an explicit srs tag
…35-nested-zip Extract nested zip files regardless of conform file filter
Brings the fork up to upstream 10.2.0. Conflicts resolved by keeping both sides: upstream's KML support note and curve-geometry linearization, plus the fork's --disable-centroids guard and Geocodio fork notes. Claude-Session: https://claude.ai/code/session_01JWLFBWRfhWhLUqFFqbPCGm
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
fix(conform): never write NaN coordinates to the output GeoJSON
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The fork was 25 commits behind
upstream/master(now at v10.2.0) and 8 ahead. This merges upstream in, preserving every fork-only patch. It's the prerequisite for the NaN-geometry fix, which stacks on this branch.Merged rather than rebased, matching the shape of the previous sync (
024c1fc6).Changes
Two conflicts, both resolved by keeping both sides:
README.md-- upstream's KML support sentence alongside the fork-notes section.openaddr/conform.py-- upstream's newHasCurveGeometry()linearization block landed directly above theif source_config.layer == "addresses"line the fork patched, so git couldn't auto-merge. The result keeps both: linearization andand not disable_centroids.All fork-only work survives:
--disable-centroids(2d31d44e,666cf974,b1bb4cda,b0880837)process_one.py:410,conform.py:707,858,956,__init__.py:105989ffd20)Dockerfile:7, auto-mergedbuilding_namesupport (9ce4f6b0,5145ed48)conform.py:49,1225Verification
--disable-centroidshas no test coverage, so it was exercised end-to-end after the merge rather than just checked structurally:The KML sources new in 10.2.0 route through
ogr_source_to_csv, which already acceptsdisable_centroids, so the flag covers them too.Full suite via the CI path (
docker run machine): 128 tests, OK.