codegen: keep the module's type information #36
No reviewers
Labels
No labels
area:defold
area:docs
area:infra
area:protocol
area:testing
kind:bug
kind:chore
phase:breadth
phase:core
phase:hardening
phase:slice
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
yuris/spacetimedb-defold!36
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/25-codegen-types"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #25. Foundation for #21, #23, #26, #27, #28 and eventually #9 — none of which can work without column types.
gen-schema.shfetched the module's complete type information and kept only the column names.algebraic_typesat next to every name and went in the bin, along with unique constraints, indexes, and reducer parameter types.What is emitted now
Per table:
columnsandpkas before, plustypes(parallel array of tag strings),unique, andindexes. Per reducer: parametertypesalongsideparams. Per procedure: parameters andreturns.Type tags are flat lowercase strings — cheap for a table-driven decoder, and the shape a BSATN codec would need later:
bool,i8..i256,u8..u256,f32,f64,stringopt_<inner>,vec_<inner>identity,connection_id,timestamp,time_duration,uuid,schedule_atsum_<Name>/product_<Name>for named compound typesunknownfor anything unrecognised, never a guessed tagThe grammar was verified against a live server, not reasoned about
A throwaway
probemodule was published covering all 16 scalars,Option,Optioninside an array, all five magic products,ScheduleAt, a named enum and a named struct. Generated output:Zero
unknown. The probe db was deleted afterwards. This also settled two shapes the Rust source left ambiguous:Arrayserializes transparently ({"Array":{"U32":[]}}) andRefas a bare number ({"Ref":1}).Two things worth reviewer attention
No compound primary key exists. Publishing two
.primaryKey()columns is rejected by the server ("tableprofilehas multiple primary key annotations"). So the newprofiletable uses a compound unique constraint plus the btree the server demands to back it. Consequence:pkis now emitted only whenprimary_keyhas exactly one column — verified with a synthetic payload that a 2-columnprimary_keyemits nopkrather than silently naming the first. Compound uniqueness surfaces asunique = trueon the backing index, sinceuniqueholds plain single-column names.Procedures are now generated, contradicting a comment this repo has carried for a while. The old header claimed the schema endpoint does not report procedures. It does — they are under
misc_exports[].Procedure. That claim is now corrected.Verification
5 passed, 0 failed; stylua clean. 14 new assertions intest_fixtures.luaon type tags,profile's compound-index/unique/no-pk metadata, reducer param types includingopt_string/vec_string, and procedure param + return tags.chat_schema.luawas regenerated from a real publish, not hand-written.Falsifiability: restoring the pre-change fixture makes the suite fail, so the new assertions genuinely depend on the new metadata.
Deliberately not done
session.jsonlwas not re-captured — frame-count and frame-tag assertions depend on it and there is no protocol change here.messageis byte-identical, so both conformance harnesses and the smoke client are unaffected. Conformance not run. The column-order paragraph in the README is left alone — that is #32's job on another branch.gen-schema.sh fetched the whole module schema and threw away everything but column names, the first primary-key column and reducer parameter names. The type sat right beside every name it kept — {"name":{"some":"id"}, "algebraic_type":{"U64":[]}} — and special-type conversion, unique-index lookups, typed facades and any future BSATN codec all need it. A wire codec is impossible without column types, so this is the piece the rest of 1.0 is waiting on. Each table now carries `types` (one tag per column, in column order), `unique` and `indexes`; each reducer carries its parameter `types`. Procedures are emitted too, with parameter types and the type of what they return: they were never absent from this endpoint, they live under misc_exports[].Procedure rather than a `procedures` key, which is what the old header comment mistook for absence. Tags are flat lowercase strings — `u64`, `opt_string`, `vec_u8`, `identity`, `sum_Status` — not nested tables, because the consumer is a table-driven decoder: one string lookup per column, and the same tags will drive BSATN. Special types are detected STRUCTURALLY, by the magic field names sats puts inside them (`__identity__`, `__timestamp_micros_since_unix_epoch__` and friends), so a column *named* timestamp that holds a u64 is tagged u64 and a column named x that holds a Timestamp is tagged timestamp. Nothing gets a guessed tag; anything unrecognised comes out as `unknown`. Verified against a live 2.8.2 server rather than reasoned about: a probe module covering all sixteen scalars, Option, an Option inside an array, all five magic products, ScheduleAt, a named enum and a named struct generates zero `unknown`. It also settled two shapes the source alone left ambiguous — Array serializes transparently ({"Array":{"U32":[]}}) and Ref as a bare number. The new `profile` table gives the generated fixture metadata `message` cannot carry. Its key is a compound UNIQUE constraint rather than a compound primary key because the server rejects a module that annotates two ("table `x` has multiple primary key annotations"), so a compound pk is unreachable from a module today. The server also refuses a unique constraint no index supports, which is why compound uniqueness survives as `unique = true` on the backing index — the only place it can. For the same reason `pk` is emitted only for a single-column primary key: should the server ever allow two, this emits no `pk` rather than silently naming the first column. `message` itself is untouched, byte for byte: the captured frames, the cache tests and both conformance harnesses decode its rows positionally. Reducers and procedures are emitted one field per line, like tables, because a wide parameter list on one line exceeds the 120 columns stylua enforces and this fixture is committed; an empty section collapses to `{}` for the same reason.Review — automated reviewer, posted for the record
Review — PR #36
feat/25-codegen-typesVerdict: closes #25's acceptance criteria, but not mergeable as written. The design is right, the empirical rigour is real (the committed fixture genuinely reproduces byte-for-byte from a live publish — I checked twice), but the walker emits silently wrong tags in the direction that matters most, and three new header comments assert wire facts the live server contradicts. Everything below is reproduced against the 2.8.2 dev server; every throwaway db was deleted and
chatwas restored to["message"].What I ran: cloned the branch,
5 passed, 0 failed; republisheddev/server/index.jsto a throwaway db and regenerated — byte-identical to the committed fixture; ran the fulldev-server.shpipeline — fixture regenerated with zero diff; published 8 probe modules; drove av1.json.spacetimedbsocket to capture real row encodings; exercised the jq over 5 hostile synthetic payloads viafile://.Blockers
B1 — A user-defined type is silently tagged as a built-in
dev/tools/gen-schema.sh:119-134The structural checks run before the typespace-name check, so any module-declared type whose shape happens to collide is tagged as the built-in and its name is destroyed. Published probe, generated output:
t.object('FakeIdent', { __identity__: t.string() })identityproduct_FakeIdentt.object('FakeTs', { __timestamp_micros_since_unix_epoch__: t.string() })timestampproduct_FakeTst.enum('FakeOpt', ['some','none'])opt_unknownsum_FakeOptt.enum('FakeSched', ['Interval','Time'])schedule_atsum_FakeSchedt.array(FakeOpt)vec_opt_unknownvec_sum_FakeOptThe
some/nonecollision needs no exotic configuration: under the defaultCaseConversionPolicy,t.enum('Perhaps', ['Some','None'])canonicalises tosome/noneand I gotopt_unknownon a plain publish. A decoder will then try to read an Option payload out of aPerhaps. This is exactly the failure mode you asked me to hunt for — wrong, not absent — and #23/#26 will trust it.Why the fix is safe and one-shot: the real special types and real
Optionare never typespace entries. In the positive probe,.typescontained onlyContainers, Noop, Point, Scalars, Specials, Status—Identity/Timestamp/Optionall arrive inlined, so$name == ""for every genuine one. Every false positive arrives through aRefto a named entry. So: check the name first.Pair it with
:117— change the Ref-name fallback from($r | tostring)to"", otherwise an unnamed Ref target would now produceproduct_3, a tag naming a typespace index (unstable across module edits) baked into a committed file.Verified: with this patch the positive-grammar probe output is unchanged, all five false positives are fixed, and
chat_schema.luaregenerates byte-identical from a real publish. Zero regression.README.md:344-347needs the converse caveat too — it currently only documents the true-negative direction.B2 —
sum_<Name>/product_<Name>are dead ends, and the header says they aren'tdev/tools/gen-schema.sh:53-60False. Captured off a live socket (
InitialSubscription,v1.json.spacetimedb):stateis a named sum (Status{Idle,Busy}) and arrives as[1,{}]— ordinal, not name. Same forOption:[0,payload]/[1,{}], not{"some":…}. So the JSON protocol writes variants by ordinal too, andsum_Statusalone cannot be decoded — you needStatus's variant list in ordinal order. The generated module carries no type declarations at all, so #26 (full decoding) is blocked by precisely the gap this comment claims does not exist. A futureproduct_Pointis only decodable because nested products happen to arrive as named objects here; if one ever arrives positionally (which the same comment allows for), it is undecodable too.Fix: emit the typespace now, while it is cheap. Additive, so strictly it could land later — but the comment is the blocker, because it will send #26 down a dead end.
Name it
typespace, nottypes—typesis already the per-table key.Should-fix
S1 — A compound unique constraint silently disappears
dev/tools/gen-schema.sh:211[$uniques[] | select(. == $ix.cols)]compares column-id lists for exact equality. But the server stores them differently: a unique constraint is canonically sorted, a btree index preserves declared order. Verified:btree(['b','a'])→{"BTree":[1,0]};btree(['c','b','a'])→{"BTree":[2,1,0]}(order kept — good, and the generator faithfully emitscolumns = { "b", "a" })unique(['handle','realm'])→{"Unique":{"columns":[0,1]}}(sorted)The server accepts
unique(realm, handle)backed bybtree(handle, realm)— I published it. Result: constraint[0,1], index[1,0], no match,unique = trueis dropped. And since single-column-onlyunique(:74-75) means compound uniqueness surfaces nowhere else, the constraint vanishes from the generated schema entirely. #28 would see a plain non-unique index.Verified: recovers
unique = trueon the reverse-order case, no change tochat.S2 — The stylua limitation is understated ~10× and will break CI
dev/tools/gen-schema.sh:76-77The PR body says "a table with >120 columns of names". It is >120 characters. There is no
.stylua.toml, so the default width is 120, and CI runsstylua --check stdb dev/test(.forgejo/workflows/ci.yml:43) — the fixture is in scope.Current headroom: 12 characters.
chat_schema.lua:24is at 108. My 13-column probe table blew straight past it andstylua --checkfailed. So the next column added toprofile, or one longer index name, makes CI fail on a file nobody hand-edited, with a format error unrelated to the change. "Run stylua over the output if you commit it" doesn't help, becausedev-server.sh:~175is what commits it and it doesn't — andstyluaisn't even in theconformancedevshell (flake.nix:192-212).Fix: add
pkgs.styluato the conformance shell and pipe the regeneration through it:Verified:
stylua --stdin-filepathon the wide probe output produces a clean file; the{}collapse and-- GENERATEDheader survive. That also lets you delete the two{ }-workaround comments (:165-166,:258-259) and theparamlinesline-splitting rationale (:155-157).S3 —
t.resultandt.unitproduceunknown, and the "no anonymous types" claim is falsedev/tools/gen-schema.sh:53-55Falsified by the positive probe.
t.result(t.string(), t.u32())→ inline anonymous{"Sum":{"variants":[{"name":{"some":"ok"}…},{"name":{"some":"err"}…}]}}→unknown.t.unit()→ inline{"Product":{"elements":[]}}→unknown. Both are first-classtbuilders, so a user hitsunknownon an ordinary column.unknownis at least honest, unlike B1. Butunitis trivially exact (empty product), andok/erris as structurally detectable assome/none. Addunit, and eitherresult_<ok>_<err>or an explicit "Result is not tagged, by choice" note. Delete the false clause either way.S4 — The unit-payload comment mixes two encodings and gets both wrong
dev/tools/gen-schema.sh:62-63{"U64":[]}is AlgebraicType schema JSON — true and relevant.{"none":[]}is presented as a value encoding and is wrong twice: anonevalue is[1,{}](ordinal + empty object). Since this file is where the repo records its protocol facts, and #21/#26 will read it, split the two encodings apart or drop the value half.Design assessment (the shape being frozen)
Parallel
typesarray — keep. Rows arrive positionally in the captured session, sotypes[i]aligned withcolumns[i]matches the decode loop exactly; keying by column name would add a lookup and lose the order that is the whole reason this generator exists.Flat string tags — keep. A decoder is
for i, tag in ipairs(def.types), andopt_/vec_prefix stripping is two string ops.opt_vec_opt_vec_stringround-tripped correctly through four levels. Not awkward.uniqueas single-column names — keep, and it correctly includes the pk (message.unique = { "id" }), which is what #28 wants.Compound uniqueness on
indexes[].unique— the weak joint. It conflates "this index is unique" with "these columns are jointly unique", it is order-fragile (S1), and it makes #28 read two places. I would still keep it rather than putting nested lists inunique— but fix S1 and say in the header that compound uniqueness lives only onindexes.pkomitted for a compound primary key — correct, and better-supported than the PR argues.stdb/schema.lua:12already documents omittingpk, andstdb/cache.lua:58-65already falls back to a field-wise scan (with its ownponytail:note about the O(n) cost). Emitting a column list would breakschema.lua:26-35andcache.lua:41, both of which treatpkas a scalar string. So this is the right answer for the right reason — the reason is just stated as "unreachable today" when it is in fact "already a supported first-class case".Missing: the typespace (B2). That is the one thing I would add while it is cheap.
Robustness of the jq — it fails in the worse direction
You asked which failure this produces. Wrong tags, not malformed files. Every reachable defect (B1, S1, S3) is a silently-wrong or silently-missing value in a file that parses perfectly. Malformed output is essentially unreachable. Details:
{"Bogus":[]}unknown, parses{"Ref":99}unknown, parses{}/nullasalgebraic_typeunknown, parses"unnamed", correct type, parsesprimary_keycolumns = {}, nopk, parses"or\luastrat first; it is rightunfinished string near '"new')typespace[0] = Array(Ref 0))The Ref cycle is genuinely unreachable: the server itself panics first. Attempting to publish a recursive type killed the standalone process —
panicked at crates/schema/src/schema.rs:1189: validated module should have all types resolve: RecursiveTypeRef(AlgebraicTypeRef(5)). Such a schema can never be served. And the blast radius is contained anyway:set -euo pipefailpropagates the jq failure, anddev-server.sh'sif … then mv … else rm … log warningleaves the committed fixture untouched. Nit at most. (I restarted the server and restoredchat.)Note the walker is safe against recursion by construction, not by accident:
Productnever recurses into its elements, sostruct Node { kids: Vec<Node> }terminates atproduct_Node. OnlyArray,Ref, and an Option'ssomepayload recurse, and only a typespace entry that is itself anArray/Refcan close a loop.Claims audit — everything else checks out
misc_exports[].Procedure, old comment wrong{name, params:{elements}, return_type}.Arraytransparent{"Array":{"U32":[]}},Refbare number{"Ref":1}types[].tyis a bare number, which is what makes:117'sselect(.ty == $r)work..primaryKey()table `twopk` has multiple primary key annotations.primary_keyemits nopk, tested overfile://file://, so the payload must be namedschema, notschema?version=9.messagebyte-identicalorigin/mainis insertions only, zero deletion lines.session.jsonlcorrectly not re-captured, frame tests pass5 passed, 0 failed.profile/claim_profilecannot perturb conformanceSELECT * FROM message(capture-fixtures.mjs:38,smoke.script:82,141) — no wildcard anywhere. Andsmoke.script:77builds its own inlinestdb.schema.new({…}), it never loads the generated fixture.dev-server.sh→git diffon the fixture is empty.4 passed, 1 failed. Nit: it fails withbad argument #1 to 'concat' (table expected, got nil)rather than a clean assertion; atest.assert_eq(profile.types ~= nil, true, …)guard first would read better.Acceptance criteria (#25)
dev/server/index.js:26-30. I would only ask that the substitution be reflected in #25 itself, since #27/#28 will read that issue.So: #25 is closed. B1, S1 and S2 are the three I would not merge without.
Nits
gen-schema.sh:117— Ref-name fallback($r | tostring)bakes a typespace index into a tag. Use"". (Required alongside B1.)gen-schema.sh:112—keys_unsorted[0]silently picks one key from a multi-key AlgebraicType. Fine, but a comment would help.types = {}is always present for a parameterless reducer, butindexes/uniqueare omitted when empty. Consumers nil-check three fields and not two. Harmless, slightly surprising."unnamed", colliding. Unreachable via either module SDK.gen-schema.sh:7-9still says rows arrive as positional arrays. True formessage, but my capture shows named objects for a 7-column table — both forms occur, andtest_fixtures.lua:157already tests the named-object path. #32's territory, flagging only so it isn't forgotten.Cleanup
Deleted all 9 throwaway databases (
rev36pr,rev36probe{a,b,d,e,f,g,h},rev36final). Restarted the server after the recursive-type panic and republished from the workspace's own module — livechatis back to["message"]. No files edited, nothing pushed, no PR comment.Verdict: Closes #25 on the letter of its acceptance criteria, but I would not merge it as-is: two of the type tags it emits are silently WRONG (not
unknown), one metadata field silently vanishes, and three of the new header comments state protocol facts that the live server contradicts. All fixes are small and I verified them end to end against a real 2.8.2 publish.78642a103dc1e44ae98b