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

refactor: move all dialect conditional logic into subclass #12217

Merged
merged 2 commits into from May 3, 2020

Conversation

SimonSchick
Copy link
Contributor

@SimonSchick SimonSchick commented May 3, 2020

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

Description of change

Note:

The idea of this PR is to de-couple the dialects from the core query interface.
This reduces overhead as not all dialects required certain pre-computing that was performed in some places.

Some of these breaking changes are not really breaking due to not being in the official API but better safe than sorry.

Many of the postgres specific query interface APIs were never documented, should they?

Some of the changes (such as the rename of QueryInterface -> queryInterface and the removal of stack aren't strictly needed but do streamline the API.

BREAKING CHANGE:

All instances of QueryInterface and QueryGenerator have been renamed to their lowerCamelCase variants eg. queryInterface and queryGenerator when used as property names on Model and Dialect, the class names remain the same.

Utils.stack has been removed.

QueryInterface is now an abstract class.

query-interface.js was moved to lib/dialects/abstract/query-interface and the exports have been reduced to QueryInterface.
default and module.exports no longer refer to this class.

QueryInterface.addConstraints now only takes 2 parameters, tableName and options, previously the second parameter could be a list of column names to apply the constraint to, this list must now be passed in options as the fields property.
The fourth rawTableName parameter was entirely removed as it was effectively unused.

@SimonSchick SimonSchick force-pushed the refactor/query-interface-dialects branch 2 times, most recently from 3052864 to 3734525 Compare May 3, 2020
Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Using an abstract class for dialect specific query interface is more natural, so I am fine with this change. Let me know when this is ready for review

@SimonSchick
Copy link
Contributor Author

@SimonSchick SimonSchick commented May 3, 2020

Mostly looking for feedback on the 'breaking' changes here, the build should be fixed by tomorrow.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented May 3, 2020

Breaking changes you have listed are fine for me

@SimonSchick SimonSchick force-pushed the refactor/query-interface-dialects branch from 3734525 to dfb0d32 Compare May 3, 2020
@codecov
Copy link

@codecov codecov bot commented May 3, 2020

Codecov Report

Merging #12217 into master will increase coverage by 0.48%.
The diff coverage is 97.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12217      +/-   ##
==========================================
+ Coverage   95.85%   96.33%   +0.48%     
==========================================
  Files          92       95       +3     
  Lines        8949     9112     +163     
==========================================
+ Hits         8578     8778     +200     
+ Misses        371      334      -37     
Impacted Files Coverage Δ
lib/associations/helpers.js 100.00% <ø> (ø)
lib/dialects/mariadb/connection-manager.js 100.00% <ø> (ø)
lib/dialects/mysql/connection-manager.js 96.29% <ø> (ø)
lib/dialects/sqlite/query-generator.js 96.60% <ø> (ø)
lib/utils.js 98.30% <ø> (-0.06%) ⬇️
lib/dialects/abstract/query-interface.js 93.59% <93.75%> (ø)
lib/model.js 96.57% <95.83%> (ø)
lib/dialects/sqlite/query-interface.js 98.55% <98.50%> (-1.45%) ⬇️
lib/dialects/postgres/query-interface.js 98.93% <98.91%> (+0.52%) ⬆️
lib/dialects/mariadb/index.js 100.00% <100.00%> (ø)
... and 16 more

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 777fbd3...44b87eb. Read the comment docs.

@SimonSchick SimonSchick marked this pull request as ready for review May 3, 2020
@SimonSchick
Copy link
Contributor Author

@SimonSchick SimonSchick commented May 3, 2020

That was.. easier to fix than I thought.

@SimonSchick
Copy link
Contributor Author

@SimonSchick SimonSchick commented May 3, 2020

OH, there are some unrelated changes in support.js which made sqlite integration tests MOSTLY pass on windows, file locking was super fucky.

lib/dialects/mariadb/index.js Outdated Show resolved Hide resolved
@private
* Returns an object that treats MSSQL's inabilities to do certain queries.
*
* @private
Copy link
Contributor

@sushantdhiman sushantdhiman May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously noted in other comment, remove @private directive. Comment can be improved to indicate this is a MSSQL Query Interface

lib/dialects/mysql/index.js Outdated Show resolved Hide resolved
@param {object} options

@private
* Returns an object that treats MySQL's inabilities to do certain queries.
Copy link
Contributor

@sushantdhiman sushantdhiman May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update documentation here, say this is the MySQL query interface. This comment will be exposed in documentation

BREAKING CHANGE:

All instances of `QueryInterface` and `QueryGenerator` have been renamed to their lowerCamelCase variants eg. `queryInterface` and `queryGenerator` when used as property names on Model and Dialect, the class names remain the same.

`Utils.stack` has been removed.

`QueryInterface` is now an abstract class.

`query-interface.js` was moved to `lib/dialects/abstract/query-interface` and the exports have been reduced to `QueryInterface`, `default` and `module.exports` no longer refer to this class.

`QueryInterface.addConstraint`s now only takes 2 parameters, `tableName` and `options`, previously the second parameter _could_ be a list of column names to apply the constraint to, this list must now be passed in `options` as the `fields` property.
The fourth `rawTableName` parameter was entirely removed as it was effectively unused.

`QueryInterface`s `dropEnum` `dropAllEnums` and `pgListEnums` are now
ONLY available on the `PostgresQueryInterface` class, they were
previously no-ops when using any other dialog.
@SimonSchick SimonSchick force-pushed the refactor/query-interface-dialects branch from dfb0d32 to 6f8bbe5 Compare May 3, 2020
@SimonSchick
Copy link
Contributor Author

@SimonSchick SimonSchick commented May 3, 2020

@sushantdhiman I'm off for tonight, feel free to make any amends to this PR as you see fit directly rather than through comment, I will look at any changes tomorrow.

*
* @override
*/
async dropTable(tableName, options) {
Copy link
Contributor

@sushantdhiman sushantdhiman May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactor


class SQLiteQueryInterface extends QueryInterface {
/**
* A wrapper that fixes SQLite's inability to remove columns from existing tables.
Copy link
Contributor

@sushantdhiman sushantdhiman May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs improvement as well

const instanceTable = this.sequelize.modelManager.getModel(tableName, { attribute: 'tableName' });

if (!instanceTable) {
// throw error??
Copy link
Contributor Author

@SimonSchick SimonSchick May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yea, a comment here would be appreciated.

Copy link
Contributor

@sushantdhiman sushantdhiman May 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, this case is for Migrations where models may not be available. So can't throw here. Ideally low level API's should not be aware of models.

@sushantdhiman sushantdhiman merged commit c624c0e into master May 3, 2020
4 checks passed
@sushantdhiman sushantdhiman deleted the refactor/query-interface-dialects branch May 3, 2020
@papb
Copy link
Member

@papb papb commented May 18, 2020

Fantastic work, @SimonSchick !!

papb added a commit that referenced this issue May 18, 2020
for (let i = 0; i < keyLen; i++) {
if (instanceTable.rawAttributes[keys[i]].type instanceof DataTypes.ENUM) {
const sql = this.queryGenerator.pgEnumDrop(getTableName, keys[i]);
options.supportsSearchPath = false;
Copy link
Contributor

@jifeon jifeon Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimonSchick @sushantdhiman

Looks like options are optional here, so if you invoke Model.drop without options, there will be the error:

TypeError: Cannot set property 'supportsSearchPath' of undefined
    at PostgresQueryInterface.dropTable (node_modules/sequelize/lib/dialects/postgres/query-interface.js:237:36)
    at process._tickCallback (internal/process/next_tick.js:68:7)

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