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

Manipulation of the OpenAPI instance: complex object? #194

Open
jmini opened this issue Aug 24, 2018 · 7 comments
Open

Manipulation of the OpenAPI instance: complex object? #194

jmini opened this issue Aug 24, 2018 · 7 comments

Comments

@jmini
Copy link

jmini commented Aug 24, 2018

Given the an OpenApi3 instance called model parsing result of this small spec:

openapi: 3.0.1
info:
  title: Test extensions
  description: API under test
  version: 1.0.7
paths:
  /foo/bar:
    get:
      responses:
        200:
          description: it works!
          content:
            application/json:
              schema:
                title: inline_response_200
                type: object
                properties:
                  name:
                    type: string

I can manipulate the OpenAPI by doing:

model.getPaths().get("/foo/bar").getGet().setOperationId("xxx");

This is great to change a value.

But what if I want to add a server to the server list? There is this method:

model.addServer(server);

It expects a server instance. Should I do new ServerImpl(...) what are the requested parameter?
Is there a factory somewhere to create the server instance that I need?

Thank you in advance.

@andylowry
Copy link
Contributor

@jmini Thanks for the question, and sorry the documentation isn't nearly where it ought to be!

You should use ServerImpl.factory.create(...) to create a new server object. You can feed it an existing JsonNode object, or null if you want to just create an empty server.

The constructors are all private, because when the passed JSON is a reference node, and we've already parsed that JSON, we need to return a shared reference to the existing object, rather than creating a new one. The factories take care of that.

There's an intention (see #12) to generate fluent builders for all the model types, but that's currently not implemented.

Please close this issue if this response works for you.

@jmini
Copy link
Author

jmini commented Aug 25, 2018

The constructors are all private, because when the passed JSON is a reference node, and we've already parsed that JSON, we need to return a shared reference to the existing object, rather than creating a new one. The factories take care of that

This make sense.

Note that the constructors are public:

@Generated("com.reprezen.jsonoverlay.gen.CodeGenerator")
public ServerImpl(JsonNode json, JsonOverlay<?> parent, ReferenceManager refMgr) {
super(json, parent, factory, refMgr);
}
@Generated("com.reprezen.jsonoverlay.gen.CodeGenerator")
public ServerImpl(Server server, JsonOverlay<?> parent, ReferenceManager refMgr) {
super(server, parent, factory, refMgr);
}

I can ignore those, not a problem.


I had a look at the mentioned factory, I do not get how the create methods there help me:

	public JsonOverlay<V> create(V value, JsonOverlay<?> parent, ReferenceManager refMgr)
	public JsonOverlay<V> create(JsonNode json, JsonOverlay<?> parent, ReferenceManager refMgr)

a/ what are the parameters? You mention null, but I need a JsonOverlay and a ReferenceManager instance.

b/ this do not create a Server instance but a JsonOverlay<Server>.

@andylowry
Copy link
Contributor

andylowry commented Aug 25, 2018

@jmini Ah, sorry, I had forgotten some of the factory evolution, and thanks for pointing out the constructors are indeed public.

Unfortunately, I can see that things are a bit screwed up at the moment. In truth, I've got tests for the write methods in general, but actually "creating" a new instance via the factory for incorporation in an existing structure does not appear to be covered, and it doesn't currently appear to be possible, at least not in a completely kosher fashion.

However, that whole ReferenceManager thing is really for ensuring that references are handled consistently throughout a structure - e.g. references to the same JSON value end up being represented by shared objects in the model. I'm guessing that's not very important to your use case, at least as regards new objects you're adding after the initial parse.

If that's the case, then you should be OK with something like this:

Server server = ServerImpl.factory.create((Server) null, parent, new ReferenceManager());

where parent is the object you're adding the server to (the top-level model, or a Path or an Operation).

I have to run in one minute, else I'd test this before shooting off this response, but I'll check into it later. And I'll definitely circle back and make improve on this in the code.

@andylowry
Copy link
Contributor

@jmini

Ok, so my guess above does not work. Sorry if you wasted any time on it.

The following does work, but it's pretty clumsy.

Server s = (Server) ServerImpl.factory.create(JsonNodeFactory.instance.objectNode(), null, new ReferenceManager());

I'll try to get something reasonable in the next week or so. It won't be a true builder, but it probably will take the form of a builder to be aligned with future intention. So expect something like:

Server s = Server.builder(model).build();

and maybe a convenience method:

Server = Server.create(model);

The latter would just do the former. And the former would later be extended with typical fluent builder methods so you could do things like:

Server s = Server.builder(model).withDescription("My Server").withUrl("https://my.domain/api").build();

The model paramters to the builder method are what will enable a proper ReferenceManager for the new instance, so references will be coordinated with the overall model.

@jmini
Copy link
Author

jmini commented Aug 26, 2018

Ok, so my guess above does not work. Sorry if you wasted any time on it.

Do not worry, I am in an evaluation phase, so I am glad to hear that this feature will be improved.
I appreciate the quick responses.

The model paramters to the builder method are what will enable a proper ReferenceManager for the new instance, so references will be coordinated with the overall model.

This is very important. Consider a case like this:

openapi: 3.0.1
info:
  title: Test reference
  description: API under test
  version: 1.0.7
paths:
  /foo/bar:
    get:
      responses:
        200:
          description: it works!
          content:
            application/json:
              schema:
                title: inline_response_200
                type: object
                properties:
                  name:
                    type: string
components:
  schemas:
    Simple:
      properties:
        property:
          type: integer
          format: int64

I would like to do:

model.getPaths().get("/foo/bar").getGet().getResponse("200").getContentMediaType("application/json").setSchema(model.getSchema("Simple"));

And I expect the yaml to be edited with a $ref: "#/components/schemas/Simple"
This does not work now.

And of course it should also work if I do it for a new object (a new response or a new content-type).

@andylowry
Copy link
Contributor

@jmini Yes, you're right... setting a value that's already present elsewhere in the model should result in a reference. I'm not sure I'll be able to manage that quickly, though. It gets tricky in a multi-file scenario Single file would probably be simpler, and if that's your use case I could try to do that first.

@andylowry
Copy link
Contributor

@jmini Just a heads-up that a new major revision of the parser, v4.0.0, is now available on maven central. In this version, you will find:

  • A new class, Builder<V>, which can be used to create representations of values of type V. Its build() method returns a value of type JsonOverlay<V>.
  • The Builder constructor takes two arguments: the factory associated with type V, and any object appearing in the model. The latter is to enable the coordination of references, as discussed above.
  • The JsonOverlay base class now includes two new methods (which are therefore available in all generated and fixed subclasses):
    • builder() creates a builder, automatically passing the appropriate factory and using itself for reference coordination
    • create() just returns builder().build() - constructs a new instance of the required type.
  • Generated interfaces now all have static build and create methods, too, like the nonstatic methods of JsonOverlay just mentioned - except that they require an existing model object (since there's no this to use)

Bottom line is you should now be able to write Server server = Server.create(model); to cons up a brand new server instance, to do with as you wish.

Not appearing with these changes are:

  • Elaborated fluent builder APIs built atop the Builder class mentioned above
  • Assignment semantics discussed above in this issue, e.g. that if a server object already appears in the model in being operated on, adding it elsewhere should cause it to be reflected as a reference.

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

2 participants