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

Add support for a list of taggable IDs in the tagging conditions #1053

Merged

Conversation

moliver-hemasystems
Copy link
Contributor

In the event of trying to fetch tags for multiple records, where the IDs of those records are already known, adding support for the :id option to accept an array of IDs to use directly, instead of using a subquery to find the same list of IDs.

I had run into a situation, where I was trying to get the tags for a series of records associated to the ones I was listing, and SQL resulting from calling tag_counts_on was using a subquery to fetch the IDs that were already being used in the query chain.

For example, say I was listing cases for multiple projects, and wanted to be able to filter those cases by tags on the projects. If I did:

Project.where(id: cases.distinct.pluck(:project_id)).tag_counts_on(:tags)

... it would result in a SQL that used the following sub-query:

AND taggings.taggable_id IN (
  SELECT "projects"."id"
  FROM "projects"
  WHERE "projects"."id" IN ({list of IDs})
)

This is causing the database to do extra work. There's an option that can be used to pass in an ID to tag_counts_on, but it currently only works with a single ID. This PR aims to extend that option to support an array of IDs as well, so the following could be used:

Project.tag_counts_on(:tags, id: cases.distinct.pluck(:project_id))

... and have the generated SQL be this instead:

AND taggings.taggable_id IN ({list of IDs})

@moliver-hemasystems moliver-hemasystems marked this pull request as ready for review October 19, 2021 21:30
@seuros seuros self-assigned this Nov 9, 2021
@seuros seuros modified the milestones: 6.0.0, 9.0.0 Nov 9, 2021
@seuros
Copy link
Collaborator

seuros commented Nov 9, 2021

@moliver-hemasystems Thank you for your contribution, can you rebase your PR ? I will release it in version 9.0.0

In the event of trying to fetch tags for multiple records, where the IDs of those records are already known, adding support for the `:id` option to accept an array of IDs to use directly, instead of using a subquery to find the same list of IDs.
@moliver-hemasystems moliver-hemasystems force-pushed the taggable_conditions_id_list_support branch from eca3233 to 2526083 Compare November 9, 2021 17:11
@moliver-hemasystems
Copy link
Contributor Author

I rebased the PR. I didn't think of it at the time, but did you want me to adjust the changelog/version?

@seuros
Copy link
Collaborator

seuros commented Nov 9, 2021

Yes, please.

Per feedback, bumping the versions from 8.2.0 to 9.0.0
@seuros seuros merged commit 52d7dae into mbleigh:master Nov 9, 2021
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.

2 participants