Support numeric (Int/BigInt) entity ids and foreign keys - #1482
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds support for non-string entity IDs, propagating ChangesTyped entity IDs
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SchemaParser
participant Codegen
participant Handler
participant Runtime
participant PostgreSQL
SchemaParser->>Codegen: resolve declared entity ID scalar
Codegen->>Handler: emit scalar-specific operations
Handler->>Runtime: submit typed entity changes
Runtime->>PostgreSQL: encode IDs and execute typed SQL
PostgreSQL-->>Runtime: return typed history and rollback IDs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd26b77928
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GqlScalar::ID | ||
| | GqlScalar::String | ||
| | GqlScalar::Int | ||
| | GqlScalar::BigInt(_) => (), |
There was a problem hiding this comment.
When a @derivedFrom field points at a scalar FK, Hasura maps the parent table's id to that child field (Hasura.res builds the derived mapping as "id": relationalKey). By accepting any Int/BigInt here without checking it against the parent entity's id scalar, a schema such as Parent.id: ID! with Child.parentId: Int! now passes CLI validation but generates a relationship comparing a TEXT id to an INTEGER field, which fails when metadata/queries are created. Please only accept scalar direct fields whose type matches the derived entity's id scalar.
Useful? React with 👍 / 👎.
| getOrCreate: 'entity => promise<'entity>, | ||
| set: 'entity => unit, | ||
| deleteUnsafe: string => unit, | ||
| deleteUnsafe: EntityId.t => unit, |
There was a problem hiding this comment.
This changes only the internal handler context to accept opaque raw ids; the generated public handler type in codegen_templates.rs still emits deleteUnsafe: string => unit. For an entity with id: Int! or id: BigInt!, generated ReScript handlers cannot call context.Entity.deleteUnsafe(10) even though storage now supports numeric deletes; the new test bypasses the generated type by casting args.context to a custom deleteUnsafe: int shape. Please generate the per-entity id scalar for this operation.
Useful? React with 👍 / 👎.
| .get_field("id") | ||
| .ok_or_else(|| anyhow!("Entity {} is missing an 'id' field", self.name))?; |
There was a problem hiding this comment.
If a schema uses the previously-supported uppercase primary-key field ID, this exact lookup reports the entity as missing an id (and the new runtime Table.getIdFieldOrThrow has the same exact-name assumption). Existing code treats both id and ID as primary keys via case-insensitive checks, so projects using ID: ID! now fail relation codegen or id-typed storage/history paths despite still having a valid primary key. Please resolve the id field with the same case-insensitive rule used elsewhere.
Useful? React with 👍 / 👎.
| ~committedCheckpointId, | ||
| Delete({ | ||
| entityId, | ||
| entityId: entityId->EntityId.unsafeOfString, |
There was a problem hiding this comment.
This rollback path now wraps ids as EntityId.t, but the Postgres rollback reader still parses the row-state id with S.string (PgStorage.res:1251-1252). For an Int! id entity that has a pre-target history row, Postgres returns the selected id as a number, so prepareRollbackDiff throws before creating the restore/delete diff during a reorg; BigInt ids can similarly be kept as strings instead of the raw id type. Please thread table.getIdSchema through getRollbackData instead of stringifying rollback ids.
Useful? React with 👍 / 👎.
| GqlScalar::ID | ||
| | GqlScalar::String | ||
| | GqlScalar::Int | ||
| | GqlScalar::BigInt(_) => {} |
There was a problem hiding this comment.
Once schemas with Int! or BigInt! ids are accepted here, the by-id read APIs still expose and route ids as string (context.Entity.get/getOrThrow, the generated handler context, and LoadLayer.loadById). For a numeric-id table, following those generated types and calling get("137") makes the Postgres load filter serialize a string through the table's S.int/BigInt id schema and fail, while calling get(137) is rejected by the generated types. Please make these by-id read signatures use the entity's actual id scalar and pass that raw value through.
Useful? React with 👍 / 👎.
Entity ids may now be Int! or BigInt! in addition to ID!/String!, and a
relation's foreign key adopts the referenced entity's id type instead of
always being text/string. Previously an Int! id column was numeric while its
foreign keys stayed text, producing a type mismatch across the relation.
CLI codegen:
- Resolve a relation's Postgres primitive and ReScript/TS type to the
referenced entity's id scalar (entity_parsing.rs), so a foreign key mirrors
the id it points at. The Entity primitive variant is removed; a relation is
now a normal typed field carrying linked_entity for the _id name and Hasura.
- Reject entity ids outside {ID, String, Int, BigInt}, and non-nullable
non-list ids, at parse time.
- Allow Int/BigInt targets for @derivedFrom.
- Drop the single global `type id = string`; id and _id fields render as their
concrete type (string/int/bigint) in ReScript and TypeScript.
Runtime (packages/envio):
- Add opaque EntityId.t used by the id-generic layers (Change, InMemoryTable,
entity history); concrete generated types stay string/int/bigint. Dict keys
go through EntityId.toKey.
- Derive id column type and value schema per table (Table.getIdPgFieldType /
getIdSchema); delete-by-id, history backfill, and history-delete SQL cast to
the id column type instead of text, and serialize ids via the id schema.
- Thread the id schema through the Postgres and ClickHouse entity-history set
and delete schemas.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
c0943ce to
b34e2c2
Compare
b34e2c2 to
f516fab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f516fab6d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| writeln!(code).unwrap(); | ||
| writeln!(code, "module {} = {{", entity.name.capitalized).unwrap(); | ||
| writeln!(code, " type id = {}", id_type).unwrap(); |
There was a problem hiding this comment.
When an Int!/BigInt! entity has a relation to an ID! entity, the foreign-key TypeIdent::ID still formats as id; this new per-module type id = int/bigint then makes the FK field and its getWhere filter use the owning entity's numeric id alias instead of string. For example, Numeric.id: Int! with parent: Parent! where Parent.id: ID! generates parent_id: id inside module Numeric, so users cannot set the required string FK even though the runtime schema/DB column are text. Render ID foreign keys as string or an unshadowed outer alias instead of the current module's id alias.
Useful? React with 👍 / 👎.
| } | ||
| if deleted->Array.length > 0 { | ||
| entityObj->Dict.set("deleted", deleted->(Utils.magic: array<string> => unknown)) | ||
| entityObj->Dict.set("deleted", deleted->(Utils.magic: array<EntityId.t> => unknown)) |
There was a problem hiding this comment.
For an Int! or BigInt! id entity deleted during createTestIndexer().process(), this now puts the raw EntityId.t value into the returned deleted array, but packages/envio/index.d.ts still declares EntityChangeValue.deleted as readonly string[]. TypeScript tests written against the generated process result will be told to expect strings even though runtime returns numbers or bigints, so please type these deleted ids as EntityId<Entity>[] on the public test-indexer change surface.
Useful? React with 👍 / 👎.
| if let Ok(GqlScalar::BigInt(precision)) = entity.get_id_scalar() { | ||
| let stored_as_numeric = | ||
| precision.is_some_and(|p| p <= CLICKHOUSE_DECIMAL_MAX_PRECISION); |
There was a problem hiding this comment.
This ClickHouse BigInt guard only checks the entity's own id, so a ClickHouse entity can still use @storage(clickhouse: {orderBy: ["owner"]}) where owner is a relation to an entity whose id: BigInt! has no usable precision. Because this commit makes relation columns adopt the referenced id primitive, that FK column is emitted as a ClickHouse String and enters ORDER BY lexicographically, while the existing orderBy validator only sees the schema field's scalar as Custom and misses it. Resolve relation fields to their target id scalar before accepting them in ClickHouse sort keys.
Useful? React with 👍 / 👎.
| format!(" \\\"{name}\": testIndexerEntityOperations<Entities.{name}.t>,") | ||
| } else { | ||
| format!( | ||
| " \\\"{name}\": testIndexerEntityOperationsWithCustomId<Entities.{name}.t, Entities.{name}.id>,", |
There was a problem hiding this comment.
These direct test-indexer fields now get the custom-id operations, but the generated getTestIndexerEntityOperations helper below still returns only testIndexerEntityOperations<'entity>. In a numeric-id project, tests that use the helper form (indexer->Indexer.getTestIndexerEntityOperations(Indexer.Entities.IntIdEntity)) still see get/getOrThrow as string-keyed and cannot call get(1), even though the direct indexer."IntIdEntity".get(1) field is typed correctly. Please make the helper carry the entity id type too.
Useful? React with 👍 / 👎.
| if let Ok(GqlScalar::BigInt(precision)) = entity.get_id_scalar() { | ||
| let stored_as_numeric = | ||
| precision.is_some_and(|p| p <= CLICKHOUSE_DECIMAL_MAX_PRECISION); | ||
| if !stored_as_numeric { |
There was a problem hiding this comment.
When a ClickHouse entity supplies @storage(clickhouse: {orderBy: [...]}), the ClickHouse history DDL uses those user columns plus envio_checkpoint_id and drops id from the sorting key, so an unbounded BigInt id is not being ordered lexicographically in that configuration. This unconditional rejection blocks schemas such as a BigInt-id event table ordered by a timestamp, even though non-sort BigInt columns already fall back to String. Only require BigInt id precision when the table will actually use the default id ordering.
Useful? React with 👍 / 👎.
| let entityName = prop["entity"]->Option.getOrThrow | ||
| (Table.Entity({name: entityName}), S.string->S.toUnknown) | ||
| } | ||
| | other => JsError.throwWithMessage("Unknown field type in entity config: " ++ other) |
There was a problem hiding this comment.
Dropping the "entity" field type means every pre-upgrade project with a relationship has stored envio_info JSON that no longer matches the newly generated public config, even when the relation still points at a string id and the database schema is unchanged. On resume, Persistence.init compares the stored JSON with the current one and calls Config.throwIfIncompatible, so these projects are forced to reset or start a parallel indexer solely because relationship properties changed from type: "entity" to their underlying scalar. Please normalize the legacy representation during config diffing or continue emitting/parsing a compatibility shape for string-id relations.
Useful? React with 👍 / 👎.
* Key entity operations by the real id scalar (string/int/bigint) Extend numeric-id support to the user-facing API. The generated handler context and test-indexer operations (get/getOrThrow/deleteUnsafe) now adopt each entity's id type instead of hardcoding string, on both the ReScript and TypeScript surfaces: - Each generated entity module exposes `type id`, and the operation types gain an `'id` parameter resolved from it. - `EntityOperations`/`TestIndexerEntityOperations` in index.d.ts derive the id type from the entity via an `EntityId<Entity>` helper. - `Internal.entityHandlerContext` uses `EntityId.t` for get/getOrThrow to match deleteUnsafe. Add a numeric-id codegen unit test, regenerate the scenario indexers, cover the generated typed API with compile-time checks plus a ClickHouse unbounded-BigInt fallback test, and document numeric ids in the schema/ handlers skills. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr * Keep string-id entity operations id-argument-free Address review feedback: - Emit `type id` before `type t` in each generated entity module. - Split the operation types: string-id entities use the plain `handlerEntityOperations`/`testIndexerEntityOperations` (no id type argument, id forced to string), and only non-string ids use the `...WithCustomId` variants. String-only projects regenerate to the original id-argument-free shape. - Tighten the schema/handlers skills: `ID!` is recommended (not a "default"), and drop the obvious id-type restatements. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr * Reject unbounded BigInt id on ClickHouse entities ClickHouse stores a BigInt with no precision (or precision above its Decimal ceiling of 38) as a String, sorted lexicographically. Since `id` is ClickHouse's mandatory sort key, such an id would order wrong. Validate in `validate_entity_storage` (which sees each entity's effective ClickHouse storage, including the config-level `default: true` fallback), mirroring the existing `validate_clickhouse_order_by_fields` rejection: a BigInt id on a ClickHouse entity must set `@config(precision: N)` with N <= 38 so it stores as a numeric Decimal. Cover positive and negative flows (per-entity directive and default backend) via InternalTestIndexer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr * Assert the full ClickHouse BigInt-id error, not a substring vitest's toThrowError(string) only checks containment. Capture the thrown message via try/catch and assert the exact, full error with toBe so the test documents the complete message and fails if any of it changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr * Add a strict toThrowErrorEqual vitest matcher The built-in toThrowError only checks the thrown message contains the argument. Add a strict sibling matcher, toThrowErrorEqual, that requires the whole message to match — registered via expect.extend in each test package's setup and exposed on the ReScript Vitest binding. Overriding toThrowError itself would break the ~15 existing assertions that intentionally match on a substring, so this is a separate matcher. Use it for the ClickHouse BigInt-id errors so those tests pin the full message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr * Make the throw matcher strict everywhere Replace toThrowError (substring) with toThrowErrorEqual (exact) across the ReScript test suites and drop the substring binding, so every throw assertion pins the complete error message. Existing assertions that had only a substring are updated to the full message. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KDdTB5oid2AvSy2D1k1Jqr --------- Co-authored-by: Claude <noreply@anthropic.com>
f516fab to
276a04f
Compare
Two defects found in review of the numeric-id support: - Each entity module declares its own `type id`, which shadows the shared `type id = string` alias. A relation to a string-id entity rendered as that bare alias, so inside a numeric-id module its foreign key resolved to the owner's `int`/`bigint` id while the column stays text. Foreign keys now render the concrete id scalar. - The Postgres rollback reader parsed the row-state id with `S.string`, so a reorg on an `Int!` id entity threw before building the restore/delete diff (Postgres returns the id as a number). The schema is now built per table from the table's id schema, and `EntityId.t` is threaded through `getRollbackData` so ids keep their real type. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
…key checks
`EntityChangeValue.deleted` declared `readonly string[]`, but the test indexer
reports the raw id, so a numeric-id entity returned numbers/bigints against a
string type. It now derives from `EntityId<Entity>`, covered by runtime tests
asserting the reported ids are the raw scalars and by compile-time checks on
the generated surface.
ClickHouse sort-key validation ignored which columns the sorting key actually
holds. `@storage(clickhouse: {orderBy: [...]})` replaces `id` in the key, so:
- Fields listed in `orderBy` are now validated, resolving a relation to the id
it stores (a relation's own scalar never matched the BigInt check, so a sort
by a relation to an unbounded-BigInt id silently became a lexicographic
String column).
- The unbounded-BigInt `id` rejection now only applies when `orderBy` is absent
and `id` is therefore the sorting key. Its message points at `orderBy` too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/envio/src/PgStorage.res (1)
1781-1786: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winParse removed rollback IDs with the table-specific ID schema.
removedIdRowsis cast directly toEntityId.t, unlike restored rows at Line 1799. ABigIntID returned in the driver's raw representation can therefore reach rollback deletes as a string, causing typed serialization to fail or exposing the wrong deleted-ID type.Proposed fix
+let removedIdRowsSchema: Table.table => S.t<array<EntityId.t>> = Utils.WeakMap.memoize(table => + S.array(S.object(s => s.field(Table.idFieldName, table->Table.getIdSchema))) +) + ... - ->(Utils.magic: promise<unknown> => promise<array<{"id": EntityId.t}>>), + ->(Utils.magic: promise<unknown> => promise<array<unknown>>), ... - let removedIds = removedIdRows->Array.map(row => row["id"]) + let removedIds = removedIdRows->S.parseOrThrow(removedIdRowsSchema(entityConfig.table))Also applies to: 1796-1803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/PgStorage.res` around lines 1781 - 1786, Update the rollback ID handling around makeGetRollbackRemovedIdsQuery so removedIdRows is decoded with the table-specific entity ID schema before being used for deletes. Apply the same schema-aware parsing to the restored-row path as well, replacing direct casts to EntityId.t and preserving the resulting typed IDs for rollback processing.
🧹 Nitpick comments (3)
packages/envio/src/db/EntityHistory.res (1)
151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the cast instead of a bare
Obj.magic.Neighbouring calls in this file use
Utils.magicwith an explicitinput => outputannotation (lines 117, 169). Matching that keeps the encoded-ids shape documented at the call site.As per coding guidelines: "When using
Utils.magicfor type casting in ReScript, always add explicit type annotations:value->(Utils.magic: inputType => outputType)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/envio/src/db/EntityHistory.res` at line 151, Update the cast in the surrounding EntityHistory conversion to use Utils.magic instead of bare Obj.magic, and add an explicit input-to-output type annotation matching the encoded IDs shape, consistent with the neighbouring calls.Source: Coding guidelines
packages/cli/src/hbs_templating/codegen_templates.rs (2)
88-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a named return instead of a bare
(bool, String).Call sites read
entity_id_type(entity).0, which doesn't convey "is the default string id". A small enum or named struct (or a separatehas_default_idhelper) would make lines 1503/1508/1886 self-explanatory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/hbs_templating/codegen_templates.rs` around lines 88 - 101, Replace the bare `(bool, String)` return from `entity_id_type` with a named struct or enum that clearly identifies the ID type and whether it is the default string ID. Update all call sites, including the usages around lines 1503, 1508, and 1886, to access named fields or variants instead of tuple indices while preserving existing behavior.
1845-1880: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth branches duplicate the whole
testIndexerEntityOperationsblock.Only the extra
...WithCustomIdtype differs; the shared type is copy-pasted verbatim, so any future edit must be made twice. Mirror the handler-context approach (line 1521) and append only the conditional suffix.♻️ Compose base + conditional suffix
- let test_indexer_entity_ops_type = if has_custom_id_entity { - r#"/** Entity operations for direct access outside handlers. */ -type testIndexerEntityOperations<'entity> = { - ... -} - -type testIndexerEntityOperationsWithCustomId<'entity, 'id> = { - ... -}"# - } else { - r#"/** Entity operations for direct access outside handlers. */ -type testIndexerEntityOperations<'entity> = { - ... -}"# - }; + let base_test_indexer_ops = r#"/** Entity operations for direct access outside handlers. */ +type testIndexerEntityOperations<'entity> = { + /** Get an entity by ID. */ + get: string => promise<option<'entity>>, + /** Get all entities. */ + getAll: unit => promise<array<'entity>>, + /** Get an entity by ID or throw if not found. */ + getOrThrow: (string, ~message: string=?) => promise<'entity>, + /** Set (create or update) an entity. */ + set: 'entity => unit, +}"#; + let custom_id_test_indexer_ops = if has_custom_id_entity { + r#" + +type testIndexerEntityOperationsWithCustomId<'entity, 'id> = { + /** Get an entity by ID. */ + get: 'id => promise<option<'entity>>, + /** Get all entities. */ + getAll: unit => promise<array<'entity>>, + /** Get an entity by ID or throw if not found. */ + getOrThrow: ('id, ~message: string=?) => promise<'entity>, + /** Set (create or update) an entity. */ + set: 'entity => unit, +}"# + } else { + "" + }; + let test_indexer_entity_ops_type = + format!("{base_test_indexer_ops}{custom_id_test_indexer_ops}");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/hbs_templating/codegen_templates.rs` around lines 1845 - 1880, Refactor the testIndexerEntityOperations template construction to define the shared type block once and append a conditional suffix containing only testIndexerEntityOperationsWithCustomId when has_custom_id_entity is true. Update the surrounding let test_indexer_entity_ops_type logic while preserving the generated type definitions and formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/hbs_templating/codegen_templates.rs`:
- Around line 1882-1892: The entity mapping in getTestIndexerEntityOperations
must select the custom-ID accessor for entities with custom IDs. Update the
entity_id_type branching so custom-id entities return
testIndexerEntityOperationsWithCustomId with the appropriate Entities.{name}.id
type, preserving the standard accessor for default string-ID entities.
In `@packages/envio/src/bindings/Vitest.res`:
- Around line 173-177: Update the None branch of the no-throw assertion to
represent the missing thrown error with a non-string sentinel or otherwise
distinguish it from any actual error message, so an expected message of "<the
function did not throw>" cannot pass incorrectly; preserve the existing message
handling and toBe assertion behavior for functions that do throw.
---
Outside diff comments:
In `@packages/envio/src/PgStorage.res`:
- Around line 1781-1786: Update the rollback ID handling around
makeGetRollbackRemovedIdsQuery so removedIdRows is decoded with the
table-specific entity ID schema before being used for deletes. Apply the same
schema-aware parsing to the restored-row path as well, replacing direct casts to
EntityId.t and preserving the resulting typed IDs for rollback processing.
---
Nitpick comments:
In `@packages/cli/src/hbs_templating/codegen_templates.rs`:
- Around line 88-101: Replace the bare `(bool, String)` return from
`entity_id_type` with a named struct or enum that clearly identifies the ID type
and whether it is the default string ID. Update all call sites, including the
usages around lines 1503, 1508, and 1886, to access named fields or variants
instead of tuple indices while preserving existing behavior.
- Around line 1845-1880: Refactor the testIndexerEntityOperations template
construction to define the shared type block once and append a conditional
suffix containing only testIndexerEntityOperationsWithCustomId when
has_custom_id_entity is true. Update the surrounding let
test_indexer_entity_ops_type logic while preserving the generated type
definitions and formatting.
In `@packages/envio/src/db/EntityHistory.res`:
- Line 151: Update the cast in the surrounding EntityHistory conversion to use
Utils.magic instead of bare Obj.magic, and add an explicit input-to-output type
annotation matching the encoded IDs shape, consistent with the neighbouring
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7123a60d-da73-4ea9-986c-59b29d22beaf
⛔ Files ignored due to path filters (6)
packages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_generated_for_svm.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_generates_correct_types_and_values.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__indexer_code_multiple_chains.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_generated_for_evm.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_all_options.snapis excluded by!**/*.snappackages/cli/src/hbs_templating/snapshots/envio__hbs_templating__codegen_templates__test__internal_config_json_code_with_lowercase_contract_name.snapis excluded by!**/*.snap
📒 Files selected for processing (47)
packages/cli/src/config_parsing/entity_parsing.rspackages/cli/src/config_parsing/field_types.rspackages/cli/src/config_parsing/public_config.rspackages/cli/src/config_parsing/system_config.rspackages/cli/src/hbs_templating/codegen_templates.rspackages/cli/templates/static/shared/.claude/skills/indexer-handlers/SKILL.mdpackages/cli/templates/static/shared/.claude/skills/indexer-schema/SKILL.mdpackages/envio-tests/test/ClientAddressFilter_test.respackages/envio-tests/test/Config_test.respackages/envio-tests/test/EntityFilter_test.respackages/envio-tests/test/MockIndexerHandlers_test.respackages/envio-tests/test/UserApiValidation_test.respackages/envio-tests/test/Utils_test.respackages/envio-tests/test/lib_tests/ColumnNameFormat_test.respackages/envio/index.d.tspackages/envio/src/Change.respackages/envio/src/Config.respackages/envio/src/EntityId.respackages/envio/src/InMemoryStore.respackages/envio/src/InMemoryTable.respackages/envio/src/Internal.respackages/envio/src/Persistence.respackages/envio/src/PgStorage.respackages/envio/src/TestIndexer.respackages/envio/src/UserContext.respackages/envio/src/bindings/ClickHouse.respackages/envio/src/bindings/Vitest.respackages/envio/src/db/EntityHistory.respackages/envio/src/db/Table.resscenarios/fuel_test/src/Indexer.resscenarios/svm_test/src/Indexer.resscenarios/test_codegen/schema.graphqlscenarios/test_codegen/src/Indexer.resscenarios/test_codegen/test/ConcurrentWrite_test.resscenarios/test_codegen/test/EventBlockFilter_test.resscenarios/test_codegen/test/EventFilters_test.resscenarios/test_codegen/test/EventHandler.test.tsscenarios/test_codegen/test/HandlerRegisterLifecycle_test.resscenarios/test_codegen/test/WriteRead_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/EffectState_test.resscenarios/test_codegen/test/lib_tests/EntityIdType_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/Rollback_test.res
💤 Files with no reviewable changes (3)
- packages/cli/src/config_parsing/public_config.rs
- packages/cli/src/config_parsing/field_types.rs
- packages/envio/src/Config.res
Review follow-ups: - `getRollbackData` parsed the pre-target rows with the table's id schema but cast the removed-id rows straight to `EntityId.t`. Postgres hands back a NUMERIC id as a string, so a BigInt-id entity produced string ids on one half of the rollback diff and bigints on the other, and re-serializing those strings through the id schema would fail. Both queries now parse through it. - `toThrowErrorEqual` compared a "<the function did not throw>" placeholder against the expected message, so asserting that exact string passed for a function that never threw. It compares options instead, which drops the placeholder and the branch along with it. - Annotate the backfill ids cast with `Utils.magic` per the repo convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
The name-keyed test-indexer accessor returned `testIndexerEntityOperations`, whose `get`/`getOrThrow` are string-keyed, so a numeric-id entity reached through the helper form could not be looked up by its real id even though direct field access was typed correctly. `Indexer.res` is user-facing, so this was reachable from user tests. Carry the id on the entity-name GADT (`name<'entity, 'id>`) so the accessor can recover it, and return the id-aware operations. Direct fields are unchanged, so `ID!` entities keep their id-argument-free shape and the accessor resolves to the same string-keyed operations they had before. The custom-id operations type is now emitted unconditionally since the accessor always refers to it, which also drops the duplicated template branch. Note: annotating `Entities.name` explicitly now needs the extra type parameter (as MockIndexer does); using the constructors as values is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
Allowing Int/BigInt derived-from fields left the field's type unchecked against the entity deriving from it. Hasura maps that entity's `id` onto the derived column (the `"id": relationalKey` mapping in Hasura.res), so a schema like `Parent.id: ID!` with `Child.parentId: Int!` passed validation and then produced a relationship comparing a text id to an integer column, failing when the metadata is created. Previously this could not happen: only ID/String were accepted and every id was a string, so the two sides always agreed. Scalar derived-from fields now have to share an id kind with the entity's id. Kinds are compared rather than raw scalars, since ID and String share a text column (relied on by the e2e_test schema, which derives an `id: ID!` entity from a `String!` field) and a BigInt's precision only sets the column width. A relation back to the deriving entity keeps its existing pass-through: the foreign key adopts that entity's id type by construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9da53bad4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /** The entity's `id` type. `ID!`/`String!` ids are `string`; `Int!` is `number` | ||
| * and `BigInt!` is `bigint`, so id-keyed operations accept the real scalar. */ | ||
| type EntityId<Entity> = Entity extends { readonly id: infer Id } ? Id : string; |
There was a problem hiding this comment.
When a schema declares an entity named EntityId, codegen emits export type EntityId = Entities["EntityId"] in the declare module "envio" augmentation, which conflicts with this new module-local generic and makes TypeScript report TS2300: Duplicate identifier 'EntityId'. Such an entity name is currently accepted, so inline this conditional type, choose a namespace that cannot receive generated aliases, or explicitly reject the name during schema validation.
Useful? React with 👍 / 👎.
2 participants
Summary
Extends entity id support beyond strings to include
Int!andBigInt!scalars. Foreign keys now adopt the referenced entity's id type, ensuring type consistency between id columns and their references in the database and generated code.Key Changes
Entity id validation: Added validation in
entity_parsing.rsto restrict entity ids to supported scalars (ID,String,Int,BigInt), rejecting unsupported types, nullable ids, arrays, and derived id fields upfront.Foreign key type resolution: Modified
GqlScalar::to_underlying_postgres_primitiveandto_rescript_typeto resolve foreign key types through the referenced entity's id scalar, rather than hardcoding them asEntityorString. This ensures a foreign key to anInt!id becomesInt32, while one to anID!id remainsString.SQL type casting: Updated delete-by-id and history backfill queries to cast arrays to the id column's Postgres type (
$1::INTEGER[],$1::NUMERIC[]) instead of hardcodedtext[].Opaque id type: Introduced
EntityId.tmodule to represent ids generically across storage/in-memory layers without exposing the underlying scalar. The runtime value is always the real id (string/int/bigint);toKeyderives a string form for JS object keys.Schema updates: Modified
Table.resto expose id field metadata (getIdFieldOrThrow,getIdPgFieldType,getIdSchema,encodeIdsToJson) and removed theEntityvariant fromfieldTypesince foreign keys now resolve to concrete scalars.Change tracking: Updated
Change.tto useEntityId.tforentityIdfields, and adjusted in-memory and storage layers to key entities by stringified id while preserving real id values in SQL bindings.Test coverage: Added comprehensive test suite (
EntityIdType_test.res) covering id type resolution, SQL generation, ClickHouse mapping, schema serialization, and end-to-end round-tripping of numeric ids through the indexer.Notable Implementation Details
linked_entityfor_idcolumn naming and Hasura relation metadata, but theirfield_typenow reflects the referenced entity's id scalar.EntityId.toKey) for stable JS object lookups across all id types.idSchemais extracted per-table and used to serialize id arrays for SQL binding, ensuring the correct Postgres type is applied.IntandBigIntin addition toIDandStringfor consistency.https://claude.ai/code/session_01DNs5ezETo32JzCmFun221s
Summary by CodeRabbit
EntityIdscalar types (including custom-id operation variants).@derivedFromid-mismatch errors.type: "entity"fields with a clearer error.