dotnet / dotNext

Next generation API for .NET
https://dotnet.github.io/dotNext/
MIT License
1.56k stars 119 forks source link

How to use Optional<string> as query parameter? #213

Closed michaelbmorris closed 5 months ago

michaelbmorris commented 5 months ago

I have a .NET 8 API. It has a controller with an action:

[HttpGet("query")]
public async Task<IActionResult> QueryAsync([FromQuery] QueryParameters parameters)
{
    // Do stuff here
}

QueryParameters.cs:

public class QueryParameters
{
    public Optional<string> SomeOptionalString { get; init; }
}

When I make a request to the action it throws this exception:

fail: Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware[1]
       An unhandled exception has occurred while executing the request.
       System.ArgumentException: The type 'System.String&' may not be used as a type argument.
          at System.RuntimeType.SanityCheckGenericArguments(RuntimeType[] genericArguments, RuntimeType[] genericParameters)
          at System.RuntimeType.MakeGenericType(Type[] instantiation)
          at Microsoft.Extensions.Internal.PropertyHelper.MakeFastPropertyGetter(Type openGenericDelegateType, MethodInfo propertyGetMethod, MethodInfo openGenericWrapperMethod)
          at Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DefaultModelMetadataProvider.CreateSinglePropertyDetails(ModelMetadataIdentity propertyKey, PropertyHelper propertyHelper)
          at Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DefaultModelMetadataProvider.CreatePropertyDetails(ModelMetadataIdentity key)
          at Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DefaultModelMetadataProvider.GetMetadataForProperties(Type modelType)
          at Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DefaultModelMetadata.get_Properties()
          at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexObjectModelBinderProvider.GetBinder(ModelBinderProviderContext context)
          at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderFactory.CreateBinderCoreUncached(DefaultModelBinderProviderContext providerContext, Object token)
          at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderFactory.CreateBinderCoreCached(DefaultModelBinderProviderContext providerContext, Object token)
          at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexObjectModelBinderProvider.GetBinder(ModelBinderProviderContext context)
          at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderFactory.CreateBinderCoreUncached(DefaultModelBinderProviderContext providerContext, Object token)
          at Microsoft.AspNetCore.Mvc.ModelBinding.ModelBinderFactory.CreateBinder(ModelBinderFactoryContext context)
          at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.GetParameterBindingInfo(IModelBinderFactory modelBinderFactory, IModelMetadataProvider modelMetadataProvider, ControllerActionDescriptor actionDescriptor)
          at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.CreateBinderDelegate(ParameterBinder parameterBinder, IModelBinderFactory modelBinderFactory, IModelMetadataProvider modelMetadataProvider, ControllerActionDescriptor actionDescriptor, MvcOptions mvcOptions)
          at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvokerCache.GetCachedResult(ControllerContext controllerContext)
          at Microsoft.AspNetCore.Mvc.Routing.ControllerRequestDelegateFactory.<>c__DisplayClass12_0.<CreateRequestDelegate>b__0(HttpContext context)
          at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
          at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)

I know I can go back to using string? instead here, but I wanted to consistently use Optional instead. Is there a way to make this work?

sakno commented 5 months ago

This is a limitation of ASP.NET Core Model Binding, not an issue of Optional<T>. You need to have your own Type Converter for Optional<T> as described here.

sakno commented 5 months ago

Moreover, the binding architecture of ASP.NET Core passes empty string to TryParse for the type with custom binding. In case of Optional<string> it means that you'll get non-empty Optional container with empty string instead of None.

michaelbmorris commented 5 months ago

Agreed regarding your comment about TryParse not being helpful for this situation. I thought I'd be able to use an IModelBinder but I'm still getting the same error:

public class OptionalStringBinder : IModelBinder
{
    public Task BindModelAsync(ModelBindingContext bindingContext)
    {
        var modelName = bindingContext.ModelName;
        var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);

        if (valueProviderResult == ValueProviderResult.None)
        {
            bindingContext.Result = ModelBindingResult.Success(Optional.None<string>());
            return Task.CompletedTask;
        }

        var value = valueProviderResult.FirstValue;
        bindingContext.Result = ModelBindingResult.Success(new Optional<string>(value));
        return Task.CompletedTask;
    }
}

Is this something worth asking about on the aspnetcore project, or would you advise that I go back to using nullable strings for my scenario?

