Skip to content

✨ feat: enhance schema to adopt constraints data #1168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Apr 10, 2025

Conversation

tnyo43
Copy link
Member

@tnyo43 tnyo43 commented Apr 6, 2025

Issue

  • resolve: Improve schame expression to adopt constraints data, such as primary key, unique, foreign key and check constraitns.

Why is this change needed?

The current db-structure schema drops the constraints data of tbls schema. Especially, check constraints of the schema can't be expressed with the current schema.
This PR will enhance the schema to adopt constraints data and prepare for implementing the UI using it.

What would you like reviewers to focus on?

  • implementation of the constraints schema
  • implementation of the tbls format parser with constraints

Testing Verification

  • add tests for the schema and parser

What was done

🤖 Generated by PR Agent at 8acfdbe

  • Introduced support for database constraints in schema definitions.
    • Added support for PRIMARY KEY, FOREIGN KEY, UNIQUE, and CHECK constraints.
    • Enhanced schema parsing to include constraints for various database parsers.
  • Updated test cases to validate new constraint functionalities.
    • Added tests for PRIMARY KEY, FOREIGN KEY, UNIQUE, and CHECK constraints.
    • Verified error handling for duplicate constraints in overrides.
  • Enhanced override functionality to support adding constraints to existing tables.
  • Updated schema definitions to include constraints and their types.

Detailed Changes

Relevant files
Enhancement
8 files
parser.ts
Add constraints handling to Prisma schema parser                 
+5/-2     
parser.ts
Add constraints handling to Schema.rb parser                         
+4/-2     
converter.ts
Add constraints handling to PostgreSQL schema converter   
+6/-2     
parser.ts
Add constraints handling to tbls schema parser                     
+66/-7   
dbStructure.ts
Define schema for constraints and integrate into table schema
+63/-15 
factories.ts
Update table factory to include constraints                           
+3/-0     
index.ts
Export constraints-related types and schemas                         
+2/-1     
overrideDbStructure.ts
Add support for overriding constraints in database structure
+26/-3   
Tests
2 files
index.test.ts
Add tests for constraints in tbls schema parser                   
+169/-0 
overrideDbStructure.test.ts
Add tests for overriding constraints in database structure
+71/-0   
Documentation
1 files
swift-keys-design.md
Add changeset for constraints enhancement                               
+5/-0     

Additional Notes


Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    changeset-bot bot commented Apr 6, 2025

    🦋 Changeset detected

    Latest commit: 76f9454

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 2 packages
    Name Type
    @liam-hq/db-structure Patch
    @liam-hq/cli Patch

    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

    Copy link

    vercel bot commented Apr 6, 2025

    @tnyo43 is attempting to deploy a commit to the ROUTE06 Core Team on Vercel.

    A member of the Team first needs to authorize it.

    @@ -95,6 +95,7 @@ async function parsePrismaSchema(schemaString: string): Promise<ProcessResult> {
    columns,
    comment: model.documentation ?? null,
    indices: {},
    constraints: {},
    Copy link
    Member Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This PR won't update the other format parsers other than tbls because I think it will make this change too large.

    const cardinalitySchema = v.picklist(['ONE_TO_ONE', 'ONE_TO_MANY'])
    export type Cardinality = v.InferOutput<typeof cardinalitySchema>

    const foreignKeyConstraintSchema = v.picklist([
    Copy link
    Member Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    One of the new constraint schemas will be named as foreignKeyConstraintSchema, so it will rename this schema as foreignKeyConstraintReferenceOptionSchema (it's a little bit hard to find the diffs because this PR will change the order of declarations 😓 )

    I refer to the MySQL document to to name the schema as ReferenceOption. https://dev.mysql.com/doc/refman/8.4/en/create-table-foreign-keys.html

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍🏻

    @tnyo43 tnyo43 marked this pull request as ready for review April 6, 2025 04:09
    @tnyo43 tnyo43 requested a review from a team as a code owner April 6, 2025 04:09
    @tnyo43 tnyo43 requested review from hoshinotsuyoshi, FunamaYukina, junkisai, MH4GF and NoritakaIkeda and removed request for a team April 6, 2025 04:09
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Constraint Validation

    The constraint parsing logic doesn't validate if the referenced tables and columns exist in the schema before creating foreign key constraints. This could lead to invalid references.

    if (
      constraint.type === 'FOREIGN KEY' &&
      constraint.columns?.length && // not unidefined and equal to or greater than 1
      constraint.referenced_columns?.length ===
        constraint.columns?.length &&
      constraint.referenced_table
    ) {
      const { updateConstraint, deleteConstraint } =
        extractForeignKeyActions(constraint.def)
      constraints[constraint.name] = {
        type: 'FOREIGN KEY',
        name: constraint.name,
        columnNames: constraint.columns,
        targetTableName: constraint.referenced_table,
        targetColumnNames: constraint.referenced_columns,
        updateConstraint,
        deleteConstraint,
      }
    Type Consistency

    The ForeignKeyConstraintReferenceOption type is used in multiple places but defined only once. Ensure consistent naming and usage across the codebase.

    const foreignKeyConstraintReferenceOptionSchema = v.picklist([
      'CASCADE',
      'RESTRICT',
      'SET_NULL',
      'SET_DEFAULT',
      'NO_ACTION',
    ])
    export type ForeignKeyConstraintReferenceOption = v.InferOutput<
      typeof foreignKeyConstraintReferenceOptionSchema
    >

    This comment was marked as resolved.

    @tnyo43 tnyo43 force-pushed the feat/add-constraints branch from 8acfdbe to f67e2df Compare April 6, 2025 04:51
    Copy link
    Member

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks!! almost good, please check comments!

    const cardinalitySchema = v.picklist(['ONE_TO_ONE', 'ONE_TO_MANY'])
    export type Cardinality = v.InferOutput<typeof cardinalitySchema>

    const foreignKeyConstraintSchema = v.picklist([
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍🏻

    @tnyo43 tnyo43 requested a review from MH4GF April 9, 2025 11:28
    Copy link
    Member

    @MH4GF MH4GF left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍🏻👍🏻

    @MH4GF MH4GF added this pull request to the merge queue Apr 10, 2025
    Merged via the queue into liam-hq:main with commit bd337fd Apr 10, 2025
    6 of 9 checks passed
    @tnyo43 tnyo43 deleted the feat/add-constraints branch April 10, 2025 13:03
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants