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: use different default schema per dialect, escape quotes in identifiers instead of removing them, normalize Model.getTableName #15274

Merged
merged 23 commits into from Nov 17, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Nov 11, 2022

Description Of Change

This PR is yet-another preliminary PR for #14687 in order to prevent that other PR from growing endlessly.

The goal of this PR is to:

  • Introduce AbstractQueryGeneratorTypeScript and migrate some base QueryGenerator methods to TypeScript
  • Normalize which schema is used by introducing getDefaultSchema, and rewriting extractTableDetails. We should eventually arrive at a state where all query generator methods default the table schema to either the schema set through Sequelize's options, or the per-dialect default schema.
  • change quoteIdentifier with one very important change:
    queryGenerator.quoteIdentifier('"abc"') previously unquoted then quoted its input. Quotes are actually valid as identifiers, as long as they're escaped. Instead of returning "abc", this method will now return """abc""" (triple " because one is the actual identifier delimiter, and two consecutive "" is how we escape ").
  • deprecate addTicks, removeTicks, and TICK_CHAR, wrapSingleQuote for the above reason

Because quoteIdentifier doesn't remove existing quotes anymore, I had to tweak parts of queryGenerator. Functions like selectQuery passed its identifiers to quoteIdentifier multiple times. Causing duplicate quotes.


My current plan for AbstractQueryGeneratorTypeScript is to only move APIs once they are considered stable & suitable for public use.

@ephys ephys changed the title fix: cleanup query generator fix: use different default schema per dialect, escape quotes in identifiers instead of removing them Nov 12, 2022
@ephys ephys changed the title fix: use different default schema per dialect, escape quotes in identifiers instead of removing them fix: use different default schema per dialect, escape quotes in identifiers instead of removing them, normalize Model.getTableName Nov 12, 2022
@ephys ephys marked this pull request as ready for review November 12, 2022 18:35
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

src/dialects/abstract/query-interface.d.ts Show resolved Hide resolved
src/dialects/abstract/query-interface.js Show resolved Hide resolved
src/dialects/db2/index.ts Show resolved Hide resolved
src/dialects/db2/query-generator.js Show resolved Hide resolved
src/dialects/snowflake/index.ts Outdated Show resolved Hide resolved
src/utils/dialect.ts Outdated Show resolved Hide resolved
test/integration/model/create.test.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
@ephys ephys requested a review from WikiRik November 15, 2022 21:33
@WikiRik WikiRik marked this pull request as draft November 16, 2022 13:13
@WikiRik
Copy link
Member

WikiRik commented Nov 17, 2022

SQLite unit tests fail since I included a test for the schema, but we've said that sqlite does not support schemas so we'll need to see how to deal with that

@WikiRik WikiRik marked this pull request as ready for review November 17, 2022 17:20
@WikiRik WikiRik merged commit 778c6dc into main Nov 17, 2022
@WikiRik WikiRik deleted the ephys/query-generator-cleanup branch November 17, 2022 17:35
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