Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: make pattern and patternProperties unicode ECMA-262 compliant #145

Merged
merged 2 commits into from
Jan 13, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Jan 4, 2022

Description

This PR makes the regular expressions found in pattern and patternProperties fields unicode compliant (according to ECMA-262). It only changes the 2.3.0.json schema but if you consider this should be applied to the rest, I can expand it.

It means now the regular expressions are compatible with u flag, flag that some JSON Schema parsers use when parsing a JSON Schema document. For example the one from Hyperjump](https://github.com/hyperjump-io/json-schema-validator). See implementation here and [here}(https://github.com/hyperjump-io/json-schema-validator/blob/521818176d71d82344450a40a405fff2029e6bcc/lib/keywords/pattern.js). This is being used by the online tool https://json-schema.hyperjump.io.

This is not introducing a BC since the regular expressions didn't change their meaning. You can see each version with each regex explanation on the following links:

Related issue(s)
Based on #128 but fixing the proposed for the patternProperties with a proper unicode character.

cc @jonaslagoni @fmvilas

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM, as it solves the problem 👍

Weird that the implementation of hyperjump actually supports ^x-[\\w\\d.\\-_]+$ but proper regex implementation does not 😅

Only had one comment, but guess regex behaves differently in different scenarios 🤷

@@ -9,7 +9,7 @@
],
"additionalProperties": false,
"patternProperties": {
"^x-[\\w\\d\\.\\-\\_]+$": {
"^x-[\\w\\d\\.\\x2d_]+$": {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!

Hmmm 🤔

I guess the problem is because it is inside a matching list [...] 🤔

Otherwise I guess we needed to do: "^x\\x2d[\\w\\d\\.\\x2d_]+$": {

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, you guessed correctly. It is because that - is inside a list.

@smoya
Copy link
Member Author

smoya commented Jan 4, 2022

Weird that the implementation of hyperjump actually supports ^x-[\\w\\d.\\-_]+$ but proper regex implementation does not 😅

It is because it uses an old version of the validator:

"@hyperjump/oas-schema-validator": "^0.5.0",

The u flag was not added by then in the code. See https://github.com/hyperjump-io/json-schema-validator/blob/v0.5.0/lib/keywords/patternProperties.js

@smoya smoya requested a review from jonaslagoni January 7, 2022 15:33
@fmvilas
Copy link
Member

fmvilas commented Jan 7, 2022

It only changes the 2.3.0.json schema but if you consider this should be applied to the rest, I can expand it.

If it's not breaking anything and it's actually a bug fix, I think this should be applied to all the files.

@smoya
Copy link
Member Author

smoya commented Jan 10, 2022

If it's not breaking anything and it's actually a bug fix, I think this should be applied to all the files.

I did push a commit updating the 2.x schemas. I skipped 1.x because patterns are way different.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@smoya
Copy link
Member Author

smoya commented Jan 13, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 6b65cb8 into asyncapi:2022-01-release Jan 13, 2022
@smoya smoya deleted the fix/patterns branch January 13, 2022 10:48
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.13.0-2022-01-release.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants