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 count helper function for self-referencing relations #22297

Merged
merged 4 commits into from
Apr 24, 2024

Conversation

hanneskuettner
Copy link
Member

@hanneskuettner hanneskuettner commented Apr 23, 2024

Scope

With a self referencing collection (e.g. node with the related field parent and children), when to filter it with the count(*) function the following query was generated:

select * from `node` where ((select count(*) from `node` where `parent` = `node`.`id`) > ?) limit ?

The inner where clause mistakingly references the inner table node for comparison, instead of the outer node table.

This PR fixes this by always aliasing the table in the inner where generated by the _relationalCount helper:

select * from `node` where ((select count(*) from `node` as `pizml` where `pizml`.`parent` = `node`.`id`) > ?) limit ?

What's changed:

  • Introduce table alias in _relationalCount helper method

Potential Risks / Drawbacks

  • None? This change should only introduce an alias that is redundant in the best case, and necessary in the other cases.

Review Notes / Questions

  • Since blackbox tests aren't really a thing right now for different DB vendors I have tested this change on
    • SQLite ✅
    • Postgres ✅
    • MySQL ✅
    • MariaDB ✅
    • CockroachDB ✅
    • MSSQL ❓
    • Oracle ❓
    • Redshift ❓

Fixes #14338

Copy link

changeset-bot bot commented Apr 23, 2024

🦋 Changeset detected

Latest commit: 55c60b6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Nice fix!

@paescuj paescuj merged commit bc3f698 into main Apr 24, 2024
4 checks passed
@paescuj paescuj deleted the fix-14338-self-referencing-count branch April 24, 2024 12:07
@github-actions github-actions bot added this to the Next Release milestone Apr 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Issue with filtering using recursive O2M count
2 participants