Import places from a GPX file of waypoints #152
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-212-gpx-place-import"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Extends task 211's CSV import (ADR-0127) to accept a GPX file of waypoints too, per task 212 / ADR-0128.
PlaceImportRow,PlaceImportRunner,PlaceImportReport,ImportReportState,ImportReportBanner, …) — one pipeline, two parsers.CsvPlaceImportParserkeeps its name;GpxPlaceImportParseris new.PlaceFormPanel, format chosen by file extension; an extension that does not match the file's actual content is refused rather than fed to the wrong parser.XmlReader.Create/XDocument.Load, never a bareXDocument.Parse— confirmed directly that the bare overload expands a DTD entity on the pinned .NET 10 SDK, and that the hardened path refuses it. The explicitDtdProcessing.Prohibit/XmlResolver = nullsettings match this runtime's own defaults and are kept as defence in depth, not because they override an unsafe default that exists today.PlaceImportReport.ItemNoun(default"row", GPX passes"waypoint") is how the report avoids ever calling a waypoint a row, without renamingRowNumberitself — see ADR-0128's Alternatives for why.PlaceImportRunner.MaxItems);CsvPlaceImportParser.MaxRowsis now an alias.Partially superseded by 0128); README index regenerated viascripts/regenerate-adr-readme.cs, diff confined to the table rows.Correction (review round 1): the original DTD-hardening doc comments and ADR-0128 wrongly claimed the guard was mutation-tested by removing the explicit
DtdProcessing.Prohibit/XmlResolver = nullsettings. On the pinned .NET 10 SDK those settings already matchXmlReaderSettings's own defaults, so that mutation would not have reddened the test, and per re-derivation it does not. The guard that actually discriminates the behaviour is the choice to parse viaXmlReader.Create/XDocument.Loadrather than a bareXDocument.Parse— confirmed by substituting the bare overload and watching both DTD tests genuinely redden, then restoring it. Doc comments and ADR-0128 corrected to describe that mutation; the code itself was never insecure.Full solution: 1892 tests, 1878 passed, 14 skipped (10 E2E need a browser/live services, 4 pre-existing unrelated WebUI skips), 0 failed.
Verdict: changes needed
Reviewed at
04ada4c. The rename is genuinely behaviour-preserving (CsvPlaceImportParser.csdiff is a pure type-rename,CsvPlaceImportParserTests.csuntouched,PlaceImportRunnerTests.csdiff is rename-only,ImportReportState's scoped DI registration and every bUnitAddSingleton(new ImportReportState())call site are consistent with pre-existing behaviour,ButtonWeightCoverage's exemption was correctly updated to the new file/class name, the strayHandleCsvImportCompletedAsynccomment is fixed, ADR-0127 and the README index are annotated correctly with no prose loss).ItemNoun/RowsNotAttemptedwording, the extension-mismatch guard, the sharedMaxItems/MaxRowsbound, and the GPX parsing itself (namespace handling,lat/lonas attributes, no fallback name,ADR-0112null-not-empty description, tracks/routes counted not imported) all check out against the ticket and are exercised by tests that actually discriminate the behaviour.One real problem, in the one place this PR needed to be most careful: the DTD/XXE hardening claim.
GpxPlaceImportParser.ParsesetsDtdProcessing = DtdProcessing.ProhibitandXmlResolver = nullexplicitly, and the doc comment onParse_FileCarryingADtd_IsRefusedRatherThanExpandedclaims this was "mutation-tested by removingDtdProcessing = DtdProcessing.Prohibit... and confirming this test reddens." I checked this against the pinned runtime (.NET 10):new XmlReaderSettings()already defaultsDtdProcessingtoProhibit, andXmlReaderSettings.XmlResolveralready defaults tonull(the property is write-only in modern .NET specifically because there's no non-null default to read back). Removing either explicit assignment leaves the framework default, which still refuses the DTD — the test cannot have reddened the way the comment describes, on this or any recent .NET.The code is not insecure — it correctly builds an
XmlReaderSettingsand parses viaXmlReader.Create(...) → XDocument.Load(xmlReader)rather than a bareXDocument.Parse(gpxText), and I confirmed separately thatXDocument.Parseon the same string does expand the entity (&lol;resolves tolol) — that's the mutation that actually matters, and this code correctly avoids it. But the specific claim in the doc comment (and by extension the PR body's "both proved by tests watched failing before being trusted") describes a mutation that could not have produced the stated result, given .NET's own defaults. This is exactly the "watched red" trust failure CLAUDE.md's Tests section names three times already — please either re-verify against a mutation that's actually discriminating (e.g. replacing theXmlReader.Create/XDocument.Loadpair with a bareXDocument.Parse(gpxText), which the test suite doesn't currently guard against at all) and correct the comment to describe that, or soften the claim to what's actually true: the settings are defensive/self-documenting and match the framework default rather than override an unsafe one.Everything else here is solid. Once the DTD-hardening claim is corrected (or the test strengthened to genuinely discriminate the
XDocument.Parseregression), this is mergeable.@ -0,0 +75,4 @@using var stringReader = new StringReader(gpxText);using var xmlReader = XmlReader.Create(stringReader, new XmlReaderSettings{DtdProcessing = DtdProcessing.Prohibit,new XmlReaderSettings()already defaultsDtdProcessingtoProhibitandXmlResolvertonullon .NET 10 (verified locally) — these two explicit assignments don't change behaviour relative to the framework default. That's fine as defensive/self-documenting code, but it means the actual guard against a default-configured parse is the choice to go throughXmlReader.Create(...) → XDocument.Load(xmlReader)at all, rather than a bareXDocument.Parse(gpxText)(which I confirmed does expand a DTD entity on this runtime). See the comment on the DTD test for the follow-on issue this causes.@ -0,0 +255,4 @@/// accepted rather than refused), then restoring it./// </summary>[Fact]public void Parse_FileCarryingADtd_IsRefusedRatherThanExpanded()This mutation-testing claim doesn't hold: removing
DtdProcessing = DtdProcessing.Prohibitfrom theXmlReaderSettingsleaves the framework default, which is alsoProhibiton .NET 10 — the test would not redden. Same forXmlResolver = null(the framework default is alreadynull). I checked both against the pinned SDK. The mutation that would actually matter — swapping the hardenedXmlReader.Create/XDocument.Loadpath for a bareXDocument.Parse(gpxText)— isn't guarded by any test here, and I confirmed it does let the entity expand. Please correct the "watched failing" claim (it describes a mutation that can't have produced the stated result) and, ideally, add a test or comment that actually pins down theXDocument.Parseregression this code protects against.Verdict: mergeable
Reviewed at
914c7f2. This is a re-review of the correction alone; the round-1 findings (rename behaviour-preserving, DI/bUnit registrations,ButtonWeightCoverageexemption, ADR-0127 annotation, README index, GPX parsing/extension guard/ItemNounwording/sharedMaxItemsbound) all still hold at this head — nothing outside the DTD-hardening doc/ADR text changed.I performed the mutation myself rather than accepting the corrected claim: built a scratch harness with the same two payloads as
Parse_FileCarryingADtd_IsRefusedRatherThanExpandedandParse_FileCarryingADtdWithAnExternalEntity_IsRefusedRatherThanFetched, on .NET 10.0.110 (the pinned SDK, 10.0.100, isn't installed here — a patch-level gap I don't expect to move XML reader defaults, but noting it for the record).new XmlReaderSettings().DtdProcessingisProhibit, andXmlResolveris a write-only property with no way to read back a non-null default. RemovingDtdProcessing = DtdProcessing.Prohibit/XmlResolver = nulland going throughXmlReader.Create/XDocument.Loadwith plain default settings still refused both the internal-entity and external-entity payloads — the original claim really doesn't hold, exactly as the correction says.XDocument.Parse(gpxText)for theXmlReader.Create/XDocument.Loadpair let both payloads' entities expand (&lol;resolved tolol,&xxe;resolved to empty rather than refusing) — i.e.IsRefusedwould readfalseand both named tests genuinely redden. This matches what the doc comments, the test remarks and ADR-0128 now say, and it's the mutation that actually discriminates the guard.Spot-checked two more "watched red" claims for the same failure pattern (a mutation that leaves equivalent behaviour in place):
PlaceFormPanel.HandleImportFileSelectedAsync— ADR-0128 itself flags that the.gpx-with-non-XML-content direction is also caught byGpxPlaceImportParser's own malformed-XML refusal, which made me suspicious the bUnit tests might not actually discriminate the panel-level sniff. Traced it through: if the sniff were deleted, GPX-extension/CSV-content falls through toGpxPlaceImportParser.Parse, which still refuses (XmlException→ "not valid XML"), but that message doesn't contain the literal.gpxthe test asserts on, and CSV-extension/GPX-content falls through toCsvPlaceImportParser.Parse, which also still refuses (no header match) but without the literal.csvthe test asserts on. BothPlaceFormPanel_CsvExtensionButGpxContent_IsRefusedRatherThanFedToTheCsvParserandPlaceFormPanel_GpxExtensionButCsvContent_IsRefusedRatherThanFedToTheGpxParserdo genuinely redden if the sniff is removed, just via message wording rather than theIsRefusedflag. Holds up.catch (XmlException)inGpxPlaceImportParser.Parse) — removing the catch lets the exception propagate unhandled out ofParse, which fails the test with an unhandled exception rather than an assertion mismatch. Straightforwardly discriminating; no equivalent-behaviour trap here.Nothing else raised. CI (
run #634, SHA914c7f2) was still reportingrunningwith the known negative-duration artefact when I checked; the identical pipeline succeeded in 5m23s for the prior commit on this PR, so I'm not treating the in-flight status as a blocker per the review brief.