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.

Sign up for GitHub

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

204 response enhancements #5090

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

204 response enhancements #5090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions src/NSwag.CodeGeneration/Models/OperationModelBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,21 +102,28 @@ public string UnwrappedResultType
{
get
{
var response = GetSuccessResponse();
if (response.Value == null || response.Value.IsEmpty(_operation))
TResponseModel response = GetSuccessResponseModel();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is calling a new method. Other call sites still use the original unchanged GetSuccessResponse method.

if (response?.Response == null || response.Response.IsEmpty(_operation))
{
return "void";
}

if (response.Value.IsBinary(_operation))
if (response.Response.IsBinary(_operation))
{
return _generator.GetBinaryResponseTypeName();
}

var isNullable = response.Value.IsNullable(_settings.CodeGeneratorSettings.SchemaType);
var schemaHasTypeNameTitle = response.Value.Schema?.HasTypeNameTitle;
bool isNullable = response.IsNullable;

if (!isNullable)
{
// If one of the success types is nullable, we set the method return type to nullable as well.
isNullable = Responses.Any(r => r.IsSuccess && r.Type == response.Type + "?" && r.IsNullable);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new method call gives us access to the response type to see if we have additional success responses with the same underlying type that are null. Which means we would want to mark the operation as a whole nullable.

}

var schemaHasTypeNameTitle = response.Response.Schema?.HasTypeNameTitle;
var hint = schemaHasTypeNameTitle != true ? "Response" : null;
return _generator.GetTypeName(response.Value.Schema, isNullable, hint);
return _generator.GetTypeName(response.Response.Schema, isNullable, hint);
}
}

Expand Down Expand Up @@ -304,6 +311,24 @@ protected KeyValuePair<string, OpenApiResponse> GetSuccessResponse()
return new KeyValuePair<string, OpenApiResponse>("default", _operation.ActualResponses.FirstOrDefault(r => r.Key == "default").Value);
}

/// <summary>Gets the success response model, including type information.</summary>
/// <returns>The response model.</returns>
protected TResponseModel GetSuccessResponseModel()
{
if (Responses.Any(r => r.StatusCode == "200"))
{
return Responses.Single(r => r.StatusCode == "200");
}

var response = Responses.FirstOrDefault(r => HttpUtilities.IsSuccessStatusCode(r.StatusCode));
if (response != null)
{
return response;
}

return DefaultResponse;
}

