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(functions): schema fn should allow redundant escapes #1787

Merged
merged 3 commits into from
Aug 31, 2021

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Aug 24, 2021

Fixes #1782

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

This is not a bug in Ajv or anything, it's just how the unicode flag works.

@P0lip P0lip added the t/bug Something isn't working label Aug 24, 2021
@P0lip P0lip self-assigned this Aug 24, 2021
@P0lip P0lip requested a review from mmiask August 24, 2021 11:17
strict: false,
allowUnionTypes: true,
logger,
unicodeRegExp: false,
Copy link
Contributor

@mmiask mmiask Aug 27, 2021

Choose a reason for hiding this comment

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

Are you sure we want to remove Unicode-related features? This definitely fixes the issue, but introduces some limitation to the shape of regex (according to MDN)

WDYT about somehow passing String.raw$stringliteral`` instead of string when we create `new RegExp()`? (although this might require pushing changes to `Ajv` itself)

Or just changing \\ to '\\\\ whenever we see \\ in a string literal pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT about somehow passing String.raw$stringliteral`` instead of string when we create new RegExp()? (although this might require pushing changes to `Ajv` itself)
Or just changing \ to '\\ whenever we see \ in a string literal pattern?

That won't fix the issue, as this changes the semantics of the regular expression.
Replacing\\ with \\\\ leads to a different regular expression.
For instance

new RegExp('[\\_-]').test('_'); // true
new RegExp('[\\_-]').test('\\'); // false
new RegExp('[\\\\_-]').test('\\'); // true

In a nutshell, the problem here is that when a u (unicode) flag is supplied, the behavior of \ changes a bit, and it's up to a end user to account for that.
Unfortunately, this means that the only choices we're left here include
a) tell users to write more proper regular expressions
b) get rid of redundant escapes - very tricky do to right
c) switch to non-unicode regexps

In other words, there's no good solution, each of them has some trade-offs.
Luckily, I haven't really seen much usage of unicode, the vast majority of users doesn't use it at all. The the spec itself does not enforce the unicode support, it recommends it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip you convinced me. We'll always have to pick lesser evil

@P0lip P0lip enabled auto-merge (squash) August 31, 2021 14:53
@P0lip P0lip merged commit 5a29d44 into develop Aug 31, 2021
@P0lip P0lip deleted the fix/functions/schema-unicode-regexp branch August 31, 2021 14:59
stoplight-bot pushed a commit that referenced this pull request Sep 1, 2021
# [@stoplight/spectral-functions-v1.2.0](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-functions-v1.1.2...@stoplight/spectral-functions-v1.2.0) (2021-09-01)

### Bug Fixes

* **functions:** schema fn should allow redundant escapes ([#1787](#1787)) ([5a29d44](5a29d44))

### Features

* **functions:** output Ajv runtime exceptions ([#1801](#1801)) ([5a19625](5a19625))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-functions-v1.2.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Certain valid patterns are not accepted by schema function
4 participants