✨ enhance the schema.rb parser to support Constraints #1408
Conversation
🦋 Changeset detectedLatest commit: 4ac6aa3 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 ↗︎
|
|
Updates to Preview Branch (feat/schemarb-constraints) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
This comment was marked as resolved.
This comment was marked as resolved.
| const idPrimaryKeyConstraint: PrimaryKeyConstraint = { | ||
| type: 'PRIMARY KEY', | ||
| name: 'PRIMARY_id', | ||
| columnName: 'id', | ||
| } |
There was a problem hiding this comment.
It generates PRIMARY KEY constraint object at the same time of it generates id column.
| if (index.unique && index.columns[0]) { | ||
| const uniqueConstraint: UniqueConstraint = { | ||
| type: 'UNIQUE', | ||
| name: `UNIQUE_${index.columns[0]}`, | ||
| columnName: index.columns[0], | ||
| } | ||
| constraints.push(uniqueConstraint) | ||
| } |
There was a problem hiding this comment.
It will set UNIQUE constraints when unique index objects is created.
https://api.rubyonrails.org/v8.0.2/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_index
I understand this is the only way to set unique constraints on the database.
In order to add a uniqueness database constraint on your database, use the add_index statement in a migration and include the unique: true option.
https://guides.rubyonrails.org/active_record_validations.html#:~:text=In%20order%20to%20add%20a%20uniqueness%20database%20constraint%20on%20your%20database%2C%20use%20the%20add_index%20statement%20in%20a%20migration%20and%20include%20the%20unique%3A%20true%20option.
You can see how it works in index (unique: true) test case.
There was a problem hiding this comment.
I have the same understanding!👍
| @@ -278,6 +309,51 @@ function extractRelationshipTableNames( | |||
| return ok([primaryTableName, foreignTableName]) | |||
| } | |||
|
|
|||
| function extractCheckConstraint( | |||
There was a problem hiding this comment.
Here is a function to get check constraint from add_check_constraint statement.
It will extract the table name, detail and constraint name from the statement.
|
Oops, it looks However, I'm not sure if all the result must be the same. For example, |
The expected objects of the test cases of schama.rb's parser are different than the other parsers, especially than the tbls's parser. They will requires their own test results.
| }) | ||
|
|
||
| it('foreign key (one-to-many)', async () => { | ||
| it('foreign key', async () => { |
There was a problem hiding this comment.
I created a test of "foreign key" to test the behaviour with add_foreign_key by checking both relationships and constraints.
Since the existing cases like "foreign key (one-to-many)" were intended to test relationship cardinalities, I grouped them under a describe block named "foreign key cardinality". (I think using "Hide whitespace" to will make the diff easier to read)
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:
|
||||||||||||||
| if (index.unique && index.columns[0]) { | ||
| const uniqueConstraint: UniqueConstraint = { | ||
| type: 'UNIQUE', | ||
| name: `UNIQUE_${index.columns[0]}`, | ||
| columnName: index.columns[0], |
There was a problem hiding this comment.
Just a quick question for clarification!
Are we extracting only the first item from the columns array here because of the reasoning mentioned in this comment?
There was a problem hiding this comment.
extracting only the first item from the columns array here because of the reasoning mentioned in this comment
Yes, and same for the other parsers as well (e.g. this is an implementation of tbls's Unique constraints)
| if (node.name === 'add_check_constraint') | ||
| this.handleAddCheckConstraint(node) |
There was a problem hiding this comment.
In recent versions of Rails (7.1 and later) and with the mysql2 adapter, t.check_constraint is now supported and will be included in schema.rb.👀
like this:
create_table "quiz_choices", force: :cascade do |t|
t.integer "display_order", default: 0, null: false
t.check_constraint "`display_order` between 1 and 4", name: "display_order_check"
end
ref: https://kazuya-engineer.com/2024/02/03/rails-migrations-allow-specific-values/
If it is to be supported, it would be in the form of adding it to extractTableDetails function.💭
I don't think we must necessarily address this in this pull request!
There was a problem hiding this comment.
Thanks, I missed the method.
Yes, it seems necessary to support, but maybe in a separate PR. I'll create an issue later.
| @@ -311,25 +388,31 @@ function extractForeignKeyOptions( | |||
| case 'column': | |||
| if (value instanceof StringNode || value instanceof SymbolNode) { | |||
| relation.foreignColumnName = value.unescaped.value | |||
| foreignKeyConstraint.columnName = value.unescaped.value | |||
There was a problem hiding this comment.
(sorry, I wasn't able to submit my comment.)
In lines 391, 397, and 406, it would be better to set “foreignKeyConstraint.name” as well. The name seemed to remain empty.
like this?💭
case 'column'
foreignKeyConstraint.name = `fk_${value.unescaped.value}`
case 'on_update'
foreignKeyConstraint.name = `on_update_${updateConstraint}`
case 'on_delete'
foreignKeyConstraint.name = `on_delete_${updateConstraint}`
There was a problem hiding this comment.
Thanks for pointing it out, but unfortunately I can't reproduce the problem on my end.
Could you please share the schema you used? That would help me understand the problem more easily.
It seemds like the add_foreign_key method always provides a name property, even if it isn't explicitly set, so I guess there is no need toadd extra lines for it 🤔
In any cases, a concrete example will be super helpful 😉
:name
The constraint name. Defaults to fk_rails_<identifier>.
https://api.rubyonrails.org/v4.2.0/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key
There was a problem hiding this comment.
📝 To reproduce, visit https://liam-app-git-feat-schemarb-constraints-route-06-core.vercel.app/erd/p/github.com/mastodon/mastodon/blob/1bc28709ccde4106ab7d654ad5888a14c6bb1724/db/schema.rb?active=account_aliases
The constraint name should be fk_rails_<identifier> and the identifier is a 10 character long random HEX string if the name is not provided (please refer to this).
I created a random HEX string generator and set the constraint name and column name if not provided explicitly in this commit 83cc5e5
Thanks for your kind review 👍
Foreign key constraints name will include 10 character long random identifier if not specified. ref: https://api.rubyonrails.org/v4.2.0/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key The original implementation returns 10 character long random HEX string (e.g. "c5b6f2d87a", "4e2f7ab9c1", ...) ref: https://github.com/rails/rails/blob/77fe87cccd90859e67c958cbf63d7e5662174fd8/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb#L984-L988
tnyo43
force-pushed
the
feat/schemarb-constraints
branch
from
f792f4e to
83cc5e5
Compare
…lationship name. I think the logic was too complicated #1408 (comment)
| if (relation.foreignColumnName === '') { | ||
| relation.foreignColumnName = `${singularize(relation.primaryTableName)}_id` | ||
| const columnName = `${singularize(relation.primaryTableName)}_id` | ||
| relation.foreignColumnName = columnName | ||
| foreignKeyConstraint.columnName = columnName | ||
| } | ||
|
|
||
| if (relation.name === '') { | ||
| relation.name = defaultRelationshipName( | ||
| const relationshipName = defaultRelationshipName( | ||
| relation.primaryTableName, | ||
| relation.primaryColumnName, | ||
| relation.foreignTableName, | ||
| relation.foreignColumnName, | ||
| ) | ||
| relation.name = relationshipName | ||
| foreignKeyConstraint.name = relationshipName | ||
| } | ||
| } |
There was a problem hiding this comment.
I set the same values for the constraint's name and columnName as those of the relationship's name and columnName if they are not specified explicitly.
Although that name do not strictly follow the add_foreign_key specification (fk_rails_<identifier>. identifier is a 10 character long random string), I chose to assign these values because reproducing the exact specification would make the implementation overly complex.
I initially tried implementing the specification-compliant names (please check this commit if you are interested in), but the implementation became too complicated, so I decided not to proceed with it.
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 schema.rb 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 --input schema.rbto generate scriptsnpx http-server distto run the application and go to http://localhost:8080/schema.rb
What was done
🤖 Generated by PR Agent at 8d71055
Detailed Changes
parser.ts
Add constraint extraction and storage to schema.rb parserfrontend/packages/db-structure/src/parser/schemarb/parser.ts
CHECK constraints
factories.ts
Add constraint factories for schema objectsfrontend/packages/db-structure/src/schema/factories.ts
constraints
index.ts
Export constraint types and factories from schema modulefrontend/packages/db-structure/src/schema/index.ts
UNIQUE, CHECK)
index.test.ts
Add and update tests for constraint parsingfrontend/packages/db-structure/src/parser/schemarb/index.test.ts
KEY, UNIQUE, CHECK)
wet-zoos-add.md
Add changeset for schema.rb constraint support.changeset/wet-zoos-add.md
constraints
Additional Notes