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

Regression from 13.6.2 to 13.7.0 #2995

Open
paulovila opened this issue Aug 11, 2020 · 44 comments
Open

Regression from 13.6.2 to 13.7.0 #2995

paulovila opened this issue Aug 11, 2020 · 44 comments

Comments

@paulovila
Copy link

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;
  ...  
@jeremyVignelles
Copy link
Collaborator

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.

@paulovila
Copy link
Author

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">

@jeremyVignelles
Copy link
Collaborator

jeremyVignelles commented Aug 11, 2020

I think you might be hitting the breaking change I saw:
#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.

@paulovila
Copy link
Author

paulovila commented Aug 11, 2020

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">

@jeremyVignelles
Copy link
Collaborator

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 ?

@RicoSuter
Copy link
Owner

Is this with asp.net core?

@paulovila
Copy link
Author

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.

@paulovila
Copy link
Author

@jeremyVignelles I can confirm, from the swagger web page it works ok:
image
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:
image

@jeremyVignelles
Copy link
Collaborator

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

@paulovila
Copy link
Author

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)?

@jeremyVignelles
Copy link
Collaborator

The 200 you see is the documented expected HTTP Status code.
Have a look at ProducesResponseType

@paulovila
Copy link
Author

paulovila commented Aug 12, 2020

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.

@jeremyVignelles
Copy link
Collaborator

jeremyVignelles commented Aug 12, 2020

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.

@paulovila
Copy link
Author

paulovila commented Aug 12, 2020

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.

@jeremyVignelles
Copy link
Collaborator

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.

@paulovila
Copy link
Author

Thanks Jeremy, I've created a PR showcasing the situation jeremyVignelles/TestNSwagNetCoreApp#1

@jeremyVignelles
Copy link
Collaborator

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.

@jeremyVignelles
Copy link
Collaborator

jeremyVignelles commented Aug 14, 2020

Related to #1259 in that both should be fixed at the same time IMO.

@dgmagno
Copy link

dgmagno commented Aug 17, 2020

This is a breaking change and the version should have been changed to 14.0.0 instead of 13.7.0

@paulovila
Copy link
Author

@RicoSuter, @jeremyVignelles I'd suggest to unlist that package version from nuget, in order to avoid the error spreading across to other users.

@jeremyVignelles
Copy link
Collaborator

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 : #1259 (comment)

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.

@RicoSuter
Copy link
Owner

RicoSuter commented Aug 21, 2020

Just checked, in the 13.6.2 version, if the spec was correctly specifying 204 this already throw an exception:

image

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.

@paulovila
Copy link
Author

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 ?

@RicoSuter
Copy link
Owner

This needs to be tested if asp allows that - otherwise we can build a custom processor which does that (second point in the list).

@RicoSuter RicoSuter unpinned this issue Sep 28, 2020
@blushingpenguin
Copy link

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>();
})

@paulovila
Copy link
Author

Hi @RicoSuter @jeremyVignelles is there any update on this issue?

@mattwhitfield
Copy link
Contributor

#3038 (comment) - 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.

@jeremyVignelles
Copy link
Collaborator

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.

@augustoproiete
Copy link
Sponsor

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.

@mattwhitfield
Copy link
Contributor

@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.

@jeremyVignelles
Copy link
Collaborator

Regarding the "Unexpected/undocumented 204" issue, I reported that on aspnetcore's bugtracker

I looked into NSwag's code this afternoon to find a way to properly merge 2xx, but it requires heavy refactorings I had no time to do. I think that we could merge all success status codes (as long as types are compatible), and also merge that with the "default" response type.
I'm still early in the investigation, but I think that it would help simplify the code path ("default response type" is handled differently right now.) : We would have the error codes (throwing excptions) on one side, and default/success response type on the other side, with the return. That might also mean breaking changes, and thus a version 14.

@lafritay
Copy link

FWIW, we were able to workaround this issue by copying Client.Class.liquid from the templates in the source repo and changing line 286 to be this:

if (status_ == {{ response.StatusCode }}{% if response.CheckChunkedStatusCode %} || status_ == 206{% endif %}{% if response.IsNullable == true %} || status_ == 204{% endif %})

That's not a great long-term workaround because of the fact that you have to keep up with changes to the .liquid file but it's a workaround nonetheless.

@paulovila
Copy link
Author

paulovila commented Nov 26, 2020

Sure @jeremyVignelles take your time, temporarily what someone can do is to change the return type so that the null value is expected inside of an object that is never null,
for example:

Task<MyClass> GetMyClass() => _myInterface.GetMyClassInternal();

by

async Task<(MyClass, string)> GetMyClass()
{
    var r = await _myInterface.GetMyClass();
    return (r, r != null);
}

