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

[Sqlite3] Fix can not get table sql because of case sensitive #5509

Merged
merged 5 commits into from Mar 30, 2023
Merged

[Sqlite3] Fix can not get table sql because of case sensitive #5509

merged 5 commits into from Mar 30, 2023

Conversation

zydmayday
Copy link
Contributor

Example

Here is an example how to reproduce the issue.
https://github.com/zydmayday/learn-knex/blob/main/knex/knex.test.ts#L34-L44

Details

Use sqlite3 as database for testing, and has some migration scripts for setting up tables.

If we have one script create a table called "some_table(https://github.com/zydmayday/learn-knex/blob/main/knex/migrations/1_create_table.ts)",
and another script alter table but using table name "Some_Table(https://github.com/zydmayday/learn-knex/blob/main/knex/migrations/2_add_columns.ts)".
Then after running jest, we will have the following error logs.

TypeError: Cannot read properties of undefined (reading 'sql')

      at client.transaction.connection (node_modules/knex/lib/dialects/sqlite3/schema/ddl.js:56:13)

How to fix

Because the related SQL script is case sensitive, change it to run in a case insensitive way.

@coveralls
Copy link

coveralls commented Mar 7, 2023

Coverage Status

Coverage: 92.838% (+0.4%) from 92.392% when pulling 613ab50 on zydmayday:master into 0d27bcb on knex:master.

@zydmayday
Copy link
Contributor Author

@OlivierCavadenti Thanks for the label, i added the tests.

@kibertoad
Copy link
Collaborator

If SQLite is case-sensitive, why should knex hide that?
does it behave differently between DDL operations and querying? would same code work case-insensitively for querying?

@zydmayday
Copy link
Contributor Author

If SQLite is case-sensitive, why should knex hide that? does it behave differently between DDL operations and querying? would same code work case-insensitively for querying?

Hi @kibertoad
Thanks for replying.
Actually it is not because sqlite3 is case sensitive.
It is because the query in function getTableSql is case sensitive.

When we try to get some tables DDL, we don't care if we pass the UPPERCASE or lowercase of table name.
In fact, sqlite3 is not case sensitive as I searched.

@kibertoad
Copy link
Collaborator

got it, thanks!

@OlivierCavadenti
Copy link
Collaborator

@zydmayday I will check coveralls configuration before merge that. It's seems coverage decreased computation take into account not only coverage of code, but also coverage of tests... I will not ask you to make a test for test :D.

@OlivierCavadenti OlivierCavadenti merged commit ab1f9ae into knex:master Mar 30, 2023
53 checks passed
@zydmayday
Copy link
Contributor Author

@zydmayday I will check coveralls configuration before merge that. It's seems coverage decreased computation take into account not only coverage of code, but also coverage of tests... I will not ask you to make a test for test :D.

@OlivierCavadenti
Thanks! Good to know the reason! haha

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

4 participants