Skip to content
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

sql_type check in have_db_column #1432

Closed
LyleDavis opened this issue Mar 21, 2021 · 3 comments · Fixed by #1555
Closed

sql_type check in have_db_column #1432

LyleDavis opened this issue Mar 21, 2021 · 3 comments · Fixed by #1555

Comments

@LyleDavis
Copy link

Would you guys be open to a PR to add a method into the have_db_column chain that checks the sql_type of the column? Currently there is only one for type which I think is insufficient when dealing with certain column types.

For instance, say you are storing guids in a char(36) column, rails will interpret that as :string. In this case it is quite important that the column is actually a mysql char(36) instead.

irb(main):008:0> MyModel.columns.find { |c| c.name == 'some_guid_column' }.type
=> :string
irb(main):009:0> MyModel.columns.find { |c| c.name == 'some_guid_column' }.sql_type
=> "char(36)"
irb(main):010:0>

I was thinking to just add an additional method into have_db_column so you can specify something like:

it { is_expected.to have_db_column(:some_guid_column).of_sql_type('char(36)') }

I'm happy to do the PR if you guys are open to it.

@mcmire
Copy link
Collaborator

mcmire commented Mar 22, 2021

On the surface it seems like a small change, but I'm curious, are you worried that Rails will do the wrong thing here and not assign the correct SQL type? I have been increasingly questioning the value of have_db_column as it seems to be testing configuration more than logic. But I first want to make sure there is not something I'm missing about how the matcher is getting used.

@LyleDavis
Copy link
Author

I suppose it is more testing configuration than logic when you think hard about it, but given that Rails models are so inherently tied to the database it seems sensible to me to have a buffer between changes.

When you have migrations being picked up from so many different engines like we do it makes sense to have the engine that requires the presence of a particular field also validate that the field exists when the tests across all engines are run (makes sure another engine doesn't have conflicting migrations etc). Especially when those migrations use actual sql types rather than the rails built-in string with limit.

It's also pretty good documentation for the model itself that saves having to dive through schema.rb files.

I guess it may make less sense in the typical rails app setup rather than an application split into 20 or so engines? It's up to you, I wrote a custom matcher for our stuff that did this - I just thought it'd be nice to upstream it if other people would also like to validate db schema in the same way.

@mcmire
Copy link
Collaborator

mcmire commented Mar 22, 2021

Ah, engines, okay. I can see how that could end up creating potentially confusing situations. And my second question was going to be about whether you are using t.column :some_guid_column, 'char(36)' or t.string :some_guid_column, :string, limit: 36, but looks like you already answered that question.

Okay, I can see the use case behind this. Send a PR over and we will be happy to merge this. :)

matsales28 added a commit to matsales28/shoulda-matchers that referenced this issue Apr 28, 2023
This commit intends to implement the `of_sql_type` qualifier on the
`have_db_column` matcher. The implementation is quite simple and is
following the pattern of the `HaveDbColumnMatcher` class.

This commit closes this issue
thoughtbot#1432
matsales28 added a commit to matsales28/shoulda-matchers that referenced this issue Apr 29, 2023
This commit intends to implement the `of_sql_type` qualifier on the
`have_db_column` matcher. The implementation is quite simple and is
following the pattern of the `HaveDbColumnMatcher` class.

This commit closes this issue
thoughtbot#1432
matsales28 added a commit that referenced this issue May 2, 2023
…1555)

This commit intends to implement the `of_sql_type` qualifier on the
`have_db_column` matcher. The implementation is quite simple and is
following the pattern of the `HaveDbColumnMatcher` class.

This commit closes this issue
#1432
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants