fix jsonb operator nullability inference #408
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
🦋 Changeset detectedLatest commit: cf14451 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughPatch release prep for @ts-safeql/generate adds a changeset entry and updates AST typing logic to infer nullability for jsonb operators (->>, #>>) when operands include column references. New helper scans ASTs for column refs. Tests expanded to cover jsonb extraction/build scenarios and nullability outcomes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Test/Query
participant Type as getType()
participant Null as getNullable()
participant Scan as hasColumnReference()
Dev->>Type: Describe AExpr (jsonb ->> / #>>)
Type->>Null: Compute nullability
Null->>Scan: Check AST for ColumnRef
alt LHS has column reference
Note over Null,Type: Enforce nullable for ->>, #>>
else No column reference
Note over Null: Use literal/object rules and column flags
end
Type-->>Dev: Type with updated nullability (e.g., string \| null)
Note over Type: Object literals in AExpr resolved as jsonb
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/generate/src/generate.test.ts (1)
2044-2061: Avoid asserting unknownColumns to reduce test fragility.unknownColumns depends on generator internals and can change unrelated to behavior. Recommend not asserting it here.
Apply this diff:
test(`jsonb subselect ->> key => string | null`, async () => { await testQuery({ query: `SELECT (SELECT data FROM employee LIMIT 1) ->> 'myKey' as extracted_value`, expected: [ [ "extracted_value", { kind: "union", value: [ { kind: "type", value: "string", type: "text" }, { kind: "type", value: "null", type: "null" }, ], }, ], ], - unknownColumns: ["extracted_value"], }); });packages/generate/src/ast-describe.ts (2)
355-369: Targeted nullable inference for ->> and #>> is appropriate.
- Honors explicit non-null markers and operand nullability.
- Adds operator-aware nullability when LHS includes a column reference.
Consider extending later to other JSON extraction operators if needed.
1108-1171: Extract and harden hasColumnReference
- Move this helper into ast-describe.utils for reuse and testing
- Add handling for ParamRef and RelabelType:
function hasColumnReference(node: LibPgQueryAST.Node | undefined): boolean { if (node === undefined) { return false; } if (node.ColumnRef !== undefined) { return true; } + if (node.ParamRef !== undefined) { + return true; + } if (node.A_Const !== undefined) { return false; } if (node.TypeCast !== undefined) { return hasColumnReference(node.TypeCast.arg); } + if (node.RelabelType !== undefined) { + return hasColumnReference(node.RelabelType.arg); + } // … }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/clever-rockets-act.md(1 hunks)packages/generate/src/ast-describe.ts(3 hunks)packages/generate/src/generate.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/generate/src/ast-describe.ts (4)
packages/ast-types/src/index.ts (1)
Node(14-253)packages/generate/src/utils/get-nonnullable-columns.ts (1)
isColumnNonNullable(79-201)packages/generate/src/ast-get-sources.ts (1)
resolveColumn(426-436)packages/generate/src/ast-decribe.utils.ts (2)
isColumnUnknownRef(15-19)isColumnStarRef(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, 20)
🔇 Additional comments (7)
.changeset/clever-rockets-act.md (1)
5-5: Good, concise changeset entry.Accurately reflects the behavior change for ->> and #>> nullability.
packages/generate/src/generate.test.ts (4)
2037-2042: Nice coverage for jsonb #>> with literal path.Validates non-nullable string result for constant jsonb and path.
2063-2079: Good: jsonb_build_object with column ->> key yields string | null.Matches nullable inference when LHS includes a column reference.
2081-2086: Good: jsonb_build_object without column ->> key yields string.Confirms non-null for fully-constant LHS.
2369-2385: Solid direct operator coverage on column jsonb ->> key.Ensures the common case returns string | null.
packages/generate/src/ast-describe.ts (2)
311-313: Correctly treats object-typed expressions as jsonb for operator resolution.This enables downcasting for jsonb_build_object(...) on the LHS of operators.
370-401: Using computed nullable in resolveType is the right integration point.Keeps type mapping separate from nullability policy.
Summary by CodeRabbit
Bug Fixes
Tests
Chores