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

Almost JSON-spec compliant validation of numeric values #119

Merged
merged 1 commit into from
Mar 14, 2019

Conversation

kosty
Copy link
Contributor

@kosty kosty commented Feb 21, 2019

Fixes #118 , yet leaves +1 as a valid option

@stevehu
Copy link
Contributor

stevehu commented Feb 22, 2019

@stevehu stevehu changed the base branch from master to develop February 28, 2019 14:54
Copy link
Contributor

@jiachen1120 jiachen1120 left a comment

Choose a reason for hiding this comment

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

Hey @kosty. Thank you for your contribution. I just tested your code and found that the following issues may be improved.

For some fractions, such as '.1', '1.', '.1e1'. Should they be considered as valid numbers? They can be parsed into numbers by yaml, but by using your method, they are invalid.

Perhaps a more reliable way is to use the following code to valid the number directly

try {
new BigDecimal(StringToBeValidated);
return true;
} catch {
return false;
}

What do you think?

@kosty
Copy link
Contributor Author

kosty commented Mar 13, 2019

@jiachen1120 Thanks for feedback. To provide some more context I tried to be as close to original JSON spec which actually points to those as invalid numeric representations.

With that said, I am happy to have numeric parsing in any consistent way. Would BigDecimal be consistent with go, js, python et. al. environments?
Also, in that case should we stick to BigInteger as validation for integers?

P.S. this is what I refer to as "spec"

Screen Shot 2019-03-13 at 10 51 33

@stevehu
Copy link
Contributor

stevehu commented Mar 13, 2019

@kosty @jiachen1120 I have learned a long time ago that it is very hard to ensure that the JSON schema validation works exactly the same for all languages. Different languages have different limitations and sometimes we have to accept the differences. There are still several open issues in Java that we cannot match the Javascript implementation, but these are all edge cases and we never bothered.

For me, I am trying to avoid using try/catch as it has performance issue compare with other approaches whenever necessary. For this particular case, I think we are OK if we can cover all possible cases without using try/catch. @NicholasAzar @ddobrin What do you think?

@kosty
Copy link
Contributor Author

kosty commented Mar 13, 2019

@stevehu @jiachen1120 I do agree on performance hit with try/catch. Arguably pattern matching has a similar downside. Also good point on challenges achieving consistency across different languages.

@ddobrin
Copy link
Contributor

ddobrin commented Mar 13, 2019

I am generally avoiding this try-catch approach and prefer the "exception barrier" concept when an exception is left to bubble up.

Otherwise we pay the price for each call, regardless of pass/fail.

NumberFormatException is a RuntimeException and it would be caught if that where to happen, at the latest by the ExceptionHandler in the exception module

@stevehu
Copy link
Contributor

stevehu commented Mar 13, 2019

The exception will be caught in the same block, so it won't bubble up. I am wondering how other languages handlers this kind of problem if they don't use the try/catch block. I'd prefer a generic solution; however, I am not against the try/catch if it ends up the simplest solution.

@ddobrin
Copy link
Contributor

ddobrin commented Mar 13, 2019

I was suggesting to let it up bubble up :) and maybe go that route.

@kosty
Copy link
Contributor Author

kosty commented Mar 13, 2019

Did some googling on try/catch performance and it seems to be the throw and all the stack-walking to blame:

stackoverflow answer claiming ~66 times slowdown
more stackoverflow
a blog post with benchmarks and graphs

In my case validator is quite deep in the stack, thus exception might be really harming overall performance. Also result seems predictable, so exception feels more like an overkill

@jiachen1120
Copy link
Contributor

jiachen1120 commented Mar 14, 2019

@kosty Thank you for sharing. As summarized below,

the Java virtual machine uses the method call stack to keep track of a series of method invocation procedures in each thread. The stack holds local information for each method called. Each thread has a separate method call stack. For the main thread of the Java application, the bottom of the stack is the main() method of the program's entry method. When a new method is called, the Java virtual machine puts the stack structure describing the method on top of the stack, and the method at the top of the stack is the method being executed.

When a method completes normally, the Java virtual machine pops the stack structure of the method from the call stack and continues processing the previous method. If an exception is thrown during the execution of a method, the Java virtual machine must find a catch block that catches the exception. It first looks to see if the current method has such a block of code. If it exists, then the block is executed. Otherwise, the Java virtual machine pops the stack structure of the method from the call stack and proceeds to the previous method to find the appropriate capture code block. In the backtracking process, if the Java virtual machine finds a block of code that handles the exception in a method, the stack structure of the method becomes the top element of the stack, and the program flow goes to the exception handling code part of the method to continue execution.

If the code block that throws the exception and the code block that catches the exception are in the same place, the impression will be smaller; if the Java virtual machine must search the method call stack to find the exception handling code block, the impact on performance will be greater. Especially when the exception handling code is at the bottom of the call stack, the Java virtual machine locating exception handling code requires a lot of work.

Therefore, maybe we should not use the exception handling mechanism to control the normal flow of the program when the exception is not handled in place.

Thanks again for sharing, learned more about the exception handling. 👍

@stevehu stevehu merged commit 0f0a24e into networknt:develop Mar 14, 2019
@stevehu
Copy link
Contributor

stevehu commented Mar 14, 2019

@kosty Thanks a lot for your help and I have learned a lot from the discussion.

@stevehu
Copy link
Contributor

stevehu commented Mar 14, 2019

@kosty @jiachen1120 @ddobrin I have released 1.0.4 to the maven central and will update the light-4j framework to depend on this version in the next release. Thanks a lot for your help.

https://github.com/networknt/json-schema-validator/releases

@kosty kosty deleted the scratchpad/ISSUE-118-akostenko branch July 22, 2019 17:41
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.

4 participants