Skip to content

fix(concerto-core): serialize maps with cross-namespace concept values - #1279

Open
mttrbrts wants to merge 3 commits into
mainfrom
fix/serialize-cross-namespace-map-values
Open

fix(concerto-core): serialize maps with cross-namespace concept values#1279
mttrbrts wants to merge 3 commits into
mainfrom
fix/serialize-cross-namespace-map-values

Conversation

@mttrbrts

Copy link
Copy Markdown
Member

Problem

When a Concerto map has a concept value type that is imported from another namespace, (de)serialization breaks. Both the serializer and the populator resolved the value concept's declaration by searching only the map's own model file:

mapDeclaration.getModelFile().getAllDeclarations().find(decl => decl.name === value.getType())

An imported value concept isn't in that model file, so the lookup returns undefined, and:

  • Serializer.toJSON throws TypeError: Cannot read properties of undefined (reading 'accept').
  • Serializer.fromJSON silently returns the raw JSON object instead of a populated Concept, which breaks round-tripping (toJSON(fromJSON(x))).

Reproduction

namespace org.acme.unit@1.0.0
concept MonetaryUnit { o String code  o Integer scale }
namespace org.acme.registry@1.0.0
import org.acme.unit@1.0.0.{MonetaryUnit}
map UnitMap { o String  o MonetaryUnit }
concept Registry { o UnitMap units optional }

Building a Registry with a populated units map and calling serializer.toJSON(...) throws. Same-namespace maps (e.g. the existing Rolodex/Directory tests) are unaffected.

Fix

Resolve the value declaration across all loaded models rather than just the map's own model file:

  • jsongenerator.ts (visitMapDeclaration): resolve the map's declared value type honouring imports, via ModelFile.getFullyQualifiedTypeName(...) + ModelManager.getType(...). This works for both factory-created and populator-created value instances (some of which don't expose getFullyQualifiedType()).
  • jsonpopulator.ts (processMapType): search declarations across every model file, preserving the existing .find(...)/fallthrough semantics so same-namespace behaviour is unchanged.

Tests

Added to test/serializer/maptype/serializer.js:

  • serialize → deserialize → re-serialize (round-trip) of a map whose value concept is imported from another namespace (the previously-crashing case).

