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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c4b25e
fix(types): change attributes type to 'object | undefined'
aryem Aug 28, 2021
12e1b49
style: remove addition semi colon
aryem Aug 28, 2021
60f64b1
fix(types): update type of attributes in 'BulkUpdate' & 'BulkInsert'
aryem Oct 13, 2021
d55a876
Merge branch 'main' into bug-13466
sdepold Dec 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions types/lib/query-interface.d.ts
View file
Open in desktop
Expand Up @@ -498,7 +498,7 @@ export class QueryInterface {
tableName: TableName,
records: object[],
options?: QueryOptions,
attributes?: string[] | string
attributes?: object
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@aryem aryem Oct 3, 2021 •
edited

Choose a reason for hiding this comment

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

@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) {

Copy link
Member

@wbourne0 wbourne0 Oct 5, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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> = {

Copy link
Member

Choose a reason for hiding this comment

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

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.

image

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

): Promise<object | number>;

/**
Expand All @@ -520,7 +520,7 @@ export class QueryInterface {
values: object,
identifier: WhereOptions<any>,
options?: QueryOptions,
attributes?: string[] | string
attributes?: object
): Promise<object>;

/**
Expand Down
4 changes: 2 additions & 2 deletions types/test/query-interface.ts
View file
Open in desktop
Expand Up @@ -65,9 +65,9 @@ async function test() {

await queryInterface.bulkDelete({ tableName: 'foo', schema: 'bar' }, {}, {});

const bulkInsertRes: Promise<number | object> = queryInterface.bulkInsert({ tableName: 'foo', as: 'bar', name: 'as' }, [{}], {});
const bulkInsertRes: Promise<number | object> = queryInterface.bulkInsert({ tableName: 'foo', as: 'bar', name: 'as' }, [{}], {}, {});

await queryInterface.bulkUpdate({ tableName: 'foo', delimiter: 'bar', as: 'baz', name: 'quz' }, {}, {});
await queryInterface.bulkUpdate({ tableName: 'foo', delimiter: 'bar', as: 'baz', name: 'quz' }, {}, {}, {});

await queryInterface.dropTrigger({ tableName: 'foo', as: 'bar', name: 'baz' }, 'foo', {});

Expand Down