This repository was archived by the owner on Nov 8, 2024. It is now read-only.
Work around infinite loop bug when using pattern and minLength in OAS 2 #273
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tjanc
reviewed
Jul 3, 2019
| // doesn't match minLength. | ||
| // | ||
| // JSON Schema Faker will fail in that case and get into an infinite loop. | ||
| actualSchema.pattern = schema.pattern.replace('*$', '+$'); |
Contributor
There was a problem hiding this comment.
Is this replacing all occurrences of *$? From what I understand from this PR we should only modify the end.
Member
Author
There was a problem hiding this comment.
$ refers to the end of the line in regex. So replacing *$ (usually would happen once at the end), if it is in the schema multiple times I think it should be fine to replace it.
| // doesn't match minLength. | ||
| // | ||
| // JSON Schema Faker will fail in that case and get into an infinite loop. | ||
| actualSchema.pattern = schema.pattern.replace('*$', '+$'); |
Contributor
There was a problem hiding this comment.
Is there a way to make the modification closer to the invocation of that faker tool?
Member
Author
There was a problem hiding this comment.
Unfortuantely not, without copying every schema and traversing over it at the JSON Schema Faker entrypoint. I would like to avoid that, it would be a far more expensive operation.
tjanc
approved these changes
Jul 4, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changelog entry in diff explains the problem that I am working around. Effectively, if you have a pattern
^[A-z]*$and aminLengthof1then the pattern with*can resolve to an empty string which doesn't match validation. I'm working around it by detecting this particular issue and "patching" the regex to use+instead of*so that it is a "one or more" not "zero or more".This is due to the current behaviour with JSON Schema Faker which gets into an infinite loop in this state. I think it is seeing that the pattern string doesn't generate the correct length and then it tries again, and again (forever). As we've seeded our "randomness" to produce consistent results it gets stuck.
This is a short-sighted solution, there are likely other cases this can happen which I am not counting for. This particular fix is to solve a majority problem that I am seeing a small amount of our customers hit (and subsequent platform instability). A proper fix should be made in json-schema-faker, that's more time consuming (and involves waiting for a fix to be released which given JSF release cycles can be months or more). So I've made this interim fix to solve platform instablity and customers not being able to use their OAS 2 document in our products. I've created APIARY-5932 to track this going forward (and for us to solve the problem in JSON Schema Faker).
I did consider providing a warning annotation to the user, but given how this part of the code base is architected, it wouldn't be possible to have source maps for the annotation without larger refactoring (which may in turn slow the parser down).