-
-
Notifications
You must be signed in to change notification settings - Fork 562
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
feat(count): initial implementation of loadCount #955
Conversation
please remove all code style changes. |
Ow, didn't see all the changes made by the vscode plugin :|, will fix 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.
left some comments, but in general its looking good, ty!
I'm having an issue that has been fixed for #956 when doing tests for n:m relations ( |
Yes, you need to rebase and resolve conflicts. Or close it and create new PR if you struggle with that, and just pick the changes you made. |
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
Co-authored-by: Martin Adámek <banan23@gmail.com>
} | ||
|
||
if (refresh || !Utils.isDefined(this._count)) { | ||
this._count = await em.count(this.property.type,{ [this.property.mappedBy || this.property.inversedBy]: this.owner.__helper!.getPrimaryKey() }); |
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.
So, createCondition
was incorrect for n:m relations as it just returned a condition dictionary with empty ids: {id : {$in : [] } }
I'm not sure what I did is the correct way of creating the condition though ...
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.
createCondition
works fine, the collection items need to be set upfront to use it. in other words, it is not the method you should be using for this use case.
take a look at loadFromPivotTable
, that is where this happens (but it loads the items - their PKs - so its again not what you should use, but a method for inspiration).
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.
Are there existing entities with composite primary keys and many-to-many relationships so that I can test this case ?
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.
I'm having an issue when testing the loadCount with many-to-many relationships : |
That's a bug in knex, sqlite dialect does not support distinct count with multiple fields. I'd say ignore that for now, maybe comment that test out and point to this issue as this is blocked by knex: In the long run I am planning to remove the dependency on knex and build the queries myself to have absolute control, there are more and more issue popping up that are hard or impossible to work around... |
There are few minor things I'd do differently, but let's not clutter this up :] Last thing worth testing is how this works with mongo, as the m:n collections are implemented differently (no pivot tables). Also there is still one brach not tested in |
Yes, I still need to add more tests (mysql composite keys) |
There are no composite keys in mongo, don't worry about that. |
This pull request introduces 1 alert when merging e8dec6f into 732b45b - view on LGTM.com new alerts:
|
and mongodb n:m relations
I'm having an issue with the count method of the entitymanager for mongo-db : |
There are no joins in mongo, it is not possible to query anything from the inverse side, so yes, this is correct behaviour in mongo unfortunately. Also M:N collections are implemented as arrays on the owning entity, so the actual count on the entity will be always the correct one. You should make it conditional in a similar way to the |
All right, I think everything's good now :) |
Ok, let's merge it, thanks! |
Perfect, thanks ! |
Instead of populating a collection to get the number of items, fetches the count directly from the database. (
collection.loadCount()
)An optional parameter
refresh
(false by default) can be passed to the function to force a database query instead of using the cache.I still need to update the documentation.
Any feedback would be greatly appreciated :)