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

Support for OpenAPI3 #563

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Support for OpenAPI3 #563

merged 2 commits into from
Mar 15, 2023

Conversation

souenzzo
Copy link
Contributor

@souenzzo souenzzo commented Sep 5, 2022

  • Schema -> OpenAPI3
  • Malli -> OpenAPI3 (using json-schema)
  • Spec -> OpenAPI3
  • Coercion for all content-types
  • Coercion per content-type

Fixes #84

@souenzzo
Copy link
Contributor Author

souenzzo commented Sep 5, 2022

Malli is the only one that supports multiple content types

The API:

  • Add :content-types ["application/json" "application/foo+json"] to your path/route/router.
  • A key :content-type will be present in the option map, in your transformer.

I'm not sure if this API is enough.

I would like to have a feedback on it.

If this API seems to be enough, I can extend it to schema/spec too.

Copy link
Contributor

@piotr-yuxuan piotr-yuxuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I praise this very good work, and am thankul that you did it! I wanted it so much but shamefully didn't work on it. I haven't tried this to run this diff but it looks quite great. I command your code quality and the extensive work you did 🚀 I quite like that Open API 3.0 and Swagger may coexist alongside.

I'm just a rando an GitHub and have no power on this repo, but I left a couple of minor comments that I believe might help smooth the approval process. Take them very freely. Please don't take the style comments very seriously: I didn't pedantically left them, they are more to help me understand and reflect on how to structure code 🙂

Comment on lines +161 to +167
{:requestBody {:content (into {}
(map (fn [content-type]
(let [schema (->schema-object body {:in :requestBody
:type :schema
:content-type content-type})]
[content-type {:schema schema}])))
content-types)}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a very minor suggestion for this snippet and below. Feel free to discard.

Suggested change
{:requestBody {:content (into {}
(map (fn [content-type]
(let [schema (->schema-object body {:in :requestBody
:type :schema
:content-type content-type})]
[content-type {:schema schema}])))
content-types)}})
(->> content-types
(map (juxt identity
(comp #(do {:schema %})
(partial ->schema-object body)
#(do {:in :requestBody
:type :schema
:content-type %}))))
(assoc-in {} [:requestBody :content]))

project.clj Outdated
@@ -86,7 +87,7 @@
[metosin/muuntaja "0.6.8"]
[metosin/sieppari "0.0.0-alpha13"]
[metosin/jsonista "0.3.5"]
[metosin/malli "0.8.2"]
[metosin/malli "0.8.9"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do accept that this is most desirable (in some personal projects I exclude malli from reitit and import the latest version), but is it required by this diff? Would it be more appropriate in a dependency-related PR?

@@ -156,6 +157,7 @@
(into {}))))
(-get-apidocs coercion specification)))))


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this vertical space compare with neighbouring ones?

| :parameters | optional input parameters for a route, in a format defined by the coercion
| :responses | optional descriptions of responses, in a format defined by coercion

Example:
Copy link
Contributor

@piotr-yuxuan piotr-yuxuan Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this thorough, nice docstring. I noticed top-level directories /examples and /doc, do you think it would be worth adding an example and some explanations there? might be just for the sake of completion, hinting at possible current limitations, and showing copy-paste-ready code.

(if (and data (not no-doc))
[method
(meta-merge
#_base-openapi-spec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Completely worth keeping if it's a helpful breadcrumb for later developer, I can't judge.

@StankovicMarko
Copy link

any plans on this getting merged?

@stathissideris
Copy link

Is there any work left on this? I'd be willing to help out.

@ikitommi
Copy link
Member

Thanks for all the work on this! As this is a big change, planning to make a maintenance release before digging into this.

@ikitommi
Copy link
Member

Merged with master as thought it was ok but there is a change in malli json schema rendering causing the tests fail now. we can fix those when review, sorry.

@coyotesqrl
Copy link

This is exciting. I've already had to hack in swagger->openapi on one project and never want to do that again, so being able to autogenerate openapi as easily as swagger would be great.

One thing I noticed on a very brief scan was that there didn't seem to be an analog to reitit.swagger-ui/create-swagger-ui-handler. Is that something likely to be added sometime shortly after this is released (hopefully that's soon), or am I seriously underestimating the effort required for that?

@opqdonut
Copy link
Member

opqdonut commented Mar 1, 2023

I'm looking at what's needed to be able to merge this. It's a good base. The most notable missing piece is the validation/coercion for the per-content-type models.

@opqdonut opqdonut mentioned this pull request Mar 3, 2023
@opqdonut
Copy link
Member

opqdonut commented Mar 3, 2023

My work is in #588. It contains the stuff in this PR.

@opqdonut opqdonut merged commit 0648296 into metosin:master Mar 15, 2023
@souenzzo souenzzo deleted the feature/openapi branch May 3, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support for OpenAPI3
7 participants