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

Feat/define global responses #1612

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kamilzki
Copy link

Enables defining global responses which are inherited by all server routes.

fixes #884

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #884

What is the new behavior?

DocumentBuilder.addResponse has been added as a way to specify API responses globally. All server routes are then by default extending global responses.

SwaggerModule.createDocument(
  app,
  new DocumentBuilder()
    .addResponse({
      status: 500,
      description: 'Internal server error'
    }) // Same type as @ApiResponse decorator: ApiResponseOptions
   ...
)

Does this PR introduce a breaking change?

[ ] Yes
[ x] No

Other information

@jsonhero
Copy link

Any updates on this?

@ronan-f
Copy link

ronan-f commented Jun 20, 2022

Looks like this is passing. Is it possible to merge it? This would be very helpful to have. @kamilmysliwiec

@skiryuk
Copy link

skiryuk commented Jul 11, 2022

@kamilzki, @kamilmysliwiec Maybe we will make a merge or we are waiting for something else?

@kamilzki
Copy link
Author

kamilzki commented Sep 2, 2022

@skiryuk When I created the PR, I tried to contact with @kamilmysliwiec, but unfortunately, I've never got any response.
@ronan-f I as well wish to have that helpful feature!

@vizardkill
Copy link

up

@tuvshinbay4r
Copy link

Is it possible to merge? @kamilmysliwiec

@xadamxk
Copy link

xadamxk commented May 30, 2023

Ran into a scenario where this would be useful. Any chance this could be merged?

@MatiasDuhalde
Copy link

MatiasDuhalde commented Jun 28, 2023

I also have a need for this. AFAIK currently there's no way to directly modify the components.responses element of the OpenAPI document, so It would be a nice addition. But if I understand correctly, also the intention of the PR is to apply these responses to every defined path. However, I don't think that should be the default behaviour, since as per the OpenAPI spec, the idea is for the elements in this section to be reusable rather than global.

In that case, this is very similar to how currently the security methods work, there's the individual ones that add elements to components.securitySchemes, but also addSecurityRequirements that makes a scheme global. The difference is that here that translates into simply adding a root security attribute, whereas with responses, every path operation has to be modified.

Also, another difference here is that responses in components.responses do not specify a status code (they may include it as a custom attribute but it's not part of the spec), but it must be provided when added to the response element of a path operator. With that in mind my idea for the API would be something like this:

// ResponsesObject comes from lib/interfaces/open-api-spec.interface.ts
const myResponse: ResponseObject = {
  description: "My description",
  headers: {
    // ...
  },
  content: {
    "application/json": {
      // ...
    }
    // ...
  },
  links: {
    // ...
  }
}


new DocumentBuilder()
  .addResponse(response, "MyResponseName")
  .addGlobalResponse("MyResponseName", "4XX")

So the resulting OpenAPI document would be something like this:

{
  "openapi" : "3.0.0",
  "paths" : {
    "/some/path" : {
      "get" : {
        "operationId" : "getThing",
        "responses" : {
          "200" : {
            "description" : "This is a previously defined response",
            "content" : {
              "application/xml" : {
                "schema" : {
                  "$ref" : "#/components/schemas/SomePreviouslyDefinedResponse"
                }
              }
            }
          },
          "4XX" : {
            "$ref" : "#/components/responses/MyResponseName"
          }
        }
      }
    }
  },
  "components" : {
    "responses" : {
      "MyResponseName" : {
        "description" : "My description.",
        "headers": {
          // ...
        },
        "content" : {
          "application/json" : {
            // ...
          }
        },
        "links": {
          // ...
        }
      }
    }
  }
}

Just my two pennies' worth, it would be a shame to see a SECOND pull request go stale 😅

@guilopesn
Copy link

Any evolution in this PR?

@timi137137
Copy link

I'm sorry to bother you, but I would like this pull request to be merged.
As MatiasDuhalde upstairs said, the OpenAPI specification proves the feasibility and necessity of understanding coupling.

For example, in the Components Object section of Swagger's official documentation
image

Components Object holds a set of reusable objects for different aspects of the OAS. All objects defined within the components object will have no effect on the API unless they are explicitly referenced from properties outside the components object.

This paragraph comes from the OpenAPI specification v3.1 released in 2021, but this year is already 2024, this feature can not be used in nest, personal feeling will not be a little outdated?
image

I want to push forward with this PR merger, and it's my request alone. please.

@mgohin
Copy link

mgohin commented Mar 26, 2024

Kinda need it too, used to do it in java and the initial PR is 3 years old :/

#933

@Lyokolux
Copy link

Lyokolux commented Apr 16, 2024

I would also push for this, becaus the 500 error response is so common. I would also imagine some APIs have all their endpoints guarded, so it would be easier to manage a 401.
Is there any blocker for this that can be solved?

@Varagos
Copy link

Varagos commented Apr 17, 2024

Yeah agree, I am looking for the same functionality for 500 status code errors

@pknameer
Copy link

Any update?

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.

feature: define api response description globally