Change default of DefaultResponseReferenceTypeNullHandling to NotNull #2215

RicoSuter · 2019-06-07T07:28:45Z

This PR:

Changes the default of DefaultResponseReferenceTypeNullHandling from Null to NotNull.
Adds support for the "nullable" xml docs attribute

Details:

Currently there is no way to specify nullability of the response with ProducesResponseTypeAttribute. This could be solved with a new "nullable" attribute in the XML docs (which always wins if specified):

/// <summary>
/// Gets an order.
/// </summary>
/// <response code="200" nullable="true">Order created.</response>
...

Most of the time (I assume?) people want a not nullable response and use 404 in the not found case and do not return null
I think Swagger UI 3 still cannot handle nullable responses (spec in the first example)
In the Swagger 2 the difference is only in the x-nullable property and thus the impact is lower.

Sample operation:

[HttpGet("{petId}", Name = "FindPetById")]
[Produces("application/json")]
[ProducesResponseType(typeof(Pet), StatusCodes.Status200OK)]
[ProducesResponseType(typeof(SerializableError), StatusCodes.Status400BadRequest)]
[ProducesResponseType(StatusCodes.Status404NotFound)]
public async Task<ActionResult<Pet>> FindById(int petId)

With DefaultResponseReferenceTypeNullHandling = Null (current default, OpenAPI 3)

    "/pet/{petId}": {
      "get": {
        "operationId": "Pet_FindById",
        "parameters": [
          {
          }
        ],
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "nullable": true,
                  "oneOf": [
                    {
                      "$ref": "#/components/schemas/Pet"
                    }
                  ]
                }
              }
            }
          },

With DefaultResponseReferenceTypeNullHandling = NotNull (new default, OpenAPI 3)

    "/pet/{petId}": {
      "get": {
        "operationId": "Pet_FindById",
        "parameters": [
          {
          }
        ],
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Pet"
                }
              }
            }
          },

Open questions:

Should we change the default?
- Pro: Most devs probably already design their APIs so that they don't return null and thus expect the default to be NotNull
- Con: NotNull does not really reflect what the actual API is allowed to return (i.e. it is a missmatch between the spec and the runtime)

/cc @pranavkm @dougbu What's your take on this?

RicoSuter · 2019-06-07T07:38:56Z

How does that impact the generated C#/TS code?

CSharp does not change because reference types cannot be made non nullable
TypeScript will change from Person | null as response to Person

You can still change this default back to how it is now (v12)

RicoSuter · 2019-06-07T08:59:22Z

I see another problem in the future:

