fix(types): change attributes type to 'object | undefined'

[aryem]

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

change attributes type to 'object':

• attributes param in QueryInterface.bulkInsert() change to 'object | undefined' from 'string[] | string | undefined'.
• attributes param in QueryInterface.bulkUpdate() change to 'object | undefined' from 'string[] | string | undefined'.
• add '{}' as last param in bulkInsert() & bulkUpdate() tests

Closes #13466

[wbourne0]

huh, why are you using the query interface directly? Is this correct on the model definition but not queryInterface?

[wbourne0]

This isn't right either actually.

I don't know if there's already a type for this or not. At the top of my head, it should be moreso like:

type Attribute = Literal | string | [Literal | Fn | Col , string]

type Attributes = Array<Attribute> | { include?: Array<Attribute>, exclude: Array<Attribute> }

note: this is mostly psuedocode, I don't remember the exact typings for col, fn, and literals.

[aryem]

@allawesome497 Thanks for the suggestions and you are right. It isn't correct either. I think ModelAttributes from Model.d.ts seems to be appropriate here. What do you say?.

sequelize/types/lib/model.d.ts

Line 1358 in 23aa67e

export type ModelAttributes<M extends Model = Model, TCreationAttributes = any> = {

I might be wrong here, But my suggestion are deduced from its usage in the following methods:

sequelize/lib/dialects/abstract/query-generator.js

Line 250 in 23aa67e

bulkInsertQuery(tableName, fieldValueHashes, options, fieldMappedAttributes) {

sequelize/lib/dialects/abstract/query-generator.js

Line 337 in 23aa67e

updateQuery(tableName, attrValueHash, where, options, attributes) {

[wbourne0]

Such as this?

type Attribute<
  ModelAttr extends string
> = Literal | ModelAttr | [Literal | Fn | Col, ModelAttr];

type Attributes<
  ModelAttr extends string = string
> = Array<
  Attribute<ModelAttr> | {
    include?: Array<Attribute<ModelAttr>>,
    exclude:? Array<Attribute<ModelAttr>>,
  }
>;

In usage:

attributes?: Attributes<keyof ModelAttributes>

If so, then I think that would be the ideal solution.

[aryem]

@allawesome497 Sorry, I don't understand why do we need { include?: Array<Attribute<ModelAttr>>, exclude:? Array<Attribute<ModelAttr>>, } and in what scenario Attribute could be Literal | ModelAttr | [Literal | Fn | Col ?.

Based on the usage in QueryInterface.BulkUpdate & QueryInterface.BulkInsert type of attributes should:

attributes?: ModelAttributes<any, any>

ModelAttributes interface is already defined in model.d.ts

sequelize/types/lib/model.d.ts

Line 1361 in 5e9c209

export type ModelAttributes<M extends Model = Model, TCreationAttributes = any> = {

[wbourne0]

So { include?: Array<Attribute<ModelAttr>>, exclude:? Array<Attribute<ModelAttr>>, } is needed because you can specify those instead of an array. It's mentioned here, though it doesn't go into depth about it.

in what scenario Attribute could be Literal | ModelAttr | [Literal | Fn | Col]
So sequelize allows the following attributes:

• col: the name of a column. Might not be accepted in sequelize, not sure.
• fn: A function that is to server as an attribute. Say, fn('COUNT', col('my_column')) for example. This might need to have an alias
• literal: Literal sql to be injected as an attribute. I believe this needs an alias specified, so it would only be valid in that tuple.
• [item, alias]: this is translated to ${item} AS ${alias}.

I could be wrong on a few things here though, since this is all from memory. I'm confident on fn and literal being usable in attributes though, at least with an alias specified.

[wbourne0]

Oh I've recently seen another type that I didn't realized existed - SequelizeMethod. This is going to be what we want, rather than Literal | Fn | Col as those functions extend this.

[ephys]

We merged #13945 a little while ago which updated bulkInsert. I'm pretty sure the new typings for bulkInsert are correct and should be an object instead of an array based on their usage in query-generator.js

bulkUpdate hasn't been touched by that other PR however based on their usage in updateQuery in query-generator, it looks like their type should be an object too, not an array

If you're still willing to update this pretty old PR (sorry about that), I can take a look into merging this

[ephys]

Looks like this has since been fixed by another. Thank you for opening this one nonetheless. Sorry we never got around to merging it :)