The generated client, instead of returning a MyClass, returns a ValueTupleOfMyClassAndBool.
Then result.Item1 can be considered as null if result.Item2 is false
It's appalling I know, but I don't see any other workaround

@paulovila
Copy link
Author

@RicoSuter @jeremyVignelles is this still on your radar?

@jeremyVignelles
Copy link
Collaborator

Hi,
I've been thinking about it, but I haven't been able to "merge" the return type due to the complexity of the code base (don't know what I'm breaking and where).
I've been thinking about this, about #1259 and about oneOf/anyOf scenarii ( #1903 #1721 #2991). I am creating a library to support that kind of type union, that you could find here : https://github.com/jeremyVignelles/ooak

The goal would be to merge all success status code in either one type, one nullable type (200|201 for example) or using a TypeUnion (would require a dependency) if the scenario is more complex. At the time, I have no idea what the path would be to introduce such change, so if you want to have a look...

@Pzixel
Copy link

Pzixel commented Mar 5, 2021

Somehow the problem didn't get fixed with 13.7.0.

Here is an endpoint:

#nullable enable
class MyData {}

/// <response code="204" nullable="true"></response>
[HttpGet("Foo/MyData")]
[ProducesResponseType(typeof(MyData), StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status204NoContent)]
[ProducesResponseType(typeof(Dictionary<string, string[]>), StatusCodes.Status400BadRequest)]
[ProducesResponseType(typeof(MessageModel), StatusCodes.Status401Unauthorized)]
[ProducesResponseType(typeof(MessageModel), StatusCodes.Status404NotFound)]
public async Task<MyData?> MyEndpoint(Guid orderId) =>
	null;

Here is schema produced:

"Foo/MyData": {
  "get": {
	"tags": [	],
	"summary": "",
	"operationId": "Foo_MyEndpoint",
	"parameters": [],
	"responses": {
	  "200": {
		"description": "",
		"content": {
		  "application/json": {
			"schema": {
			  "$ref": "#/components/schemas/MyData"
			}
		  }
		}
	  },
          "204": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "nullable": true
                }
              }
            }
          },
	  "400": {
		"description": "",
		"content": {
		  "application/json": {
			"schema": {
			  "type": "object",
			  "additionalProperties": {
				"type": "array",
				"nullable": true,
				"items": {
				  "type": "string",
				  "nullable": true
				}
			  }
			}
		  }
		}
	  },
	  "401": {
		"description": "",
		"content": {
		  "application/json": {
			"schema": {
			  "$ref": "#/components/schemas/MessageModel"
			}
		  }
		}
	  },
	  "404": {
		"description": "",
		"content": {
		  "application/json": {
			"schema": {
			  "$ref": "#/components/schemas/MessageModel"
			}
		  }
		}
	  }
	}
  }
},

And this is code generated:

public async System.Threading.Tasks.Task<ApiResponse<MyData>> Foos_GetMyDataAsync(System.Threading.CancellationToken cancellationToken)
{
	var urlBuilder_ = new System.Text.StringBuilder();
	urlBuilder_.Append("Foo/MyData");

	var client_ = _httpClient;
	var disposeClient_ = false;
	try
	{
		using (var request_ = new System.Net.Http.HttpRequestMessage())
		{
			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);
			var disposeResponse_ = true;
			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<MyData>(response_, headers_, cancellationToken).ConfigureAwait(false);
					if (objectResponse_.Object == null)
					{
						throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
					}
					return new ApiResponse<MyData>(status_, headers_, objectResponse_.Object);
				}
				else
				if (status_ == 204)
				{
					string responseText_ = ( response_.Content == null ) ? string.Empty : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
					throw new ApiException("A server side error occurred.", status_, responseText_, headers_, null);
				}
				else
				if (status_ == 400)
				{
					var objectResponse_ = await ReadObjectResponseAsync<System.Collections.Generic.IDictionary<string, System.Collections.Generic.ICollection<string>>>(response_, headers_, cancellationToken).ConfigureAwait(false);
					if (objectResponse_.Object == null)
					{
						throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
					}
					throw new ApiException<System.Collections.Generic.IDictionary<string, System.Collections.Generic.ICollection<string>>>("A server side error occurred.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null);
				}
				else
				if (status_ == 401)
				{
					var objectResponse_ = await ReadObjectResponseAsync<MessageModel>(response_, headers_, cancellationToken).ConfigureAwait(false);
					if (objectResponse_.Object == null)
					{
						throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
					}
					throw new ApiException<MessageModel>("A server side error occurred.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null);
				}
				else
				if (status_ == 404)
				{
					var objectResponse_ = await ReadObjectResponseAsync<MessageModel>(response_, headers_, cancellationToken).ConfigureAwait(false);
					if (objectResponse_.Object == null)
					{
						throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
					}
					throw new ApiException<MessageModel>("A server side error occurred.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null);
				}
				else
				{
					var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false); 
					throw new ApiException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
				}
			}
			finally
			{
				if (disposeResponse_)
					response_.Dispose();
			}
		}
	}
	finally
	{
		if (disposeClient_)
			client_.Dispose();
	}
}

