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

repaire a bug; when Duplicate with \r ,that makes match() return null #10320

Merged
merged 8 commits into from Jan 18, 2019
Merged

repaire a bug; when Duplicate with \r ,that makes match() return null #10320

merged 8 commits into from Jan 18, 2019

Conversation

@zhangshichun
Copy link
Contributor

@zhangshichun zhangshichun commented Jan 5, 2019

Pull Request check-list

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

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

Description of change

as this issue
#6725
when get a \r in the err.message,match() will return null

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 7, 2019

Where in the error message would this newline be, when are newlines contained in error messages, please provide more info, this seems like a larger issue.

@zhangshichun
Copy link
Contributor Author

@zhangshichun zhangshichun commented Jan 7, 2019

Where in the error message would this newline be, when are newlines contained in error messages, please provide more info, this seems like a larger issue.

with using mysql,this may case the error:
user.create({ name: 'john\r' }); user.create({ name: 'john\r' });
(if name is unique)
the error code is 1062,and the match() just return null.

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 7, 2019

In this case your fix is not sufficient.
There are many occurrences where err.message is matched via regex and .replace(string, string) only replaces a single occurrences, my suggestion is you add a util method called removeNewlines(str) { return str.replace(/[\r\n]/g, ''); and apply it to all of them.

@zhangshichun
Copy link
Contributor Author

@zhangshichun zhangshichun commented Jan 10, 2019

In this case your fix is not sufficient.
There are many occurrences where err.message is matched via regex and .replace(string, string) only replaces a single occurrences, my suggestion is you add a util method called removeNewlines(str) { return str.replace(/[\r\n]/g, ''); and apply it to all of them.

do you think that is a good idea to change
(.*)
to
([\s\S]*)
when match err.message?

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 10, 2019

That could work too, depending on @sushantdhiman stance on v5 node version we could also use the /s flag (dotall) if we were to target v10.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jan 10, 2019

Hehe, at this point I just want to release v5 asap. Please just use simple Node 6 constructs for now

@codecov
Copy link

@codecov codecov bot commented Jan 15, 2019

Codecov Report

Merging #10320 into master will decrease coverage by 0.85%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10320      +/-   ##
==========================================
- Coverage   96.28%   95.42%   -0.86%     
==========================================
  Files          68       65       -3     
  Lines        9775     9556     -219     
==========================================
- Hits         9412     9119     -293     
- Misses        363      437      +74
Impacted Files Coverage Δ
lib/dialects/mysql/query.js 98.46% <100%> (ø) ⬆️
lib/dialects/mariadb/query-generator.js 10.41% <0%> (-87.5%) ⬇️
lib/dialects/mariadb/data-types.js 52.88% <0%> (-36.54%) ⬇️
lib/dialects/abstract/connection-manager.js 88.49% <0%> (-1.77%) ⬇️
lib/query-interface.js 90.48% <0%> (-1.47%) ⬇️
lib/sequelize.js 95.75% <0%> (-0.61%) ⬇️
lib/dialects/abstract/query-generator.js 97.45% <0%> (-0.18%) ⬇️
lib/dialects/mariadb/index.js
lib/dialects/mariadb/connection-manager.js

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 0f0255e...78000e7. Read the comment docs.

zhangshichun and others added 5 commits Jan 15, 2019
zhangshichun
@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Jan 16, 2019

Some tests randomly fail due to timing issues, you can ignore these.

@knoxcard
Copy link
Contributor

@knoxcard knoxcard commented Jan 16, 2019

v5 really is the way to go!

@sushantdhiman sushantdhiman merged commit 554b223 into sequelize:master Jan 18, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jan 18, 2019

Looks this PR broke some MariaDB stuff https://travis-ci.org/sequelize/sequelize/jobs/481222290

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