Document Ecto jsonb usage, fix two codec spec gaps - #40
Merged
Merged
Conversation
Spectral already has everything needed to store typed values in a jsonb
column: `:pre_encoded` and `:pre_decoded` meet Ecto at the map boundary,
where the database driver does its own JSON serialization. Nothing in the
library changed here, but that was not written down anywhere, so it kept
getting reported as missing.
Adds a README section covering the `Ecto.ParameterizedType` wrapper and
three ways to handle a column whose type varies per row:
- a self-describing union, with the discriminator inside the document
- a type reference taken from a sibling column at call time
- a discriminating codec, for one lookup instead of a linear scan
Each pattern has a support module and tests, including a real JSON round
trip standing in for the database driver.
Three traps found while writing the tests and now documented:
- `Ecto.Type` dispatches to parameterized types before its own nil
shortcut, so `cast/2`, `load/3` and `dump/3` all receive nil for a NULL
column. Plain `Ecto.Type` modules never see nil.
- Recursive codec calls need a resolved type node from
`Spectral.TypeInfo.get_type/3`. Passing a `{:type, name, arity}` tuple
fails with a `type_mismatch` naming an unexpected type.
- `schema/5` needs a catch-all returning `:continue`, or generating a
schema for any other type in the codec module raises. The callback is
declared to return a map, but spectra accepts `:continue` here.
No Ecto dependency is added. None of the patterns need one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvE2JYfi1tDoVW8pfrc4UU
andreashasse
commented
Sep 11, 2026
Review feedback on the JSONB docs, plus the two library gaps it surfaced.
Library fixes:
- `Spectral.Codec.encode/5`, `decode/5` and `schema/4` now resolve a
`{:type, name, arity}` or `{:record, name}` reference, as their
`sp_type_or_ref()` specs have always claimed. Previously only a resolved
`sp_type()` node worked; a reference fell through and failed with a
`type_mismatch` naming a type the caller never asked about.
- The `schema/5` callback is declared `map() | :continue`. Returning
`:continue` for unhandled types was already documented in the README and
supported by spectra, but the callback spec said `map()`.
Both are covered by `test/spectral_codec_helpers_test.exs`, which fails
against the previous implementation.
Review changes:
- Drop the discriminating codec pattern, its support module and its tests.
It carried more weight than the point it made.
- Give the tag field a struct default, so `%Circle{radius: 1.5}` encodes with
`kind` filled in. No encode helper needed.
- Rename the test helper to `json_round_trip`. It shows the dumped value
survives JSON serialization; it is not a database and does not touch Ecto.
Say so in the moduledoc instead of implying otherwise.
- Retitle the README section to "Spectral and Ecto", and leave the
`Ecto.ParameterizedType` wrapper to a separate `spectral_ecto` library that
can depend on Ecto and test against a real Postgres.
- Rename the sibling-column example function to `decode_payload/1`.
- Correct the stale `schema/6` heading in the codec docs to `schema/5`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvE2JYfi1tDoVW8pfrc4UU
andreashasse
commented
Sep 11, 2026
- Open the section with what a reader needs first: Ecto encodes and decodes jsonb values as Elixir maps, and `:pre_encoded` / `:pre_decoded` convert those maps to and from your types. The explanation of why the driver hands over a map was in the way. The library paragraph now follows directly. - Drop the two notes on writing the `Ecto.ParameterizedType` wrapper by hand. With `spectral_ecto` as the answer, there is no reason to coach people through rolling their own. Both points are recorded in #41, where whoever builds it will need them. - Remove `test/spectral_jsonb_test.exs` and its fixtures. The behaviour it covered is Spectral's existing encode and decode, and nothing in it touched Ecto or a database, so the real tests belong in `spectral_ecto`. The codec helper tests stay. They cover library changes, not jsonb. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WvE2JYfi1tDoVW8pfrc4UU
andreashasse
marked this pull request as ready for review
September 14, 2026 05:53
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
README examples and callback documentation contain the noted inconsistencies and need correction before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Documents Ecto jsonb usage and fixes codec reference resolution and schema callback typing.
Changes:
- Resolves type and record references in codec helpers.
- Allows schema callbacks to return
:continue, with regression tests. - Adds Ecto guidance and changelog updates.
File summaries
| File | Summary |
|---|---|
test/support/codec_ref_module.ex |
Adds codec reference fixtures. |
test/spectral_codec_helpers_test.exs |
Tests reference resolution and schema fallback. |
README.md |
Adds Ecto documentation. Five nit findings remain regarding callback contracts, stale arities, example validity, nested struct qualification, and Ecto schema placement (1–2 votes each). |
mix.exs |
Excludes new fixtures from generated documentation. |
lib/spectral/codec.ex |
Resolves references and updates callback specifications. |
CHANGELOG.md |
Records the fixes and documentation additions. |
Review details
Suppressed comments (3)
README.md:388
Ecto.Type.dump/3itself returns{:ok, term}or:error, not a bare map. This comment makes the Ecto callback contract misleading; describe the map as the value encoded inside the dump callback instead.
# Ecto.Type.dump/3 returns a map for Ecto to store
README.md:345
- The new heading and paragraph use
schema/5, but the following existing "Type Parameters" section still says the schema callback isschema/6at README.md:482. Please update that reference too so the README does not present both callback arities.
The `schema/5` callback is optional. If a codec module does not export it, calling `Spectral.schema/3` for a type owned by that codec raises `{:schema_not_implemented, Module, TypeRef}`. Once you do export it, it receives every type defined in the codec module, so give it a catch-all clause returning `:continue` for the types the codec does not handle, exactly as with `encode/6` and `decode/6`.
README.md:473
- This code block is not valid as presented:
MyApp.Notificationis closed beforefieldanddef decode_payload, sofield/3and the function definition are at top level,__MODULE__resolves to the wrong module, and the shown module does not enableEcto.Schema. Keep these declarations and the function inside the Ecto schema module, or explicitly label and show them as an insertion fragment.
def decode_payload(%__MODULE__{kind: kind, payload: payload}) do
Spectral.decode(payload, MyApp.Notification, kind, :json, [:pre_decoded])
end
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
All five are documentation correctness, verified against the code:
- The custom codec example still used the pre-0.12.0 signatures:
`encode/7`, `decode/7` and `schema/6` with a separate `params`
argument. Copying it produced callbacks that do not match the declared
behaviour. Updated to the current `encode/6`, `decode/6`, `schema/5`,
matching the `Spectral.Codec` moduledoc, and given the `schema/5`
catch-all the section itself recommends.
- The `type_parameters` section described `params` as a callback
argument. It was removed in 0.12.0; codecs read it with
`:spectra_type.parameters/1` on `target_type`.
- `%Circle{}` in the shapes example sat outside `MyApp.Shapes` with no
alias, so it would not compile. Now fully qualified.
- The sibling-column example closed its module before the `field`
declarations and `decode_payload/1`, leaving them at top level with
`__MODULE__` resolving to the wrong module. They now live in an
`Ecto.Schema` module, which is also where a reader would put them.
- The dump comment implied `Ecto.Type.dump/3` returns a bare map. It
returns `{:ok, term}`, so the comment now describes the map as what
goes inside that tuple.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvE2JYfi1tDoVW8pfrc4UU
…jsonb-plugin-n6uf8b # Conflicts: # CHANGELOG.md
…jsonb-plugin-n6uf8b # Conflicts: # CHANGELOG.md
andreashasse
pushed a commit
that referenced
this pull request
Sep 14, 2026
Merges in the Ecto jsonb docs and codec spec fixes (#40), which landed on main after this branch. Only CHANGELOG.md conflicted — folded both Fixed lists and the new Added section into the shared 0.14.0 heading. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTMXez4gy2fHp6HgovtfGG
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.
Why
Spectral already handles
jsonbcolumns. Ecto encodes and decodes those values as Elixir maps, and:pre_encoded/:pre_decodedconvert those maps to and from your types. That was never written down, so it kept getting reported as a missing feature.Library fixes
Working through this surfaced two places where
Spectral.Codecpromised one thing and did another. Both are fixed here.Recursive helpers ignored type references.
Spectral.Codec.encode/5,decode/5andschema/4are spec'd to takesp_type_or_ref(), which issp_type() | sp_type_reference(), and a reference is exactly{:type, name, arity}or{:record, name}. Passing one did not work. It fell through the traversal as an unrecognised term and surfaced as atype_mismatchnaming a type the caller never asked about. The helpers now resolve references, mirroring spectra's own privateresolve_type_ref/2.The
schema/5callback could not decline a type. The README already said to return:continuefor types a codec does not handle, andspectra_json_schemamatches oncontinueand falls through. Only the@callbackdisagreed, declaringmap(). Nowmap() | :continue, with the doc noting that once implemented, the callback receives every type in the module.Covered by
test/spectral_codec_helpers_test.exs. Verified it fails against the previous implementation: 3 of 7 tests fail without the change, all 7 pass with it.Documentation fixes
The codec docs had drifted since the arity change in 0.12.0. Anyone copying the custom codec example got callbacks that do not match the declared behaviour, which is a two-release-old bug rather than a tidy-up, so it is in the changelog under Fixed.
MyGeoModuleexample definedencode/7,decode/7andschema/6with a separateparamsargument. It now usesencode/6,decode/6andschema/5withcaller_type_infoandtarget_type, matching theSpectral.Codecmoduledoc, plus theschema/5catch-all the section below it recommends.paramsas a callback argument. It was removed in 0.12.0; codecs readtype_parameterswith:spectra_type.parameters/1ontarget_type.Optional schema/6 callbackheading is nowschema/5.New documentation
A README section, "Spectral and Ecto", covering the map boundary and two ways to handle a column whose type varies per row:
:mapThe tagged variants carry their tag as a struct default, so
%Circle{radius: 1.5}encodes withkindfilled in and callers never write it by hand.The section also flags a silent failure mode worth knowing about. Unions are first-match-wins and extra JSON keys are ignored, so an untagged variant whose fields are a subset of another's swallows documents meant for the later variant and drops the extra keys with no error. The literal tag is what prevents it, which is why the docs push it rather than mention it.
What is deliberately not here
The
Ecto.ParameterizedTypewrapper. It belongs in a separatespectral_ectolibrary, tracked in #41, which can depend on Ecto normally and test against a real Postgres. Keeping it out avoids optional-dependency gymnastics here, and avoids pulling indecimaland its unfixed MEDIUM advisory (CVE-2026-32686) for every contributor.Tests for the jsonb patterns. An earlier revision had them. They exercised Spectral's existing encode and decode without touching Ecto or a database, so they were removed in review. The meaningful tests need Ecto and Postgres, which is
spectral_ecto's job.Verification
make format && make cipasses on the current head: 224 tests, Credo strict clean, no duplication, Dialyzer 0 errors.mix test --coverreports 14TypeInfoEquivalenceTestfailures. Those reproduce on unmodifiedmainand are unrelated. Cover-compilation rewrites the BEAM those tests read abstract code from.Merges from main
mainpicked up #42 (non-empty list shorthand) while this was open. Merged in; the only conflict was both branches adding entries under the same### Fixedheading in the changelog, resolved by keeping both.🤖 Generated with Claude Code
https://claude.ai/code/session_01WvE2JYfi1tDoVW8pfrc4UU