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

test case for filtering null and not null on relation #539

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

daniel-maegerli
Copy link

As discussed, I've written a new test case for checking $null on relations, and changed the existing test case for checking properly on $not:$null on relations. The first test case is failing as expected/described within the issue. Since this is my first time working on this repo (and submitting a PR to an open source project in general), let me know if this is fine.

Issue #538

@ppetzold
Copy link
Owner

Oh I see. I think the issue is simply that we always LEFT JOIN. So nulled relations will be listed as well. I suppose it must be INNER JOIN to achieve what you expect.

I am currently only working on a sequelize project. Sequelize has the require property on joins. Looking over TypeORM's docs I cannot find the equivalent. Do you know what TypeORMs notation for this is? If so, we can try to imitate.

@Helveg Helveg force-pushed the issue-538-test-case-null-on-relations branch from 58ee944 to be99407 Compare September 29, 2024 14:04
@Helveg
Copy link
Collaborator

Helveg commented Oct 4, 2024

@ppetzold I fixed the null queries on relationships by storing that when we use a relationship in a filter that it should be inner joined. On top of that I allow the user to configure the default join method, and the join method per column, since this all converged on the same approach I needed to extend for building the relationships either way.

Oh and and !!! IMPORTANT !

Important

It also fixes an error in the following scenario:

{ filterableColumns: ['home.countCat'] }

Using that filter without including the home in relations used to throw errors like:

column "__root_home_rel_countCat" does not exist

That's fixed now! Whenever you use a relationship in filter it is automatically inner joined :)

This PR is ready for review :)

@Helveg Helveg requested review from Helveg and ppetzold October 4, 2024 16:22
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.

3 participants