-
Notifications
You must be signed in to change notification settings - Fork 512
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
Insertion of example in query/path/body #459
Insertion of example in query/path/body #459
Conversation
Can you translate your text above? I apologize, but I can’t make sense if it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add to the readme.md file to explain the new decorator. There are other requested changes.
@CristianoVotre is this PR still necessary after someone clarified that there is an alternative approach that already exists? See here: #180 (comment) |
I believe this would still be a good addition since you can provide specific examples per path. (And would, therefore, be closer to the |
@@ -33,7 +33,7 @@ export function Request(): Function { | |||
* | |||
* @param {string} [name] The name of the path parameter | |||
*/ | |||
export function Path(name?: string): Function { | |||
export function Path(name?: string, example?: string): Function { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string
is not accurate either since path variables can be numbers, date, bool, etc (I believe)
@@ -2,7 +2,7 @@ | |||
* Inject http Body | |||
* @param {string} [name] properties name in body object | |||
*/ | |||
export function Body(): Function { | |||
export function Body(example?: string): Function { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all of these decorators, we need to make room for multiple configurations that could come down the line. So we need something extendable. That means an object. So instead of taking on primitive, instead have it be:
export interface BodyDecoratorConfig<T extends object> {
swaggerOverride?: {
description?: string,
example?: T
}
}
@Body(config?: BodyDecoratorConfig)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts @WoH / @lukeautry / @endor on the notion of allowing for people to override any of the Swagger properties? I ask this because this PR only allows the override of the example and I describe here how we need to be able future proof by structuring the function to allow an object, for extensibility purposes: https://github.com/lukeautry/tsoa/pull/459/files#r319777012
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. Easiest things first: Yes, every decorator should have a well-specified config interface. I am not against the idea of overriding specific high-level things on a per-request basis, but I'd have to take a look how far I'd go. For now I think, everything annotating the entire interface is reasonable (description and example). I've had cases where I'd have to duplicate an interface to adjust the example I'm giving (one that's valid in the context of the request).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the harder thing: The OpenAPI spec says example
is of type any
which would be unknown
in typescript.
However, it explicitly mentions:
The example SHOULD match the specified schema and encoding properties if present. The example field is mutually exclusive of the examples field. Furthermore, if referencing a schema which contains an example, the example value SHALL override the example provided by the schema. To represent examples of media types that cannot naturally be represented in JSON or YAML, a string value can contain the example with escaping where necessary.
In order to get this checked, the config should allow a type argument so you are able to get type checking, with the fallback to unknown
(Assuming we can't get typescript to infer the type of a decorated interface).
If we specify an example, we allow to specify all the properties of an example object though: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#example-object
I'll have a look at this tomorrow, but these are the ideas at the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To represent examples of media types that cannot naturally be represented in JSON or YAML, a string value can contain the example with escaping where necessary.
This isn’t a problem for us because tsoa has an established stance of only handling JSON. So I've decided that we should start with strict typing and we can relax it later if someone requests that we make a change/enhancement. Let’s leave the type of example
as unknown. However, we can't allow tsoa users to make silly mistakes.
So we need to run swaggerOverrides.example
through ValidationService.ValidateParam()
during the generation to ensure the developer provided the correct type.
Why am I asking us to run validation instead of this idea of @WoH’s? (keep reading)
In order to get this checked, the config should allow a type argument so you are able to get type checking, with the fallback to unknown (Assuming we can't get typescript to infer the type of a decorated interface).
That would be awesome. I would prefer that the TypeScript compiler inform the user that the example doesn’t match, but unfortunately there is a known issue with inference of parameter decorators as illustrated by this SO question: https://stackoverflow.com/questions/55144265/how-to-limit-the-type-of-parameter-that-the-given-parameter-decorator-may-be-app).
Reminder @CristianoVotre, I still want the input to be a config object like this:
interface PathDecoratorConfig {
name?: string,
swaggerOverride: {
example: unknown
}
}
const parameterDecorator = (nameOrConfig?: string | PathDecoratorConfig) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still check at compile time, but I think this would require a lot of work. If we'd want to have TS do the work, which is where I was looking at, I'd like to see something like @Body<T>(config: Config<T>)
where interface Config<T> = { description?: string, examples?: { summary?: string, value: T }[] }
. I disagree with the naming, since this is not overriding anything. These examples are explicit part of the OAPI3 spec. Swagger 2 should just ignore and warn that this can't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still check at compile time, but I think this would require a lot of work. If we'd want to have TS do the work, which is where I was looking at, I'd like to see something like @Body
I wish we could, but the problem is that T
can’t be inferred for decorators of parameters. So that’s why the StackOverflow issue presents this possible problem:
function postSomething( @Body<Thing1>() aParemeterWhosTypeIsNotInferred: Thing2 ): Thing2
^ that’s a really unfortunate problem that the T of Body (I.e Thing1
) can be different than the variable’s type (I.e. Thing2
). So as far as I can tell, having TS do the checking is blocked by microsoft/TypeScript#30102
I disagree with the naming, since this is not overriding anything.
Okay, what should the config’s interface look like. Not that it must have a name
property since we always allowed an alias.
These examples are explicit part of the OAPI3 spec. Swagger 2 should just ignore and warn that this can't be used.
No, swagger 2 supports example
just not examples
. See here: https://swagger.io/docs/specification/2-0/adding-examples/. I’d recommend that we go live with example?: unknown
and then we can add examples?: unknown[]
if someone asks us for that feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we could, but the problem is that T can’t be inferred for decorators of parameters.
It can, because we are in the fortunate situation to be able to parse the AST. However, since we both agreed we should not do it, there's no reason to discuss further imo.
that’s a really unfortunate problem that the T of Body (I.e Thing1) can be different than the variable’s type (I.e. Thing2).
Yes. I'd say that's better than unknown
, but I'm happy either way.
No, swagger 2 supports example just not examples.
This is incorrect in this context. This PR addresses query, path and body example(s). None of this is supporte d in Swagger 2 and therefore which the article you linked confirms by not mentioning any of these.
A[ Swagger 2] API specification can include examples for:
- response MIME types,
- schemas (data models),
- individual properties in schemas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarifying @WoH. What version of the spec are you using @CristianoVotre?
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days |
Hi, I develop a form to put examples in the request, its was an old issue:
'Is there a way to provide example of request body? #180'
Now, we can put:
processarDesconTurbo( @Path('idUsuario', 'ABCDEF125') idUsuario: string, @Path('idServico', '1') idServico: number, @Query('valorCompra', '19.90') valorCompra: number)
or in body tag:
inserirPagamento( @Body(
{"usuario": {
"codigo": "ABCDEF125"
},
"servico": {
"codigo": 1
},
"desconTurbo": false,
"parcela": 1,
"valor": 19.90,
"valorOnes": 19.90
}
) pagamento: Pagamento)
And in the generated swagger will show these values on the fields.