Skip to content
This repository was archived by the owner on Apr 17, 2020. It is now read-only.

fix: is null for deleted#75

Open
boh1996 wants to merge 2 commits intoknorm:masterfrom
boh1996:fix/is-null-for-deleted
Open

fix: is null for deleted#75
boh1996 wants to merge 2 commits intoknorm:masterfrom
boh1996:fix/is-null-for-deleted

Conversation

@boh1996
Copy link

@boh1996 boh1996 commented Dec 9, 2018

When using left join on possible null values or doing a one to many join, that are allowed to return zero results. Then the deleted = false check would mean that the rows are taken out of the query.
Therefore i also added an or IS NULL.

@coveralls
Copy link

coveralls commented Dec 9, 2018

Pull Request Test Coverage Report for Build 392

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.205%

Totals Coverage Status
Change from base Build 367: 0.0%
Covered Lines: 89
Relevant Lines: 91

💛 - Coveralls

@joelmukuthu
Copy link
Collaborator

Thanks for the contribution @boh1996! It’s quite the change though, to consider rows with deleted: null as non-deleted rows (or vice-versa). In fact, that will be a breaking change. So we have to limit the scope of this to

  • only joined queries (this.config.joined === true) and
  • only when no other deletion options (withDeleted, onlyDeleted, where({ deleted: true|false })) have been set.

That would still solve the original bug, right?

For the tests, we’ll need to use @knorm/relations to test it in scope of joined queries. Also we need to test that deleted: null is not applied for:

  • non-joined queries, with and without deletion options
  • joined queries with deletion options

Would you like me to help out or you got it covered? :)

@boh1996
Copy link
Author

boh1996 commented Jan 2, 2019

Hey @joelmukuthu !
Requiring this part: "only when no other deletion options (withDeleted, onlyDeleted, where({ deleted: true|false })) have been set." will not solve the bug, because i am quite sure that i tested with withDeleted and it still failed.

If you have the time i think it would be nice to have some help - because you know the codebase better, the plugin setup and your testing setup.

@boh1996
Copy link
Author

boh1996 commented Mar 4, 2019

@joelmukuthu ping!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants