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: don't treat \ as escape in standard strings, support E-strings, support vars after ->> operator #14700

Merged
merged 8 commits into from Jul 3, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Jun 29, 2022

Pull Request Checklist

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

Description Of Change

Closes #14693

This PR fixes injectReplacements and mapBindParameters to support:

  • "standard conforming strings", aka strings in which \ cannot be used to escape '
  • Postgres' E-prefixed strings (escape strings), which support \-based escaping, even if standardConformingStrings is true
  • Parsing bind parameters and replacements after the JSONB -> & ->> operators

As a result, these two methods will now throw if the SQL includes unterminated string literals.

It was painful to test that one dialect threw (because it did not support \ escaping), and another did not, so I introduced a new expectPerDialect utility based on expectsql

Here is such an example:

image

I also expanded expectsql to accept a combination of dialects separated by a space as the key, to reduce duplication (mainly for mysql/mariadb). e.g. the following is now possible:

expectsql({
  default: 'SELECT ...',
  'mysql mariadb': 'SELECT ...',
})

@ephys ephys self-assigned this Jun 29, 2022
@WikiRik
Copy link
Member

WikiRik commented Jun 29, 2022

Do we need a similar PR for v6?

@ephys
Copy link
Member Author

ephys commented Jun 29, 2022

Yep! I'll need to backport this :)

@ephys ephys marked this pull request as draft June 29, 2022 18:48
@ephys
Copy link
Member Author

ephys commented Jun 29, 2022

Adding an extra fix

@ephys ephys marked this pull request as ready for review June 29, 2022 18:54
@ephys ephys changed the title fix: don't treat \ as escape in standard strings, support E-strings fix: don't treat \ as escape in standard strings, support E-strings, support vars after ->> operator Jun 29, 2022
@ephys ephys requested review from a team and removed request for a team July 1, 2022 12:44
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Looks good! Few small questions

src/utils/sql.ts Outdated Show resolved Hide resolved
test/unit/utils/sql.test.ts Show resolved Hide resolved
src/dialects/postgres/index.js Outdated Show resolved Hide resolved
src/utils/sql.ts Show resolved Hide resolved
@ephys ephys requested a review from WikiRik July 3, 2022 09:54
@ephys ephys merged commit 1c85d01 into main Jul 3, 2022
@ephys ephys deleted the ephys/fix-backslash-replacement branch July 3, 2022 10:29
|| (
dialect.supports.escapeStringConstants
// is this a E-prefixed string, such as `E'abc'` ?
&& sqlString[i - 1] === 'E'
Copy link

Choose a reason for hiding this comment

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

Shouldn't this also check for lowercase 'e'?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should! Thanks for double checking :)

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2022

🎉 This PR is included in version 7.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@WikiRik
Copy link
Member

WikiRik commented Sep 30, 2022

@sdepold you mentioned you wanted to backport this to v6 (together with #14733 ). Did you work on that already? Might be a nice PR to make for hacktoberfest

@sdepold
Copy link
Member

sdepold commented Oct 2, 2022

Hey. Didn't have the time so far. Looking at this pr I got the impression that it was rather tricky but given that it's mostly a matter of replicating things, sure why not :)

@sdepold sdepold added the funded label Oct 6, 2022
sdepold added a commit that referenced this pull request Oct 16, 2022
We also support support vars after ->> operator now. Backport of #14700
sdepold added a commit that referenced this pull request Oct 19, 2022
…support vars after ->> operator, treat lowercase e as valid e-string prefix (#15139)

* fix: don't treat \ as escape in standard strings, support E-strings

We also support support vars after ->> operator now. Backport of #14700

* fix(postgres): treat lowercase e as valid e-string prefix

* fix: nullish coalescence

* fix: cleanup

* fix: spaces

* fix: postgres tests

* fix: add third param to trigger right behavior

* fix: tests

* fix: formatting

* fix: remove unused code

* meta: more formatting

* meta: more formatting

* fix: merge test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String ending with backslash breaks PostgreSQL value replacements
4 participants