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

Feat/#199 fail fast #202

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Conversation

khiftikhar
Copy link
Contributor

@khiftikhar khiftikhar commented Oct 24, 2019

Adding the feature to fail-fast once the very first validation error is discovered.

@stevehu
Copy link
Contributor

stevehu commented Oct 26, 2019

@khiftikhar Sorry to review the PR late, I am working on the release of the light platform during this weekend. For the fail-fast flag, I see you have added to the ValidationContext which is an object that carries the runtime info from one validator to another. The best place for the fail-fast flag would be the SchemaValidatorsConfig class which is designed to add more configuration flags in the future. In this way, we can keep the signature of JsonSchema and JsonSchemaFactory as config object already passed in. This will make the API static without impact current users. What do you think? Thanks a lot for your help.

@khiftikhar
Copy link
Contributor Author

@stevehu I have moved the fail-fast flat to the SchemaValidatorsConfig.java now. Please take a look when you do have time.

@stevehu
Copy link
Contributor

stevehu commented Oct 27, 2019

@khiftikhar It looks very good to put the fail-fast flag into the config. This also gives us an example of how to add another flag in the future. I see that you are using an exception to throw the message when fail-fast is true. Is there any other way to return the error message when it happens in any validator? For some users, the exception might be treated as an alarm. I will look into it when I complete the release. Thanks.

@khiftikhar
Copy link
Contributor Author

@stevehu I had been looking into how we can stop the processing otherwise, but it did required a bit more changes into the code.

I also thought to introduce a checked exception to builder().failFastmethod but then I thought it might make the usability a bit uglier.

Now, setting up this flag is a conscious decision of the user, and the documentation clearly specifies that no error message is returned, rather and exception is thrown. So it's the user's responsibility to take of it.

@stevehu
Copy link
Contributor

stevehu commented Oct 28, 2019

@khiftikhar You are right. This might be the easiest way to stop the validators. I think we need to start working on the documentation as the library is getting more and more features. Thanks a lot for your help.

@stevehu stevehu merged commit 194dabe into networknt:master Oct 28, 2019
@stevehu
Copy link
Contributor

stevehu commented Oct 28, 2019

@khiftikhar The 1.0.23 is released. Thanks a lot for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants