Nswag: Merge 2xx responses with the same type and handle them in one block

[RicoSuter]

merge 2xx responses with the same type and handle them in one if (status... || block

[jeremyVignelles]

If 204 (No content) is in the possible answers, just make the result nullable.

The point is : 2xx status codes should not throw in the client, because they semantically indicate success.

What would be the behavior if two different types are declared, e.g. for 200 and 201 ?

• Will code generation for C# crash, since typescript can handle that?
• Will that automatically create a wrapper?

[RicoSuter]

To avoid bigger breaking changes, I'd suggest to only merge the responses which do not require a changed return type otherwise generate the same code... for generating wrappers etc., we have additional settings (e.g. WrapResponses, etc.)

[Blackbaud-MitchellThomas]

My team was caught off guard to see a 2xx response, in our case a 204, throw an exception, especially with a message in the exception of "Success", which is kind of a bizarre thing by itself. I would consider this more of a bug than an enhancement (how it's currently tagged).

We are totally fine with different blocks for each status code even with the same response type. Merging the code blocks is an optimization that I wouldn't hitch to a solution for this, especially with the complexity it would add.

Is anyone working on this?

[RicoSuter]

Is anyone working on this?

Probably not

[Blackbaud-MitchellThomas]

I'll see if I can help out!

[dutchmartin]

I was surprised to see that a 201 throws an exception on a post operation since it is a positive response code.
https://httpstatuses.com/

[jchannon]

Just hit this by inspecting the generated code. Why is the default behaviour to throw if not 200 or 204. Seems a very odd decision. I would personally prefer if there was an option such as ThrowExceptions that hides this default feature. As a client I don't think I should be expecting an exception, it should be my responsibility to check the status code and process that how I want.

[kensykora]

The fact that this is tagged enhancement is really bizarre to me. 202 Accepted is super common in rest patterns for long running requests.

See https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#132-stepwise-long-running-operations

To me this is a bug with the library. I understand it'd cause backward compatibility issues, but a code generation flag, as was suggested earlier seems like a very reasonable solution. The fact that we need to handle this "normally" with a try/catch is very frustrating to me.

[RicoSuter]

As a client I don't think I should be expecting an exception, it should be my responsibility to check the status code and process that how I want.

The idea is to hide HTTP specific stuff (avoid leaky abstractions).

But i agree that 2xx should not throw but either return a result (200) or null (2xx) - if they dont have another model.. ie merge if possible. Would this be ok?

[xaviergxf]

Hi @RicoSuter any ideas implementation of this feature? When will this be available?

[ken2k]

Hi,

Anything new related to this issue?

[Jogai]

Can we expect the pr from @Trentm95 to get merged anywhere in the near future?

[Reintjuu]

@RicoSuter, what is the current state of this issue? Do you need help with merging, or is there some other issue I'm not seeing?

[jeremyVignelles]

I probably broke the PR by refactoring the status code handling. We need to rewrite it with the new status code handling

[jeremyVignelles]

Related to #2995 because both should be fixed at the same time : if a 204 is declared (or any empty response), the generated response type should be changed to T? (adds nullability to the response type), unless it is already void.

[nealsu]

Referencing item #1602, would be great to cater for the scenario mentioned there.

[jeremyVignelles]

@nealsu :please don't cross-post. issues are already linked and they will be fixed all at once

[jeremyVignelles]

I talked on gitter this evening with @RicoSuter and here is what we decided:

We need to merge the success (2xx) responses properly, to be able to fix issues like #1602 . But we need to be careful, because it will likely break things. Here is what was defined for the result merging:

• Only applicable if WrapResponse is not used (no need for a merge in that case)
• Can merge one or more success responses if they have the same response type
• If one of the declared success responses has no type, it can merge with the result, but that makes the result nullable
• If one of the declared success responses has a different type than the others, an exception is thrown in the C# generator (still discussing, see below)
• Consequently, all the success cases are handled under the same if, the only one that has a return
• It is worth a major version bump, to signal it's a breaking change.

If several different types are declared as success, we need to resolve the conflict. It doesn't seem to me that the conflict will occur in the typescript generator, because of the language's flexibility. What follows only applies to the C# client generator.

• Either we throw if such conflict is encountered, crashing the generation. The message would suggest to use WrapResponse to be able to return different types and that would allow to accomodate funny specs.
• Or we decide to generate a valid code anyway. Some success responses would be treated as returns, and others would throw a SwaggerException. Then question arises like "How do we decide which result is a return?" "Is that logical?" "Why does my 201 throws? You didn't fix anything!" etc...

I'm in favor of the first option.

Any opinion welcome.

[eholman]

A major version bump would be i.m.h.o. a necessary action. Breaking changes should not hold you back from improvements (because by-design stuff).

I don't fully understand the first option... I'm not using WrapResponse.

In my situation I don't explicitly specify Success responses but do return a nullable reference type. In cases it's returning null I get a 204 code otherwise it's 200.

Maybe it's an idea to only/at least merge 2xx explicitly declared response codes with the same response type - I think it's option 2?
Maybe for the 4th option it's possible to pass a generic type argument so the client consumer is responsible for a correct implementation - it can lead to confusing situations.

[jeremyVignelles]

How would you deserialize generics with Newtonsoft.Json in a generated code (which is meant to be generic, and not specific to your implementation), without having knowledge about what is going to be deserialized ? This is really tricky.

In the restful way of thinking, being able to return both 200 and 204 sounds like a bad API design : if it's not found (null), you should return 404 instead

[eholman]

I agree it sounds like a bad API design in restful way of thinking with respect to collections/item. But I have a Post Operation that can return a value or not; how about those situations?

If the method does some processing but does not create a new resource, the method can return HTTP status code 200
and include the result of the operation in the response body. Alternatively, if there is no result to return, the method can
return HTTP status code 204 (No Content) with no response body

https://azure.microsoft.com/mediahandler/files/resourcefiles/api-design/Azure_API-Design_Guide_eBook.pdf

[jeremyVignelles]

I understand your quote as:
"If you're doing an API that process things (like with the POST method), you can choose whether it can return something, and thus return 200, or cannot return anything, and return 204"
I don't think they are talking about POST apis that sometimes return something and sometimes not (what would be the use case), and pretty sure they're not talking about GET requests.

Remember: when you're calling a GET method, you're not supposed to do some processing, but returning a document. Either that document exist ("200, here's the document you're asking for") or doesn't ("404, not found")

What's your use case?

[eholman]

Maybe; I read it like a simple if-statement :-)

I have a POST operation that does not create a new resource but determines based on the incoming parameters what it needs to do. If all is OK it will return a string that contains an URL. In the other case there won't be an URL; hence the string? return type.

I have a collection Tickets. These can be paid; using a Payment Service Provider (external party -> URL redirect).

On my Tickets (= collection) Controller I have a method Pay bound to the route [Route("{transactionId}/Pay")] - so, in restful terms I want to initiate an operation on an item of a collection, right? According to specs this should be done via POST.