/// <summary>Gets the name of the parameter variable.</summary>
/// <param name="parameter">The parameter.</param>
/// <param name="allParameters">All parameters.</param>
Expand Down
30 changes: 17 additions & 13 deletions src/NSwag.CodeGeneration/Models/ResponseModelBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ namespace NSwag.CodeGeneration.Models
public abstract class ResponseModelBase
{
private readonly IOperationModel _operationModel;
private readonly OpenApiResponse _response;
private readonly OpenApiOperation _operation;
private readonly JsonSchema _exceptionSchema;
private readonly IClientGenerator _generator;
Expand All @@ -37,7 +36,7 @@ protected ResponseModelBase(IOperationModel operationModel,
string statusCode, OpenApiResponse response, bool isPrimarySuccessResponse,
JsonSchema exceptionSchema, TypeResolverBase resolver, CodeGeneratorSettingsBase settings, IClientGenerator generator)
{
_response = response;
Response = response;
_operation = operation;
_exceptionSchema = exceptionSchema;
_generator = generator;
Expand All @@ -50,6 +49,9 @@ protected ResponseModelBase(IOperationModel operationModel,
ActualResponseSchema = response.Schema?.ActualSchema;
}

/// <summary>The underlying response</summary>
internal OpenApiResponse Response { get; }

/// <summary>Gets the HTTP status code.</summary>
public string StatusCode { get; }

Expand All @@ -61,14 +63,14 @@ protected ResponseModelBase(IOperationModel operationModel,

/// <summary>Gets the type of the response.</summary>
public string Type =>
_response.IsBinary(_operation) ? _generator.GetBinaryResponseTypeName() :
Response.IsBinary(_operation) ? _generator.GetBinaryResponseTypeName() :
_generator.GetTypeName(ActualResponseSchema, IsNullable, "Response");

/// <summary>Gets a value indicating whether the response has a type (i.e. not void).</summary>
public bool HasType => ActualResponseSchema != null;

/// <summary>Gets or sets the expected child schemas of the base schema (can be used for generating enhanced typings/documentation).</summary>
public ICollection<JsonExpectedSchema> ExpectedSchemas => _response.ExpectedSchemas;
public ICollection<JsonExpectedSchema> ExpectedSchemas => Response.ExpectedSchemas;

/// <summary>Gets a value indicating whether the response is of type date.</summary>
public bool IsDate => ActualResponseSchema != null &&
Expand All @@ -77,21 +79,21 @@ protected ResponseModelBase(IOperationModel operationModel,
_generator.GetTypeName(ActualResponseSchema, IsNullable, "Response") != "string";

/// <summary>Gets a value indicating whether the response requires a text/plain content.</summary>
public bool IsPlainText => !_response.Content.ContainsKey("application/json") && _response.Content.ContainsKey("text/plain");
public bool IsPlainText => !Response.Content.ContainsKey("application/json") && Response.Content.ContainsKey("text/plain");

/// <summary>Gets a value indicating whether this is a file response.</summary>
public bool IsFile => IsSuccess && _response.IsBinary(_operation);
public bool IsFile => IsSuccess && Response.IsBinary(_operation);

/// <summary>Gets the response's exception description.</summary>
public string ExceptionDescription => !string.IsNullOrEmpty(_response.Description) ?
ConversionUtilities.ConvertToStringLiteral(_response.Description) :
public string ExceptionDescription => !string.IsNullOrEmpty(Response.Description) ?
ConversionUtilities.ConvertToStringLiteral(Response.Description) :
"A server side error occurred.";

/// <summary>Gets the response schema.</summary>
public JsonSchema ResolvableResponseSchema => _response.Schema != null ? _resolver.GetResolvableSchema(_response.Schema) : null;
public JsonSchema ResolvableResponseSchema => Response.Schema != null ? _resolver.GetResolvableSchema(Response.Schema) : null;

/// <summary>Gets a value indicating whether the response is nullable.</summary>
public bool IsNullable => _response.IsNullable(_settings.SchemaType);
public bool IsNullable => Response.IsNullable(_settings.SchemaType);

/// <summary>Gets a value indicating whether the response type inherits from exception.</summary>
public bool InheritsExceptionSchema => ActualResponseSchema?.InheritsSchema(_exceptionSchema) == true;
Expand All @@ -110,9 +112,11 @@ public bool IsSuccess
}

var primarySuccessResponse = _operationModel.Responses.FirstOrDefault(r => r.IsPrimarySuccessResponse);

// We should ignore nullability when evaluating if both responses have the same return type.
return HttpUtilities.IsSuccessStatusCode(StatusCode) && (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, this check is factually incorrect.
A response is a success, when the status code is between 200 and 300 (400), no?
A 204 has by design no content, which means Data? is wrong as response type.
It should be void.

Also, if someone would want to send a string on 201, but an object on 200, this check would still mark the response as failure

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that more changes should be made here. I am trying to make this particular PR be minimally impactful so we could potentially do this as a minor version change with no behavior changes for existing consumers of NSwag unless they opt in by using the new functionality. Changing this check in general would be more of a major version change, which may take much longer to get published. I would suggest we tackle that part separately unless you can find a way to keep it backwards compatible without a behavior change for existing users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, we could introduce an option which says to treat only the same response types as success which is true by default. Not sure, if this class can have easy access to the options

primarySuccessResponse == null ||
primarySuccessResponse.Type == Type
primarySuccessResponse.Type.TrimEnd('?') == Type.TrimEnd('?')
);
}
}
Expand All @@ -121,9 +125,9 @@ public bool IsSuccess
public bool ThrowsException => !IsSuccess;

/// <summary>Gets the response extension data.</summary>
public IDictionary<string, object> ExtensionData => _response.ExtensionData;
public IDictionary<string, object> ExtensionData => Response.ExtensionData;

/// <summary>Gets the produced mime type of this response if available.</summary>
public string Produces => _response.Content.Keys.FirstOrDefault();
public string Produces => Response.Content.Keys.FirstOrDefault();
}
}
23 changes: 22 additions & 1 deletion src/NSwag.Generation.AspNetCore/Processors/OperationResponseProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//-----------------------------------------------------------------------