Which clearly states that it throws for 204 when it shouldn't.

Versions used:

NSwag.AspNetCore Version="13.10.7" - to generate openapi.json
NSwag.MSBuild Version="13.10.7" - to generate client using openapi.json

@Pzixel
Copy link

Pzixel commented Mar 5, 2021

Another problem is

throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);

Model says it's a NRT, but attributes may condradict with it but I didn't find any information about how do I specify that response may be null in some cases but can't in anothers. I've only found an option to set the default, but nothing about overriding it on per-handle basis. For example, I have this handler that returns MyData? and another which returns MyData. I'd like to express it via attributes so NSwag would generate an appropriate spec but apparently I couldn't find any way to do so.

Any workaround/insights for this? cc @RicoSuter @jeremyVignelles

@taspeotis
Copy link

taspeotis commented Aug 1, 2022

I have encountered this with the latest version of NSwag and downgraded to 13.6.2 - however the problem persists somewhat differently. I apologise if this is covered before (it seems to be hinted at in this comment - default response types) but I have a swagger file with a default: operation.

According to the spec:

You can use the default response to describe these errors collectively, not individually. “Default” means this response is used for all HTTP codes that are not covered individually for this operation.

If I explicitly add a 204: operation with nothing other than a description: Success NSwag still looks for a response body. If I remove the default: operation 13.6.2 generates code that works (returns default(TResponse)).

So I am stuck somewhat because 13.7.0 and newer don't seem to understand what a HTTP 204 implies, and 13.6.2 seems to use the schema from default: despite the 204: operation explicitly not having a schema.

@r-rc-christopher
Copy link

My solution was to remove the HttpNoContentOutputFormatter as described above and add this comment on my endpoint

/// <response code="200" nullable="true">{something}</response>

Then, the generated client code is :

if (status_ == 200)
{
    var objectResponse_ = await ReadObjectResponseAsync<Payload>(response_, headers_, cancellationToken).ConfigureAwait(false);
    return objectResponse_.Object;
}

.NET 5
NSwag v13.17.0

@htuomola
Copy link

@r-rc-christopher I think that does indeed work around the runtime error calling the service. However, the generated API client response type will be Task<Payload> even if client is generated with nullable reference types enabled (so it looks like it's not null). This leads to confusion on callers as they wouldn't expect null.

Btw. instead of removing HttpNoContentOutputFormatter, it can also be configured to not do this status code changing (nut sure what else it does, tbh):

services.AddMvc(options => 
{
    var noContentFormatter = options.OutputFormatters.OfType<HttpNoContentOutputFormatter>().FirstOrDefault();
    if (noContentFormatter != null)
    {
        noContentFormatter.TreatNullValueAsNoContent = false;
    }
});

@fvenezia
Copy link

Hello, no fix in 13.8 but since we migrated to .NET 7, upgrade was mandatory for us.
Do you think proper handling of Nullable types combined with [ProducesResponseType(StatusCodes.Status204NoContent)] can be in next release?
Otherwise, how can I help?

@janseris
Copy link

janseris commented May 6, 2023

/// <response code="200" nullable="true">{something}</response>

Did not work for me. I still get the null response exception

Response was null which was not expected.
Status: 200

via regenerated client. Am I missing something?
My controller:

/// <summary>
/// Loads icon for project if it exists
/// <br><c>null</c> if it doesn't exist (its existence is optional)</br>
/// </summary>
/// <response code="200" nullable="true"></response>
[HttpGet("{ID}/icon", Name = nameof(GetProjectIcon))]
[ProducesResponseType(StatusCodes.Status200OK)]
public async Task<byte[]> GetProjectIcon(int ID)
{
    return await service.GetIcon(projectID);
}

I am using the client in .NET Framework 4.8 and I don't use nullable types anywhere.

Also [return: CanBeNull] (using NJsonSchema.Annotations;) did not work with combination of /// <response code="200" nullable="true">{something}</response>

@ciceronevesorbis
Copy link

Any updates on this?

@postolka
Copy link

What worked to me was adding both 200 and 204 response types and explicitly allow nullable for 204 one:

    /// ...
    /// <response code="204" nullable="true">No data.</response>
    [HttpGet("my-data")]
    [ProducesResponseType(typeof(DataModel), StatusCodes.Status200OK)]
    [ProducesResponseType(typeof(DataModel), StatusCodes.Status204NoContent)]
    // ... more response types
    public async Task<IActionResult> ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests