fix(geoservices): preserve imported calendar dates (#4833) - #4839
mikemcdougall wants to merge 9 commits into
Conversation
…nly-20260914 # Conflicts: # tests/dotnet/Honua.Db.Postgres.Tests/Features/Import/GeoservicesImportSubtypePersistenceTests.cs
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160910f2ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Claude Review: No major issues found. Reviewed commit: |
|
Claude Review: No major issues found. Reviewed commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7b55bd3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? DateTimeOffset.FromUnixTimeMilliseconds(element.GetInt64()) | ||
| : null, | ||
|
|
||
| "ESRIFIELDTYPEDATEONLY" => |
There was a problem hiding this comment.
When an import is auto-published, BuildInventoryField preserves this source type as esriFieldTypeDateOnly, but MigrationCatalogReconciler.NormalizeFieldType only recognizes the stripped token date and returns Unknown for dateonly (MigrationCatalogReconciler.cs:680-692). CollectFieldTypeFinding then skips the comparison entirely, so the reconciliation report can classify the import as passing even if the published field regresses to String or DateTime, failing to verify the calendar-date preservation introduced here; map dateonly to MetadataV2FieldType.Date in the reconciler.
Useful? React with 👍 / 👎.
| MetadataV2FieldType.Boolean => "esriFieldTypeSmallInteger", | ||
| MetadataV2FieldType.DateTime => "esriFieldTypeDate", | ||
| MetadataV2FieldType.Date => "esriFieldTypeDate", | ||
| MetadataV2FieldType.Date => "esriFieldTypeDateOnly", |
There was a problem hiding this comment.
For a canonical Date field whose DefaultValue uses the supported legacy epoch representation, this mapping now advertises esriFieldTypeDateOnly, but MapFieldInfo still copies the JsonElement through ConvertJsonElement unchanged at lines 637-650. Consequently the fields array in a normal /query response contains a numeric default for a field whose values are declared as ISO calendar-date strings; the MapServer mapper has the same raw-default behavior. Normalize date defaults through the calendar-date conversion when constructing field metadata.
Useful? React with 👍 / 👎.
| // rows. JSONB stores dates as either ISO strings (seeds) or epoch-ms longs | ||
| // (applyEdits); coerce both via the shared GeoServices date convention (matches query). | ||
| var dateFieldNames = GeoServicesFieldConventions.ResolveDateFieldNames(layer.Resource); | ||
| var temporalFieldTypes = GeoServicesFieldConventions.ResolveTemporalFieldTypes(layer.Resource); |
There was a problem hiding this comment.
When identify processes a dynamic joinTable, right-side Date fields are now advertised as esriFieldTypeDateOnly by BuildJoinedFields, but this temporal map contains only the left resource's unqualified field names. ApplyDynamicJoinAttributes emits right-side values as <qualifier>.<field>, so a right-side canonical date using the supported legacy epoch representation never matches this map and is returned as a number instead of yyyy-MM-dd. Include the right resource's temporal fields under their qualified names before coercing joined attributes.
Useful? React with 👍 / 👎.
|
Claude Review: No major issues found. Reviewed commit: |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var result in results.EnumerateArray()) | ||
| { | ||
| var attributes = result.GetProperty("attributes"); | ||
| attributes.GetProperty("right_parcels.day").GetString().Should().Be("2024-02-29"); | ||
| attributes.GetProperty("right_parcels.timestamp").GetInt64().Should().Be(epoch); | ||
| attributes.GetProperty("right_parcels.nullable_day").ValueKind.Should().Be(JsonValueKind.Null); | ||
| } |
1 participant
Pull Request
Issue Link
Closes #4833.
Subtype/dimension prerequisite #4830 is merged; this branch is updated against trunk.
Related SDK discovery defect: honua-io/honua-sdk-js#1718.
Summary
Importing an Esri date-only field stored it as text, so the helicopter sample's original DATE filter failed. Imports now store calendar dates as PostgreSQL DATE and publish date-only metadata and ISO values, while timestamp fields keep epoch-millisecond behavior. Grouped calendar statistics also return ISO dates consistently across server cultures, allowing the helicopter date selector to load.
Changes Made
Testing
Earlier import/protocol verification passed 7 PostGIS tests and 97 protocol tests. The current statistics follow-up passes all 13 targeted PostGIS/calendar cases (zero failures or skips), including six culture cases across both readers, the real imported DATE/grouped-count case, two subtype cases and four Z/M cases. The isolated build initially exposed a test asset-path issue; a hash-verified private copy of the repository seed and migration files allowed the same compiled tests to pass with --no-build --no-restore in 16.677 seconds. Scoped dotnet format and git whitespace checks pass. Broad pre-pr/AOT/architecture checks were not repeated locally; That evidence is tied to ef7b55b; the new review follow-up is pending verification.
Fresh local job abccf487f1c5 imports 1,963 rows with zero row failures. Public metadata returns esriFieldTypeDateOnly; original-case and normalized DATE filters both return 1,963. Independent fresh source/target comparison finds no key, non-OID attribute, topology or dimension differences across 3,926 vertices (max XY delta 4.49e-10 degrees; Z 7.30e-12). Source PBF independently confirms enum 14 with ISO strings. Installed SDK 0.1.9-beta.0 reads all 1,963 imported rows with the original native DATE filter and zero temporal-value errors. The published SDK still reports calendar fields as unknown; the coordinated installed SDK candidate e77171323 from honua-io/honua-sdk-js#1720 now passes discovered-schema DATE queries for all 1,963 records against both original Esri and this fresh Honua import. JSON/PBF selected attributes agree in Honolulu, UTC and UTC+14, with no binary fallback. This is candidate evidence, not a registry release.
The job remains NeedsReview: internal reconciliation sends its filter through the legacy raw-WHERE parser, which rejects DATE literals. Public query filtering succeeds; source-aware reconciliation remains #4826. The local evidence uses a recorded managed component overlay on an older host, not a fresh full-host build. The four-app conversion remains incomplete.
Gate Impact
Docs or Contract Impact
Release/Deploy Impact
Coordinate release with SDK PR honua-io/honua-sdk-js#1720 for DateOnly discovery and PBF attribute decoding. SDK PR #1720 has merged; this server PR is ready for review with the additional grouped-statistics correction. No production deployment is requested. The merged #4830 prerequisite is now on trunk; resolving the test-file merge conflict retained the exact tested date regression.
Breaking Changes
Canonical Date fields now advertise esriFieldTypeDateOnly and emit yyyy-MM-dd strings instead of esriFieldTypeDate epochs. Arrow represents them as Date32 rather than timestamps. Canonical DateTime behavior is unchanged. Existing imported text columns require a fresh import; no in-place data rewrite occurs.
Pre-PR Checklist
scripts/ci/pre-pr-check.shand all checks passedtype: description (#issue)OPENAPI_BREAKING_CHANGE_APPROVED? intentional Admin API break acknowledged for this PRAdditional wire-path validation
The queryTopFeatures JSON regression passed in CI for all three temporal response paths, and its review thread was resolved with the TRX evidence. Grouped statistics subsequently exposed a separate culture-dependent DateOnly.ToString fallback. Both Postgres readers now emit yyyy-MM-dd with invariant culture. The protected candidate backend returns dateofflight=2026-01-11 and count=1963; the converted app's calendar and source-aligned chart rendered against it. This is limited UI evidence: whole-aircraft selection still depends on the separately tracked SDK transport and server geometry-budget fixes, and the full four-app migration remains incomplete.
Calendar metadata and joined identify follow-up
Commit 85b7e9e recognizes esriFieldTypeDateOnly during catalog reconciliation, normalizes defaults through the shared temporal converter for FeatureServer and MapServer metadata, and normalizes qualified right-side dates/timestamps in dynamic joined identify results. New regressions cover rejected String/DateTime substitutions, defaults in buffered/streaming/top-features output, and joined identify plus dynamic-layer metadata. Focused Core/protocol tests and scoped formatting are queued; PR remains draft and the review findings are not yet marked resolved. Prior ef7b55b PR Gate and affected shards passed. Its documentation gate found generated capability pages stale on the CI merge; regeneration/check on the branch passes with no changes, so that CI failure is not claimed fixed.