Skip to content

OpenAPI Vs Swashbuckle #56711

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

Closed
1 task done
Varorbc opened this issue Jul 10, 2024 · 5 comments
Closed
1 task done

OpenAPI Vs Swashbuckle #56711

Varorbc opened this issue Jul 10, 2024 · 5 comments
Labels
feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Status: Resolved

Comments

@Varorbc
Copy link
Contributor

Varorbc commented Jul 10, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

public class WeatherForecast
{
    public DateOnly Date { get; set; }

    public int TemperatureC { get; set; }

    public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);

    [DefaultValue("123")]
    public string? Summary { get; set; }

    public DateOnly? Date1 { get; set; }

    public int? TemperatureC1 { get; set; }

    [Required]
    public DateOnly? Date2 { get; set; }

    [Required]
    public int? TemperatureC2 { get; set; }
}

OpenAPI

image

Swashbuckle

image

@captainsafia

  • TemperatureF has no method set, and should not appear in the model
  • TemperatureC2 is a nullable attribute, but it adds a required attribute and should no longer appear nullable

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jul 10, 2024
@martincostello martincostello added feature-openapi old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jul 10, 2024
@KennethHoff
Copy link

TemperatureC2 is a nullable attribute, but it adds a required attribute and should no longer appear nullable

Requiredness and nullability are two orthogonal concepts. Required means you have to set it. Nullable means it can be set to the value NULL.

// Required, nullable
{ age: null } // Valid 
{ age: 10 } // Valid
{ } // Invalid

// Required, non-nullable
{ age: null } // Invalid 
{ age: 10 } // Valid
{ } // Invalid

// Non-required, nullable
{ age: null } // Valid
{ age: 10 } // Valid
{ } // Valid

// Non-required, non-nullable 
{ age: null } // Invalid 
{ age: 10 } // Valid
{ } // Not sure what happens.

@captainsafia
Copy link
Member

Thanks for chiming in @KennethHoff! Your response around required vs. nullable is correct.

With regard to the other comment:

TemperatureF has no method set, and should not appear in the model

There's a bit of subtlety to reason through with this one. TemperatureF has no set method but it does have a get method. That means that it is totally valid for it to appear in the schema for responses, but not necessarily in the schema for request bodies.

The "correct" way to model this in OpenAPI is using the readOnly property to indicate that the property is not settable but does exist on the schema. OpenAPI will use readOnly as a clue for whether a property should be validated as present when it is in the response (and vice versa for writeOnly). #56353 is tracking modeling this correctly. This isn't currently in yet because we're working through how to handle readOnly correctly with an eye for the long-term direction of OpenAPI (see #56470 (comment) for more info).

I'm gonna mark this issue as "resolved" for now given we're tracking the feedback presented here elsewhere. If you have any other questions, feel free to comment before the bot autoresolves or open another issue.

@captainsafia captainsafia added the ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. label Jul 10, 2024
@Varorbc
Copy link
Contributor Author

Varorbc commented Jul 10, 2024

// Required, nullable
{ age: null } // Valid 

I think this situation should be mutually exclusive, not usable. From the perspective of user expectations, since it is mandatory, it should not be set to be null.So the attribute display in Swashbuck is more in line with expectations,

@martincostello
Copy link
Member

Specified but null is useful for scenarios such as an HTTP PATCH where you want to explicitly clear a value of an object, where as not providing the property at all would mean "don't edit it at all, leave it with whatever value it may already have".

Copy link
Contributor

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-openapi ✔️ Resolution: Answered Resolved because the question asked by the original author has been answered. old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Status: Resolved
Projects
None yet
Development

No branches or pull requests

4 participants