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

Sets response headers in the OpenAPI spec #258

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Sets response headers in the OpenAPI spec #258

merged 2 commits into from
Dec 10, 2024

Conversation

EwenQuim
Copy link
Member

@EwenQuim EwenQuim commented Dec 9, 2024

As per the title.

A few questions @dylanhitt @rizerkrof @ccoVeille:

  • How do we handle different response types? Currently, it only sets the response headers for 200 response, which is nice but do not cover all existing cases.
    • Handling all response codes : bad idea
    • Supporting only 200 : bad idea too
    • Have the code in the list of parameters : probably a good idea, but it should be a list. I don't like have a list of functoin parameters but meh, why not. nil == 200 should handle most of the cases
    • Have a param.StatusCode(200, 201, 404) : why not, but will pollute the OpenAPIParam which are for request and thus don't care about status code. if not specified, for 200 only
  • Should we have some kind of validation? I'm currently trying to raise a warning if the header is marked as required but do not exist in the HTTP response. Any other ideas?

Option A (implemented here ✅) : Clean usage, pollutes internals (acceptable?)

image

Option B : Ugly (acceptable?) usage, clean internals

image

@ccoVeille
Copy link
Collaborator

I'm unsure about the need here, also I'm not a guru in fuego, just a random Gopher reviewing some PRs.

I played with OpenAPI enough to know it can be fun.

OpenAPI allows you to define multiple response for the same http code. Think about different validation error messages.
These are often displayed in documentation with something called as "examples".

Also, the format could be totally different between a 403, 404, 500, 503, 429, 200, 201, 204

So is your request about dealing with the non-200 replies?

Does it takes into account the fact the OpenAPI reply can be totally different?

If not, it means it has to include an allOf or anyOf in the definition (I'm unsure about the exact term sorry).

Because for me, if you plan to support different code, the logic of handling different replies should be also considered

@EwenQuim
Copy link
Member Author

EwenQuim commented Dec 9, 2024

OpenAPI allows you to define multiple response for the same http code

I'm not sure that's true, that's what OpenAPI 4.0 "moonwalk" is mostly about. But you can answer different schemas for the same response, with oneOf/anyOf/allOf properties for example, as you said.

Because for me, if you plan to support different code, the logic of handling different replies should be also considered

And yes you're right, and it's a big issue. I think what Fuego is doing right now is a really nice sweet spot between usability and design.
For now, Fuego controllers allow:

  • one response for 200 status code (we might assign the default "positive" response to a 201 or a 204 in the future, that's a good issue)
  • another schema for errors (all 40x, 50x status codes)

I know it's not perfect, we might want to support other architecture, but that's not the scope of this PR.


That said, this PR aims to represent response header.

  • In a first draft, we can add it to only 200, then, we will add the response to only "positive" (non-error), whether it is 201, 204, 206.
  • Or we can see right now if we want to declare it directly for several correct status code! I think it's the good thing to do, so now I'm wondering what syntax should we use.

@EwenQuim EwenQuim marked this pull request as ready for review December 9, 2024 14:55
@ccoVeille
Copy link
Collaborator

I'm not sure that's true, that's what OpenAPI 4.0 "moonwalk" is mostly about.

That's why I talked about the examples sections.

The examples will be simple, but the spec would have to use anyOf and define each type.

option.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@dylanhitt dylanhitt left a comment

Choose a reason for hiding this comment

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

One thought about this MR though. Whenever i get around to working on #247.

In the struct

type Response struct{
  ContentTypes []string
  Type any
}

Do we want to add Headers to that input struct?

option.go Outdated Show resolved Hide resolved
@EwenQuim
Copy link
Member Author

Do we want to add Headers to that input struct?

Yeah probably, depends on how you'll be structuring that

@dylanhitt
Copy link
Collaborator

Yeah, I'm not sure either. I'm sure I'll find reason to do it or not once i'm in the code.

@EwenQuim EwenQuim merged commit caac676 into main Dec 10, 2024
5 checks passed
@EwenQuim EwenQuim deleted the response-header branch December 10, 2024 16:16
@EwenQuim
Copy link
Member Author

Thanks again for proofreading this @dylanhitt !

We passed the 500 tests milestone 🥳

image

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.

3 participants