Even with Nullable Reference Types (C# 8) you cannot describe nullability with the ProduceResponseTypeAttribute, because this [ProducesResponseType(typeof(Pet?), StatusCodes.Status200OK)] is not allowed (because it's just a Type object with no context and there is no place to generate the NRT NullableAttribute).

Maybe it would be best to also add an IsNullable attribute to the ProducesResponseTypeAttribute?
/cc @rynowak @dougbu @pranavkm

StasPerekrestov · 2019-06-07T12:53:04Z

BTW, some good news concerning the mentioned UI issue
swagger-api/swagger-ui#3803 (comment)

RicoSuter · 2019-06-07T17:14:25Z

@StasPerekrestov good to know... what’s your opinion on the default?

dougbu · 2019-06-10T02:26:42Z

Most of the time (I assume?) people want a not nullable response and use 404 in the not found case and do not return null

I don't know the whole history here but agree with this sentiment. Provide a well-documented override of this default and all would be well from my perspective.

you cannot describe nullability with the ProduceResponseTypeAttribute

Your point is valid but leave nullable reference types out of it: Knowing the compiler will allow a null assignment is very different from knowing the user will assign null in some cases.

Agree a ReturnsNull property or similar may be needed if we find users return null in a few but not all actions.

RicoSuter · 2019-06-10T09:07:34Z

Your point is valid but leave nullable reference types out of it: Knowing the compiler will allow a null assignment is very different from knowing the user will assign null in some cases.

The problem here is the inconsistency from a user perspective: As a user in an NRT world, I'd expect that typeof(Pet) is automatically treated as not nullable and typeof(Pet?) is nullable (but this probably does not compile, right?), but from a tooling perspective we cannot assume not nullable because typeof(Pet?) is not allowed. As a user I'd like to express nullability in the same way as I do it everywhere else (e.g. parameters) - not with an additional property on the attribute...

StasPerekrestov · 2019-06-10T09:21:33Z

I guess, you are looking for something like Optional<T>/Maybe to avoid using attributes, however usage of these patterns isn't common in c# world...

…

On Mon, Jun 10, 2019, 12:07 PM Rico Suter ***@***.***> wrote: Your point is valid but leave nullable reference types out of it: Knowing the compiler will allow a null assignment is very different from knowing the user will assign null in some cases. The problem here is the inconsistency from a user perspective: As a user in an NRT world, I'd expect that typeof(Pet) is automatically treated as not nullable and typeof(Pet?) is nullable (but this probably does not compile, right?), but from a tooling perspective we cannot assume not nullable because typeof(Pet?) is not allowed. As a user I'd like to express nullability in the same way as I do it everywhere else (e.g. parameters) - not with an additional property on the attribute... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2215?email_source=notifications&email_token=AAGMTTKPGBYC32FLGPAYFQTPZYKVZA5CNFSM4HVSWCIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJK7EI#issuecomment-500346769>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGMTTKXBBQ7XY4PWRBZ6TDPZYKVZANCNFSM4HVSWCIA> .

RicoSuter · 2019-06-10T09:25:03Z

I guess, you are looking for something like Optional/Maybe to avoid
using attributes, however usage of these patterns isn't common in c#
world...

Or Nullable<T> but that's only for value types - however as a user I just expect that Pet vs Pet? just works everywhere and thus I'm a bit worried that people will be quite confused.

StasPerekrestov · 2019-06-10T09:37:28Z

On one hand we have the issue described in the task, On the other hand there is an issue related to reference types handling, so, basically, we have two related issues. In c# world we don't have to many options to go with. We need either to wrap instances, e.g. Maybe monad, but c# doesn't have appropriate sugar syntax to deal with it, or use Attributes to express the disired behavior.

…

On Mon, Jun 10, 2019, 12:25 PM Rico Suter ***@***.***> wrote: I guess, you are looking for something like Optional/Maybe to avoid using attributes, however usage of these patterns isn't common in c# world... Or Nullable<T> but that's only for value types - however as a user I just expect that Pet vs Pet? just works everywhere and thus I'm a bit worried that people will be quite confused. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2215?email_source=notifications&email_token=AAGMTTM3JN2KAP3YQ76GFPTPZYMXLA5CNFSM4HVSWCIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXJMETY#issuecomment-500351567>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGMTTLLPY37HHTSPTFPM6TPZYMXLANCNFSM4HVSWCIA> .

RicoSuter · 2019-06-10T09:42:25Z

In c# world we don't have to many options to go with. We need either to
wrap instances, e.g. Maybe monad, but c# doesn't have appropriate sugar
syntax to deal with it, or use Attributes to express the disired behavior.

My point is that ideally this should have been solved with the C# 8 NRT feature somehow - right now typeof() cannot "store" nullability info, but e.g. the params in Method(string foo, string? bar) can (because params can have attributes and typeof is "produces" just a Type object)...

However this PR is not really about NRT or C# 8 but about changing the default of DefaultResponseReferenceTypeNullHandling to NotNull and adding the "nullable" attribute to the xml docs (this will solve the problems for now). My big question is: What is the best default for DefaultResponseReferenceTypeNullHandling?

StasPerekrestov · 2019-06-10T12:29:05Z

What concerns me, is that XML docs now change the behavior of XML schema generation. This is something, not many developers are aware of and also may be a source of confusion.

From my point of view, I'd rather look for an appropriate .net attribute to change the required behavior than for an attribute of XML doc.

StasPerekrestov · 2019-06-10T12:55:30Z

Regarding the new defaults, I'm OK with that. However, the information that describes the behavior and the way of changing it should be more easy to find.

RicoSuter · 2019-06-10T13:15:20Z

What concerns me, is that XML docs now change the behavior of XML schema generation. This is something, not many developers are aware of and also may be a source of confusion.

Yes, somehow you are right, but the alternative is extremely verbose:

/// <summary>
/// Gets an order.
/// </summary>
/// <response code="200">The order.</response>
[HttpGet("{orderId}", Name = "GetOrder")]
[ProducesResponseType(typeof(Pet), StatusCodes.Status200OK)]
[ProducesResponseTypeNullable(true, StatusCodes.Status200OK)]
public async Task<ActionResult<Order>> GetOrder(int orderId)

I think it's the still the nicest way to do as xml docs attribute as it is the only way to avoid having specifying a single response code three times...

Ideally there's just a IsNullable (or similar) property on the ProducesResponseTypeAttribute but for backward compat. we'd still keep the "nullable" xml docs attribute.

RicoSuter · 2019-06-10T22:03:17Z

Docs: https://github.com/RicoSuter/NSwag/wiki/AspNetCoreOpenApiDocumentGenerator#response-nullability

Change default of DefaultResponseReferenceTypeNullHandling to NotNull

9178d8b

Add support for nullable response xml docs attribute

e08f17f

RicoSuter mentioned this pull request Jun 7, 2019

Response types using Nullable/oneOf are invisible in swagger-ui (OpenApi3) #2071

Closed

RicoSuter added state: ready type: discussion labels Jun 7, 2019

This was referenced Jun 7, 2019

SwaggerUI doesn't render example values for a non-enumerable types #2156

Closed

Epic: NSwag v13 #1700

Closed

RicoSuter mentioned this pull request Jun 7, 2019

The schema reference path has not been resolved. Worked in previos versions #2211

Closed

RicoSuter merged commit 0987a1a into master Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default of DefaultResponseReferenceTypeNullHandling to NotNull #2215

Change default of DefaultResponseReferenceTypeNullHandling to NotNull #2215

RicoSuter commented Jun 7, 2019 •

edited

Loading

RicoSuter commented Jun 7, 2019 •

edited

Loading

RicoSuter commented Jun 7, 2019 •

edited

Loading

StasPerekrestov commented Jun 7, 2019

RicoSuter commented Jun 7, 2019

dougbu commented Jun 10, 2019

RicoSuter commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019 via email

RicoSuter commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019 via email

RicoSuter commented Jun 10, 2019 •

edited

Loading

StasPerekrestov commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019

RicoSuter commented Jun 10, 2019 •

edited

Loading

RicoSuter commented Jun 10, 2019

Change default of DefaultResponseReferenceTypeNullHandling to NotNull #2215

Change default of DefaultResponseReferenceTypeNullHandling to NotNull #2215

Conversation

RicoSuter commented Jun 7, 2019 • edited Loading

RicoSuter commented Jun 7, 2019 • edited Loading

RicoSuter commented Jun 7, 2019 • edited Loading

StasPerekrestov commented Jun 7, 2019

RicoSuter commented Jun 7, 2019

dougbu commented Jun 10, 2019

RicoSuter commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019 via email

RicoSuter commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019 via email

RicoSuter commented Jun 10, 2019 • edited Loading

StasPerekrestov commented Jun 10, 2019

StasPerekrestov commented Jun 10, 2019

RicoSuter commented Jun 10, 2019 • edited Loading

RicoSuter commented Jun 10, 2019

RicoSuter commented Jun 7, 2019 •

edited

Loading

RicoSuter commented Jun 7, 2019 •

edited

Loading

RicoSuter commented Jun 7, 2019 •

edited

Loading

RicoSuter commented Jun 10, 2019 •

edited

Loading

RicoSuter commented Jun 10, 2019 •

edited

Loading