KaiZen-OpenApi-Parser icon indicating copy to clipboard operation
KaiZen-OpenApi-Parser copied to clipboard

Add position information to validation items

Open andylowry opened this issue 5 years ago • 9 comments

This depends on completion of https://github.com/RepreZen/JsonOverlay/issues/27

andylowry avatar Aug 09 '18 14:08 andylowry

@ghillairet I've still got a fair bit of work to do, but you can exercise KZOP with position-aware validation items using two OSS staging repositories. the URLs are:

  • For KZOP: https://oss.sonatype.org/content/repositories/comreprezenkaizen-1051/
  • For JsonOverlay: https://oss.sonatype.org/content/repositories/comreprezenjsonoverlay-1029/

When looking at a validation item following a parse, you can use ValidationItem#getPositionInfo() to get position information. There currently isn't a document URL in the position info, which there needs to be. I'll add that shortly. But you do get character offset, line #, and column # for both the beginning and end of the region occupied by the identified JSON value. And you also get the JsonPointer that addresses the element in its file.

Let me know if there's more you need in order to try to integrate this into KZOE. I'll try to be as responsive as possible, given that I'm vacationing with family this week.

Currently this should only be tested with YAML files, which of course makes sense with KZOP. I haven't tested position capture for JSON files, and it's probably buggy.

andylowry avatar Aug 12 '18 23:08 andylowry

@ghillairet New staging repos. PositionInfo now provides the URL of the containing document

  • KZOP: https://oss.sonatype.org/content/repositories/comreprezenkaizen-1052/
  • JsonOverlay: https://oss.sonatype.org/content/repositories/comreprezenjsonoverlay-1030/

andylowry avatar Aug 13 '18 12:08 andylowry

@andylowry I think you forgot to add an accessor for the private field pos here https://github.com/RepreZen/KaiZen-OpenApi-Parser/blob/task/180/kaizen-openapi-parser/src/main/java/com/reprezen/kaizen/oasparser/val/ValidationResults.java#L140. Also it looks like the build for JsonOverlay here https://oss.sonatype.org/content/repositories/comreprezenjsonoverlay-1030/ does not contain changes to add position infos.

I made a local build to have those changes but position is always null.

ghillairet avatar Aug 15 '18 08:08 ghillairet

@ghillairet Sorry about the accessor... comes from using a test that only relies on toString. I'll push changes to that shortly.

But I'm not seeing your issue with the position info always coming out null.

For example, the following program shows position info both in the context of a validation item and when accessed directly from the parsed model:

package test;

import com.reprezen.jsonoverlay.Overlay;
import com.reprezen.kaizen.oasparser.OpenApi3Parser;
import com.reprezen.kaizen.oasparser.model3.OpenApi3;
import com.reprezen.kaizen.oasparser.val.ValidationResults.ValidationItem;

public class Test {

	public static void main(String[] args) throws Exception {
		OpenApi3 model = new OpenApi3Parser().parse(Test.class.getResource("test.yaml"), true);
		for (ValidationItem item : model.getValidationItems()) {
			System.out.println(item);
		}
		System.out.println(Overlay.of(model).getPositionInfo().get().toString(false));
	}
}

Here's my test.yaml file:

---
openapi: 3.0.0
info:
  version: "1.0"
  title: Foo

paths:
  /foo:
    get:
      responses:
        200:
          description: "OK"
          content:
            default:
              schema:
                type: xxx

Can you check your "Maven Dependencies" in Eclipse project view to see that you actually are getting v3.0.2.201808131204 from the staging repo?

andylowry avatar Aug 15 '18 11:08 andylowry

@ghillairet New KZOP staging repo has new accessor:

https://oss.sonatype.org/content/repositories/comreprezenkaizen-1053/

andylowry avatar Aug 15 '18 11:08 andylowry

@andylowry Ok thanks. I'm getting the positions now. It's because I was using:

new OpenApi3Parser().parse(tree, url, true);

and not

new OpenApi3Parser().parse(url, true);

ghillairet avatar Aug 15 '18 13:08 ghillairet

@ghillairet Latest staging repos:

  • JOvl: https://oss.sonatype.org/content/repositories/comreprezenjsonoverlay-1032/
  • KZOP: https://oss.sonatype.org/content/repositories/comreprezenkaizen-1055/

This includes a significantly simplified validation framework, and all validation items should now come with position info.

@tfesenko 's updates for v3.0.1 of the spec are not included... I'll manually merge them next. (automated merge is certain to fail miserably because the signatures of all the methods in the validation framework have changed quite a bit).

Still, I think this will give you a pretty good basis for more broadly testing integration in KZOE.

andylowry avatar Aug 19 '18 17:08 andylowry

@ghillairet @tedepstein

It turns out that the Jackson JSON parser provides extremely broken token location information, and has done so for many versions. It makes extracting usable location information pretty much impossible, IMO.

If I use the YAML parser to parse JSON, I get far better results. As of v1.2, YAML is (or claims to be) a true superset of JSON,. (There's a caveat that JSON doesn't explicitly prohibit duplicate keys in an object, while YAML does. But JSON also doesn't say what a parser should do with duplicate keys, so the argument is that a YAML parser's behavior in this situation - flagging as an error - is in any case preferable to a JSON parser that probably silently accepts it and chooses one of the values in some implentation-dependent fashion.)

My feeling is that we should just parse JSON and YAML using the Jackson YAML parser, rather than either of the other options:

  • Try to fudge the position info in the JSON parser - seems very fragile and problematic
  • Omit position information altogether for JSON input

For now I'll go with my recommended approach, but if that sounds problematic we can discuss other options.

andylowry avatar Aug 20 '18 13:08 andylowry

This would have helped me out today, I got a ValidationItem that only showed in toString representation, "List may not be empty" (I had a missing "responses" section on one of my endpoints).

pjroth avatar May 21 '20 19:05 pjroth