Fix bug with the default param not showing in the doc #1090
Conversation
|
|
||
| var schema = schemaRegistry.GetOrRegister(paramDesc.ParameterDescriptor.ParameterType); | ||
| if (parameter.@in == "body") | ||
| parameter.schema = schema; | ||
| else | ||
| parameter.PopulateFrom(schema); | ||
|
|
||
| parameter.@default = paramDesc.ParameterDescriptor.DefaultValue; |
There was a problem hiding this comment.
I just hit this bug myself. This is almost correct, but unfortunately reverses the existing problem. If I understand the design correctly, the default value that comes from the schema is more for scenarios like assigning 0 for number types. The HttpParameterDescriptor.DefaultValue could report null, which would cause a separate set of issues.
I think you have the sequence of assignment correct here; however, parameter.@default should only be updated when paramDesc.ParameterDescriptor.DefaultValue is not null. That will prevent it from mistakenly overriding values assigned by the schema.
There was a problem hiding this comment.
Got it! fixing it now...
From @commonsensesoftware I think you have the sequence of assignment correct here; however, parameter.@default should only be updated when paramDesc.ParameterDescriptor.DefaultValue is not null. That will prevent it from mistakenly overriding values assigned by the schema.
|
@commonsensesoftware |
|
Perfect. Thanks! |
|
The PRs seem to be merging quite slowly. Since it's basically in the same place in the code, is there any chance we can sneak at least the first half of #1101 into this PR? This is the only thing required at or after line 190: parameter.description = paramDesc.Documentation;I don't mind opening a new PR, but they don't seem to be burning down. These two changes are ideal for the API Explorer about to be released for API Versioning. It will also enable API version parameters to be automatically added, documented, and contain a default value. :) If you can't sneak it in, don't worry about it. I can eventually create a PR once yours has been merged in. Thanks. |
|
You got it! I'm adding your request... |
|
You rock! Thanks! |
|
@commonsensesoftware |
|
Hey @heldersepu, did this get merged? It doesn't look like it. What happened? |
|
@commonsensesoftware, @domaindrivendev |
|
@heldersepu thanks. that's unfortunate, but I understand. @domaindrivendev if you're able to eventually merge this PR, it would give congruence with the ASP.NET Core version. This PR contains effectively the same changes that I've submitted as 413 on the ASP.NET Core side. If not, I'll likely submit a new PR that duplicates these changes. I'm in no rush to merge things and the changes are honestly no sweat off my back. I'm only trying improve the out-of-box experience for service authors creating versioned REST APIs that also use Swagger with Swashbuckle. I've added all the infrastructure and metadata possible to enable that, but I can't force Swashbuckle to use it. |
Fix Issue #1089