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(sqlite): quote table names in sqlite getForeignKeysQuery #12752

Closed
wants to merge 1 commit into from
Closed

fix(sqlite): quote table names in sqlite getForeignKeysQuery #12752

wants to merge 1 commit into from

Conversation

steveschmitt
Copy link
Contributor

@steveschmitt steveschmitt commented Oct 10, 2020

Table names were not quoted in getForeignKeysQuery that was called by queryInterface.describeTable for the sqlite dialect.

I discovered this defect when I had a sqlite table named "Order" while working on sequelize-auto

This PR fixes it, using the same quoteTable function used elsewhere.

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • (N/A) Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • (N/A) Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

In dialects/sqlite/query-generator.ts, fix getForeignKeysQuery so that it quotes the table names in the same manner that is done in other methods.

Before:

Executing (default): PRAGMA foreign_key_list(Order)

After:

Executing (default): PRAGMA foreign_key_list(`Order`)

@steveschmitt steveschmitt changed the title Fix quoting of table names in sqlite (fix) quote table names in sqlite getForeignKeysQuery Oct 10, 2020
@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #12752 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12752   +/-   ##
=======================================
  Coverage   96.32%   96.32%           
=======================================
  Files          95       95           
  Lines        9261     9261           
  Branches       90       80   -10     
=======================================
  Hits         8921     8921           
  Misses        323      323           
  Partials       17       17           
Impacted Files Coverage Δ
src/dialects/sqlite/query-generator.js 96.55% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update febc083...d5d33a9. Read the comment docs.

@steveschmitt steveschmitt changed the title (fix) quote table names in sqlite getForeignKeysQuery fix(sqlite) quote table names in sqlite getForeignKeysQuery Oct 22, 2020
@steveschmitt steveschmitt changed the title fix(sqlite) quote table names in sqlite getForeignKeysQuery fix(sqlite): quote table names in sqlite getForeignKeysQuery Oct 22, 2020
@aprilandjan
Copy link

I encountered the same problem with a table which name is started with a number (for example, a md5 string 29537fd809b1f303ac917c3eecac7cdc). Hope this will be merged ASAP.

References:

  1. in sqlite it is recommended to quote all names
  2. what are valid table names in sqlite

@jlherren
Copy link

jlherren commented Aug 8, 2021

Any chance of getting this merged?

@sdepold
Copy link
Member

sdepold commented Oct 24, 2021

Hey there :)

First of all: Thanks a bunch for your contribution to Sequelize! Much appreciated!
Second: Unfortunately we haven't had the chance to look into this ever since you created it. Sorry for that!

A couple of months ago, we have switched from master to main branch as our primary development branch and hence this PR is now outdated :(

If you still think this change is making sense, please consider recreating the PR against main. Thanks in advance and sorry for the additional work.

✌️

@steveschmitt steveschmitt changed the base branch from master to main October 24, 2021 19:07
@steveschmitt steveschmitt changed the base branch from main to master October 24, 2021 19:07
@steveschmitt
Copy link
Contributor Author

@sdepold I've created a new PR #13587 into the main branch to replace this one.

Please review and merge the new PR! Thanks.

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