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(types): fix QueryInterface#bulkInsert attribute arg type #13945

Merged
merged 9 commits into from Jan 14, 2022

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Jan 13, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • (N/A) Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This is a very small PR to improve the typings to bulkInsert and the JSON/JSONB data types.

Motivation: I have a typescript seeders file that is seeding some JSONB columns in a postgres database. The seed file uses queryInterface.bulkInsert to insert a bunch of rows read from a file. But since queryInterface does not know the model column types, it does know to convert a few specific columns to JSONB. It was suggested in #8310 (comment) to pass attributes as a 4th arg to bulkInsert. But the existing types for attributes seems to be incorrect. (Additionally I discovered new Sequelize.JSONB() was throwing a "not constructable" type error.)

PS: I'm pretty new to ORMs. Sequelize has been great so far. That's for it!

An alternative to this might be to give `AbstractDataTypeConstructor` a constructor method, along the lines of:

```ts
interface AbstractDataTypeConstructor {
  new (): AbstractDataType;
  key: string;
  warn(link: string, text: string): void;
}
```

Then all the types would become constructable. Is that desirable?
@ChristopherChudzicki
Copy link
Contributor Author

ChristopherChudzicki commented Jan 13, 2022

Both DB2 CI tests are failing with

A communication error has been detected. Communication protocol being used: "TCP/IP".  Communication API
being used: "SOCKETS".  Location where the error was detected: "127.0.0.1".  Communication function
detecting the error: "selectForConnectTimeout".  Protocol specific error code(s):
"115", "*", "*".  SQLSTATE=08001

https://github.com/sequelize/sequelize/runs/4798649046?check_suite_focus=true#step:7:2122

unsure what that is :/

@ephys
Copy link
Member

ephys commented Jan 13, 2022

Don't worry about the DB2 tests, we'll merge anyway :)

ephys
ephys previously requested changes Jan 13, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

It's looking good. All that's missing is a type-test in the types/test folder (basically just use the method with the right parameters).

Also, is your issue with AbstractDataTypeConstructor resolved if we change its definition from

interface AbstractDataTypeConstructor {
  key: string;
  warn(link: string, text: string): void;
}

to

interface AbstractDataTypeConstructor extends Function {
  key: string;
  warn(link: string, text: string): void;
}

instead of adding the constructor? Because that's a change I was planning on doing

@ChristopherChudzicki
Copy link
Contributor Author

ChristopherChudzicki commented Jan 13, 2022

@ephys Thanks for the quick review! I'll take a look at the types/test.

I don't think that making interface AbstractDataTypeConstructor extends Function {...} is sufficient. (See example: https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgIICMDOYqLAETjDgBUBPABwgGEB7EbKAVwTFqmQgA9IQATTMgBiTEK2D1kAbwBQyZAGsIZAFzJGoAOYBuOcgDucKCAAUAG1AK1GkJoA0ySD2s4tASjUA3WsD66AvjIyCPTY6nAgwGBk1AAWEAgKyAC8yCYAyrQAthCExORUahiMeHmklDShOCxsUG4pAHzSegD0LY6YelAQYEzGyCAQ+siZOWUFECZuAdpAA ). My understanding is that interface X extends Y {..} means that X has methods/properties of instances of Y. Since Function instances don't have a new method, the AbstractDataTypeConstructor won't either.

Adding new to AbstractDataTypeConstructor

This might be a better option than what I did in the PR:

interface AbstractDataTypeConstructor {
  key: string;
  warn(link: string, text: string): void;
  new (): AbstractDataType;
  (): AbstractDataType;
}

then:

  1. these would all become constructable:
    export const JSON: AbstractDataTypeConstructor;
    export const JSONB: AbstractDataTypeConstructor;
    export const CIDR: AbstractDataTypeConstructor;
    export const INET: AbstractDataTypeConstructor;
    export const MACADDR: AbstractDataTypeConstructor;
    export const CITEXT: AbstractDataTypeConstructor;
    export const TSVECTOR: AbstractDataTypeConstructor;
    This seems desirable?
  2. DateDataType would become constructable, too:
    interface DateDataTypeConstructor extends AbstractDataTypeConstructor { /* omitted from this comment* /}
    export interface DateDataType extends AbstractDataTypeConstructor {
      options: DateDataTypeOptions;
    }
    But it seems like maybe there's a typo here and DateDataType should be extending AbstractDataType?

I was trying to keep my changes a little more isolated, but if you have any thoughts on the above approach, it might be better.

Looking around issues/PRs, I see sequelize is moving to convert more of the repo to Typescript, so hopefully these manual d.ts typings won't be permanent...

PS: Also, should I rebase or squash the commits or anything?

@ephys
Copy link
Member

ephys commented Jan 13, 2022

Thank you for the tests!

I don't think that making interface AbstractDataTypeConstructor extends Function {...}

You're right, let's continue with your solution

Adding new to AbstractDataTypeConstructor

That means new DataTypes.JSON will return AbstractDataType instead of JSONDataType but given it's all typing already anyway (you can't use instanceof on AbstractDataType), it doesn't really matter.
Go for it!

But it seems like maybe there's a typo here and DateDataType should be extending AbstractDataType?

Definitely looks like a mistake, would you mind adding it to your changes?

I was trying to keep my changes a little more isolated, but if you have any thoughts on the above approach, it might be better.

I'm fine with including a small change like this one

Looking around issues/PRs, I see sequelize is moving to convert more of the repo to Typescript, so hopefully these manual d.ts typings won't be permanent...

Yes! :) Hopefully we'll be done with this soon enough
There's already some work in progress to migrate DataTypes #13799

PS: Also, should I rebase or squash the commits or anything?

No need, unless you've carefully crafted your commits & request to not squash, we squash and merge PRs :)


// JSONB
expectTypeOf(new JSONB()).toEqualTypeOf<DataTypes.AbstractDataType>();
expectTypeOf(JSONB()).toEqualTypeOf<DataTypes.AbstractDataType>();
Copy link
Member

Choose a reason for hiding this comment

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

Can you make sure that there is an empty line at the end of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, apparently I failed at the new-line even after re-requesting review. Looking into it... I have the newline locally, but it seems not to be showing up on github, which is confusing me. Looking into it....

I see what you meant 👍
Screen Shot 2022-01-13 at 6 19 09 PM

should extend AbstractDataType not AbstractDataTypeConstructor
for the types affected by new constructor signature on AbstractDataType
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

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.

Yes, just needs a better title and then we can merge. @ephys can you take care of that and add this to the v6 project?

@ephys ephys added the type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Jan 14, 2022
@ephys ephys changed the title Bulk insert types fix(types): fix queryInterface.bulkInsert attribute arg type Jan 14, 2022
@ephys ephys changed the title fix(types): fix queryInterface.bulkInsert attribute arg type fix(types): fix QueryInterface#bulkInsert attribute arg type Jan 14, 2022
@ephys ephys merged commit 36ad644 into sequelize:main Jan 14, 2022
@ephys
Copy link
Member

ephys commented Jan 14, 2022

Thank you @ChristopherChudzicki :)

sdepold pushed a commit that referenced this pull request Jan 22, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

🎉 This PR is included in version 7.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
…ze#13945)

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants