Skip to content

test(data-access): add ElectroDB Service construction CI gate#1637

Merged
rainer-friederich merged 4 commits into
mainfrom
chore/electrodb-service-construction-test
May 28, 2026
Merged

test(data-access): add ElectroDB Service construction CI gate#1637
rainer-friederich merged 4 commits into
mainfrom
chore/electrodb-service-construction-test

Conversation

@rainer-friederich
Copy link
Copy Markdown
Contributor

Why

This PR adds a small CI gate that would have caught the regression we just fixed in #1636 at PR time, not days later in a downstream consumer's CI run.

The original incident (3.72.0): #1622 introduced the Preflight entity with createdBy and error declared as type: 'map' without a properties block. ElectroDB validates schema shape eagerly at new Service(...) time and throws InvalidAttributeDefinition on that combination. Every existing unit test in this package passed because the v3 PostgREST runtime never instantiates an ElectroDB Service — so invalid attribute declarations are entirely invisible to this repo's CI.

The bug surfaced two days later in spacecat-api-service#2506 (the LLMO-5190 Phase 1 PR that bumped to 3.73.0) because test/controllers/fixes.test.js constructs the full v1 Service from EntityRegistry.getEntities() for indexing-side-effect reasons. By then, 3.72.0 was already published.

Fix shipped in #1636 → 3.73.1. This PR is the prevention.

What

  • Adds electrodb@3.5.0 as a devDependency of spacecat-shared-data-access (no runtime dep added — the test-only import).
  • Adds test/unit/models/base/electrodb-service-construction.test.js with two assertions:
    1. new electrodb.Service(EntityRegistry.getEntities()) does not throw.
    2. Every registered entity is exposed on the resulting service (sanity check that the registry/service mapping stays 1:1).

Why this version of electrodb

Pinned to electrodb@3.5.0 — the version transitively resolved by spacecat-api-service today. That's the version that actually exercises the failing code path in downstream CI, so this is the version we want to defend against. Bumping locally won't help if api-service stays on the older version.

Why no per-entity wiring

The test reads the full registry at runtime. Any entity added to entity.registry.js is automatically covered — no boilerplate per new entity. The cost is one ~720ms test.

Verified the test actually catches the bug

Ran it against origin/main (pre-#1636) — failed exactly as predicted:

ElectroError: Invalid "properties" definition for Map attribute: "undefined".
The "properties" definition must describe the attributes that the Map will accept
  at Attribute.buildChildMapProperties (/.../electrodb/src/schema.js:208:13)
  at new MapAttribute (/.../electrodb/src/schema.js:658:34)
  ...
  at new Service (/.../electrodb/src/service.js:168:14)

So this is not a tautological "the schema is what the schema is" check — it genuinely exercises the validation path that broke.

Out of scope (intentional)

  • Not converting the existing schemas to a "proper" ElectroDB Map (with properties blocks). That's the kind of cleanup that creates more drift than it removes — the validate functions already enforce shape, and adding redundant properties declarations would be dual-source-of-truth without buying anything.
  • Not adding a runtime ElectroDB dependency. The package's PostgREST runtime stays unchanged; this is strictly a test-time safety net for downstream consumers.

Dependency on #1636

This branch cherry-picks the #1636 fix commit so CI passes immediately. The two PRs are functionally independent — whichever lands first, the other rebases cleanly.

🤖 Generated with Claude Code

rainer-friederich and others added 2 commits May 28, 2026 13:28
…ce construction succeeds

The Preflight entity introduced in 3.72.0 declared `createdBy` and `error`
as `type: 'map'` without a `properties` schema. ElectroDB rejects that at
`new Service(...)` time with `InvalidAttributeDefinition`, which breaks
any downstream consumer that constructs the full v1 Service from the
entity registry (e.g. spacecat-api-service `fixes.test.js`). No consumer
hit it because api-service was pinned at 3.71.1 — surfacing now in the
LLMO-5190 Phase 1 PR that bumps to 3.73.0.

Aligns both attributes to `type: 'any'` to match the neighbor `result`
attribute on the same entity. The existing `validate` functions already
enforce the precise shape, so dropping the `map`+`properties` declaration
loses no schema-level enforcement.

Adds a type-assertion regression guard in
`test/unit/models/preflight/preflight.schema.test.js` so a future
revert to `type: 'map'` without `properties` fails fast.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Catches the class of regression that shipped in 3.72.0 and was fixed in
3.73.1 (PR #1636 — Preflight `createdBy` / `error` declared as `type: 'map'`
without a `properties` block). ElectroDB validates schema shape eagerly at
`new Service(...)` time, but this package never instantiates a Service in
its runtime (PostgREST path), so invalid attribute declarations slip
through unit tests and only surface days later in downstream consumers
that DO construct a v1 Service from the registry (e.g.
`spacecat-api-service/test/controllers/fixes.test.js`).

Adds `electrodb` as a devDependency and a unit test that builds
`new electrodb.Service(EntityRegistry.getEntities())` against the live
registry. Any new entity is automatically covered — no per-entity wiring
required.

The test was verified to fail on origin/main before the #1636 fix
(`InvalidAttributeDefinition: Invalid "properties" definition for Map
attribute`) and pass after, so it is not a tautological assertion.

Pinned to `electrodb@3.5.0` to match the version transitively resolved
by spacecat-api-service today — the version that actually exercises this
code path in production downstream CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

This PR will trigger a patch release when merged.

@GeezerPelletier
Copy link
Copy Markdown
Contributor

Great prevention layer — this is exactly the kind of safety net that should have been here from the start. The self-maintaining approach via the full registry is the right call; no per-entity boilerplate and any future addition is automatically covered. Thanks for closing the loop on this so quickly. LGTM 🙏

@rainer-friederich rainer-friederich enabled auto-merge (squash) May 28, 2026 11:40
@rainer-friederich rainer-friederich requested review from MysticatBot-Dev and removed request for MysticatBot-Dev May 28, 2026 12:06
@rainer-friederich rainer-friederich merged commit a969062 into main May 28, 2026
5 checks passed
@rainer-friederich rainer-friederich deleted the chore/electrodb-service-construction-test branch May 28, 2026 12:20
@solaris007
Copy link
Copy Markdown
Member

🎉 This PR is included in version @adobe/spacecat-shared-data-access-v3.73.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants