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): allow Op.contains on a jsonb column #16167

Merged
merged 5 commits into from
Jul 21, 2023
Merged

Conversation

jwatzman
Copy link
Contributor

@jwatzman jwatzman commented Jun 23, 2023

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository? N/A
  • 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?
  • Does the name of your PR follow our conventions?

Description Of Change

In the case of a jsonb column, this wasn't working properly. Previously, the below would be a TypeScript error; this fixes it. Because a jsonb column isn't a Range, we need to add a specific case for it.

const someJson = ...;
const e = await MyEntity.findAll({
  where: {
    myJsonbCol: { [Op.contains]: someJson },
  },
});

In the case of a jsonb column, this wasn't working properly. Previously,
the below would be a TypeScript error; this fixes it. Because a jsonb
column isn't a Range, we need to add a specific case for it.

```
const someJson = ...;
const e = await MyEntity.findAll({
  where: {
    myJsonbCol: { [Op.contains]: someJson },
  },
});
```
packages/core/src/model.d.ts Outdated Show resolved Hide resolved
@jwatzman
Copy link
Contributor Author

@ephys thanks for taking a look! I think I properly fixed the issue with interfaces (by using object instead of Record<>), and have attempted to add some tests. Hopefully I did everything right!

@jwatzman jwatzman requested a review from ephys June 23, 2023 16:58
packages/core/test/unit/sql/where.test.ts Outdated Show resolved Hide resolved
packages/core/test/unit/sql/where.test.ts Outdated Show resolved Hide resolved
packages/core/test/unit/sql/where.test.ts Outdated Show resolved Hide resolved
packages/core/src/model.d.ts Outdated Show resolved Hide resolved
@jwatzman jwatzman requested a review from ephys June 26, 2023 11:08
@jwatzman
Copy link
Contributor Author

Hey @ephys, anything else I need to do here in order for this to be merged? (And apologies if I haven't set the github review status correctly, I'm not super familiar with the procedural conventions in OSS projects!)

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.

I think this should be good, thank you!

@ephys ephys added this pull request to the merge queue Jul 21, 2023
Merged via the queue into sequelize:main with commit 70c2bf5 Jul 21, 2023
48 checks passed
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