improve insert stmt validation #415
Conversation
🦋 Changeset detectedLatest commit: 231f67f 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Rate limit exceeded@Newbie012 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds INSERT-statement validation into the generator pipeline, a new validation utility, a test helper for running generator tests in transactions, comprehensive INSERT tests, a changeset, and a small schema metadata extension for identity columns. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Generator
participant Parser
participant Validator
rect rgba(100,180,120,0.06)
Test->>Generator: run generate(sql, options)
end
Generator->>Parser: parse(sql.text)
Parser-->>Generator: ParseResult
alt parsed is INSERT
rect rgba(180,200,255,0.06)
Generator->>Validator: validateInsertResult(parsed, pgColsBySchemaAndTableName, query)
Validator-->>Generator: ok or throw PostgresError
end
end
alt validation passed or not INSERT
Generator->>Generator: describe(parsed) / continue generation
Generator-->>Test: result or error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/shiny-books-glow.md(1 hunks)packages/generate/src/generate-insert.test.ts(1 hunks)packages/generate/src/generate.test.ts(2 hunks)packages/generate/src/generate.ts(4 hunks)packages/generate/src/test-utils.ts(1 hunks)packages/generate/src/utils/validate-insert.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/generate/src/utils/validate-insert.ts (3)
packages/ast-types/src/index.ts (3)
RawStmt(803-807)InsertStmt(852-860)ParseResult(4-7)packages/generate/src/generate.ts (1)
PgColRow(630-641)packages/shared/src/errors.ts (2)
QuerySourceMapEntry(210-214)PostgresError(216-280)
packages/generate/src/generate.ts (1)
packages/generate/src/utils/validate-insert.ts (2)
isParsedInsertResult(10-14)validateInsertResult(16-62)
packages/generate/src/generate.test.ts (1)
packages/generate/src/test-utils.ts (1)
createTestQuery(11-84)
packages/generate/src/generate-insert.test.ts (3)
packages/generate/src/test-utils.ts (2)
SQL(9-9)createTestQuery(11-84)packages/test-utils/src/setup-test-database.ts (2)
setupTestDatabase(19-32)generateTestDatabaseName(34-36)packages/shared/src/common.ts (1)
normalizeIndent(102-114)
packages/generate/src/test-utils.ts (2)
packages/generate/src/generate.ts (3)
createGenerator(97-110)ResolvedTargetEntry(33-33)GenerateParams(50-60)packages/shared/src/errors.ts (2)
isPostgresError(282-292)InternalError(132-167)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/generate/src/utils/validate-insert.ts (1)
64-82: Fix positional INSERT validation.When an INSERT omits the explicit column list (e.g.,
INSERT INTO test_tbl VALUES (DEFAULT, 'abc')), PostgreSQL maps the provided values onto every table column from left to right. Currently,stmt.colsis an empty array[]for such queries, but line 65 checksif (stmt.cols)which is truthy even for empty arrays. This causes lines 66-68 to return an empty array, incorrectly flagging all NOT NULL columns as missing even though the statement is valid.Apply this diff to check array length instead:
function getInsertColumns(stmt: LibPgQueryAST.InsertStmt, tableCols: PgColRow[]): string[] { - if (stmt.cols) { + if (stmt.cols && stmt.cols.length > 0) { return stmt.cols .map((col) => col.ResTarget?.name) .filter((name): name is string => Boolean(name)); } if (stmt.selectStmt === undefined) { return []; } const valuesFromSelect = stmt.selectStmt.SelectStmt?.valuesLists?.at(0)?.List?.items; if (valuesFromSelect !== undefined) { return tableCols.slice(0, valuesFromSelect.length).map((c) => c.colName); } return tableCols.map((c) => c.colName); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/generate/src/generate-insert.test.ts(1 hunks)packages/generate/src/utils/validate-insert.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/generate/src/generate-insert.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/generate/src/utils/validate-insert.ts (3)
packages/ast-types/src/index.ts (3)
RawStmt(803-807)InsertStmt(852-860)ParseResult(4-7)packages/generate/src/generate.ts (1)
PgColRow(630-641)packages/shared/src/errors.ts (2)
QuerySourceMapEntry(210-214)PostgresError(216-280)
🔇 Additional comments (2)
packages/generate/src/utils/validate-insert.ts (2)
1-14: LGTM!The type definition and type guard are well-structured and correctly validate whether a parsed result contains an INSERT statement.
16-43: LGTM!The validation logic correctly identifies NOT NULL columns without defaults that are missing from the INSERT statement. The filter at lines 37-39 appropriately checks for non-nullable columns (
colNotNull), absence of defaults (!colHasDef), non-identity columns (colIdentity === ""), and verifies they're not in the insert list.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/ci.yml
23-23: expecting a single ${{...}} expression or array value for matrix variations, but found plain text node
(syntax-check)
24-24: expecting a single ${{...}} expression or array value for matrix variations, but found plain text node
(syntax-check)
Summary by CodeRabbit
New Features
Tests
Chores