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

Allow different description per status code #3664

Closed
3 tasks done
stefan-schweiger opened this issue Jun 26, 2024 · 13 comments · Fixed by #4322
Closed
3 tasks done

Allow different description per status code #3664

stefan-schweiger opened this issue Jun 26, 2024 · 13 comments · Fixed by #4322
Assignees
Labels
bug Something isn't working lib:http triaged:core

Comments

@stefan-schweiger
Copy link

stefan-schweiger commented Jun 26, 2024

Clear and concise description of the problem

Currently when you use @errosDoc the doc comment will get applied to all errors with OpenApi.

@error
model ValidationError {
  @statusCode _: 400;
  message: string;
}

@error
model TechnicalError {
  @statusCode _: 500;
  message: string;
}

@route("/widgets")
@tag("Widgets")
interface Widgets {
  @errorsDoc("This will describe both errors") @get list(): Widget[] | ValidationError | TechnicalError;
}
paths:
  /widgets:
    get:
      tags:
        - Widgets
      operationId: Widgets_list
      parameters: []
      responses:
        '200':
          description: The request has succeeded.
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Widget'
        '400':
          description: This will describe both errors
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ValidationError'
        '500':
          description: This will describe both errors
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/TechnicalError'

Ideally it would be possible to have a description per possible error response. With my limited knowledge of what is possible with decorators I think it would probably need to look something like this:

@error
model TechnicalError {
  @statusCode("This is a Technical Error") _: 500;
  message: string;
}

// or

@error
model TechnicalError {
  @doc("This is a Technical Error")
  @statusCode _: 500;
  message: string;
}

// or

@error
model TechnicalError {
  @statusCode
  @statusCodeDescription("This is a Technical Error")
  _: 500;
  message: string;
}

Which would produce the following output:

'500':
  description: This is a Technical Error
  content:
    application/json:
      schema:
        $ref: '#/components/schemas/TechnicalError'

As far as I can tell in some older versions of typespec it was possible to use the @doc decorator to set this description, but now it will only add it to the schema but not directly below the status code.

Checklist

  • Follow our Code of Conduct
  • Read the docs.
  • Check that there isn't already an issue that request the same feature to avoid creating a duplicate.
@stefan-schweiger stefan-schweiger changed the title Allow @errorsDoc to have different descriptions per error type Allow different description per error type Jun 26, 2024
@stefan-schweiger
Copy link
Author

stefan-schweiger commented Jun 26, 2024

After playing around a bit more with things I actually think this might be a bug or at least an unconsidered edge case. If your error response does not have a body then the description is put to the correct place. But if you add something to the body the description will be placed on the schema and some "default" description will be used.

Reproduction

@stefan-schweiger stefan-schweiger changed the title Allow different description per error type Allow different description per status code Jun 26, 2024
@dfioravanti
Copy link

After playing around a bit more with thinks I actually think this might be a bug or at least an unconsidered edge case. If your error response does not have a body then the description is put to the correct place. But if you add something to the body the description will be placed on the schema and some "default" description will be used.

I observed the same behavior, I would really like to have the ability to add a description to the response. Is this a bug or expected behavior?

@timotheeguerin
Copy link
Member

timotheeguerin commented Jul 1, 2024

So this is kinda an expected behavior that we've had for a long time Cases

Basically the logic is that we only use the doc if the response model is an envelope(Has explicit body or no body). If you were to return the logic model directly then we don't include it. This so you don't have the doc carry over in those cases

/** This is a detailed doc on what is a pet */
model Pet {}
op read(): Pet;

in this case it doesn't make that much sense to have This is a detailed doc on what is a pet as the doc for the 200 response of read

Now the case Two in the playground above we could argue that this is an envelope as you specify an explicit status code.

You are also not the first person to try to put doc on the statusCode property expecting that to document the response so that also could be a possibility.

@timotheeguerin timotheeguerin removed their assignment Jul 1, 2024
@allenjzhang allenjzhang added the compiler:core Issues for @typespec/compiler label Jul 1, 2024
@dfioravanti
Copy link

@timotheeguerin thanks for the explanation. As a first time user of typespec to me

You are also not the first person to try to put doc on the statusCode property expecting that to document the response so that also could be a possibility.

would feel the most natural way to do it. Everything else is documented in this way, so why not routes? Maybe it is technically really challenging, I am just pointing out what makes the most sense to me

@timotheeguerin
Copy link
Member

I don't think it's really hard to figure out in the code only tricky part is deciding which description wins if provided in various place s

@markcowl
Copy link
Contributor

  • fix the issue in which docs for an implicit body response is not recognized.

@timotheeguerin
Copy link
Member

PR #4322 fixes the case 2 mentioned above. If you think we also should consider the description on the statusCode property to be moved there can you file a new issue

@stefan-schweiger
Copy link
Author

@timotheeguerin can you give me an example of the input and the output of this PR?

@timotheeguerin
Copy link
Member

timotheeguerin commented Sep 3, 2024

Before playground ➡️ PR preview playground

For the following code

import "@typespec/http";

using TypeSpec.Http;

/** One doc */
model One {
  @statusCode _: 200;
}

/** Two doc */
model Two {
  @statusCode _: 201;
  implicit: 200;
}

/** Three doc */
model Three {
  @statusCode _: 202;
  @body explicit: 204;
}

op created(): One | Two | Three;

Before case 2 wouldn't respect the doc but this pr makes it that it does.

@stefan-schweiger
Copy link
Author

stefan-schweiger commented Sep 4, 2024

@timotheeguerin well it respects the doc, but it only puts it on the schema, I would still argue that there needs to be a way to influence the docs for the response code itself.

image

Is something like this possible?

model Two {
  /** Two doc */
  @statusCode _: 201;
  implicit: 200;
}

@timotheeguerin
Copy link
Member

Was that looking at the Before playground? with the pr one this updates the 201 description
image

@stefan-schweiger
Copy link
Author

@timotheeguerin sorry my bad I must have gotten the playgrounds confused, because the PR playground is only showing me a blank page :( but if that's the actual output then I'm happy

@timotheeguerin
Copy link
Member

sounds good, if you still feel like it makes sense that the status code property description is used to set that as well can you file a follow up issue just about that but I suspect that we might want more feedback before doing this one

sarangan12 pushed a commit to sarangan12/typespec that referenced this issue Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lib:http triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants