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

Determine and implement a strategy for "merging" response metadata when redirecting to a route for swagger #526

Closed
1 task
myssto opened this issue Nov 7, 2024 · 2 comments
Assignees
Labels
epic Feature that requires significant effort to implement project:API Items related to the API project type:documentation

Comments

@myssto
Copy link
Contributor

myssto commented Nov 7, 2024

Tasks:

  • Implement a custom Swashbuckle.AspNetCore.SwaggerGen.IOperationFilter in namespace API.SwaggerGen to populate potential responses for redirected actions

Synopsis

Through testing I have noticed a quirk of swagger that causes endpoints that redirect to not populate the forwarded response codes / response types.

For example, GET /me will redirect to GET /users/{id} by forwarding the authorized user's id

    /// <summary>
    /// Get the currently logged in user
    /// </summary>
    /// <response code="302">Redirects to `GET` `/users/{id}`</response>
    [HttpGet]
    [Authorize(Roles = OtrClaims.Roles.User)]
    [ProducesResponseType(StatusCodes.Status302Found)]
    public IActionResult Get() =>
        RedirectToAction("Get", "Users", new { id = User.GetSubjectId() });

Objectively, this documentation is completely correct since redirects are separate requests, meaning a request to this endpoint (without following the redirect) will return a response code of 302 with no body.

However, this setup causes swagger to not generate response type information because it does not "look forward" into what the redirected request will return so to speak:

{
  "paths": {
    "/api/v1/me": {
      "get": {
        "summary": "Get the currently logged in user",
        "description": "\n\nRequires Authorization:\n\nClaim(s): user",
        "operationId": "Me_get",
        "responses": {
          "302": {
            "description": "Redirects to `GET` `/users/{id}`"
          }
       }
    }
}

This is a problem because any code generator will not be able to infer the response type of this operation. The above definition creates the following client code with nswag:

    /**
    * Get the currently logged in user
    *
    * Requires Authorization:
    * 
    * Claim(s): user
    * @return Returns the currently logged in user
    */
    // ** Notice the response type of OtrApiResponse<void> instead of OtrApiResponse<UserDTO> **
    get(cancelToken?: CancelToken): Promise<OtrApiResponse<void>> { }

Solution

At this time, since we only use redirects in a limited capacity I have simply just copy / pasted the ProducesResponseTypeAttribute<> and <response code=""> of the end destination onto the original request. This achieves the desired result for now but it is not refactor proof and will surely cause issues in the future with any schema changes or changes to the end destination's response codes.

For example (copying the response attributes from GET /users/{id} to GET /me):

    /// <summary>
    /// Get the currently logged in user
    /// </summary>
    /// <response code="302">Redirects to `GET` `/users/{id}`</response>
    // ** Copied from `/users/{id}`
    /// <response code="200">Returns the currently logged in user</response>
    [HttpGet]
    [Authorize(Roles = OtrClaims.Roles.User)]
    [ProducesResponseType(StatusCodes.Status302Found)]
    // ** Copied from `/users/{id}`
    [ProducesResponseType<UserDTO>(StatusCodes.Status200OK)]
    public IActionResult Get() =>
        RedirectToAction("Get", "Users", new { id = User.GetSubjectId() });

Produces the desired definition with populated response references:

{
  "paths": {
    "/api/v1/me": {
      "get": {
        "summary": "Get the currently logged in user",
        "description": "\n\nRequires Authorization:\n\nClaim(s): user",
        "operationId": "Me_get",
        "responses": {
          "302": {
            "description": "Redirects to `GET` `/users/{id}`"
          },
          "200": {
            "description": "Returns the currently logged in user"
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/UserDTO"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/UserDTO"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/UserDTO"
                }
              }
            }
          },
       }
    }
}

A desired permanent fix would alter the swagger generator to look ahead into the redirected action and replicate its potential response metadata onto the original action. This can likely be achieved with a custom Swashbuckle.AspNetCore.SwaggerGen.IOperationFilter. The operation filter exposes the entire swagger document as well as the MethodInfo for the current operation which can likely be reflected on to determine if a redirect will occur.

@github-project-automation github-project-automation bot moved this to Backlog in otr-beta-v2 Nov 7, 2024
@myssto myssto self-assigned this Nov 7, 2024
@myssto myssto added type:documentation epic Feature that requires significant effort to implement project:API Items related to the API project labels Nov 7, 2024
@myssto
Copy link
Contributor Author

myssto commented Nov 7, 2024

I realize this is super wordy and seems pointless to write out since I'll probably tackle this myself anyways but it helps me to walk myself through the problem :ehW:

@hburn7
Copy link
Collaborator

hburn7 commented Nov 20, 2024

Even if you're the one who will implement, it's very important that everything is written out. So, good stuff.

@hburn7 hburn7 removed this from otr-beta-v2 Jan 19, 2025
@myssto myssto closed this as not planned Won't fix, can't repro, duplicate, stale Feb 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic Feature that requires significant effort to implement project:API Items related to the API project type:documentation
Projects
None yet
Development

No branches or pull requests

2 participants