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

#183 Schema validation doesn't work with YAML Aliases #184

Merged
merged 3 commits into from
Aug 19, 2016
Merged

Conversation

tfesenko
Copy link
Member

Because Jackson parse does not handle YAML aliases well, we first process the JSON by snake-yaml which expands aliases, then we transform expanded YAML it to a JsonNode which is provided to JSON Schema validator.

I have performance concerns about too many passes required for validation:

  1. parseYaml() creates a org.yaml.snakeyaml.nodes.Node
  2. parseJson() uses snake-yaml to create an Object (a Map)
  3. The map is provided to Jackson to build a JsonNode
  4. Then, JSON Schema validator traverses the JsonNode to see if it conforms to the Swagger JSON Schema
    As discussed at today's SUM, these concerns can be addressed later.

@andylowry can you please review and try to merge ASAP?

@ghillairet can you please review too?

1. Aliases are correctly dereferenced and expanded in-place by
SnakeYaml. 
2. Then we convert the object (usually it's a `Map`) to a string, which
is passed to Jackson's Mapper.

I hope to find a better way of transforming Snake-YAML results into a
`JsonNode`
@tedepstein
Copy link
Collaborator

This one is time-sensitive and I can't review right now. I know @andylowry and @daffinm are busy with knowledge transfer on multi-file support. @tfesenko , maybe we should merge this, get it into API Studio for testing by our user, and complete code review next week, after your performance optimization (and hopefully with @ghillairet here to do the code review).

@tedepstein
Copy link
Collaborator

Leaving this to you, @tfesenko

@tfesenko
Copy link
Member Author

Ok, merging.
We can always roll it back if needed.

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.

2 participants