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

Fix OpenAPI Callback circe encoder #2098

Merged
merged 7 commits into from
May 6, 2022

Conversation

seglo
Copy link
Contributor

@seglo seglo commented May 2, 2022

fixes #2095

Omits pathItems from being encoded into output JSON, similar to Encoder[Responses] and Encoder[PathItem]. Produces valid OpenAPI spec for callbacks.

The test is a big hack to add a callback, because it's not currently supported in the Endpoint DSL. I can drop that if you like.

@adamw
Copy link
Member

adamw commented May 4, 2022

Thanks!

But maybe the model is simply wrong? I mean the case class Callback itself. It should be a Map[String, PathItem], but without the wrapper? Question is, where do extensions go, and how references can be made.

Also, if you'd need a better api to add callbacks, let's simply add this to Operation for example.

@seglo
Copy link
Contributor Author

seglo commented May 4, 2022

Hi @adamw. I think you're right and the model is wrong. According to the Open API spec the callbacks field in an Operation object is defined as:

Map[string, [Callback Object](https://swagger.io/specification/#callback-object) | [Reference Object](https://swagger.io/specification/#reference-object)]

Where a Callback contains a single PathItem. It's basically a Map[String, PathItem] as you said.

An operation Callback can also reference re-usable callbacks defined in Components. I'm not sure how the reference relationship works, I'd have to dig into it more. The Callbacks page only includes an example of a callback defined within an Operation itself.

I can spend more time getting tapir's Open API model to encode the correct schema for callbacks, but I have some other priorities this week that I need to work on first.

Can you clarify what you mean with this statement:

Also, if you'd need a better api to add callbacks, let's simply add this to Operation for example.

Are you proposing extending Endpoint API to accept callbacks? This would certainly be an ideal outcome, but it would take me longer to implement. For my purposes I would be alright with the Open API model being encoded correctly.

@seglo
Copy link
Contributor Author

seglo commented May 4, 2022

Question is, where do extensions go,

I see now how the encoder in this PR is handling extensions and it's indeed incorrect. I don't think extensions belong at this level since it's supposed to represent a key/value mapping of callback names to callbacks. Extensions might be handled in the PathItem itself, the same way they're already done.

@adamw
Copy link
Member

adamw commented May 4, 2022

Are you proposing extending Endpoint API to accept callbacks? This would certainly be an ideal outcome, but it would take me longer to implement. For my purposes I would be alright with the Open API model being encoded correctly.

No, I think adding to Endpoint would be too niche, I though about some helper methods on the OpenAPI model classes, once you navigate to them from the generated value :)

@seglo
Copy link
Contributor Author

seglo commented May 4, 2022

No, I think adding to Endpoint would be too niche, I though about some helper methods on the OpenAPI model classes, once you navigate to them from the generated value :)

Got it. Works for me!

@seglo
Copy link
Contributor Author

seglo commented May 5, 2022

I think this solution is a little convoluted. I may be missing something with the design considerations in the OpenApi model.

The current implementation demonstrates adding callbacks to operations and components, as well as referencing a reusable callback.

How the test's expected yaml looks like generated with redoc:

image

@seglo seglo changed the title Flatten OpenAPI Callback circe encoder Fix OpenAPI Callback circe encoder May 5, 2022
@adamw
Copy link
Member

adamw commented May 6, 2022

@seglo we need to support multiple paths in callbacks, so I added the map back, changing the encoder. I also changed the test to use quicklens, I think it's more readable now :)

@adamw adamw merged commit 5d4bf0b into softwaremill:master May 6, 2022
@seglo
Copy link
Contributor Author

seglo commented May 6, 2022

That looks much better, thanks. I had trouble adding quicklens to the project, but I see you were able to resolve that!

@seglo seglo deleted the seglo/operation-callbacks branch May 6, 2022 13:59
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

Successfully merging this pull request may close these issues.

[BUG] OpenAPI callbacks on operations doesn't match spec
2 participants