-
Notifications
You must be signed in to change notification settings - Fork 43
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: Apply filters from relations to query #207
Conversation
This change allows us to pass filters to the relatinoships of the query. Sample usage:
Output SQL
Previously, the relations' filters would not be passed down, and would result in the below query instead:
It appears that Doug left a comment in the code indicating that this was a missing feature, so i've removed it, and addressed it by calling applyFilters from the applyRelationJoinsRecursive logic. |
Shit, this is a good one. Thanks! It's true that the filters where not applied, mainly also when implementing the lookahead this is something that could never be passed down by the query itself so there was no need yet to implement that. |
Could you check the linting and please use conventional commits for the changelog. There is also no need to bump the package.json :) |
name: 'oneTestRelation', | ||
query: { | ||
filter: { | ||
numberType: { eq: 123 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it me or don't I see this one back inside the snapshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it looks like you're right. I'll try to fix. Although it looks like some of the other where filters are not being built in the snapshots for even some of the other things unrelated to this change 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments have been addressed: 0ec70e2
In this commit, I address the issue where relations filters were not being applied. Also adding tests.
ab2ea6f
to
25b892f
Compare
I've no idea what i'm doing, in regards to commit history, haha. let me know if it needs to be cleaned up. Usually i'm the type of person that does test, test, oops, test, test, fixed a thing, so this styling is new to me! |
@@ -121,6 +130,22 @@ describe('RelationQueryBuilder', (): void => { | |||
const query: Query<TestRelation> = { filter: { relationName: { eq: 'foo' } } } | |||
expectSQLSnapshot(TestEntity, testEntity, 'testRelations', query) | |||
}) | |||
it('should apply filtering from relations query filter', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I move this to filter-query tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea let's do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
55fa165 addressed!
Thanks for your work on this already 🔥 ! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #207 +/- ##
==========================================
+ Coverage 79.15% 86.28% +7.13%
==========================================
Files 688 688
Lines 9748 9750 +2
Branches 857 858 +1
==========================================
+ Hits 7716 8413 +697
+ Misses 1288 619 -669
+ Partials 744 718 -26 ☔ View full report in Codecov by Sentry. |
@TriPSs this is ready for your approval. As a side question - once this is merged, will it auto increment the version in package json ? If not, when/how do i pull down the new version with the changes |
The CI will bump versions and release it. |
No description provided.