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: allow where IsNull for ManyToOne relations #9031

Merged
merged 5 commits into from Aug 25, 2022

Conversation

pixtron
Copy link
Contributor

@pixtron pixtron commented May 23, 2022

Fixes: #8890

Description of change

Allows to query ManyToOne relations for records that do not have any relation (IS NULL).

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run format to apply prettier formatting
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@jczacharia
Copy link

jczacharia commented Jul 24, 2022

This doesn't appear to work with OR operations:
E.g.:

    return this.caseRepo.find({
      where: [
        {
          snf: { id: In(queriedIds) },
        },
        {
          snf: IsNull(),
        },
      ],
    });

It appears because the query is doing an INNER JOIN and not a LEFT JOIN for the IN operator.

@pixtron
Copy link
Contributor Author

pixtron commented Jul 25, 2022

@jczacharia this workaround might work for you.

@pleerock pleerock merged commit 72728f1 into typeorm:master Aug 25, 2022
@pleerock
Copy link
Member

Thank you for contribution! 🎉

nordinh pushed a commit to nordinh/typeorm that referenced this pull request Aug 29, 2022
* fix: allow where IsNull for ManyToOne relations

Closes: typeorm#8890

* fix direct FindOperator queries on MTO relation

* fix: allow FindOperator queries on OTO owner column

* chore: prettier formating
@LoneRifle
Copy link
Contributor

LoneRifle commented Apr 18, 2023

Hello @pixtron @pleerock - this may have introduced a regression where selecting for OneToMany relations using moreThanOrEqual or lessThanOrEqual operators are now disallowed. Is this intended, and if not, should a PR be submitted so that for relation.isOneToMany we do not throw the error?

@pixtron
Copy link
Contributor Author

pixtron commented Apr 18, 2023

@LoneRifle i just removed the throw you are mentioning locally, it wouldn't solve your issue.
But you are pretty close. moreThanOrEqual and lessThanOrEqual should probably be implemented here for relation.isOneToMany, relation.isManyToManyOwner and relation.isManyToManyNotOwner.

I had a quick test with this change, which made moreThanOrEqual and lessThanOrEqual working for isOneToMany and probably will for the other relations too.

diff --git a/src/query-builder/SelectQueryBuilder.ts b/src/query-builder/SelectQueryBuilder.ts
index d3b01053f..11b5b0ea4 100644
--- a/src/query-builder/SelectQueryBuilder.ts
+++ b/src/query-builder/SelectQueryBuilder.ts
@@ -4321,12 +4321,16 @@ export class SelectQueryBuilder<Entity extends ObjectLiteral>
                     }

                     if (InstanceChecker.isFindOperator(where[key])) {
+                        const { type } = where[key];
                         if (
-                            where[key].type === "moreThan" ||
-                            where[key].type === "lessThan"
+                            type === "moreThan" ||
+                            type === "moreThanOrEqual" ||
+                            type === "lessThan" ||
+                            type === "lessThanOrEqual"
                         ) {
-                            const sqlOperator =
-                                where[key].type === "moreThan" ? ">" : "<"
+                            const baseOperator = /^moreThan/.test(type) ? ">" : "<"
+                            const sqlOperator = baseOperator + (/OrEqual$/.test(type) ? '=' : '')
+
                             // basically relation count functionality
                             const qb: QueryBuilder<any> = this.subQuery()
                             if (relation.isManyToManyOwner) {

Probably the best is if you open a new issue, and create a PR for that issue. The PR probably has the best chance to get merged if you thoroughly test moreThanOrEqual and lessThanOrEqual for isOneToMany, isManyToManyOwner and isManyToManyNotOwner relations.

Also one could argue that the Find operators Equal, Between (and maybe some more?) should be supported as well for relations. But thats eventually yet another issue and PR.

@LoneRifle
Copy link
Contributor

LoneRifle commented Apr 19, 2023

@pixtron - I got it to work, not by removing the throw, but not throw the exception by changing the else to else if (!relation.isOneToMany) (edit: but as you point out, it did not work as intended). Also corroborated this by testing on 0.3.7 (working) and 0.3.8 (not working, contains this change), which was how I discovered this PR.

My condition change felt pretty hacky though, so I did a more invasive change similar to yours (with String.startsWith and String.endsWith instead of regexs) and verified that this other approach works just as well.

I'll see if I can find the time to submit a PR. Submitted #9978. While MoreThanOrEqual on relations matters to us, we could possibly workaround with the equivalent MoreThan expression.

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.

Unable to query data where relationship needs to be null using Repository method
4 participants