Skip to content

fix: escape dollarsign in sequelize.fn related to issue 11533#11606

Merged
sushantdhiman merged 4 commits intosequelize:masterfrom
schopr9:master
Oct 28, 2019
Merged

fix: escape dollarsign in sequelize.fn related to issue 11533#11606
sushantdhiman merged 4 commits intosequelize:masterfrom
schopr9:master

Conversation

@schopr9
Copy link
Contributor

@schopr9 schopr9 commented Oct 24, 2019

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 update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

#11533

  • changed the abstract query generator handleSequelizeMethod to escape dollar ($) in argument

@papb
Copy link
Member

papb commented Oct 24, 2019

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!

Tests are failing though, please take a look :)

Also your branch was outdated, I just brought it up to date for you (don't worry about this):

image

Let me know if you need help. Thanks!!

@papb papb added status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action type: bug DEPRECATED: replace with the "bug" issue type labels Oct 24, 2019
@codecov
Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #11606 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11606   +/-   ##
=======================================
  Coverage   96.26%   96.26%           
=======================================
  Files          94       94           
  Lines        9190     9190           
=======================================
  Hits         8847     8847           
  Misses        343      343
Impacted Files Coverage Δ
lib/dialects/abstract/query-generator.js 97.01% <100%> (ø) ⬆️

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 c77c553...eb719b9. Read the comment docs.

Copy link
Member

@papb papb left a comment

Choose a reason for hiding this comment

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

Great, I like your tests, and tests are passing, so LGTM!

@papb papb added status: awaiting maintainer and removed status: awaiting response For issues and PRs. OP must respond (or change something, if it is a PR). Maintainers have no action labels Oct 25, 2019
@sushantdhiman sushantdhiman merged commit 43c4f6b into sequelize:master Oct 28, 2019
@kristijanhusak
Copy link

I'm having this problem:

const theCode = '10$off';
PromoCodes.findAll({
  where: {
    code: sequelize.where(sequelize.fn('lower', sequelize.col('code')), sequelize.fn('lower', theCode))
  }
});

This never finds it because it does lower("code") = lower('10$$off'). Solution was to do theCode.toLowerCase(), but this is still a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug DEPRECATED: replace with the "bug" issue type

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants