Skip to content

Import the files we can read instead of discarding the whole upload - #57

Draft
njakobsen wants to merge 3 commits into
masterfrom
partial-spatial-import
Draft

Import the files we can read instead of discarding the whole upload#57
njakobsen wants to merge 3 commits into
masterfrom
partial-spatial-import

Conversation

@njakobsen

@njakobsen njakobsen commented Jul 31, 2026

Copy link
Copy Markdown
Member

Homalco Connect had 24 referral submissions whose spatial import had failed, going back to January 2024. Pulling every one of those uploads down and running them through the importer showed that most of them held perfectly importable geometry the whole time — the imports were failing on our side, not on the data.

The core defect is that an import is all-or-nothing across the files uploaded to one record. #spatial_feature_imports flat-maps create_all over every file, so an ImportError from any single one unwinds #update_features! entirely. Submission 1691 uploaded a valid shapefile alongside a ZIP of zoning-map PNGs and lost its footprint to the PNGs. Parse failures that only surface when an importer's features are first read — a shapefile archive missing its .shx — escaped through #import_features the same way.

This takes the approach #53 established for KML NetworkLinks and applies it one level up. That fix skipped an unusable part of a file and recorded a warning; the same reasoning applies to an unusable file within an upload. A source that can't be read is replaced by an Importers::UnreadableFile standing in for it, the reason travels through the existing warnings accumulator attributed to the file it came from, and the remaining sources still import. Where every source is unreadable the import ends up empty and raises EmptyImportError with those reasons as the explanation.

Three further defects surfaced in the same review, each of which failed an upload that was entirely importable:

  • Nested archives were invisible. Unzip.extract never looked inside a nested archive and FILE_PATTERNS didn't match .kmz, so a ZIP of per-layer ZIPs or a ZIP holding a KMZ read as empty — both routine when a proponent forwards the archive they were emailed. Unzip.paths now unwraps nested .zip and .kmz entries, bounded by MAX_NESTING_DEPTH, and only once the archive has yielded nothing usable, so archives that already import keep their behaviour exactly.
  • Sources collided during extraction. Every source shared one directory, so two KMZs on one record — both holding a doc.kml — collided on the second extraction and failed the import with a rubyzip error.
  • A KML holding only a <GroundOverlay> produced nothing, with no explanation. An overlay drapes a georeferenced picture over the map instead of describing an area, so it is now skipped with a warning the way a <NetworkLink> is. Four of Homalco's failed submissions attached the same WMS overlay export from the BC catalogue, believing it was their project boundary.

Import messages are rewritten for the person who uploaded the file, since they are usually the only one who can fix it — an archive with no map data names the file types it did contain, an incomplete shapefile names the missing component and explains that a shapefile is a set of files that travel together, and a missing upload no longer prints a server filesystem path. This matters more than it used to because connect_engine#484 now shows these messages to the submitter rather than staff alone.

Two latent bugs turned up on the way. Shapefile::create_all referenced the bare constant INVALID_ARCHIVE, which isn't in that class's lookup path, so it raised NameError instead of the intended ImportError whenever a model imported with 'Shapefile' directly. #feature_update_error read last_error off an unordered relation, so a record that had failed more than once reported whichever row Postgres happened to return.

Verification

bundle exec rspec — 263 examples, 0 failures (19 pending, all pre-existing and unrelated).

Beyond the suite, this was verified against the restored Homalco production database with the real uploaded files in place, running the actual #update_features! on the actual records:

Before After
Submissions importing 0 of 24 9, totalling 1,089 features
Failing with a specific, actionable reason 0 13 of the remaining 15

1636 recovers 674 features, 1287 recovers 251, 831 recovers 91, 1331 recovers 48. The two that still fail without a specific reason (474, 988) contain genuinely empty documents, where "No mapped areas could be imported" is the whole truth.

Deploying

Version bumped to 3.11.0 — minor rather than patch, since import behaviour changes and Importers::UnreadableFile is new. Needs a manual push to rubygems; connect_engine.gemspec requires ~> 3.11 and its Gemfile.lock re-resolves once it lands.

Not included

Feature-level containment. A single invalid geometry still discards its whole file — the same disease one level further down, and the cause of the remaining Homalco failures (Ring Self-intersection, geometry requires more points, point array must contain 0 or >1 elements). skip_invalid already exists for it and is off by default, and make_valid — which likely repairs the self-intersections — is set only in two client-specific places. Both change what gets imported on every deployment, so they deserve a deliberate decision rather than being folded in here.

