mikunn / openapi-schema-to-json-schema

Converts OpenAPI Schema Object to JSON Schema
75 stars 6 forks source link

Document that OpenAPI 2.0 is supported #9

Open ehmicky opened 6 years ago

ehmicky commented 6 years ago

This package actually also support OpenAPI 2.0.

Most of the OpenAPI ecosystem is (unfortunately) still stuck on 2.0 version, so documenting the fact this library supports it might be a good idea?

mikunn commented 6 years ago

Oh. Didn't know that :smiley: Well, support for 2.0 is not tested or really even considered, so I would be inclined to document that it MAY support 2.0 but is not tested. Unless, of course, the 2.0 specification of Schema Object is identical (or a subset). I'm not that familiar with the older versions.

ehmicky commented 6 years ago

The differences are as follow:

So the only things that matter here are:

mikunn commented 6 years ago

Thanks for the differences, really helpful!

So it should work otherwise except for the type: "file". It throws because it's not one of the types defined in the OpenAPI 3.0 specification. I don't even known what should be done if one is encountered - well, it should be converted somehow or removed, but in both cases hope is lost anyways :smile:

It's probably for the best to mention that it SHOULD work with 2.0 as well as long as there isn't type: "file" defined.

ehmicky commented 6 years ago

I think the correct behavior for type: "file" would be to delete the type property.

When it comes to type: "file", my understanding is that it means the MIME type is multipart/form-data and a filename is provided. It does not say whether the content is string, number, etc.

So the correct behavior could be either:

Do you think it would be possible so that this library can be used for OpenAPI 2.0 as well?

ehmicky commented 6 years ago

If you wanted to go ahead with the second idea (guessing the type) I can submit a PR.

mikunn commented 6 years ago

type: "file" seems to be available only for Parameter Object and Response Object. The former can't be converted anyways (at least not yet) and the latter doesn't really represent a JSON schema object if type: "file".

Basically all examples that define a file response have a schema object like this:

schema:
  type: file

This doesn't even try to represent any JSON kind of thing, but rather some other file, so I don't see a point trying to convert it at all. I think the correct way to handle this would be to throw an exception to point out that one is basically trying to convert a file to JSON schema.

Deleting the type property might not be a good idea as the schema above would convert to an empty JSON schema against which any JSON would be valid.

I would be interested to see cases where the type could be converted to something else based on other data in the Schema Object in a Response Object. Let me know if you have an example.

And btw, sorry for the huge delay. I'm not working for the company anymore that triggered the development of this package. But I will now continue developing it on my free time more actively.

ehmicky commented 6 years ago

I think there might still be some value to try to convert it.

Specifically when OpenAPI is used not only to document an API but also to validate requests. I.e. schemas are converted from OpenAPI to JSON schema v4 then validated against incoming requests.

In that use case, it would be odd to allow validating any request except the ones with multipart/form-data MIME type.

multipart/form-data requests could be parsed to JSON by the API, e.g. using name;filename=FILENAME as key and parsed content as value. There might be mismatches between the specific syntax used to parsed as JSON and the JSON schema validating them, but that could be solved by users of this library. Throwing an exception does not even give them that chance.

mikunn commented 6 years ago

I can see your point and it certainly would make sense for requests. But the problem is that type: "file" is defined on the root of the Parameter Object rather than in the Schema Object. And there's currently no support for Parameter Object.

ehmicky commented 6 years ago

type: "file" is present both in Parameter Object and in Response object. According to the specification:

While it is technically not on the Schema Object definition, it is an extension to it:

I think the first two cases are the most common use cases of JSON schemas though, so maybe this would be useful to support this extension?

An alternative solution would be to implement this but feature flag it behind an option?

mikunn commented 6 years ago

I think the first step is to support OpenAPI 2.0 so that any type: "file" in a Schema Object is handled reasonably as this package was originally designed to handle Schema Object conversions and nothing more. Parameter Objects and Response Objects need their own discussion in this regard.

Response Objects can have type: "file" in the Schema Object. But I highly doubt that, in most definitions, there would be enough information in the schema object to do a type conversion. Besides, although it should technically be possible to set produces: "application/json" even when type: "file", I still feel it doesn't make any sense trying to convert it. So, in this case, I would throw an exception by default. We could include options to handle it in some way if the need arises.

Since the Schema Object in a Request Object can't have type: "file", this doesn't need any action in terms to supporting OpenAPI 2.0. However, as you have pointed out, it would indeed be a beneficial feature to support type: "file" in the Request Object as well as request objects themselves in both OpenAPI 2.0 and 3.0. But we would then need to start looking outside the Schema Object. This would then be better discussed in another issue.

Let me know what you think.

ehmicky commented 6 years ago

I would personally take it from this angle: in OpenAPI 2.0, request parameters, request body, response body and response headers can be described using a slight variation of JSON schema v4. For some reasons, the designers of the specification decided to use 3 different shapes for those JSON schemas depending on where it's used:

This is arguably confusing and that's probably why they corrected it in OpenAPI 3.0 where there is only one JSON schema definition.

However many users might still want to take a request parameter or a response header and extract the JSON schema from it. That seems like a very legitimate thing users might want to do, regardless of the OpenAPI 2.0 design decisions above.

It turns out supporting OpenAPI 2.0 request parameters and response headers might not require much work since the only changes are:

What do you think?

mikunn commented 6 years ago

This certainly makes sense and I understand your point now. The discussion and requested feature set has moved quite a bit away from the title of this issue which confused me a bit :)

I suggest that we close this issue and not document at this point that OpenAPI 2.0 is supported, but rather create a new issue to actually (semantically) support OpenAPI 2.0 with its different schema shapes.

Be free to create the new issue if you feel like doing so. You seem to have a firm grasp of OpenAPI 2.0.

ehmicky commented 6 years ago

I've created #19 and #20 as follow ups to this discussion.

When both are closed, maybe documenting that OpenAPI 2.0 may be supported might be possible. So should this issue be left opened until them?

mikunn commented 6 years ago

Thanks for the issues!

It's probably better to leave this open although it might trick someone to believe that OpenAPI 2.0 is supported and not just documented.

What comes to roadmap, do you have (or know someone who has) a need for OpenAPI 2.0 support? I was thinking about implementing a more complete support for OpenAPI 3.0 including support for Parameter Object before starting to support OpenAPI 2.0 unless there's a real need.

ehmicky commented 6 years ago

I also think that OpenAPI 3.0 is the future and people will eventually leave OpenAPI 2.0. I would also personally love to just skip supporting it in my own projects :smiley:

Nevertheless I see two main reasons for supporting OpenAPI 2.0:

mikunn commented 6 years ago

I agree and I was just thinking about the priority of "completing" OpenAPI 3.0 support vs adding OpenAPI 2.0 support.

There might be flags that are relevant for OA 3.0, but not for OA 2.0 and vice versa. We might need to change the API a bit to support OA 2.0 while avoiding a flag mess. So in that sense I feel that OA 3.0 support should probably see some improvement before focusing on OA 2.0.

ehmicky commented 6 years ago

Except for discriminator (which is removed anyway in both versions since it's not valid JSON schema) and type: "file", the properties that are present in both OpenAPI 2.0 and 3.0 have the same shape. The only differences are about few JSON schema keywords that were added in OpenAPI 3.0. OpenAPI 2.0 schemas are a subset of OpenAPI 3.0 schemas, so both OpenAPI versions can be effectively supported without breaking changes or compatibility issues. E.g. supporting nullable won't have any effect on OpenAPI 2.0 schemas since they don't have that property.

Based on this, I think OpenAPI 2.0 and OpenAPI 3.0 could both be supported without neither requiring the user to choose with an option/flag nor introducing branching logic in the code. Consequently I do not think there is any work as well when it comes to flags that are only relevant for one version, because they would not break anything if a schema from the other OpenAPI version was passed.

When it comes to the work required, it seems like if #20 gets resolved, OpenAPI 2.0 will perfectly work. I don't think there is any other work to be done/prioritize to support OpenAPI 2.0, unless you see something else?

mikunn commented 6 years ago

Resolving #20 would require looking outside of schema in Parameter Object. This would mean that supporting OpenAPI 2.0 would require the possibility to pass in a parameter object.

I was revisiting #8 and thought that if we support Parameter Object for OpenAPI 2.0, we should support it for OpenAPI 3.0 as well. I would like to implement it for OpenAPI 3.0 first and then for OpenAPI 2.0 to keep the primary focus on OpenAPI 3.0 support, as OpenAPI 3.0 is undeniably the more powerful format.

ehmicky commented 6 years ago

Yes that makes sense! I created #19 for that purpose, maybe we should start working on that issue first?

mikunn commented 6 years ago

19 discusses more about OpenAPI 2.0, so I suggest that we create a new issue for OpenAPI 3.0 parameter object support and then start looking at #19 and finally #20.

It was a good idea you suggested in #8 when it comes to handling parameter objects. I'm a bit short of free time this week, so in the mean time go ahead and create the issue if you want.

ehmicky commented 6 years ago

Just to make sure I understand what you mean: you mean converting an OpenAPI 3.0 Parameter object into a JSON schema v4?

If so it might be a little tricky as OpenAPI 3.0 parameters can specify several JSON schemas (one per MIME type).

mikunn commented 6 years ago

Yes, that's what I mean. You mean multiple schemas with content instead of schema?

I think we could return an object where MIME types are the keys and and schemas the values. We convert each schema definition in content to JSON Schema v4 and modify the schemas according to the settings on the root level of the parameter object.

Do you think that would work?

ehmicky commented 6 years ago

I added #23 for continuation of this discussion.

Follow up questions are: should we allow converting the following objects?