sakno commented 5 months ago

According to docs, TryParse is the only way of binding for non-complex types. I see no benefits of Optional<T> for query parameters. Even nullable string is redundant because ASP.NET Core never passes null to the parameter.

michaelbmorris commented 5 months ago

Thanks for the help. Good point that string? can't be null if the parameter is included in the query string. I ended up going with the solution below. It works well enough for my case. Hopefully someone else will find it useful too if they need optional query parameters. Sadly it doesn't make use of the Optional<T> struct though.

public readonly struct OptionalString(string? value)
{
    public static OptionalString Undefined => new()
    {
        IsUndefined = true
    };

    public bool IsUndefined { get; private init; } = false;

    public string? Value { get; } = value;
}

public class OptionalStringModelBinder : IModelBinder
{
    public Task BindModelAsync(ModelBindingContext bindingContext)
    {
        var modelName = bindingContext.ModelName;
        var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);

        if (valueProviderResult == ValueProviderResult.None)
        {
            bindingContext.Result = ModelBindingResult.Success(OptionalString.Undefined);
            return Task.CompletedTask;
        }

        if (valueProviderResult.Values.Count > 1)
        {
            bindingContext.ModelState.TryAddModelError(modelName, "Only a single value is allowed.");
            bindingContext.Result = ModelBindingResult.Failed();
            return Task.CompletedTask;
        }

        OptionalString optionalString;

        if (string.IsNullOrEmpty(valueProviderResult.FirstValue))
        {
            // As pointed out, the value will never actually be null here so an empty string will have to suffice.
            // I'm not sure how I'd handle a scenario where the string could be undefined, null, empty, whitespace, or non-empty and non-whitespace.
            // I think it would require binding an additional parameter value.
            optionalString = new OptionalString(null);
        }
        else
        {
            optionalString = new OptionalString(valueProviderResult.FirstValue);
        }

        bindingContext.Result = ModelBindingResult.Success(optionalString);
        return Task.CompletedTask;
    }
}

public class QueryParameters
{
    // Could of course be registered globally via a ModelBinderProvider instead of an attribute.
    [ModelBinder(BinderType = typeof(OptionalStringModelBinder))]
    public OptionalString SomeString { get; init; }
}

[ApiController]
public class QueryController : ControllerBase
{
    [HttpGet("query")]
    public IActionResult Query([FromQuery] QueryParameters parameters)
    {
        // Do stuff here
    }
}

GET /query : Undefined GET /query?SomeString : null GET /query?SomeString=value : "value"

I have another model binder that I'm using for long?s as well.

// This is identical to OptionalString except for the value type.
// It's a shame generics don't work with model binding, but if they did I could just use Optional<T> instead of making these knockoffs.
public readonly struct OptionalLong(long? value)
{
    public static OptionalLong Undefined => new()
    {
        IsUndefined = true
    };

    public bool IsUndefined { get; private init; } = false;

    public long? Value { get; } = value;
}

public class OptionalLongModelBinder : IModelBinder
{
    public Task BindModelAsync(ModelBindingContext bindingContext)
    {
        var modelName = bindingContext.ModelName;
        var valueProviderResult = bindingContext.ValueProvider.GetValue(modelName);

        if (valueProviderResult == ValueProviderResult.None)
        {
            bindingContext.Result = ModelBindingResult.Success(OptionalLong.Undefined);
            return Task.CompletedTask;
        }

        if (valueProviderResult.Values.Count > 1)
        {
            bindingContext.ModelState.TryAddModelError(modelName, "Only a single value is allowed.");
            bindingContext.Result = ModelBindingResult.Failed();
            return Task.CompletedTask;
        }

        var stringValue = valueProviderResult.FirstValue;

        if (string.IsNullOrWhiteSpace(stringValue))
        {
            bindingContext.Result = ModelBindingResult.Success(new OptionalLong(null));
        }
        else if (long.TryParse(stringValue, out var value))
        {
            bindingContext.Result = ModelBindingResult.Success(new OptionalLong(value));
        }
        else
        {
            bindingContext.ModelState.TryAddModelError(modelName, "Value must be null or a long.");
            bindingContext.Result = ModelBindingResult.Failed();
        }

        return Task.CompletedTask;
    }
}