fix(csvjson): a blank --lat/--lon crashes instead of writing a null geometry - #1355
fix(csvjson): a blank --lat/--lon crashes instead of writing a null geometry#1355VXNCXNX wants to merge 2 commits into
Conversation
Catch TypeError in addition to ValueError when parsing coordinates, guard geometry membership test with feature.get(), and omit bbox when no coordinates are present. Fixes errors on rows with blank/unparseable lat/lon values.
geometry_for_row tested the coordinates for truthiness, so a latitude or longitude of 0 was treated as absent. On master that produced a TypeError once the bbox accumulator reached the None geometry; with the null-geometry handling in this branch it would instead have silently written geometry: null for a valid location. The equator, the prime meridian and null island are real coordinates, so test against None.
|
Self-review found a second defect on this same line, pushed as a follow-up commit.
if lon and lat:so a latitude or longitude of On The first commit of this PR removed that crash, which would have turned it into something worse: a valid point silently written as Now Blank and unparseable values still produce Added |
What's broken
csvjson --lat/--lonaborts on any row whose coordinates are blank.A blank coordinate is the ordinary case in a geocoded export: one address failed to resolve and the rest are fine. One such row loses the whole file.
After:
"geometry": nullis what RFC 7946 says an unlocated feature is, so the row is kept and marked rather than dropped.Three defects on one seam
float(None)raisesTypeError, notValueError. The existingexcept ValueErrorwas already there to tolerate a bad coordinate, and it does catchfloat("abc"). But an empty cell arrives asNone, not"", so it takes a different exception type and escapes the handler that exists to deal with exactly this.'coordinates' in feature['geometry']runs against aNone.geometry_for_rowreturnsNoneimplicitly for a row with no coordinates, so the membership test raisesTypeError: argument of type 'NoneType' is not iterable.--no-bboxskips this path entirely, which is why the shape can look like it works.A bbox of nulls is not valid GeoJSON. With no usable coordinates anywhere, the output was
"bbox": [null, null, null, null]. RFC 7946 requires bbox members to be numbers, and bbox itself is optional, so the right answer is to omit it.The fix
Widen the existing catch to
(TypeError, ValueError), guard the bbox accumulator withfeature.get('geometry'), and add anis_set()check so the bbox key is only emitted when at least one coordinate was seen.Rows with valid coordinates are unaffected, and so is
--no-bbox.Verification
Two cases in
tests/test_utilities/test_csvjson.pywith two small fixtures, one file mixing a good row with a blank one, one where every row is blank. They assert the bbox still covers the good row, that the blank row's geometry isnull, and that no bbox key appears when nothing was located.Reverting all three hunks fails both with the original
TypeError: float() argument must be a string or a real number, not 'NoneType'. Reverting only the bbox hunk fails with'bbox' unexpectedly found in {... 'bbox': [None, None, None, None] ...}, so each hunk is pinned separately.pytest tests/test_utilities/test_csvjson.pyis 26 passed. The full suite is4 failed, 349 passed, and the same 4 csvstat locale failures occur on a clean tree here.