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

fix(mssql): columnInfo() now works with case-sensitive database [fixes #4573] #4633

Conversation

jeremy-w
Copy link
Contributor

Before, the SQL used to implement columnInfo() for the mssql driver referenced information_schema.columns.

But in a database with a case-sensitive collation, both of those parts must be capitalized, as INFORMATION_SCHEMA.COLUMNS, or you just get an error [#4573]. (It doesn't seem sensitive to how you capitalize the column names, though. 🤷 )

Now, that capitalized form is used when interacting with mssql.

Question for Reviewer: Should I just switch the default mssql test database to use a case-sensitive collation?

In order to test this change, I needed to create a new database with a case-sensitive collation, then tear it down. This required digging around to grab the database config used in the integration test suite.

It would be cleaner to change the default test database for MSSQL to be created with a case-sensitive collation, in which case, every test would be a test of case-sensitive behavior. This does create a risk of writing SQL that relies on case-sensitivity in its function, which then fails under case-insensitive collations, but I consider that rather less likely than case-insensitive code that fails when case-sensitivity is enabled.

If we did change it to use a case-sensitive collation, I think we could remove the dedicated test suite I added, though each of those tests did turn out to be required to beat this functionality into shape, because I repeatedly missed various ways this could go wrong:

  • "I'll just addproperty": fails during alter that updates the comment
  • "I'll base add/update on whether we're creating or altering": fails when a table/column is created without any comment, and the alter needs to add, not update

Finally I came around to, "We just have to check and add/update based on what we find each time," which is the implementation you see here.

@kibertoad kibertoad merged commit 52950c6 into knex:master Aug 21, 2021
@kibertoad
Copy link
Collaborator

Thank you!

@kibertoad
Copy link
Collaborator

Released in 0.95.11

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 this pull request may close these issues.

None yet

2 participants