Refs https://github.com/culturecode/stolo_connect/issues/1962

njakobsen and others added 3 commits July 31, 2026 13:20
A referral submission whose spatial upload includes one file we can't read imported nothing at all, even when the files beside it held perfectly good geometry. Reviewing Homalco Connect's failed imports, 24 submissions were stuck in this state going back to January 2024, and most of them contained importable footprints the whole time. `#spatial_feature_imports` flat-maps `create_all` across every uploaded file, so an `ImportError` from any one of them unwound `#update_features!` entirely; parse failures that only surface when an importer's features are first read (a shapefile archive missing its `.shx`) escaped through `#import_features` the same way. Submission 1691 uploaded a valid shapefile alongside a ZIP of zoning map PNGs and lost its footprint to the PNGs.

Instead of aborting on the first source we can't read, each source is now contained: a file that fails to open is replaced by an `Importers::UnreadableFile` standing in for it, a file that fails while parsing has the reason recorded against it, and either way the remaining sources still import. The reason is carried as a warning through the existing `warnings` accumulator added with the KML NetworkLink fix, so it reaches the user attributed to the file it came from. When every source is unreadable the import ends up empty and raises `EmptyImportError` with those reasons as the explanation, rather than a bare "no features found".

Three further defects surfaced in the same review, each of which made an importable upload fail on its own:

`Unzip.extract` never looked inside a nested archive, and `Importers::File::FILE_PATTERNS` didn't match `.kmz`, so a ZIP of per-layer ZIPs or a ZIP holding a KMZ read as empty — both routine when a proponent forwards the archive they were emailed. `Unzip.paths` now unwraps nested `.zip` and `.kmz` entries and searches those too, bounded by `MAX_NESTING_DEPTH`, and only once the archive has yielded nothing usable so archives that already import keep their current behaviour exactly.

Every source shared one extraction directory, so two KMZs on one record — both containing a `doc.kml` — collided on the second extraction and failed the import with a rubyzip error. Each source now unpacks into its own directory beneath the import's tmpdir, which the existing cleanup still removes.

A KML holding only a `<GroundOverlay>` produced zero features with nothing to explain why. An overlay drapes a georeferenced picture over the map instead of describing an area, so it is now skipped with a warning the way a `<NetworkLink>` is. Four of Homalco's failed submissions attached the same WMS overlay export from the BC catalogue, believing it was their project boundary.

Import messages are rewritten for the person who uploaded the file, since they are the only one who can fix it: an archive with no map data names the file types it did contain ("It contains 6 PDF files"), an incomplete shapefile names the missing component and explains that a shapefile is a set of files that travel together, and the NetworkLink and overlay warnings no longer read as singular/plural mismatches. `Shapefile::create_all` also referenced the bare constant `INVALID_ARCHIVE`, which isn't in that class's lookup path and raised `NameError` instead of the intended `ImportError` whenever a model imported with `'Shapefile'` directly.

Verified against the real files from all 24 stuck Homalco submissions: 9 now import, recovering 1,089 features, and every one that still can't import reports a specific reason.

Refs culturecode/stolo_connect#1962

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HLgkg1oN6dkKfLzhKyDUFt
`#feature_update_error` read `last_error` off `failed_feature_update_jobs.first` with no ordering, so a record that had failed more than once reported whichever row Postgres happened to return. Homalco submission 1636 has two failed import jobs with different reasons, and the banner could show either.

Instead the failed jobs are ordered by `failed_at` descending, so the reason shown always belongs to the most recent attempt.

Refs culturecode/stolo_connect#1962

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HLgkg1oN6dkKfLzhKyDUFt
…tter sees

A file recorded on a spatial record but absent from disk raised `Errno::ENOENT`, which `#update_features!` re-raised as an `ImportError` carrying the message verbatim — `No such file or directory @ rb_sysopen - /home/sysadmin/.../private_uploads/referral_submission/spatial_files/1740/SavaryIslandLandUseDesignation.zip`. That aborted the whole import over one absent file, and now that failures are shown to the person who uploaded them it would also put a server filesystem path in front of an external user.

Instead `Errno::ENOENT` is contained per file like any other unreadable source, and `Importers::UnreadableFile` maps it to "This file is no longer available on the server. Please upload it again." — which is both the actionable instruction and free of anything internal.

Refs culturecode/stolo_connect#1962

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HLgkg1oN6dkKfLzhKyDUFt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant