sequelize icon indicating copy to clipboard operation
sequelize copied to clipboard

feat: add nested col filtering

Open molaux opened this issue 2 years ago • 1 comments

Pull Request Checklist

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

Description Of Change

Hi,

just a PR to propose a solution to filter models on nested cols without filtering includes.

I may have done things wrong, but I was unable to use $Nested.col$ with offset and limit (as far as I can see, because of includes left joins are not included in limit/offset sub query).

The other problem I encountered is that such nested cols in WHERE clause, are filtering my eager loaded includes. Instead, what I was searching for, is a way to filter main models on its associations but not its associations themselves.

Instead of opening an issue, I investigated the code to try to understand what I missed and then wrote my solution that I share here in case it can be useful.

In order to not conflict with $Nested.col$ syntax, I introduced a #Nested.filter# that injects a subQueryFilter with _generateSubQueryFilter.

Product.findAll({
  limit: 6,
  order: [
    ['id', 'ASC']
  ],
  where: {
    '#Prices.value#': {
      [Op.gt]: 10
    }
  },
  include: [ Price ]
})  

Here we get all products having at least one Price over 10, but eager loading all their Prices. My proposal has the benefit to work with limits and offsets.

I'm obviously open to suggestions or other ways to solve my need,

Thank you for your attention and all your work :)

Marc-Olivier

Todos

  • [ ] test with other db than postgres
  • [ ] ...certainly a lot of things, let me know

molaux avatar Aug 03 '23 10:08 molaux

I appreciate the PR, but I don't want to add anything to findAll without properly redesigning it first, to make sure all options play nicely with each-other. findAll has had a history of things being added to it that were not fully fleshed out

There is an open issue about adding a feature that seems to be what you need: whereHas - https://github.com/sequelize/sequelize/issues/15089

There is also a plan to completely redesign the findAll API: https://github.com/sequelize/sequelize/issues/15260. The document is a bit outdated, as some of the features described there are now available but slightly different, but I would still recommend joining that thread so we can discuss what an API could look like

ephys avatar Aug 16 '23 06:08 ephys

This pull request has been marked as maybe-abandoned and has not seen any activity for 14 days. It will be closed if no further activity occurs within the next 30 days. If you would still like to work on this, please comment to let us know and the label will be removed.

github-actions[bot] avatar Feb 26 '24 00:02 github-actions[bot]