JSON pointers in error messages

[mshaposhnik]

Hi, i'm just tried this lib, and works very well for our needs. (see https://github.com/eclipse/che/issues/12714) The only problem that yet stops us to migrate from another valitation tool, is that is has object-style pointers to the places which is failing - i mean like /tools/actions/0 object has missing required properties ["foo"] instead of [line:11,column:82] The object must have a property whose name is "foo". Object style is more convenient for us, since we're actually validating an yaml objects, which are converted to json under the hood for validation, so it is little confusing to users to see pointers to the json.

I hope those explanations are clear. Can i ask politely to implement such option :-) ? Thanks, Max.

[leadpony]

Hello @mshaposhnik. Thank you for having an interest in this small library. What you really want is a JSON Pointer which points to the location in the JSON instance to be validated, isn't it? Most of JSON validators report the problem location as a JSON pointer, not the combination of line and column numbers as this library, so I believe it is good enhancement for this library.

Also I would be happy if somebody (including you and me) would implement yet another JSON-P implementation which can parse YAML documents direclty. Such an implementation will provide all of the applications/libraries depending on the JSON-P API with YAML support without any modification.

Thank you so much.

[mshaposhnik]

That's exactly it! Never knew it is even separate spec exists for that :). Will rename this issue.

[leadpony]

OK. I will try the enhancement in the problem reporting.

[erdi]

This is not happening. I've just tried out this library for validating yaml documents and my only concern is that violations are reported on a source location and not json pointer basis. Because I have to translate the yaml document to json before being able to feed it into the parser created via JsonValidationService source location in violations are not useful at all for the person trying to understand what the violation is actually about. So then I check the issue tracker and I find this!

I will have a stab at implementing a javax.json.stream.JsonParser on Monday which tracks current json pointer as the events are streamed through it, test it internally and if all goes well I will then contribute that back. With that in place it should be then easy to add json pointers as a property to org.leadpony.justify.api.Problem. How does that sound?

[leadpony]

Hello @erdi. Thank you for your suggestion. One advantage of this library is reporting problems with the original source line/column numbers, which is useful for end users editing JSON documents by hand, such as configuration files. So I never abandon the feature.
Your idea sounds nice. As you suggest, JSON pointers should be added into Problem interface. Could you please write a PR for us?
Anyway, thank you for your valuable comment.

[erdi]

I now have a working and unit tested implementation of json pointer aware JsonParser. It's written in Groovy which is my JVM language of choice so I will need to rework it a bit but I'm hoping to start working on porting it to Java and contributing it to this project on Friday.

And to reaffirm, the idea is to enhance org.leadpony.justify.api.Problem with a json pointer at which the problem occur and not replace the org.springframework.beans.factory.parsing.Location property that's already on it.

[leadpony]

Hello @erdi. Contribution in any form will be welcomed. I also can port your JsonParser and unit tests written in Groovy into this project. Thank you.

[erdi]

Hi @leadpony!

Thanks for the offer of porting things for me but as a maintainer of an open source project myself I think it would not be fair on you.

I had a look around and noticed some things.

I was hoping to find a number of tests checking that json locations set on Problems are correct but it seems that there's only ProblemTest that's dealing with it. This is a bit disappointing because I was hoping to piggyback on the existing coverage to verify addition of json pointers to Problem.

Secondly I've noticed that problem locations are constructed after parsing of the violating value has occurred which will be problematic if using the json pointer aware JsonParser implementation I created for obtaining the json pointer of the violation. That is because the pointer obtained after parsing of the violating value would be that of the parent container and not the offending value.

So before I commit to working on this, can you please confirm that you would be happy to snapshot the location and pointer of the element that might cause a violation before it's parsed and then using that snapshot for constructing a Problem via ProblemBuilder if the violation actually happens? I'm asking because if we do that then changes will need to be applied in all places where ProblemBuilderFactory#createProblemBuilder(JsonParser) is used which will be a non-trivial change.

[leadpony]

Hello @erdi.

Thank you for very nice effort and advices. I am so sorry for you, but I have decided that I will do implement the new feature myself, because this task requires a wide range of modification in code and needs important design decisions. Additionally I am afraid that it may cause a big performance issue if not well done. I have a concern about the performance of this library as shown in Justify Benchmark. Could you please wait a little and see my own implementation? Thank you.

