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

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

Closed
softworkz opened this issue Apr 2, 2019 · 31 comments
Closed

Comments

@softworkz
Copy link
Contributor

This can be seen for example in NSwag.Sample.NETCore22 (unmodified):

Operation /pet/{petId}:

image

Definition:

    "/pet/{petId}": {
      "get": {
        "tags": [
          "Pet"
        ],
        "operationId": "Pet_FindById",
        "parameters": [
          {
            "name": "petId",
            "in": "path",
            "required": true,
            "schema": {
              "type": "integer",
              "format": "int32"
            },
            "x-position": 1
          }
        ],
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "nullable": true,
                  "oneOf": [
                    {
                      "$ref": "#/components/schemas/Pet"
                    }
                  ]
                }
              }
            }
          },

The schemas for the other responses are visible because they are array types. But the problem exists for all single object return types.

The only workaround I found is

config.DefaultReferenceTypeNullHandling = ReferenceTypeNullHandling.NotNull;
But that doesn't seem to be a good solution...

@RicoSuter
Copy link
Owner

Just nice, how are we supposed to express a nullable Pet otherwise?

@RicoSuter
Copy link
Owner

See: https://stackoverflow.com/a/48114924/876814 (they use allOf - we could use that too)

But maybe we need to raise an issue in Swagger UI to fix that?

@RicoSuter
Copy link
Owner

RicoSuter commented Apr 2, 2019

Ref: OAI/OpenAPI-Specification#1368

@handrews is this:

"application/json": {
  "schema": {
    "nullable": true,
    "oneOf": [
      {
        "$ref": "#/components/schemas/Pet"
      }
    ]
  }
}

the correct syntax for referencing a nullable Pet - or must we use "allOf"?

@softworkz
Copy link
Contributor Author

The other question besides this is whether it makes sense that a response will be indicated as nullable when it's a reference type (C# class) and non-nullable when it's a value type.

That inference doesn't make a lot of sense to me because whether the API may return an empty response or not is not a matter of value-type vs. reference-type.

@handrews
Copy link

handrews commented Apr 2, 2019

@RicoSuter when there is only one entry, oneOf, anyOf, and allOf are effectively identical. I use allOf because it's semantics are arguably the simplest and probably have the broadest tooling support.

@softworkz I prefer to leave out irrelevant fields rather than sending literal nulls. It works better with JSON Merge Patch if you decide to use that. And if you are consistent about never using JSON null, strongly typed languages can fill in appropriate null values as needed, and strip them out on serialization. But that's just my stylistic preference.

@softworkz
Copy link
Contributor Author

@handrews - I wasn't referring to nullable fields inside a schema. In this case, it's just about the root response object itself.

@handrews
Copy link

handrews commented Apr 2, 2019

@softworkz Oh, I see. When you say that the root response could be nullable do you man that the response would be an application/json document consisting just of a JSON null literal? I would probably just use a 204 No Content as a possible response instead.

@RicoSuter
Copy link
Owner

I would probably just use a 204 No Content as a possible response instead.

That is correct - but ASP.NET Core will send a null with status 200 in this case - 204 with no content has to be implemented manually...

So in theory this nullable + oneOf should be displayed correctly in Swagger UI - @softworkz can you open an issue in the Swagger UI repo (and reference this issue)?

@RicoSuter
Copy link
Owner

@handrews thanks for your fast answer!

@softworkz
Copy link
Contributor Author

Would this be fixed by using allOf? I haven't tried yet...

@handrews
Copy link

handrews commented Apr 2, 2019

That is correct - but ASP.NET Core will send a null with status 200 in this case - 204 with no content has to be implemented manually...

LOL. I hate strongly typed languages with inflexible frameworks 😝

@softworkz
Copy link
Contributor Author

I love strong typing..

Thanks a lot for your advise, though!

@RicoSuter
Copy link
Owner

I love strong typing..

Yeah me too :-)

