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

Count work when no result is found #1135

Closed
wants to merge 1 commit into from
Closed

Conversation

ramiel
Copy link

@ramiel ramiel commented Nov 27, 2020

This should fix when grouping and no result is found

This should fix when grouping and no result is found
@B4nan
Copy link
Member

B4nan commented Nov 27, 2020

To maintain the full branch coverage, this will either need a test case (that would otherwise fail without that change) or at least ignore hint for istanbul (as using optional chaining usually reports missed branches due to how it is transpiled).

@@ -174,7 +174,7 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
.where(where);
const res = await this.rethrow(qb.execute('get', false));

return +res.count;
return +res?.count || 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is it even possible that res will not be defined? I don't think so...

Copy link
Author

Choose a reason for hiding this comment

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

this is what happened to me. Sorry I should have filed a bug actually

Copy link
Author

Choose a reason for hiding this comment

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

I'm running this

await this.myRepository.count(
        { externalId: externalId },
        { groupBy: 'user' },
      );

this fails if there's no entry in myRepository that satisfies the condition. The error says cannot read count of undefined

Copy link
Member

Choose a reason for hiding this comment

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

ok then add the test case so we have that covered, to one of the EntityManager tests, em.count should be covered in all of them, so just extend the test that is already there

Copy link
Member

Choose a reason for hiding this comment

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

we might still need that istanbul ignore hint, as covering lines with optional chaining is buggy

Copy link
Member

Choose a reason for hiding this comment

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

do you plan to finish this or should I take over?

Copy link
Author

Choose a reason for hiding this comment

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

Oh sorry, I had no time to look at this. If you can take over would be great

Copy link
Member

@B4nan B4nan Dec 2, 2020

Choose a reason for hiding this comment

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

Cant replicate this. More importantly, using count with groupBy will pretty much never give you what you want. Grouping will result in multiple rows, but em.count() will give you only the first one. It already uses count distinct query, what do you need the group by for?

edit: Ok got it, the issue is with the filter that will result in no results found, together with group by you end up with no rows. still what I said above applies, it's not doing what you are expecting most probably.

@B4nan B4nan closed this in bc3cdf6 Dec 2, 2020
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.

None yet

2 participants