[erdi]

Yep, I’m totally fine with you actually implementing it. It does look like some significant changes will be required and it would probably be hard to contribute something that does not imply performance penalties especially by somebody who has no prior experience with the codebase like myself.

I’m traveling today but I’m hoping to extract the code and tests for a json pointer aware JsonParser I already have and publish it in a separate repo on github by the end of the weekend. Hopefully it will be of help for you. I will comment on this issue with a link when that happens.

Looking forward to a version with json pointers on on Problem and thanks for your willingness to accomodate my needs.

On Sat, 2 Mar 2019 at 02:14, leadpony notifications@github.com wrote:

Hello @erdi https://github.com/erdi.

Thank you for very nice effort and advices. I am so sorry for you, but I have decided that I will do implement the new feature myself, because this task requires a wide range of modification in code and needs important design decisions. Additionally I am afraid that it may cause a big performance issue if not well done. I have a concern about the performance of this library as shown in Justify Benchmark https://github.com/leadpony/justify-benchmark. Could you please wait a little and see my own implementation? Thank you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/leadpony/justify/issues/5#issuecomment-468865083, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMii3lP943OhGQ7QV0efF12-h9OwvBzks5vSdB9gaJpZM4bJIBo .

[erdi]

FYI, @leadpony: https://github.com/erdi/json-pointer-json-parser

[leadpony]

Thank you @erdi. Your code is very similar to my own implementation. Both are using state transitions. I added 6 test cases from your repositry to my test file and mentioned the original author in the file. Please note that a JSON pointer of "/" points to the value of a object property whose name is "" (empty string), as described in RFC 6901.

[leadpony]

I came across a problem. JsonPointer in the JSON Processing API does not provide a way to obtain its string representation. Neither of the Reference Implementation nor Apache Johnzon implements toString() method. I cannot test created pointers! The signature of Problem#getPointer() should be changed...

[erdi]

Your code so far looks good to me. I probably would not bother with JsonPointer as the return type of Problem#getPointer() and simply change that method to return a String.

The last thing I'm missing is some test coverage for pointers on Problems, especially for constraints like required on objects because I suspect that the pointer for Problems caused by that constraint will be incorrect and will not be pointing at the object which is missing a required property but at its parent.

[ghost]

hi @leadpony Do you have a tentative date for the new release with the JSON pointer feature.

[leadpony]

Hi @adityamandhare. I have a plan to release it on next Monday if everything are going well. Thank you for waiting.

[leadpony]

Hello everyone. @mshaposhnik , @erdi , @adityamandhare in the participating order. After struggling awhile, I'm now ready to release the new version which supports the problem reporting with JSON pointers. Please see the following notes for users.

New Problem Message

In the new release, problem printers output each problem with both the location and the JSON pointer by default as illustrated below.

[4,5][/parent] The object must have a property whose name is "child". 

If the location part should be omitted, configured printers can be built easily by using ProblemPrinterBuilder.

ProblemHandler printer = service.createProblemPrinterBuilder(System::out)
        .withLocation(false).build();

This configured printer outputs the message like below.

[/parent] The object must have a property whose name is "child". 

For all who will write your own problem handlers, Problem#getPointer() method is the place to retrieve the additional information.

Evaluation Timing Changes

The evaluation timing for some assertion keywords was modified in order to produce more correct pointers where problems were found. For example, in past releases, uniqueItems assertion dispatched a problem immediately after the duplicated item was found in the middle of the array. In the new release, this evaluation is deferred until the end of the array and the JSON pointer in the reported problem points to the array itself.

Performance

The performance is good. It is even more faster than the previous release as shown in the benchmark result.

Any feedbacks will be welcomed. Thank you very much for all who encouraged me to implement this important feature.

[mshaposhnik]

Excellent news!

[leadpony]

Hi @mshaposhnik . The new release 0.14.0 is now available from the Maven Central Repository. Thank you so much for your useful suggestion.

[ghost]

Hey @leadpony thanks for the new release. I am testing the new version. So far looks good! Cheers!

[erdi]

I've finally got around to using the latest version and I can confirm that it works as expected for us with json pointers being part of the violation messages sent to problem printer. Thank you very much, @leadpony!

[leadpony]

Hello again @erdi . It's nice to hear that you are doing it well with this library. Thank you for good news!