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

feat(count): initial implementation of loadCount #955

Merged
merged 15 commits into from
Oct 27, 2020
Merged

feat(count): initial implementation of loadCount #955

merged 15 commits into from
Oct 27, 2020

Conversation

AzariasB
Copy link
Contributor

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 :)

@B4nan
Copy link
Member

B4nan commented Oct 23, 2020

please remove all code style changes.

@AzariasB
Copy link
Contributor Author

Ow, didn't see all the changes made by the vscode plugin :|, will fix that

Copy link
Member

@B4nan B4nan left a 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!

packages/core/src/entity/Collection.ts Outdated Show resolved Hide resolved
packages/core/src/entity/ArrayCollection.ts Outdated Show resolved Hide resolved
packages/core/src/entity/ArrayCollection.ts Outdated Show resolved Hide resolved
tests/issues/GH949.test.ts Show resolved Hide resolved
tests/issues/GH949.test.ts Show resolved Hide resolved
tests/issues/GH949.test.ts Show resolved Hide resolved
tests/issues/GH949.test.ts Show resolved Hide resolved
tests/issues/GH949.test.ts Show resolved Hide resolved
@AzariasB
Copy link
Contributor Author

AzariasB commented Oct 24, 2020

I'm having an issue that has been fixed for #956 when doing tests for n:m relations (Cannot read property '__meta' of undefined), should I merge mikro-orm master's branch into my own ?

@B4nan
Copy link
Member

B4nan commented Oct 24, 2020

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.

}

if (refresh || !Utils.isDefined(this._count)) {
this._count = await em.count(this.property.type,{ [this.property.mappedBy || this.property.inversedBy]: this.owner.__helper!.getPrimaryKey() });
Copy link
Contributor Author

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 ...

Copy link
Member

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).

Copy link
Contributor Author

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AzariasB AzariasB requested a review from B4nan October 24, 2020 10:22
@AzariasB
Copy link
Contributor Author

AzariasB commented Oct 24, 2020

I'm having an issue when testing the loadCount with many-to-many relationships :
With composite keys, the count queries looks like : select count(distinct `e0`.`first_name`, `e0`.`last_name`) as count ... which is not a valid SQL query (too many arguments for count). I'm not sure how to deal with this ?

@B4nan
Copy link
Member

B4nan commented Oct 24, 2020

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:

#889

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...

@B4nan
Copy link
Member

B4nan commented Oct 26, 2020

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 Collection.ts:270.

@AzariasB
Copy link
Contributor Author

Yes, I still need to add more tests (mysql composite keys)
For mongo, is there an existing test file for composite keys ?

@B4nan
Copy link
Member

B4nan commented Oct 26, 2020

There are no composite keys in mongo, don't worry about that.

@lgtm-com
Copy link

lgtm-com bot commented Oct 26, 2020

This pull request introduces 1 alert when merging e8dec6f into 732b45b - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

and mongodb n:m relations
@AzariasB
Copy link
Contributor Author

AzariasB commented Oct 26, 2020

I'm having an issue with the count method of the entitymanager for mongo-db :
The tests fails when using count with a filter. Am I using the filters incorrectly ?

@B4nan
Copy link
Member

B4nan commented Oct 26, 2020

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 Collection.init() (based on reference type and Platform.usesPivotTable method).

@AzariasB
Copy link
Contributor Author

All right, I think everything's good now :)

@B4nan
Copy link
Member

B4nan commented Oct 27, 2020

Ok, let's merge it, thanks!

@B4nan B4nan changed the title feat(count): initial implementation of loadCount (WIP) feat(count): initial implementation of loadCount Oct 27, 2020
@B4nan B4nan merged commit 3371415 into mikro-orm:master Oct 27, 2020
@AzariasB
Copy link
Contributor Author

AzariasB commented Oct 27, 2020

Perfect, thanks !
It was a bit tougher than I expected
Thank a lot for the help throughout this pull request :), and thank you very much for the work you put into mikro-orm !

@AzariasB AzariasB deleted the load-count branch October 27, 2020 10:35
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.

2 participants