-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: regex sampler #153
feat: regex sampler #153
Conversation
64230d4
to
fdd992b
Compare
133a3ba
to
8308f08
Compare
8308f08
to
7f1045f
Compare
43071ce
to
9896c22
Compare
Vendors a tiny dependency (total dependency tree <10 KB uncompressed). Other than removing unused code from the dependency, the only change made is to change the following line: https://github.com/fent/randexp.js/blob/2b35ea607883fa8cafa63bd3bdb5c12d695f873a/lib/randexp.js#L109 to a separately customizable callback.
9896c22
to
e2a6c63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks more or less good but I'm not sure if the complexity involved is worth it.
People can always provide example manually which will be better quality.
package.json
Outdated
@@ -88,6 +88,7 @@ | |||
}, | |||
"dependencies": { | |||
"@types/json-schema": "^7.0.7", | |||
"json-pointer": "0.6.2" | |||
"json-pointer": "0.6.2", | |||
"randexp": "^0.5.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is strange to include this as dependency but not use it directly. I think it's better to add 2 other dependencies here.
But I'm also concerned by those dependencies. They are not very active. Last update 5 years ago.
That's a valid concern. One way to reduce the complexity would be to simply ignore the min_length and max_length for regexes. Another way could be to copy faker.js fromRegExp, which is less complete but has no dependencies: They also had a pretty long discussion of what to do about this: faker-js/faker#1569 |
I would cope the faker-js one. This looks good. |
0e0cc0b
to
0506daf
Compare
Done! It's definitely less capable than randexp but I have added to the README a link to their docs which note the limitations. |
src/vendor/fakerFromRegexp.js
Outdated
*/ | ||
|
||
/** @returns {boolean} */ | ||
function randBool() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, so this tool is not deterministic 😢
Can we adjust it to produce same result always.
One of the features of this tools is "deterministic" (check the readme, it's first on the list).
There are multiple reasons to want determinism in such tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, adjusted
src/samplers/string.js
Outdated
@@ -1,9 +1,18 @@ | |||
'use strict'; | |||
|
|||
import { ensureMinLength, toRFCDateTime, uuid } from '../utils'; | |||
import { fromRegExp } from '../vendor/fakerFromRegexp'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it a bit more consistent.
let's move it from vendor folder into samplers/string-regex.js
and rename the internal input from fromRegExp
to regexSample
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, adjusted. I could not import as just regexSample
due to naming conflict so I did a namespace import
src/samplers/string.js
Outdated
function regexSample() { | ||
return '/regex/'; | ||
function regexSample(min, max, _propertyName, pattern) { | ||
return pattern ? fromRegExp(pattern) : truncateString('/regex/', min, max) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to adjust it. Let's leave it is.
This is a string with format: regex
which means it has to be a valid regex. I doubt someone will provide regex for regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I misunderstood what format: regex
meant. Thank you, fixed
0506daf
to
a819346
Compare
@Redocly/keyboard-warriors please, merge it when you're ready 🙏 |
we really need this feature 😢. when can you merge it? 🙏 |
Thanks @llllvvuu for adding this feature! |
…openapi-sampler Summary: https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin Reviewed By: skurapati, dshubin Subscribers: dshubin, yugaware Differential Revision: https://phorge.dev.yugabyte.com/D34498
…ly dependency on openapi-sampler Summary: Original commit: 1333618 / D34498 https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 Backporting as the same issue is seen 2024.1 builds too https://jenkins.dev.yugabyte.com/job/github-yugabyte-db-alma8-master-clang17-release/512 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin, vkumar Reviewed By: skurapati, vkumar Subscribers: yugaware, dshubin Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D34524
…redocly dependency on openapi-sampler Summary: Original commit: 1333618 / D34498 https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin, daniel Reviewed By: skurapati, rmadhavan Subscribers: yugaware, dshubin Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D34531
…redocly dependency on openapi-sampler Summary: Original commit: 1333618 / D34498 https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin, daniel, svarshney Reviewed By: daniel Subscribers: yugaware, dshubin Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D34867
…openapi-sampler Summary: https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin Reviewed By: skurapati, dshubin Subscribers: dshubin, yugaware Differential Revision: https://phorge.dev.yugabyte.com/D34498
…ly dependency on openapi-sampler Summary: Original commit: 1333618 / D34498 https://yugabyte.slack.com/archives/C048Z7EDH0F/p1713971854833549 Backporting as the same issue is seen 2024.1 builds too https://jenkins.dev.yugabyte.com/job/github-yugabyte-db-alma8-master-clang17-release/512 The jenkins builds started failing with an error: ``` [2024-04-24T14:52:24.048Z] /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/src/main/resources/openapi /share/jenkins/workspace/github-yugabyte-db-alma8-master-clang17-release/yugabyte-db/managed/scripts [2024-04-24T14:52:24.048Z] Running bundle on openapi spec ... [2024-04-24T14:52:24.048Z] npx: installed 197 in 12.668s [2024-04-24T14:52:24.048Z] Unexpected token '?' [2024-04-24T14:52:24.048Z] cp: cannot stat 'tmp/openapi_bundle.yaml': No such file or directory ``` Could reproduce locally after switching to `nvm use v12.22.12`: ``` [sneelakantan@dev-server-sneelakantan ~]$ redocly /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/openapi-sampler/dist/openapi-sampler.js:791 pattern = ((_pattern$match = pattern.match(/\/(.+?)\//)) === null || _pattern$match === void 0 ? void 0 : _pattern$match[1]) ?? ''; // Remove frontslash from front and back of RegExp ^ SyntaxError: Unexpected token '?' at wrapSafe (internal/modules/cjs/loader.js:915:16) at Module._compile (internal/modules/cjs/loader.js:963:27) at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10) at Module.load (internal/modules/cjs/loader.js:863:32) at Function.Module._load (internal/modules/cjs/loader.js:708:14) at Module.require (internal/modules/cjs/loader.js:887:19) at require (internal/modules/cjs/helpers.js:74:18) at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:41:72746 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3444 at /nfusr/dev-server/sneelakantan/.nvm/versions/node/v12.22.12/lib/node_modules/@redocly/cli/node_modules/redoc/bundles/redoc.lib.js:1807:3449 ``` Figured out a recent change in [email protected] (dependency of redocly/cli) that is causing above error (Redocly/openapi-sampler#153). So forcing to use the older version of [email protected] here to workaround this build error. Test Plan: Ran build locally. Will monitor jenkins phab pipeline. Reviewers: skurapati, rmadhavan, steve.varnau, dshubin, vkumar Reviewed By: skurapati, vkumar Subscribers: yugaware, dshubin Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D34524
Vendors a tiny dependency (total dependency tree <10 KB
uncompressed).
Other than removing unused code from the dependency, the only change
made is to change the following line:
https://github.com/fent/randexp.js/blob/2b35ea607883fa8cafa63bd3bdb5c12d695f873a/lib/randexp.js#L109
to a separately customizable callback, in order to adjust the length to the minLength and maxLength constraints.
What/Why/How?
Fixes #152 and Redocly/redoc#2312
Reference
#152
Testing
Added unit tests and fuzz tests for both basic and advanced cases.
Screenshots (optional)
N/A
Check yourself
Security
Regex denial-of-service applies as usual, ensure code is not running on untrusted input in a place where a hanging process would be harmful.