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

Adds support for querying sqlite_master table #9645

Merged
merged 4 commits into from Jul 14, 2018
Merged

Adds support for querying sqlite_master table #9645

merged 4 commits into from Jul 14, 2018

Conversation

@tmont
Copy link
Contributor

@tmont tmont commented Jul 10, 2018

Pull Request check-list

  • 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?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Raw queries to the sqlite_master table were being special-cased and as such would return inconsistent values. For example, SELECT * FROM sqlite_master would simply return an array populated with just the value in the name column. This change allows you to query the sqlite_master just like any other table, and removes hardcoded SQL string checks in favor of using the QueryTypes enum.

This is related to #1612 and #8769. Previous workarounds were to shove the sqlite_master table into a view so that its table name wouldn't be caught by the indexOf check and query the view instead.

This changeset also fixes a DataTypes test that was failing due to timezone issues.

@codecov
Copy link

@codecov codecov bot commented Jul 10, 2018

Codecov Report

Merging #9645 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@ -152,7 +153,7 @@ function addConstraint(tableName, options) {
const describeCreateTableSql = this.QueryGenerator.describeCreateTableQuery(tableName);
let createTableSql;

return this.sequelize.query(describeCreateTableSql, options)
return this.sequelize.query(describeCreateTableSql, { type: QueryTypes.SELECT, raw: true })

This comment has been minimized.

@sushantdhiman

sushantdhiman Jul 11, 2018
Contributor

We should pass other options, probably { type: QueryTypes.SELECT, raw: true, ...options }

This comment has been minimized.

@tmont

tmont Jul 11, 2018
Author Contributor

That seemed to make things fail (although I never did try to figure out why). options was always something like

{ type: 'unique', fields: [ 'email' ] }

I believe options is meant to be the options to create the constraint, not just general query options. Are there some specific properties I could pick out? I wasn't really sure of what the possibilities were.

@sushantdhiman sushantdhiman merged commit 31113ce into sequelize:master Jul 14, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 95.98%)
Details
codecov/project 95.98% (+<.01%) compared to fea6db4
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tmont
Copy link
Contributor Author

@tmont tmont commented Jul 15, 2018

@sushantdhiman You probably already realized this, but it just occurred to me that this will be a breaking change for any query that involves the string sqlite_master for the sqlite dialect. So it probably can't be backported to v4.x (if that was even under consideration).

joaoe added a commit to joaoe/sequelize-auto that referenced this pull request Jul 27, 2019
... and added lodash.

Sequelize needed an update because of
sequelize/sequelize#9645
joaoe added a commit to joaoe/sequelize-auto that referenced this pull request Jul 27, 2019
... and added lodash.

Sequelize needed an update because of
sequelize/sequelize#9645
joaoe added a commit to joaoe/sequelize-auto that referenced this pull request Jul 27, 2019
... and added lodash.

lodash is not longer available as Sequelize.Utils._. It shall therefore be imported directly.

Sequelize needed an update because of
sequelize/sequelize#9645

Temporary commit for git-utils fixup
joaoe added a commit to joaoe/sequelize-auto that referenced this pull request Jul 27, 2019
... and added lodash.

lodash is not longer available as Sequelize.Utils._. It shall therefore be imported directly.

Sequelize needed an update because of
sequelize/sequelize#9645

Temporary commit for git-utils fixup
joaoe added a commit to joaoe/sequelize-auto that referenced this pull request Jul 28, 2019
... and added lodash.

lodash is not longer available as Sequelize.Utils._. It shall therefore be imported directly.

Sequelize needed an update because of
sequelize/sequelize#9645

Also, fixed other small issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants