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

Add position information to validation items #180

Open
andylowry opened this issue Aug 9, 2018 · 9 comments
Open

Add position information to validation items #180

andylowry opened this issue Aug 9, 2018 · 9 comments

Comments

@andylowry
Copy link
Contributor

andylowry commented Aug 9, 2018

This depends on completion of RepreZen/JsonOverlay#27

andylowry added a commit that referenced this issue Aug 12, 2018
Upgraded to Photo eclipse installation, default formatting changed. No
actual source changes here, just reformatted all source files to avoid
confusing future diffs
andylowry added a commit that referenced this issue Aug 12, 2018
This demonstrates the API by which positional info will be presented by
the validator, i.e. via ValidationItem#getPositionInfo()

Only a single validation currently adheres to this interface, namely a
check that the `type` property in a Schema is among the allowed
types. Working with a model that includes, e.g., a schema with `type:
xxx` should threfore produce a validation item with a non-null and
correct PositionInfo value.

PositionInfo really needs to include information about the source URL,
but it currently does not. That will be fixed along the way.
@andylowry
Copy link
Contributor Author

@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:

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
Copy link
Contributor Author

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

@ghillairet
Copy link
Member

@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.

@andylowry
Copy link
Contributor Author

@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
Copy link
Contributor Author

@ghillairet New KZOP staging repo has new accessor:

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

@ghillairet
Copy link
Member

@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 added a commit to ghillairet/KaiZen-OpenApi-Parser that referenced this issue Aug 16, 2018
Add missing positions when doing URL validations
andylowry added a commit that referenced this issue Aug 19, 2018
The Validation API has been significantly simplified by:

* Dropping the distinction between interface-level validatiosn and
  implementation-level validations. This is not currently supportable
  without a great deal of extra effort if we are to attach position info
  to validation messages, since the means of obtaining that info is
  built into *our* implementation. Of course, this also puts the whole
  idea of users substituting their own implementations in jeopardy as
  well, but it's not clear that was ever something that would work
  well. Maybe something for future reconsideration.
* Now that we've done the above, we can focus entirely on Overlay<?>
  values in the validation APIs. This makes things much easier, since
  all the non-API methods supported by overlay objects are easily
  available.
* Got rid of the injection mechanism for validators. This hurts
  performance and is of questionable value with the tearing down of the
  wall between intf and impl validations
* Altered the way validators get ahold of the values to be validated and
  the ValidationResults object to add items to. The former is now the
  sole argument to the 'validate' method on a validator, which then
  stashes it in a protected variable for all to see. The results are
  managed in a ThreadLocal that needs to be established by the code that
  invokes validation of the root object, and is then obtained as needed
  by validators. There's an AutoClosable to allocate this in a way that
  guarantees it'll be cleared afteward (try-resource statement)

Validations for the generated types have not been updated. That's the
next stage in this work.

Old validation code is untouched, for now (purely for reference
purposes) in 'old' packages (e.g. *.val => *.old.val, and similarly)
andylowry added a commit that referenced this issue Aug 19, 2018
andylowry added a commit that referenced this issue Aug 19, 2018
All validator classes now operate in the improved validation context,
and there are no more "overlay" validators (the ones that formerly
needed to dig below the surface of the generated API). A number of other
improvements to ValidationBase were made. The validations themselves
have chnaged very little, except that fields that formerly had no
validation (e.g. optional, unconstrained strings) nre now validated so
that the underlying JSON node type is checked for compatibility.
andylowry added a commit that referenced this issue Aug 19, 2018
JsonOverlay now defines field name constants in impl files,
e.g. "F_description" for a "description" field. These are now used
rather than literal strings in all the validators, to prevent
typos. Astonishingly, there wree none of these in the existing code,
despite sparse test coverage! (Tests were the only other way to spot
these.)
@andylowry
Copy link
Contributor Author

@ghillairet Latest staging repos:

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
Copy link
Contributor Author

@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 added a commit that referenced this issue Aug 20, 2018
[#180] Incorporate position information into validation items
@pjroth
Copy link

pjroth commented May 21, 2020

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).

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

No branches or pull requests

3 participants