RepreZen / KaiZen-OpenApi-Parser

High-performance Parser, Validator, and Java Object Model for OpenAPI 3.x
130 stars 31 forks source link

Building a model programmatically #88

Closed KevinMitchell closed 6 years ago

KevinMitchell commented 6 years ago

Hi,

I can build an OpenAPI3 model by parsing a source file. And it looks like the model is mutable. But suppose I wanted to build a new model programmatically. Is the recommended approach to parse a fixed minimal OA3 spec, and then modify it? Or is there a better way of obtaining a new "empty" OpenApi3 model? And having built a model is there any way of exporting it as a YAML version of the specification, or do I need to write this code myself?

Thanks

andylowry commented 6 years ago

@KevinMitchell This is the point of #12. I'm imagining something that would allow you to write code like this:

model = OpenApi3.builder()
  .withOpenApi("3.0.0")
  .withInfo(Info.builder()
    .withTitle("Pet Store")
    .withVersion("1.2.3"))
  .withPath(Path.builder("/pets")
    .withMethod(GET)
      .withOperationId("abcde")
      ...
      .build())
    .build())
  .build());

I'm not of the mind that this use-case will be nearly as common as parsing YAML or JSON files, so it isn't high up on my priority list at present, but I do think it's an important feature.

In the meantime, as you say, parsing a spec and using the mutation API is a way to go. The available constructors for object overlays is pretty crummy, so code can get a bit ugly. And even if that weren't the case, without the builders your code will be way more complicated than it will be with builders.

As for producing YAML, you can use toJson() or toJson(true) to create a JsonNode value that can then be easily serialized into YAML or JSON, e.g.:

String json = new ObjectMapper().writeValueAsString(model.toJson());
String yaml = new YAMLMapper().writeValueAsString(model.toJson());

(As explained in README, toJson(true) renders the model but follows all resolvable references along the way, so the result is reference-free. There is not currently any recursion check on that, so circular references will result in stack overflow.)

This code requires both jackson-databind and jackson-dataformat-yaml artifacts from com.fasterxml.jackson.core group.

andylowry commented 6 years ago

@KevinMitchell I'd like to consider this a duplicate of #12. If you're OK with that, please close this issue.

KevinMitchell commented 6 years ago

OK, happy to view it as a duplicate. As for how common it would be, I agree it's at this point a niche use-case. I was interested in building OpenAPI specs from other API modelling languages, but that's a fairly specialised use-case. I tend to use xtend a lot, so I can probably add something like your builder API on top of your API using extension methods.

Thanks, Kevin

KevinMitchell commented 6 years ago

Hmm, maybe my optimism was misplaced :-( So even if I build a model from a template it looks like I can only alter things that already exist. So from the model I can get the paths, but I can't easily create a new path and add it to the map. So it looks like I'll have to wait for a solution to the referenced issue after all. Oh well, time for "plan B" :-)

andylowry commented 6 years ago

@KevinMitchell wrote:

So even if I build a model from a template it looks like I can only alter things that already exist.

Not true. E.g. you can use model.setPath("'/newPathName", pathObj). If that path already exists, this is a replace operation (which leaves its ordering within the map as a whole unchanged). Otherwise it's an insert operation, and the new path is ordered at the end of the map.

Sorry, I really need to put together some documentation!

KevinMitchell commented 6 years ago

But how do I create pathObj from scratch? At the moment I'd presumably have to create an instance of PathImpl...

andylowry commented 6 years ago

@KevinMitchell wrote:

At the moment I'd presumably have to create an instance of PathImpl

Yes, and this is where things get fairly ugly, cuz it's not at all obvious how constructors should be used. But here's an example:

        OpenApi3 model = new OpenApi3Impl("", null, null);
        Path path = new PathImpl("/pets", null, null);
        model.setPath("/xxx", path);
        System.out.println(new YAMLMapper().writeValueAsString(model.toJson()));

The output of this code is:

---
paths:
  /xxx: {}

