✨ Enhance the PostgreSQL parser to support Constraints #1587
Conversation
🦋 Changeset detectedLatest commit: 7ad5442 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 Git ↗︎
1 Skipped Deployment
|
|
Updates to Preview Branch (feat/postgresql-constraints) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
tnyo43
changed the title
CI Feedback 🧐(Feedback updated until commit 342d6e0)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
| for (const elt of createStmt.tableElts) { | ||
| if ('ColumnDef' in elt) { | ||
| const colDef = elt.ColumnDef | ||
| if (colDef.colname === undefined) continue | ||
|
|
||
| let isPrimary = false | ||
| let isUnique = false | ||
| for (const constraint of colDef.constraints ?? []) { |
There was a problem hiding this comment.
This for loop is to unify the logics with constraints check.
It refers to colDef.constraints objects to create constraint objects, relationship objects and make flag if the columns are primary or unique, which currently used to create Column object.
| export const convertToSchema = (stmts: RawStmt[]): ProcessResult => { | ||
| export const convertToSchema = ( | ||
| stmts: RawStmt[], | ||
| rawSql: string, |
There was a problem hiding this comment.
added this argument for constraintToCheckConstraint function.
For the detail: #1587 (comment)
| return ok(undefined) | ||
| return err( | ||
| new UnexpectedTokenWarningError('contype "CONSTR_FOREIGN" is expected'), | ||
| ) |
There was a problem hiding this comment.
Because of this change, the objects of constraint variable must always satisfy constraint.contype !== 'CONSTR_FOREIGN', so that I modified this to return an error state if it doesn't.
| let openParenthesesCount = 0 | ||
| let endLocation: number | undefined = undefined | ||
| for (let i = constraint.location; i < rawSql.length; i++) { | ||
| if (rawSql[i] === '(') openParenthesesCount++ | ||
| else if (rawSql[i] === ')') { | ||
| openParenthesesCount-- | ||
| if (openParenthesesCount === 0) { | ||
| endLocation = i | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a logic to extract detail value of check constraint from the original SQL text.
The Constraint(of @pgsql) object has .location field which indicates the position where the constraint object is parsed from the original SQL text.
This is an example:
If the original SQL string is "CHECK (a(b)c(d(e)))fgh" and the constraint.location is 6, the substring (a(b)c(d(e)))fgh is scanned from the beginning. The value of openParenthesesCount changes as 1, 1, 2, 1, 1, 2, 2, 3, 2, 1, 0 while reading the string character by character. The position where the count returns to 0 is marked as endLocation (this case endLocation is 18). The correct expression (a(b)c(d(e))) can be extracted by executing rawSql.slice(constraint.location, endLocation + 1).
A check constraint consists of the key word CHECK followed by an expression in parentheses.
https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-CHECK-CONSTRAINTS
There was a problem hiding this comment.
I had tried to use pgsql-deparser package to get Check constraint once but I gave up with it.
When I execute deparse with an object created by parsing CHECK (price > 0) by running new Deparser({}, {}).Constraint(constraint), it throws "TypeError: Cannot read properties of undefined (reading 'String')" error.
When I execute deparse with an object created by parsing CHECK (price > price) (it will be false always but valid syntax), it returns "CHECK ( )" which is an unexpected result.
I think something went wrong with either parsering or deparsering. (It might be same issue with launchql/pgsql-parser#131)
I can investigate what's the matter f you are very interested in this problem, but maybe in another issue.
There was a problem hiding this comment.
I thought it was a practical solution to extract the values from rawSql. LGTM 👍🏻
|
The e2e test fails. Could you please help me to solve this problem? I guess it's better to take some actions but not in this PR. |
tnyo43
requested review from
hoshinotsuyoshi,
FunamaYukina,
junkisai,
MH4GF and
NoritakaIkeda
and removed request for
a team
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
|
Oops, sorry. |
| let openParenthesesCount = 0 | ||
| let endLocation: number | undefined = undefined | ||
| for (let i = constraint.location; i < rawSql.length; i++) { | ||
| if (rawSql[i] === '(') openParenthesesCount++ | ||
| else if (rawSql[i] === ')') { | ||
| openParenthesesCount-- | ||
| if (openParenthesesCount === 0) { | ||
| endLocation = i | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I thought it was a practical solution to extract the values from rawSql. LGTM 👍🏻
it checks `contype` in the for loop, constraintToRelationship won't have to check it.
to get constraint expressions, it refers to the original raw SQL texts and extract the expressions with constraint.location
The processRelationshipsAndConstraints function is still more complex than the threshold, the score is 16 while it should be 15 or less.
tnyo43
force-pushed
the
feat/postgresql-constraints
branch
from
342d6e0 to
7ad5442
Compare
| @@ -246,6 +311,55 @@ export const convertToSchema = (stmts: RawStmt[]): ProcessResult => { | |||
| comment: string | null | |||
| } | |||
|
|
|||
| // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: <explanation> | |||
There was a problem hiding this comment.
The index of complexity of this function is 16 while the threshold is 15. I gave up to reduce it to matches the threshold 😅
I think the parsers' functions tend to be more complex than the modules of other packages. It might be possible to make the threshold higher for this package 🤔
|
@MH4GF @FunamaYukina |
Issue
Why is this change needed?
We introduced constraints of schema and update tbls and Prisma parser to support the feature.
This PR will update PostgreSQL parser to support constraints.
ref
constraintsdata #1168What would you like reviewers to focus on?
Testing Verification
pnpm run build --filter @liam-hq/clito build clinode frontend/packages/cli/dist-cli/bin/cli.js erd build --format postgres --input schema.in.sqlto generate scriptsnpx http-server distto run the application and go to http://localhost:8080/schema.in.sql
What was done
🤖 Generated by PR Agent at 342d6e0
Detailed Changes
converter.ts
Add constraint extraction and storage to PostgreSQL schema converterfrontend/packages/db-structure/src/parser/sql/postgresql/converter.ts
constraints.
constraints.
index.ts
Update processor to support raw SQL for constraint parsingfrontend/packages/db-structure/src/parser/sql/postgresql/index.ts
index.test.ts
Add and update tests for constraint parsingfrontend/packages/db-structure/src/parser/sql/postgresql/index.test.ts
constraints.
olive-women-notice.md
Add changeset for PostgreSQL constraint support.changeset/olive-women-notice.md
constraints.
Additional Notes