NSwag
NSwag copied to clipboard
RicoSuter
Regression from 13.6.2 to 13.7.0
A SwaggerException is generated in the C# client when upgrading from 13.6.2 to 13.7.0 Beware that the entity returned contains types like enums, DateTime?, byte...
How can I determine what field is failing?
Sample method OK 13.6.2
public async System.Threading.Tasks.Task<Asset> GetAssetAsync(int assetId, int? version, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken))
{
if (assetId == null)
throw new System.ArgumentNullException("assetId");
var urlBuilder_ = new System.Text.StringBuilder();
urlBuilder_.Append(BaseUrl != null ? BaseUrl.TrimEnd('/') : "").Append("/api/Asset/GetAsset?");
urlBuilder_.Append(System.Uri.EscapeDataString("assetId") + "=").Append(System.Uri.EscapeDataString(ConvertToString(assetId, System.Globalization.CultureInfo.InvariantCulture))).Append("&");
urlBuilder_.Append(System.Uri.EscapeDataString("version") + "=").Append(System.Uri.EscapeDataString(version != null ? ConvertToString(version, System.Globalization.CultureInfo.InvariantCulture) : "")).Append("&");
urlBuilder_.Length--;
var client_ = await CreateHttpClientAsync(cancellationToken).ConfigureAwait(false);
try
{
using (var request_ = await CreateHttpRequestMessageAsync(cancellationToken).ConfigureAwait(false))
{
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));
PrepareRequest(client_, request_, urlBuilder_);
var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);
PrepareRequest(client_, request_, url_);
var response_ = await client_.SendAsync(request_, System.Net.Http.HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
try
{
var headers_ = System.Linq.Enumerable.ToDictionary(response_.Headers, h_ => h_.Key, h_ => h_.Value);
if (response_.Content != null && response_.Content.Headers != null)
{
foreach (var item_ in response_.Content.Headers)
headers_[item_.Key] = item_.Value;
}
ProcessResponse(client_, response_);
var status_ = ((int)response_.StatusCode).ToString();
if (status_ == "200")
{
var objectResponse_ = await ReadObjectResponseAsync<Asset>(response_, headers_).ConfigureAwait(false);
return objectResponse_.Object;
}
else
if (status_ != "200" && status_ != "204")
{
var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
throw new SwaggerException("The HTTP status code of the response was not expected (" + (int)response_.StatusCode + ").", (int)response_.StatusCode, responseData_, headers_, null);
}
return default(Asset);
}
finally
{
if (response_ != null)
response_.Dispose();
}
}
}
finally
{
if (client_ != null)
client_.Dispose();
}
}
...
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.1.11.0 (Newtonsoft.Json v12.0.0.0)")]
public partial class Asset : System.ComponentModel.INotifyPropertyChanged
{
private int _id;
private string _code;
private decimal _sortSequence;
private byte _pointsPlaces;
private bool _quoteBasis;
private int? _nDFFixDays;
private ForwardCodeTypeEnum _forwardCodeType;
private AssetStatus _statusCodeId;
private System.DateTime _createdDate;
private System.DateTime? _authorisedDate;
private Risk _risk;
...
private TechOverrideMask _techOverrideMask;
private System.Collections.ObjectModel.ObservableCollection<AssetRegion> _assetRegions;
private System.Collections.ObjectModel.ObservableCollection<string> _currentChanges;
...
[Newtonsoft.Json.JsonProperty("Id", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]
public int Id
{
get { return _id; }
set
{
if (_id != value)
{
_id = value;
RaisePropertyChanged();
}
}
}
...
Sample method failing 13.7.0
public async System.Threading.Tasks.Task<Asset> GetAssetAsync(int assetId, int? version, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken))
{
if (assetId == null)
throw new System.ArgumentNullException("assetId");
var urlBuilder_ = new System.Text.StringBuilder();
urlBuilder_.Append(BaseUrl != null ? BaseUrl.TrimEnd('/') : "").Append("/api/Asset/GetAsset?");
urlBuilder_.Append(System.Uri.EscapeDataString("assetId") + "=").Append(System.Uri.EscapeDataString(ConvertToString(assetId, System.Globalization.CultureInfo.InvariantCulture))).Append("&");
urlBuilder_.Append(System.Uri.EscapeDataString("version") + "=").Append(System.Uri.EscapeDataString(version != null ? ConvertToString(version, System.Globalization.CultureInfo.InvariantCulture) : "")).Append("&");
urlBuilder_.Length--;
var client_ = await CreateHttpClientAsync(cancellationToken).ConfigureAwait(false);
try
{
using (var request_ = await CreateHttpRequestMessageAsync(cancellationToken).ConfigureAwait(false))
{
request_.Method = new System.Net.Http.HttpMethod("GET");
request_.Headers.Accept.Add(System.Net.Http.Headers.MediaTypeWithQualityHeaderValue.Parse("application/json"));
PrepareRequest(client_, request_, urlBuilder_);
var url_ = urlBuilder_.ToString();
request_.RequestUri = new System.Uri(url_, System.UriKind.RelativeOrAbsolute);
PrepareRequest(client_, request_, url_);
var response_ = await client_.SendAsync(request_, System.Net.Http.HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
try
{
var headers_ = System.Linq.Enumerable.ToDictionary(response_.Headers, h_ => h_.Key, h_ => h_.Value);
if (response_.Content != null && response_.Content.Headers != null)
{
foreach (var item_ in response_.Content.Headers)
headers_[item_.Key] = item_.Value;
}
ProcessResponse(client_, response_);
var status_ = (int)response_.StatusCode;
if (status_ == 200)
{
var objectResponse_ = await ReadObjectResponseAsync<Asset>(response_, headers_).ConfigureAwait(false);
return objectResponse_.Object;
}
else
{
var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
throw new SwaggerException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
}
}
finally
{
if (response_ != null)
response_.Dispose();
}
}
}
finally
{
if (client_ != null)
client_.Dispose();
}
}
...
[System.CodeDom.Compiler.GeneratedCode("NJsonSchema", "10.1.24.0 (Newtonsoft.Json v12.0.0.0)")]
public partial class Asset : System.ComponentModel.INotifyPropertyChanged
{
private int _id;
...
What is the value of the Message property of the SwaggerException ?
I added checks for non-nullable values that were null in the message, maybe it worked before (using the default value), but now it might be forbidden.
Please check that your openApi specification document matches your data.
Thanks for the quick reply, the exception is occurring in
throw new SwaggerException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
The status_ is 204 , the response_.ReasonPhrase is "No Content".
And yes, the client and netcoreapp3.1 server and the WPF4.7 client are in-sync because it is generated via the build:
<PackageReference Include="NSwag.MSBuild" Version="13.7.0">
I think you might be hitting the breaking change I saw: https://github.com/RicoSuter/NSwag/pull/2959
I tried to be careful not to break things, and as far as I know, there is only one thing that might break: If there is an explicit success status code declared in the openApi definition, the else if (status_ == 200 || status_ == 204) that returns a default(T)! (or a wrapped instance) is not written anymore, and it will fall in the default else case. In that case where a 200 is declared, but the server returns a 204 (undeclared), the code will no longer return null but will throw an "unexpected HTTP status code". I'd argue that this is cleaner and, in fact, expected : the fix is to declare the 204 on the server side.
The service is returning data when called from the Swagger web page, I'm afraid it fails when some of the details gets deserialised.
Ok
<PackageReference Include="NSwag.AspNetCore" Version="13.3.0" />
<PackageReference Include="NSwag.MSBuild" Version="13.3.0">
No content
<PackageReference Include="NSwag.AspNetCore" Version="13.7.0" />
<PackageReference Include="NSwag.MSBuild" Version="13.3.0">
Your swagger returns data, so I assume it's a 200 ? Why are you getting a 204 when invoking from your code? Are you sure it's the same request ?
Sorry I didn't phase it properly, the client is WPF4.7 (where the exception is happening), and the server is netcoreapp3.1. I'll have to check again the parameters, I remember that there are two concurrent calls, but definitely it fails after updating, and maybe the request is never issued to the server.
@jeremyVignelles I can confirm, from the swagger web page it works ok:
but from WPF it is failing, the request is sent, and the server responds properly.
But the client processes it and mistakenly infers 204 out of the response:
Your swagger UI screenshot shows that your server is returning a 204, but also tells you that it was undocumented. The client code is working as expected, and you should document that your server may return 204s
in the Responses there is a 200, that was the value that was evaluated before (the 204 is related to the headers, not to the content). What do you mean by documenting 204, (where or how)?
The 200 you see is the documented expected HTTP Status code.
Have a look at ProducesResponseType
I see, the ProducesResponseType attribute is for the controller, which I have added, but I'm using an msbuild task to produce the client
<Exec Command="$(NSwagExe_Core31) run $(ProjectDir)AlpAmsApi.nswag" />
how do I set that in the .nswag file?
essentially, without [ProducesResponseType(StatusCodes.Status204NoContent)] it was generating:
if (status_ == "200")
{
var objectResponse_ = await ReadObjectResponseAsync<Asset>(response_, headers_).ConfigureAwait(false);
return objectResponse_.Object;
}
else
if (status_ != "200" && status_ != "204")
{
var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
throw new SwaggerException("The HTTP status code of the response was not expected (" + (int)response_.StatusCode + ").", (int)response_.StatusCode, responseData_, headers_, null);
}
and now regardless it is defined it produces this:
if (status_ == 200)
{
var objectResponse_ = await ReadObjectResponseAsync<Asset>(response_, headers_).ConfigureAwait(false);
return objectResponse_.Object;
}
else
{
var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
throw new SwaggerException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
}
I think it should still handle 204 because that was expected regardless it was defined or not. In any case if it is required, it should be possible to set it in the .nswag file, but again the NSwagStudio should save that default assumption.
how do I set that in the .nswag file?
The .nswag is just the generator's configuration. It's not its business to know about the 200/204.
essentially, without [ProducesResponseType(StatusCodes.Status204NoContent)] it was generating:
The old code indeed always handled 200 and 204, more on that below.
and now regardless it is defined it produces this:
That should not happen. If you declare a 204, a if branch should be created for that 204. Could you check that?
I think it should still handle 204 because that was expected regardless it was defined or not.
Why do you expect 204 to be always generated? It's your own use case that decides if the API can or cannot generate a 204. If you decide that your API doesn't return a nullable reference type (C#9), what should the client return in case of a 204 ? You can't return null because that would contradict the non-nullability of the result.
I really don't know what to do, but I'm in favor of being explicit as to when a specific status code is expected.
Ok, I'm starting to see what is happening, I'm defining the ProducesResponseType at a base class level which apparently doesn't work.
public class AssetController : AuthoriseController
{
[HttpGet]
public Task<Asset> GetAsset(int assetId, int? version) => _assetService.GetAsset(assetId, version);
}
[Route("api/[controller]/[action]")]
[Produces("application/json")]
[ProducesResponseType(StatusCodes.Status204NoContent)]
[Authorize]
public abstract class AuthoriseController : Controller
{
...
204 is not generated unless the attribute is defined at a method level
Even with that, I can't start decorating every single method of every controller in my apps.
The assumption on your previous version was correct:
if (status_ == "200")
{
return;
}
else
if (status_ != "200" && status_ != "204")
{
var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
throw new SwaggerException( ...
}
If a service returns an object that can be null, then the client should be able to get null. (regardless of the c# version).
I think you need to differentiate between a service returning null (on purpose) and the http response being truncated due to a network issue.
Even with that, I can't start decorating every single method of every controller in my apps.
Maybe this will help ? https://docs.microsoft.com/en-us/aspnet/core/web-api/advanced/conventions?view=aspnetcore-3.1#apply-web-api-conventions Otherwise, you would have to create a NSwag document transformation method.
What does it generate when you have this attribute on the method ? a return on the 204 case, or a throw?
The assumption on your previous version was correct:
It's just a different assumption : If you say "My method returns an orange", you can interpret it either as "It may or may not return an orange", or as "It will always return exactly one orange".
If a service returns an object that can be null, then the client should be able to get null. (regardless of the c# version).
I agree with that. What you are saying is that if a controller method returns a T?, we should interpret any 204 as a return null ?
It sounds pretty reasonable, but only if the controller method is declared as returning a nullable value.
When returning null from a controller method, will asp.net core generate a 204 ?
I think you need to differentiate between a service returning null (on purpose) and the http response being truncated due to a network issue. That's not something that can really happen, an exception would be thrown before reaching the NSwag generated code, and even if it was magically not detected by the socket, the status code would still be 200 and the deserialization would just fail.
I'd really like to have a minimal repro for this to really understand what's going on. Feel free to fork https://github.com/jeremyVignelles/TestNSwagNetCoreApp for that.
Thanks Jeremy, I've created a PR showcasing the situation https://github.com/jeremyVignelles/TestNSwagNetCoreApp/pull/1
The link to the repro is https://www.github.com/paulovila/TestNSwagNetCoreApp/tree/master/ . Thanks for the code, maybe I'll find time to have a look at this.
Related to #1259 in that both should be fixed at the same time IMO.
This is a breaking change and the version should have been changed to 14.0.0 instead of 13.7.0
@RicoSuter, @jeremyVignelles I'd suggest to unlist that package version from nuget, in order to avoid the error spreading across to other users.
We talked with @RicoSuter on gitter this evening about what to do with this issue and #1259 .
The code that was there before #2959 had a special case that would return null in case a 204 was returned, be it declared in the spec or not. It was good for users in that they didn't have to declare a 204, but it had several drawbacks:
- It generated some code magically, based on result code that were not based on the spec.
- It could generate null results, even if the return type of the method is declared as not nullable (which is just awful when you start working with NullableReferenceTypes)
- It hid a bug/unexpected behavior of ASP.net core.
I came and tried to implement #2959 for NullableReferenceTypes. I thought it wouldn't break anything, but it turns out I was wrong. There are in fact several issues:
Nullability checks
I tightened the nullability checks, based on the spec. If the spec didn't declare a return type as nullable, the client code wouldn't accept it (instead of returning null in a NRT context, which would be even worse because there would be a NullReferenceException when a user was expecting to be safe because they enabled the NRT)
The fix is to declare nullability properly in the spec, see #3011 and #3014
Unexpected 204
I removed that 204 special case. If a 204 is expected to be returned from the API, it must be declared in the spec, and everybody should be fine.
It turns out that's not the case.
Take this code for example (courtesy of @paulovila )
[HttpGet]
public Task<HelloWorldModel?> NullableModel() => Task.FromResult(default(HelloWorldModel));
ASP.net's API explorer does not generate a 204 response for this code, despite the result being nullable. This is either a bug that should be reported, or at least a behavior we didn't expect, because the result of this, is indeed a 204 with a null body.
The fix to that would be to declare a 204 result on every API like so:
[HttpGet]
[ProducesResponseType(StatusCodes.Status204NoContent)]
[ProducesResponseType(typeof(HelloWorldModel), StatusCodes.Status200OK)]
public Task<HelloWorldModel?> NullableModel() => Task.FromResult(default(HelloWorldModel));
This would be a tedious task, and we think that it could be done in a better way. Since the issue happens in ASP.Net core, we'd like to provide the user with a ready-to-use IDocumentProcessor that would automatically add 204 status codes, when the controller method return type is nullable.
Comments and PRs welcome !
When 2 responses codes are declared, only one can be the result (in C#)
If we declare both 200 and 204, we still have the issue that 200 is the result, but 204 throws an exception.
We are aware this is a regression, and would like to fix this by fixing #1259 . See the plan here : https://github.com/RicoSuter/NSwag/issues/1259#issuecomment-675734941
However, be aware that this will likely be a breaking change for others, and that we need to be careful about what we're doing.
Hope you enjoyed the detailed answer, feel free to comment if you have more questions.
Just checked, in the 13.6.2 version, if the spec was correctly specifying 204 this already throw an exception:
So the old behavior is just "as expected" when the spec is wrong (204 missing) and the server returned 204...
So in the end the client generator is now correct and before it wasnt - there is only one way to fix this:
- Merge 2xx responses +
- Add an option to automatically add 204 responses when result is nullable (default: off) OR correctly describe this with the asp attributes on all operations.
Would it be possible to define at a base class level an attribute with the desired behaviour for all the methods in the inherited classes ?
This needs to be tested if asp allows that - otherwise we can build a custom processor which does that (second point in the list).
It's also possible to work around this by stopping ASP.NET core from auto-generating 204 responses for null results by removing the HttpNoContentOutputFormatter as in the snippet below.
services.AddMvcCore(options =>
{
options.OutputFormatters.RemoveType<HttpNoContentOutputFormatter>();
})
https://github.com/RicoSuter/NSwag/issues/3038#issuecomment-688517075 - here it says
Do you have any good reason to return both a 200 and a 204 ? Yes : Please tell us why?
We have an event processing system. When a client sends an event, it may be routed, in which case we send back a 202 and you get a routing code, or it may be 204 because it was not routed, and there is no routing code to send. Both are 'OK' outcomes. We ended up having to change our code to always return 202 and return a model, which IMHO is less RESTful.
I think if someone has bothered to explicitly document that their API returns a 202 and a 204, then NSwag should just pick that up without passing judgement or requiring them to reimplement their API.
Thanks @mattwhitfield for your detailed answer. I agree that NSwag should not have to make any decision for those kind of cases, but C# is C#, and you can't return two totally unrelated types from your methods.
The default behavior there will be that NSwag will pick one result as the return type, but the other one will throw, which is not consistent with the fact that this is also a success.
That said, for these kind of use cases, I'd advise that you use the WrapDTO option which should be able to do just that, though I didn't test it myself.
C# is C#, and you can't return two totally unrelated types from your methods
Hey @jeremyVignelles whilst I agree with your statement in general, I believe that's not really what's being asked here.
What I think we're all looking for, myself included (and @mattwhitfield, correct me if I'm wrong) is for the NSwag client generator to understand null (for nullable types of course) when the response HTTP status code is 204.
As @mattwhitfield described in his scenario, his API either returns 202 with an entity, or 204 without an entity.
Right now, the API client throws an exception on 204 rather than returning null to the caller.
This should be possible. If not the default option, it should at least be configurable.
@augustoproiete is spot on - as a consumer of an API I expect there to be a result from a method, which may be null. An example would be StreamReader.ReadLine() - the result being null means there was no line to read. If something really goes wrong (like there's some sort of issue accessing the stream) then I expect an exception to be thrown.
To my mind, that maps pretty directly to the scenario in question - 2xx may or may not have an entity - but if there is an entity it is a single type. 4xx/5xx signify something going really wrong - and so I'd expect an exception.
I do fully agree that supporting 200 with type X and 202 with type Y would be a bad thing overall.