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

Document that OpenAPI 2.0 is supported #9

Open
ehmicky opened this issue May 9, 2018 · 23 comments
Open

Document that OpenAPI 2.0 is supported #9

ehmicky opened this issue May 9, 2018 · 23 comments

Comments

@ehmicky
Copy link
Contributor

ehmicky commented May 9, 2018

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
Copy link
Owner

mikunn commented May 9, 2018

Oh. Didn't know that 😃 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
Copy link
Contributor Author

ehmicky commented May 9, 2018

The differences are as follow:

  • OpenAPI 2.0-only:
    • type file
  • OpenAPI 3.0-only:
    • oneOf, anyOf and not (from JSON schema v4)
    • writeOnly (from JSON schema v7)
    • deprecated
    • nullable
  • OpenAPI 2.0/3.0 differences:
    • discriminator is a string (property name) in 2.0 and is { propertyName: string, mapping object } in 3.0

So the only things that matter here are:

  • type: "file": at the moment, this throws an exception (not sure if this is intentional)
  • discriminator: it is simply removed, i.e. works

@mikunn
Copy link
Owner

mikunn commented May 10, 2018

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 😄

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

ehmicky commented May 11, 2018

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:

  • deleting the type property
  • guessing the type from the surrounding properties. There is some code already available (used in JSON-schema-faker) that does that here

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

@ehmicky
Copy link
Contributor Author

ehmicky commented May 16, 2018

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

@mikunn
Copy link
Owner

mikunn commented Jul 28, 2018

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

ehmicky commented Jul 29, 2018

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
Copy link
Owner

mikunn commented Jul 30, 2018

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

ehmicky commented Jul 31, 2018

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:

  • available in parameters and in response body
  • not available in response headers nor in definitions

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
Copy link
Owner

mikunn commented Jul 31, 2018

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

ehmicky commented Jul 31, 2018

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:

  • request body and response body are the closest to JSON schema v4.
  • in request parameters, JSON schemas keywords are merged in with other parameter properties instead of being namespaced under a schema property.
  • request parameters and response headers have fewer JSON schema keywords.

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:

  • some keywords are not present (e.g. properties), which does not require any work.
  • some request parameters have extra properties (e.g. collectionFormat or in) which can just be removed. For required, it's a little trickier, as it would need to be skipped when it's the boolean value of a parameter object, but not when it's the JSON schema keyword (whose value is an array).
  • type: "file"

What do you think?

@mikunn
Copy link
Owner

mikunn commented Jul 31, 2018

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

ehmicky commented Aug 1, 2018

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
Copy link
Owner

mikunn commented Aug 1, 2018

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

ehmicky commented Aug 1, 2018

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 😃

Nevertheless I see two main reasons for supporting OpenAPI 2.0:

  • libraries authors (like me), i.e. developers that write an OpenAPI library/utility for other users, as opposed to directly using OpenAPI to describe their own API. In that case they might want to broaden their audience to OpenAPI users still using 2.0 (which I believe is still very common).
  • many tools in the OpenAPI ecosystem do not support OpenAPI 3.0 yet. Including some fundamental ones likes parsers like Sway. The main one Swagger-parser only very recently added OpenAPI 3.0 support and other tools that depend on it haven't upgraded yet. Some OpenAPI users might want to wait to upgrade to 3.0 if they want to use those tools.

@mikunn
Copy link
Owner

mikunn commented Aug 1, 2018

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

ehmicky commented Aug 1, 2018

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
Copy link
Owner

mikunn commented Aug 1, 2018

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

ehmicky commented Aug 1, 2018

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

@mikunn
Copy link
Owner

mikunn commented Aug 2, 2018

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

ehmicky commented Aug 2, 2018

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
Copy link
Owner

mikunn commented Aug 2, 2018

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

ehmicky commented Aug 2, 2018

I added #23 for continuation of this discussion.

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

P0lip pushed a commit to stoplightio/openapi-schema-to-json-schema that referenced this issue Aug 12, 2020
* fix: use clone instead of JSON

* add test
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