-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: migrate updateQuery to typescript #17063
base: main
Are you sure you want to change the base?
Conversation
6c58304
to
1c4e81d
Compare
2cf0c07
to
91da630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preliminary review but the few comments I posted already request some pretty big changes to update
packages/core/src/dialects/abstract/query-generator-internal.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-generator-internal.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-generator-internal.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-generator-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-generator-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-interface-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-interface-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-interface-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-interface-typescript.ts
Outdated
Show resolved
Hide resolved
Looking into merging I'd propose to fix these methods once #16988 has been merged. I'll update #16988 to exclude the changes to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As always, thank you so much for your work!
I have yet to review sqlite & mssql but I already have a few comments so I'll take a break here
const returnFields: string[] = []; | ||
|
||
if (Array.isArray(options.returning)) { | ||
returnFields.push(...options.returning.map(field => { | ||
if (typeof field === 'string') { | ||
return this.queryGenerator.quoteIdentifier(field); | ||
} else if (field instanceof BaseSqlExpression) { | ||
return this.queryGenerator.formatSqlExpression(field, options); | ||
} | ||
|
||
throw new Error(`Unsupported value in "returning" option: ${inspect(field)}. This option only accepts true, false, an array of strings, or a sql expression.`); | ||
})); | ||
} else if (modelDefinition) { | ||
for (const [name, attribute] of modelDefinition.physicalAttributes) { | ||
returnFields.push(this.queryGenerator.quoteIdentifier(attribute.columnName ?? name)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small optimization, as this function is called often:
- assigned
returnFields
to the result ofoptions.returning.map
to avoid creating an unused array - used
.values()
instead of iterating thephysicalAttributes
map directly, ascolumnName
is guaranteed to be provided, and creating an array for each entry of the map is expensive
const returnFields: string[] = []; | |
if (Array.isArray(options.returning)) { | |
returnFields.push(...options.returning.map(field => { | |
if (typeof field === 'string') { | |
return this.queryGenerator.quoteIdentifier(field); | |
} else if (field instanceof BaseSqlExpression) { | |
return this.queryGenerator.formatSqlExpression(field, options); | |
} | |
throw new Error(`Unsupported value in "returning" option: ${inspect(field)}. This option only accepts true, false, an array of strings, or a sql expression.`); | |
})); | |
} else if (modelDefinition) { | |
for (const [name, attribute] of modelDefinition.physicalAttributes) { | |
returnFields.push(this.queryGenerator.quoteIdentifier(attribute.columnName ?? name)); | |
} | |
} | |
let returnFields: string[]; | |
if (Array.isArray(options.returning)) { | |
returnFields = options.returning.map(field => { | |
if (typeof field === 'string') { | |
return this.queryGenerator.quoteIdentifier(field); | |
} else if (field instanceof BaseSqlExpression) { | |
return this.queryGenerator.formatSqlExpression(field, options); | |
} | |
throw new Error(`Unsupported value in "returning" option: ${inspect(field)}. This option only accepts true, false, an array of strings, or a sql expression.`); | |
}); | |
} else { | |
returnFields = []; | |
if (modelDefinition) { | |
for (const attribute of modelDefinition.physicalAttributes.values()) { | |
returnFields.push(this.queryGenerator.quoteIdentifier(attribute.columnName)); | |
} | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this now, LMK if it looks OK.
packages/core/src/dialects/abstract/query-generator-typescript.ts
Outdated
Show resolved
Hide resolved
packages/core/src/dialects/abstract/query-generator-typescript.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
const bind = Object.create(null); | ||
const valueHash = removeNullishValuesFromHash(attrValueHash, this.options.omitNull ?? false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to slowly phase out removeNullishValuesFromHash
and the omitNull
option
Could you move its use to the places that use updateQuery
instead of keeping it here? That will make it possible to not include it in the model method rewrite that's in progress, while keeping it in the old model methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved the removeNullishValuesFromHash
logic to the model methods.
} else if (field instanceof Literal) { | ||
// Due to how the mssql query is built, using a literal would never result in a properly formed query. | ||
// It's better to warn early. | ||
throw new Error(`literal() cannot be used in the "returning" option array in ${this.dialect.name}. Use col(), or a string instead.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is a bit outdated. The logic should accept instances of Col
, and strings, but ban all other values, not just Literal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the logic now
const returnFields: string[] = []; | ||
|
||
if (Array.isArray(options.returning)) { | ||
returnFields.push(...options.returning.map(field => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments made on the parent method apply here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation update now.
|
||
generateOutputTableFragment(returnFields: string[], modelDefinition?: ModelDefinition | null): string { | ||
if (!(modelDefinition instanceof ModelDefinition)) { | ||
throw new Error('Using returning with triggers requires specifying a model or model definition.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is specific to where this method was used. modelDefinition
should be made mandatory here, and this check should be moved to where this method is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated now.
|
||
const columnsToExclude = new Set<string>(); | ||
const attributes = map(filter(returnFields, field => field !== '*'), field => { | ||
const unquotedColumn = field.replace(/^\[/, '').replace(/\]$/, ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting then unquoting is a bad idea and will break for identifiers that require escaping
The issue is that getReturnFields
handles both formatting returning
, & setting its default value.
You need to first use a function called normalizeReturning
which:
- if
returning
is true: sets it to the list of model columns when a model is available, or*
otherwise - if
returning
is false: sets it toEMPTY_ARRAY
- if
returning
is an array: leaves it as-is
Then getReturnFields
can become formatReturnField
, which only handles serialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and have updated the implementation now.
Heads up for these and your other open PRs; we've moved to prettier in #17101 which causes a lot of merge conflicts. When resolving you can just accept your changes first and then run |
To add to this, very sorry for all the conflicts I've been introducing recently |
@lohart13 @ephys just so that we are on the same page; is this order of when we want to review/merge PRs correct?
Your other PRs are on hold while we try to focus efforts on releasing v7(-beta) soonish. Read more about our plans here; https://github.com/sequelize/meetings/wiki/Meeting-2024%E2%80%9003%E2%80%9022 |
@WikiRik, yes, this order is correct. Out of interest, when would it be likely to have PR feat: add DataTypes.DATE.PLAIN #15565 reviewed/merged? |
Pull Request Checklist
Description Of Change
This PR migrates
updateQuery
methods to typescript.