Skip to content

Commit

Permalink
Support checking for required members in minimal APIs (#45084)
Browse files Browse the repository at this point in the history
* Support checking for required members in minimal APIs

* Address feedback from peer review
  • Loading branch information
captainsafia committed Jan 3, 2023
1 parent 85e1614 commit 1099d06
Show file tree
Hide file tree
Showing 6 changed files with 253 additions and 3 deletions.
152 changes: 152 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Expand Up @@ -7075,6 +7075,158 @@ public void Create_Populates_EndpointBuilderWithRequestDelegateAndMetadata()
Assert.Same(options.EndpointBuilder.Metadata, result.EndpointMetadata);
}

private class ParameterListRequiredStringFromDifferentSources
{
public HttpContext? HttpContext { get; set; }

[FromRoute]
public required string RequiredRouteParam { get; set; }

[FromQuery]
public required string RequiredQueryParam { get; set; }

[FromHeader]
public required string RequiredHeaderParam { get; set; }
}

[Fact]
public async Task RequestDelegateFactory_AsParameters_SupportsRequiredMember()
{
// Arrange
static void TestAction([AsParameters] ParameterListRequiredStringFromDifferentSources args) { }

var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create(TestAction);
var requestDelegate = factoryResult.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert that the required modifier on members that
// are not nullable treats them as required.
Assert.Equal(400, httpContext.Response.StatusCode);

var logs = TestSink.Writes.ToArray();

Assert.Equal(3, logs.Length);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[0].EventId);
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredRouteParam"" was not provided from route.", logs[0].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[1].EventId);
Assert.Equal(LogLevel.Debug, logs[1].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredQueryParam"" was not provided from query string.", logs[1].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[2].EventId);
Assert.Equal(LogLevel.Debug, logs[2].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredHeaderParam"" was not provided from header.", logs[2].Message);
}

private class ParameterListRequiredNullableStringFromDifferentSources
{
public HttpContext? HttpContext { get; set; }

[FromRoute]
public required StringValues? RequiredRouteParam { get; set; }

[FromQuery]
public required StringValues? RequiredQueryParam { get; set; }

[FromHeader]
public required StringValues? RequiredHeaderParam { get; set; }
}

[Fact]
public async Task RequestDelegateFactory_AsParameters_SupportsNullableRequiredMember()
{
// Arrange
static void TestAction([AsParameters] ParameterListRequiredNullableStringFromDifferentSources args)
{
args.HttpContext!.Items.Add("RequiredRouteParam", args.RequiredRouteParam);
args.HttpContext!.Items.Add("RequiredQueryParam", args.RequiredQueryParam);
args.HttpContext!.Items.Add("RequiredHeaderParam", args.RequiredHeaderParam);
}

var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create(TestAction);
var requestDelegate = factoryResult.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert that when properties are required but nullable
// we evaluate them as optional because required members
// must be initialized but they can be initialized to null
// when an NRT is required.
Assert.Equal(200, httpContext.Response.StatusCode);

Assert.Null(httpContext.Items["RequiredRouteParam"]);
Assert.Null(httpContext.Items["RequiredQueryParam"]);
Assert.Null(httpContext.Items["RequiredHeaderParam"]);
}

#nullable disable
private class ParameterListMixedRequiredStringsFromDifferentSources
{
public HttpContext HttpContext { get; set; }

[FromRoute]
public required string RequiredRouteParam { get; set; }

[FromRoute]
public string OptionalRouteParam { get; set; }

[FromQuery]
public required string RequiredQueryParam { get; set; }

[FromQuery]
public string OptionalQueryParam { get; set; }

[FromHeader]
public required string RequiredHeaderParam { get; set; }

[FromHeader]
public string OptionalHeaderParam { get; set; }
}

[Fact]
public async Task RequestDelegateFactory_AsParameters_SupportsRequiredMember_NullabilityDisabled()
{
// Arange
static void TestAction([AsParameters] ParameterListMixedRequiredStringsFromDifferentSources args) { }

var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create(TestAction);
var requestDelegate = factoryResult.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert that we only execute required parameter
// checks for members that have the required modifier
Assert.Equal(400, httpContext.Response.StatusCode);

var logs = TestSink.Writes.ToArray();
Assert.Equal(3, logs.Length);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[0].EventId);
Assert.Equal(LogLevel.Debug, logs[0].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredRouteParam"" was not provided from route.", logs[0].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[1].EventId);
Assert.Equal(LogLevel.Debug, logs[1].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredQueryParam"" was not provided from query string.", logs[1].Message);

Assert.Equal(new EventId(4, "RequiredParameterNotProvided"), logs[2].EventId);
Assert.Equal(LogLevel.Debug, logs[2].LogLevel);
Assert.Equal(@"Required parameter ""string RequiredHeaderParam"" was not provided from header.", logs[2].Message);
}
#nullable enable

private DefaultHttpContext CreateHttpContext()
{
var responseFeature = new TestHttpResponseFeature();
Expand Down
Expand Up @@ -182,7 +182,9 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string
// Determine the "requiredness" based on nullability, default value or if allowEmpty is set
var nullabilityContext = new NullabilityInfoContext();
var nullability = nullabilityContext.Create(parameter);
var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull || allowEmpty;
var isOptional = parameter is PropertyAsParameterInfo argument
? argument.IsOptional || allowEmpty
: parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull || allowEmpty;
var parameterDescriptor = CreateParameterDescriptor(parameter, pattern);
var routeInfo = CreateParameterRouteInfo(pattern, parameter, isOptional);

Expand Down
Expand Up @@ -527,6 +527,46 @@ static void AssertParameters(ApiDescription apiDescription, string capturedName
}
#nullable enable

public class AsParametersWithRequiredMembers
{
public required string RequiredStringMember { get; set; }
public required string? RequiredNullableStringMember { get; set; }
public string NonNullableStringMember { get; set; } = string.Empty;
public string? NullableStringMember { get; set; }
}

[Fact]
public void SupportsRequiredMembersInAsParametersAttribute()
{
var apiDescription = GetApiDescription(([AsParameters] AsParametersWithRequiredMembers foo) => { });
Assert.Equal(4, apiDescription.ParameterDescriptions.Count);

Assert.Collection(apiDescription.ParameterDescriptions,
param => Assert.True(param.IsRequired),
param => Assert.False(param.IsRequired),
param => Assert.True(param.IsRequired),
param => Assert.False(param.IsRequired));
}

#nullable disable
public class AsParametersWithRequiredMembersObliviousContext
{
public required string RequiredStringMember { get; set; }
public string OptionalStringMember { get; set; }
}

[Fact]
public void SupportsRequiredMembersInAsParametersObliviousContextAttribute()
{
var apiDescription = GetApiDescription(([AsParameters] AsParametersWithRequiredMembersObliviousContext foo) => { });
Assert.Equal(2, apiDescription.ParameterDescriptions.Count);

Assert.Collection(apiDescription.ParameterDescriptions,
param => Assert.True(param.IsRequired),
param => Assert.False(param.IsRequired));
}
#nullable enable

[Fact]
public void TestParameterIsRequired()
{
Expand Down
4 changes: 3 additions & 1 deletion src/OpenApi/src/OpenApiGenerator.cs
Expand Up @@ -378,7 +378,9 @@ private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, Route
}
var nullabilityContext = new NullabilityInfoContext();
var nullability = nullabilityContext.Create(parameter);
var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull;
var isOptional = parameter is PropertyAsParameterInfo argument
? argument.IsOptional
: parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull;
var name = attributeName ?? (pattern.GetParameter(parameter.Name) is { } routeParameter ? routeParameter.Name : parameter.Name);
var openApiParameter = new OpenApiParameter()
{
Expand Down
40 changes: 40 additions & 0 deletions src/OpenApi/test/OpenApiGeneratorTests.cs
Expand Up @@ -928,6 +928,46 @@ static void ValidateParameter(OpenApiOperation operation, string expectedName)
ValidateParameter(GetOpenApiOperation(([FromHeader(Name = "headerName")] string param) => ""), "headerName");
}

#nullable enable
public class AsParametersWithRequiredMembers
{
public required string RequiredStringMember { get; set; }
public required string? RequiredNullableStringMember { get; set; }
public string NonNullableStringMember { get; set; } = string.Empty;
public string? NullableStringMember { get; set; }
}

[Fact]
public void SupportsRequiredMembersInAsParametersAttribute()
{
var operation = GetOpenApiOperation(([AsParameters] AsParametersWithRequiredMembers foo) => { });
Assert.Equal(4, operation.Parameters.Count);

Assert.Collection(operation.Parameters,
param => Assert.True(param.Required),
param => Assert.False(param.Required),
param => Assert.True(param.Required),
param => Assert.False(param.Required));
}
#nullable disable

public class AsParametersWithRequiredMembersObliviousContext
{
public required string RequiredStringMember { get; set; }
public string OptionalStringMember { get; set; }
}

[Fact]
public void SupportsRequiredMembersInAsParametersObliviousContextAttribute()
{
var operation = GetOpenApiOperation(([AsParameters] AsParametersWithRequiredMembersObliviousContext foo) => { });
Assert.Equal(2, operation.Parameters.Count);

Assert.Collection(operation.Parameters,
param => Assert.True(param.Required),
param => Assert.False(param.Required));
}

private static OpenApiOperation GetOpenApiOperation(
Delegate action,
string pattern = null,
Expand Down
16 changes: 15 additions & 1 deletion src/Shared/PropertyAsParameterInfo.cs
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.Extensions.Internal;

Expand Down Expand Up @@ -182,7 +183,20 @@ public override bool IsDefined(Type attributeType, bool inherit)
_underlyingProperty.IsDefined(attributeType, inherit);
}

public new bool IsOptional => HasDefaultValue || NullabilityInfo.ReadState != NullabilityState.NotNull;
public new bool IsOptional => NullabilityInfo.ReadState switch
{
// Anything nullable is optional
NullabilityState.Nullable => true,
// In an oblivious context, the required modifier makes
// members non-optional
NullabilityState.Unknown => !_underlyingProperty.GetCustomAttributes().OfType<RequiredMemberAttribute>().Any(),
// Non-nullable types are only optional if they have a default
// value
NullabilityState.NotNull => HasDefaultValue,
// Assume that types are optional by default so we
// don't greedily opt parameters into param checking
_ => true
};

public NullabilityInfo NullabilityInfo
=> _nullabilityInfo ??= _constructionParameterInfo is not null ?
Expand Down

0 comments on commit 1099d06

Please sign in to comment.