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

fix(sum): returns 0 when is empty matching #9984

Merged
merged 2 commits into from Oct 11, 2018

Conversation

@t-bonatti
Copy link
Contributor

@t-bonatti t-bonatti commented Oct 2, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Fix #6299

@codecov
Copy link

@codecov codecov bot commented Oct 2, 2018

Codecov Report

Merging #9984 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9984      +/-   ##
==========================================
+ Coverage   96.38%   96.39%   +<.01%     
==========================================
  Files          63       63              
  Lines        9389     9395       +6     
==========================================
+ Hits         9050     9056       +6     
  Misses        339      339
Impacted Files Coverage Δ
lib/model.js 96.67% <100%> (ø) ⬆️
lib/query-interface.js 91.13% <100%> (+0.02%) ⬆️
lib/dialects/postgres/data-types.js 97.81% <0%> (-0.08%) ⬇️
lib/dialects/sqlite/query-generator.js 96.44% <0%> (-0.04%) ⬇️
lib/operators.js 100% <0%> (ø) ⬆️
lib/dialects/abstract/query-generator/operators.js 90.9% <0%> (ø) ⬆️
lib/dialects/abstract/query-generator.js 97.65% <0%> (+0.01%) ⬆️
lib/dialects/sqlite/data-types.js 97.05% <0%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07fe591...997b8eb. Read the comment docs.

@t-bonatti
Copy link
Contributor Author

@t-bonatti t-bonatti commented Oct 5, 2018

@sushantdhiman Could you take a look at this fix too?

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

sum should return 0 not null as number is expected for this method. It should be solved at Model.aggregate level rather than query-interface level.

@t-bonatti
Copy link
Contributor Author

@t-bonatti t-bonatti commented Oct 10, 2018

@sushantdhiman

I changed to return 0.
I returned null before because the Postgres do that.

image

@@ -1117,7 +1117,9 @@ class QueryInterface {
const dataType = options.dataType;

if (dataType instanceof DataTypes.DECIMAL || dataType instanceof DataTypes.FLOAT) {
result = parseFloat(result);
if (!_.isNull(result)) {

This comment has been minimized.

@t-bonatti

t-bonatti Oct 10, 2018
Author Contributor

I kept this change to not return NaN

This comment has been minimized.

@sushantdhiman

sushantdhiman Oct 11, 2018
Contributor

👍

@t-bonatti t-bonatti changed the title fix(sum): returns null when is empty matching fix(sum): returns 0 when is empty matching Oct 10, 2018
@sushantdhiman sushantdhiman merged commit 5db576a into sequelize:master Oct 11, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch 100% of diff hit (target 96.38%)
Details
@codecov
codecov/project 96.39% (+<.01%) compared to 07fe591
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 11, 2018

Thanks @t-bonatti

I returned null before because the Postgres do that.

Correct behaviour if we want to know if record exists as per our condition, but I can't say other dialect adheres to this. We can simply return numerical values from aggregate for now

@t-bonatti t-bonatti deleted the t-bonatti:fix-sum branch Oct 11, 2018
@mariomixtega
Copy link

@mariomixtega mariomixtega commented Feb 24, 2019

Is this already on 4.42.0?

@holm
Copy link
Contributor

@holm holm commented Apr 6, 2019

I am just upgrading to Sequelize 5, and this caught me by surprise. It seems very strange that this method now returns 0 if the result is null. This seems to assume that the result of an aggregation is a number. This is not at all the case in general. It can really be any datatype, and by returning 0 as a default, this breaks the expected type. In our case we are using it to find the max of a date field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants