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(query): check valid warn message (#9919) #9948

Merged
merged 1 commit into from Sep 29, 2018

Conversation

@tsasaki609
Copy link
Contributor

@tsasaki609 tsasaki609 commented Sep 22, 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 #9919
validate that warning row is iterable

@codecov
Copy link

@codecov codecov bot commented Sep 22, 2018

Codecov Report

Merging #9948 into master will decrease coverage by 4.83%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9948      +/-   ##
=========================================
- Coverage   96.14%   91.3%   -4.84%     
=========================================
  Files          63      63              
  Lines        9407    9408       +1     
=========================================
- Hits         9044    8590     -454     
- Misses        363     818     +455
Impacted Files Coverage Δ
lib/dialects/mysql/query.js 20.15% <100%> (-70.47%) ⬇️
lib/dialects/mysql/query-generator.js 4% <0%> (-93.78%) ⬇️
lib/dialects/mysql/query-interface.js 30.43% <0%> (-69.57%) ⬇️
lib/dialects/mysql/connection-manager.js 26.08% <0%> (-63.77%) ⬇️
lib/dialects/mysql/data-types.js 53.21% <0%> (-43.12%) ⬇️
lib/sql-string.js 83.82% <0%> (-7.36%) ⬇️
...dialects/abstract/query-generator/helpers/quote.js 88.88% <0%> (-5.56%) ⬇️
lib/sequelize.js 93.47% <0%> (-3.11%) ⬇️
lib/dialects/abstract/query-generator.js 96.33% <0%> (-1.31%) ⬇️
... and 4 more

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 fa2f373...668072c. Read the comment docs.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 22, 2018

Test case is included in that issue, please use that or create a new one

@tsasaki609 tsasaki609 force-pushed the tsasaki609:iterate-warning-row branch from 2bfed34 to a1f2599 Sep 23, 2018
@tsasaki609
Copy link
Contributor Author

@tsasaki609 tsasaki609 commented Sep 23, 2018

Oops I forgot. I made a simple unit test.
Thanks for the review.

@@ -178,6 +178,7 @@ class Query extends AbstractQuery {
const warningMessage = 'MySQL Warnings (' + (this.connection.uuid||'default') + '): ';
const messages = [];
for (const _warningRow of warningResults) {
if (typeof _warningRow[Symbol.iterator] !== 'function') break;

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 25, 2018
Contributor

continue instead of break

test/unit/dialects/mysql/query.test.js Show resolved Hide resolved
@tsasaki609 tsasaki609 force-pushed the tsasaki609:iterate-warning-row branch 4 times, most recently from 4e950af to a564919 Sep 29, 2018
@tsasaki609 tsasaki609 force-pushed the tsasaki609:iterate-warning-row branch from a564919 to 668072c Sep 29, 2018
@tsasaki609
Copy link
Contributor Author

@tsasaki609 tsasaki609 commented Sep 29, 2018

I implemented an integration test.

@sushantdhiman sushantdhiman merged commit d76b0a8 into sequelize:master Sep 29, 2018
4 checks passed
4 checks passed
@codecov
codecov/patch 100% of diff hit (target 96.14%)
Details
@codecov
codecov/project 96.24% (+0.1%) compared to fa2f373
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 Sep 29, 2018

Thanks @tsasaki609

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.

2 participants