using System.Globalization;
using System.Net;
using System.Reflection;
using Namotion.Reflection;
using NJsonSchema;
Expand Down Expand Up @@ -74,20 +75,40 @@ public bool Process(OperationProcessorContext operationProcessorContext)
httpStatusCode = apiResponse.StatusCode.ToString(CultureInfo.InvariantCulture);
}

var returnTypeAttributes = context.MethodInfo?.ReturnParameter?.GetCustomAttributes(false).OfType<Attribute>();
if (!IsVoidResponse(returnType))
{
var returnTypeAttributes = context.MethodInfo?.ReturnParameter?.GetCustomAttributes(false).OfType<Attribute>();
var contextualReturnType = returnType.ToContextualType(returnTypeAttributes);

var nullableXmlAttribute = GetResponseXmlDocsElement(context.MethodInfo, httpStatusCode)?.Attribute("nullable");
var isResponseNullable = nullableXmlAttribute != null ?
nullableXmlAttribute.Value.Equals("true", StringComparison.OrdinalIgnoreCase) :
_settings.SchemaSettings.ReflectionService.GetDescription(contextualReturnType, _settings.DefaultResponseReferenceTypeNullHandling, _settings.SchemaSettings).IsNullable;

if (int.TryParse(httpStatusCode, out int statusCodeResult) &&
_settings.ResponseStatusCodesToTreatAsNullable.Any(code =>
code == (HttpStatusCode)statusCodeResult))
{
// If the response code of this response type is in the settings list, we treat is a nullable.
isResponseNullable = true;
}

response.IsNullableRaw = isResponseNullable;
response.Schema = context.SchemaGenerator.GenerateWithReferenceAndNullability<JsonSchema>(
contextualReturnType, isResponseNullable, context.SchemaResolver);
}
else
{
if (int.TryParse(httpStatusCode, out int statusCodeResult) &&
_settings.ResponseStatusCodesToTreatAsNullable.Any(code =>
code == (HttpStatusCode)statusCodeResult))
{
// If the response code of this response type is in the settings list, we treat is a nullable.
response.IsNullableRaw = true;
response.Schema = context.SchemaGenerator.Generate(typeof(void));
response.Schema.IsNullableRaw = true;
}
}

context.OperationDescription.Operation.Responses[httpStatusCode] = response;
}
Expand Down
8 changes: 8 additions & 0 deletions src/NSwag.Generation/OpenApiDocumentGeneratorSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// <author>Rico Suter, [email protected]</author>
//-----------------------------------------------------------------------

using System.Net;
using Namotion.Reflection;
using Newtonsoft.Json;
using NJsonSchema;
Expand Down Expand Up @@ -50,6 +51,13 @@ public OpenApiDocumentGeneratorSettings()
/// <summary>Gets or sets the default response reference type null handling when no nullability information is available (if NotNullAttribute and CanBeNullAttribute are missing, default: NotNull).</summary>
public ReferenceTypeNullHandling DefaultResponseReferenceTypeNullHandling { get; set; }

/// <summary>
/// Gets or sets a value indicating that the api method/action response should be considered nullable if this response type is documented, even if it is a void response.
/// <para>Allows for things like a 204 No Content to be treated as nullable without decorating with the <see cref="NJsonSchema.Annotations.CanBeNullAttribute"/></para>
/// <para>If the action is decorated with the <see cref="NJsonSchema.Annotations.NotNullAttribute"/></para> this setting will be ignored for that action.
/// </summary>
public HttpStatusCode[] ResponseStatusCodesToTreatAsNullable { get; set; } = [];

/// <summary>Gets or sets a value indicating whether to generate x-originalName properties when parameter name is different in .NET and HTTP (default: true).</summary>
public bool GenerateOriginalParameterNames { get; set; } = true;

Expand Down
Loading