update openapi de-serialize - #607
Merged
Merged
Conversation
Line-wrapping only, from the GeoJSON test fixtures in the previous commit: the Point geometries are longer than the placeholder dictionaries they replaced and crossed the line limit.
The path source resolved only in a workspace checkout. CI clones this repo alone,
so instantiation failed before any test ran:
ERROR: expected package `PowerCoreOpenAPIModels [b7b40286]` to exist at path
/home/runner/work/InfrastructureSystems.jl/PowerOpenAPIModels/PowerCoreOpenAPIModels.jl
That is what took down all five test jobs; the format check was passing throughout.
`subdir` is required because PowerCoreOpenAPIModels is a subdirectory package, and a
branch rev rather than a version because PowerOpenAPIModels is not registered.
KNOWN BROKEN UNTIL PowerOpenAPIModels jd/openapi_regen MERGES TO main. main is 19
commits behind it and does not yet carry model_DataSource.jl, added in 09ff27b, so
loading InfrastructureSystems currently raises:
UndefVarError: `DataSource` not defined in `PowerCoreOpenAPIModels`
Pinned to main deliberately rather than to the feature branch, so no follow-up edit
is needed once that merge lands; it resolves itself at that point with no change here.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates InfrastructureSystems’ OpenAPI (de)serialization support for supplemental attributes and tightens GeographicInfo handling by validating that stored geo_json is valid GeoJSON.
Changes:
- Added
from_openapi/to_openapiconverters forGeographicInfoandDataSource, including UTC normalization forDataSourcetimestamps. - Added GeoJSON parsing-based validation for
GeographicInfo.geo_json. - Updated tests to use valid GeoJSON
Pointobjects instead of ad-hoc dictionaries.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_system_data.jl | Updates GeographicInfo test data to valid GeoJSON. |
| test/test_supplemental_attributes.jl | Updates multiple GeographicInfo test fixtures and mutation assertions to GeoJSON Point shape. |
| test/test_serialization.jl | Updates serialization test fixture to valid GeoJSON Point. |
| src/openapi_converters.jl | New OpenAPI ↔ IS conversion layer for GeographicInfo and DataSource. |
| src/InfrastructureSystems.jl | Imports new deps and includes the new converter file. |
| src/geographic_supplemental_attribute.jl | Adds GeoJSON validation to GeographicInfo construction. |
| Project.toml | Adds new dependencies and a [sources] path override for PowerCoreOpenAPIModels. |
Suppressed comments (1)
src/geographic_supplemental_attribute.jl:31
- The docstring claims the stored value "stays the caller's own dictionary", but
GeographicInfostoresgeo_json::Dict{String, Any}. If callers pass aDict{String, T}withT != Any, it will be converted (copied) toDict{String, Any}during construction, so the original dictionary identity is not preserved. Consider clarifying the wording to avoid implying identity preservation.
The parse result is discarded — `GeoJSON` reads coordinates as `Float32`, and the stored
value stays the caller's own dictionary rather than a lossy round-trip of it.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
daniel-thom
approved these changes
Aug 15, 2026
Parsing alone did not establish validity. GeoJSON.read accepts {"type": "Point"}
with no coordinates at all, and {"type": "Feature"} with no geometry, returning an
object whose required member is `nothing` rather than raising. Both are invalid
GeoJSON, and both passed validate_geo_json, so a geometry with no coordinates could
be stored and only fail in whatever consumed it later.
The parse is now followed by a completeness check on the parsed object, dispatched
per shape so each accessor only ever meets the type it is defined for.
GeometryCollection needs its own method because it is an AbstractGeometry but is
keyed on `geometries` rather than `coordinates`. The `::Any` fallback returns true
deliberately: parsing succeeded and there is no rule for that shape, which is not
grounds to reject it.
Rejections name the parsed type and the member that is missing, rather than
repeating the generic "not valid GeoJSON" message, since the input did parse.
Verified against 13 cases: the seven valid shapes still pass, the three previously
accepted holes now fail, and the three pre-existing rejections are unchanged. Run
against the definitions extracted from this file rather than through the package,
because InfrastructureSystems still cannot load while PowerCoreOpenAPIModels is
pinned to main; this wants a real suite run once that pin resolves.
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
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.
No description provided.