The gotcha is that this won't currently work, due to a couple of bugs that I uncovered in getting this to work. I'll fix those and publish a new build, and hopefully you can make some progress on this. But I probably can't afford to put a lot of effort into making this stuff solid at this point, since as I explained earlier, I consider model building (and even the mutation API) as a relatively low-priority use case at present.

KevinMitchell commented 6 years ago

Thanks Andy. It looks like there would currently be a lot of gotchas going down this path. For example, if I load a partial spec like this:

info:
  version: 1.0.0
  title: .
paths:

And then call model.getInfo().setDescription("This is some text"); the description doesn't appear when I print the yaml. But if the description is set in the template then it gets updated with the new text and appears in the yaml. It sounds like there may be lots of little gotchas like this, so it's probably not sensible to go down this route at the present time.

andylowry commented 6 years ago

@KevinMitchell Up to you... I definitely won't claim there won't be more gotchas. But at this point all tests are passing with the fixes I've created for #89. And your example above is not a problem, once I fix the two validation errors in your partial spec:

If I turn off validation, the second problem is no longer an issue. If I also use OpenApi3Parser instead of OpenApiParser for my parser, then the first issue also goes away, since that forces the parser to assume OAS v3.0 (that is, it does that now that I've merged #92 :-) ).

I'm not sure how you got to the point of being able to set the info description if you started out by trying to parse the YAML you have above, but perhaps you didn't include all of it. In any case, I'm able to parse that YAML (or its fixed version), set the description, and then see the description when I print out the toJson() value.

I'll merge #90 and rebuild. Your choice whether to try to work with it. Mutation is definitely lightly tested, so no hard feelings if you choose to stay away for now.

KevinMitchell commented 6 years ago

Sorry, the openapi line went missing when I cut-and-paste into the comment. The actual spec was

openapi: "3.0.0"
info:
  version: 1.0.0
  title: .
paths:

I then run

ClassLoader classLoader = getClass().getClassLoader();
File file = new File(classLoader.getResource("DefaultSpecification.yaml").getFile());
OpenApi3 model = (OpenApi3) new OpenApiParser().parse(file, true);
model.getInfo().setDescription("This is some text");

try {
  String yaml = new YAMLMapper().writeValueAsString(model.toJson());
  System.out.println(yaml);
} catch (JsonProcessingException e) {}

This generates

openapi: "3.0.0"
info:
  title: "."
  version: "1.0.0"

If I set the description in the initial spec then I can successfully update it. This was with 0.0.1-SNAPSHOT. I'll try with the most recent version and see what happens.

Thanks, Kevin

andylowry commented 6 years ago

@KevinMitchell

Using your code I see the description printed out as expected. The latest version, 0.0.1.201710022120, is now available on maven central.

KevinMitchell commented 6 years ago

Thanks Andy. i've tried the latest version and setting the description works as expected now. In fact I can reduce my starting spec to just

openapi: "3.0.0"

and still build the info and paths elements :-)

I tried going further, and reducing the initial spec to the empty file! However, even using OpenApi3Parser, this resulted in a null pointer exception in the depths of the system. I then tried your suggestion of skipping the parser and using OpenApi3 model = new OpenApi3Impl("", null, null); and that worked fine.

So it looks like it's worth proceeding further and seeing how far I get.
Thanks for your help, Kevin

andylowry commented 6 years ago

@KevinMitchell OK, that's great.

But I need to warn you that in my current work I've discovered that I need to radically alter the implementation. It turns out that my planned reference treatment was totally ill-conceived.

My idea was that you could navigate to an object (e.g. a Schema) and then ask whether you got there via reference (isReference()), get details on the reference (getReference()), etc., all via operations on that object. Plus, I figured that you'd get the same overlay object no matter how you navigated to it - via reference or not.

But that's totally stupid! How can model.getSchema("Foo") be the same object (i.e. == to) schema.getProperty("foo") where the latter starts with a schema that has a property foo whose schema is $ref: "#/components/schemas/Foo"? Asking the former whether it's a reference has to return false, while for the latter it must be true. Therefore you can't be asking the same object! Doh!