BTW: With C# 8's Nullable Reference Types the default will be non nullable for this return - and only if you add ? to the type it will produce this null + oneOf, for more information, see https://blog.rsuter.com/the-output-of-nullable-reference-types-and-how-to-reflect-it/

@softworkz
Copy link
Contributor Author

I've read about it. But this behavior can't be introduced to existing projects. At least I can't imagine how this could work..

@RicoSuter
Copy link
Owner

RicoSuter commented Apr 2, 2019

As soon as you enable the feature the whole JSON Schema/OpenAPI generator would behave as if

config.DefaultReferenceTypeNullHandling = ReferenceTypeNullHandling.NotNull; 

were set... and to make something nullable you'd need to add MyType? like you can do already with primitive types (e.g. int?). But enabling this in an existing project might produce breaking changes in the spec and yield lots of compile warnings.

@softworkz
Copy link
Contributor Author

But even in the current state, I wonder whether it's a good idea to declare responses as nullable by default.
At least to me that would rather be a minority of cases where this is really desired. I don't think that this should be the default.
Or at least there should be an option that is less radical than the workaround I posted above. That option should only affect the root response object's nullability, not nullability in general..

@softworkz
Copy link
Contributor Author

What I mean is that it's not typically the intent of somebody who is expicitly declaring a response type that it would be nullable.

@RicoSuter
Copy link
Owner

Or at least there should be an option that is less radical than the workaround I posted above. That option should only affect the root response object's nullability, not nullability in general..

Maybe it would make sense to add a setting like that which only applies to response types.

But the way it is now is correct - you really can return null and it would send null JSON... - but I agree - it's an anti-pattern.

@softworkz
Copy link
Contributor Author

...right...and really in most cases not reflecting the actual behavior of the API.

@RicoSuter
Copy link
Owner

We would need to add a new setting and use it here to initialize isNullable:

https://github.com/RicoSuter/NSwag/blob/master/src/NSwag.SwaggerGeneration/Processors/OperationResponseProcessorBase.cs#L105

@RicoSuter
Copy link
Owner

But I think we still need to create an issue in Swagger UI - as this will be probably a problem for all these nullable + oneOfs (also between DTO schemas, etc.) - can you do that?

@softworkz
Copy link
Contributor Author

I've worked through their stuff half a year ago to fix some authentication issue but I'm a bit short of time right now. I've already spent a too much on doc generation..

But I'll send you another PR with something different that might be of interest.

@acmajia
Copy link

acmajia commented Apr 28, 2019

I just ran into this too. Is there any progress?

@RicoSuter
Copy link
Owner

I'll add a new setting DefaultResponseReferenceTypeNullHandling which is Null by default (avoid breaking changes) and can be changed to NotNull, ok?

RicoSuter added a commit to RicoSuter/NJsonSchema that referenced this issue May 2, 2019
@softworkz
Copy link
Contributor Author

Sounds great to me!

@RicoSuter
Copy link
Owner

v12.2.5

@RicoSuter
Copy link
Owner

@acmajia and @softworkz
Can you have a look at this PR? Is this ok for you?

#2215

@acmajia
Copy link

acmajia commented Jun 11, 2019

@RicoSuter
Sorry for the late reply. I've tested this PR in my project, and it works just fine for me.
Thank you!

@ddombrowsky
Copy link

ddombrowsky commented Aug 27, 2020

Between this one and OAI/OpenAPI-Specification#1368 , really having trouble finding the final decision on this.

Is the "correct" syntax A:

          "timeZone": {
            "oneOf": [
              { "type": "null" },
              { "$ref": "#/components/schemas/TimeZoneModel" }
            ]
          },

or B:

          "timeZone": {
            "nullable": true,
            "oneOf": [
              { "$ref": "#/components/schemas/TimeZoneModel" }
            ]
          },

?

@ddombrowsky
Copy link

Can anyone shed light on my question ^ ? @RicoSuter ? @acmajia ?

@RicoSuter
Copy link
Owner

I’d say b) but i think recently (oai 3.1) they also introduced the type “null” (Same as Json schema) and both are valid.. nswag/njsonschema should support both

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

5 participants