Full test/serializer/** suite passes (235 tests); eslint . is clean on the changed files.

Known related follow-up (not addressed here)

Serializer.fromJSON does not appear to run field-level validation on concept values inside a map — a map entry missing a required field is populated without error. That is a separate validator-depth issue (not caused by the declaration-lookup bug fixed here) and is left for a follow-up to keep this change focused.

🤖 Generated with Claude Code

Map (de)serialization resolved the value concept's declaration only within
the map's own model file. When the value concept is imported from another
namespace the lookup returned undefined, so:

- Serializer.toJSON threw "Cannot read properties of undefined (reading
  'accept')", and
- Serializer.fromJSON silently returned the raw JSON object instead of a
  populated Concept (breaking round-trips).

Resolve the value declaration across all loaded models: toJSON now resolves
the map's declared value type honouring imports via the ModelManager, and
the populator searches declarations across every model file.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes Concerto map (de)serialization when the map’s value concept type is imported from another namespace, by resolving the value declaration across loaded models instead of only within the map’s defining model file—restoring correct round-tripping behavior in concerto-core’s serializer.

Changes:

  • Update JSON generation for map entries to resolve the map value type using import-aware, model-manager-based lookup.
  • Update JSON population for map entries to find the correct declaration even when the value concept is imported from a different namespace.
  • Add a regression test covering serialize → deserialize → re-serialize for a cross-namespace map value concept.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/concerto-core/src/serializer/jsongenerator.ts Adjusts map value declaration resolution during serialization to support imported value concepts.
packages/concerto-core/src/serializer/jsonpopulator.ts Adjusts map value declaration lookup during deserialization to support imported value concepts.
packages/concerto-core/test/serializer/maptype/serializer.js Adds regression test for cross-namespace concept values in maps and round-trip behavior.

Comment thread packages/concerto-core/src/serializer/jsongenerator.ts Outdated
Comment thread packages/concerto-core/src/serializer/jsonpopulator.ts Outdated
… test

Review feedback (Copilot):
- jsongenerator: resolve the map value declaration from the instance's own
  fully-qualified type when available (preserving polymorphism for subclass
  values), falling back to the map's declared value type (honouring imports)
  for instances that do not expose a fully-qualified type.
- jsonpopulator: resolve the key/value declaration via the model manager using
  the map model file's import-aware fully-qualified name, wrapped in try/catch
  to preserve the previous "not found => treat as scalar" behaviour, instead of
  scanning every declaration by short name (which could match the wrong
  namespace or misclassify a scalar).

Build/CI fix:
- modelmanager test: assert the stable failure message for a bad github:// URL
  with a regex instead of pinning HTTP status 400, since GitHub now returns 404
  for the missing resource, causing an unrelated CI failure.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <code@rbrts.uk>
@mttrbrts
mttrbrts requested a review from a team July 13, 2026 09:26
The "Coveralls Finished" (parallel-finished) step blocked PRs whenever the
Coveralls webhook returned a 5xx/timeout, even though coverage was computed and
uploaded successfully per-matrix. Add fail-on-error: false to match the existing
per-matrix upload step so external Coveralls outages no longer block CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <code@rbrts.uk>
mttrbrts added a commit to accordproject/models that referenced this pull request Jul 16, 2026
…ries (#188)

* feat(money): add money@1.0.0 and reference registries

Proposes org.accordproject.money@1.0.0 (see #186):

- MonetaryAmount: structurally back-compatible with money@0.3.0 (doubleValue +
  currencyCode). currencyCode is widened from the CurrencyCode enum to String,
  which still validates existing "USD"-style instances while removing the
  hard-coded fiat ticker list.
- MonetaryUnit: open unit { code, scheme=iso4217, scale } — carries its own
  scale so amounts are self-describing across fiat and non-fiat schemes without
  hard-coding tickers.
- FixedPointMonetaryAmount: precision-safe amount, value (Long) minor units of
  a MonetaryUnit; value = value × 10^(-unit.scale). No discriminator, no string
  parsing.
- Removes DigitalMonetaryAmount / DigitalCurrencyCode (crypto now via
  MonetaryUnit.scheme).

Adds org.accordproject.money.reference@1.0.0 (CurrencyRegistry, a code ->
MonetaryUnit map) and reference data under data/ (iso4217, erc20 sample,
examples). Data lives outside src/ so the model build does not process it.

Distribution/helpers tracked in #187. Serializing the registries needs the
concerto-core fix in accordproject/concerto#1279.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* chore(money): add SUI to erc20 reference sample

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* chore(money): move SUI to a native-coin (slip44) registry

SUI is a native Sui L1 coin, not an ERC-20 token. Move it out of the erc20
sample into a new slip44 registry of native L1 coins (BTC, ETH, SOL, SUI)
keyed by SLIP-0044 ticker.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* feat(money): make FixedPointMonetaryAmount.value an exact integer string

Replace `Long value` with `IntegerAmount` (scalar String, regex
/^-?(0|[1-9][0-9]*)$/) so the mantissa is exact at any magnitude and scale,
not capped at 2^53 and never truncated by a JSON parser. amount =
value × 10^(-unit.scale).

Clients convert once at the boundary and do exact integer arithmetic — the
recommended path is a thin adapter over Dinero.js v2 (bigint) or big.js,
rather than a bespoke money library.

Example instances now carry string values, incl. an exact 1.5 ETH
(1500000000000000000 wei, scale 18).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* feat(money): type MonetaryAmount.currencyCode as an ISO 4217 scalar

Replace `String currencyCode` with `scalar CurrencyCode extends String
regex=/^[A-Z]{3}$/`, constraining the ISO 4217 alphabetic format (three
uppercase letters) without enumerating the code list. Every money@0.3.0
CurrencyCode enum value is three uppercase letters, so existing instances
validate unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* refactor(money): rename amount concepts to convey precision

Rename the two amount concepts to a self-documenting antonym pair on the
quality axis:

- MonetaryAmount      -> ApproximateMonetaryAmount (Double; display/estimates,
                         not for arithmetic)
- FixedPointMonetaryAmount -> PreciseMonetaryAmount (exact integer mantissa)

Properties are unchanged, so 0.3.0 instances remain structurally compatible;
only the $class name changes on migration. Example instances renamed to match.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* feat(money): address review — unit identifier, non-negative scale, canonical zero

Incorporates review feedback on #186:

- MonetaryUnit gains an optional `identifier` (CAIP-19 asset id, token contract
  address, DTI, …). `code` alone is ambiguous for token schemes — the same
  ticker exists on many chains/contracts — so the instance can now carry its
  own disambiguator rather than relying solely on a registry lookup. The USDC
  example carries its canonical CAIP-19 asset id.
- `MonetaryUnit.scale` is constrained non-negative (`range=[0,]`); a negative
  scale would silently multiply the amount.
- `IntegerAmount` regex tightened to /^(0|-?[1-9][0-9]*)$/ so "-0" is rejected,
  keeping the canonical-form promise (one spelling of zero).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* refactor(money): drop "Monetary" from concept names

The namespace already supplies domain context, and the model spans tokens and
coins (not money in the traditional sense), so the "Monetary" qualifier is
dropped from the concept names:

- ApproximateMonetaryAmount -> ApproximateAmount
- PreciseMonetaryAmount     -> PreciseAmount
- MonetaryUnit              -> Unit

Namespace stays org.accordproject.money (continuity with @0.3.0; a sensible
domain boundary). Properties unchanged, so back-compat is unaffected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

* refactor(money): rename to BigInteger scalar and unscaledValue property

Adopt Java BigDecimal terminology for the precise amount:

- scalar IntegerAmount        -> BigInteger
- PreciseAmount.value         -> unscaledValue

amount = unscaledValue × 10^(-unit.scale), matching BigDecimal's
(unscaledValue, scale) model.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Matt Roberts <matt@rbrts.uk>

---------

Signed-off-by: Matt Roberts <matt@rbrts.uk>
Co-authored-by: Matt Roberts <matt@rbrts.uk>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open with no activity. Remove the stale label or comment to keep it active. Only items with maintainer engagement are auto-closed.

@github-actions github-actions Bot added the Stale label Jul 29, 2026
@mttrbrts mttrbrts removed the Stale label Aug 5, 2026
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.

2 participants