So where I'm going with this is that the internal overlay structure will be a graph, with edges that contain navigation information. An edge will always have a "path" that says how the JSON tree is navigated from container to containee. It can also include a reference object if that navigation JSON node at the end of that navigation is a reference.

So asking about references will now always be in the context of a container.

Obviously, this will have fairly dramatic impact on the references API. I'll try to keep further impact on the API to a minimum, but I may not get away with further impact.

Sorry for the yo-yo-ing that you must be feeling with this.

KevinMitchell commented 6 years ago

My idea was that you could navigate to an object (e.g. a Schema) and then ask whether you got there via reference

At the time I saw it I thought it was a bit weird. I just assumed you were building multiple instances of the schema, one for each reference, and although this seemed odd I didn't know enough about your context to judge.

Speaking of things that seem odd ( :-) ) I don't really understand why json nodes are so visible in the API. You aren't the only one; the RAML model seems to go down a similar path. But JSON, YAML etc are just particular input and output formats to me. So it would have seemed cleaner to define the appropriate abstract model with no dependencies on JsonNode. And then write a translator from a tree of Json Nodes to this model. And similarly for the output. So you could then perform transformations on the model without having to continually be concerned about maintaining an underlying Json-based model. But I'm probably missing some subtlety about your particular use-case.

KevinMitchell commented 6 years ago

Hi Andy,
Things were progressing well until I got to building schemas programmatically. I tried something like this:

    Parameter parameter = new ParameterImpl("foo", null, null);
    parameter.setName("foo");
    parameter.setIn("query");
    Schema schema = new SchemaImpl("string", null, null);
    schema.setType("string");
    parameter.setSchema(schema);
    operation.addParameter(parameter);

It doesn't complain, but when I generate the yaml the parameter type is missing. Is there something else I need to initialise in the schema object for it to be accepted. Or is this another one of those cases where it needs tweaking a bit to be useable when there's no JSON model?

Kevin

KevinMitchell commented 6 years ago

It looks like calling setSchema on the model with a manually created schema also fail, this time with a NPE. I was assuming it would add it to the schemas map inside the (implicit) components object.

andylowry commented 6 years ago

Yeah, it was a pretty boneheaded miss on the references.

As for why JSON/YAML is so central, it's no mystery. Swagger - and now OpenAPI 3.0 - are both defined on a foundation of JSON or YAML. They're essentially DSLs that use JSON for their basic syntax and structure (and YAML comes for free because JSON is very nearly a strict subset of YAML, and it offers what many find to be a far more human-readable syntax.

Likewise, RAML is a DSL based on YAML.

The JsonOverlay framework I'm creating is meant to be an easy way of creating parsers and other utiltities for JSON- or YAML-based DSLs, and OpenAPI3Parser is the driving example. Relying on a JSON/YAML library like Jackson for base-level parsing and for serialization seem like a pretty natural choice in that context.

This may not be the most welcome option for someone with a use-case like yours, focusing on translating and transforming various modeling languages. But from the point of view of creating utilities for JSON/YAML-based DSLs, I think it makes sense.

That said, the nodes themselves are not really intended to be that visible in the API, other than during parsing and serialization. And I'm not sure that even now, they're very prominent outside those contexts.

andylowry commented 6 years ago

@KevinMitchell Re your latest bugs, sorry for your troubles. As I've said, I'm not really able to put a lot of effort into shoring up the mutation APIs at this point, but will get to it eventually. So as much as I hate to say this, your efforts might be better directed to an alternative approach for now.

KevinMitchell commented 6 years ago

your efforts might be better directed to an alternative approach for now.

Yes, I'm beginning to think that might be the case. To begin with I thought I might be able to use your interfaces, and just define my own classes on top. And then I could replace these by your classes if/when support for mutation is available. However, it's not ideal as there are still references to IJsonOverlay etc in the interfaces. I can just stub them out, and implement my own toJson() method. I just need to figure out whether it's worth keeping the connection with your interfaces or not.

For another app I'm parsing Open API specs and then transforming them to something else, so there I'm using your API as originally intended. Even though they are two different applications it will probably cause less confusion if I keep most of the names from your API even if I have to build a completely different implementation.

Thanks, Kevin