liam icon indicating copy to clipboard operation
liam copied to clipboard

✨ feat: enhance schema to adopt `constraints` data

Open tnyo43 opened this issue 9 months ago • 4 comments

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 8acfdbe30f4d1398773e4ba980733435a5fba548

  • 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.
  • tnyo43 avatar Apr 06 '25 03:04 tnyo43

    🦋 Changeset detected

    Latest commit: 76f945446fd2e6e3c3cf58c57fecbdec8e4e34bf

    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

    changeset-bot[bot] avatar Apr 06 '25 03:04 changeset-bot[bot]

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

    A member of the Team first needs to authorize it.

    vercel[bot] avatar Apr 06 '25 03:04 vercel[bot]

    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
    >
    

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix UNIQUE constraint validation

    The current condition only allows UNIQUE constraints with exactly one column,
    but UNIQUE constraints can apply to multiple columns. The comment suggests it
    should be "equal to or greater than 1", but the code is checking for exactly 1.

    frontend/packages/db-structure/src/parser/tbls/parser.ts [162-165]

     if (
       constraint.type === 'UNIQUE' &&
    -  constraint.columns?.length === 1 // not unidefined and equal to or greater than 1
    +  constraint.columns?.length >= 1 // not undefined and equal to or greater than 1
     ) {
    
    • [ ] Apply this suggestion <!-- /improve --apply_suggestion=0 -->
    Suggestion importance[1-10]: 8

    __

    Why: The current code only allows UNIQUE constraints with exactly one column (using === 1), but the comment indicates it should handle constraints with one or more columns. This mismatch could prevent multi-column UNIQUE constraints from being properly processed.

    Medium
    • [ ] Update <!-- /improve --more_suggestions=true -->