domaindrivendev / Swashbuckle.AspNetCore

Swagger tools for documenting API's built on ASP.NET Core
MIT License
5.22k stars 1.3k forks source link

Records (positional) and DataAnnotations #2212

Open CaringDev opened 3 years ago

CaringDev commented 3 years ago

I'd like to combine C# 9 positional records, System.ComponentModel.DataAnnotations, model validation and Swashbuckle.AspNetCore (current latest 6.2.1) in a concise and convenient way:

Ideally, this record

public record Foo([StringLength(2)] string Bar);

would produce

"Foo": {
    "type": "object",
    "properties": {
      "bar": {
        "maxLength": 2,
        "minLength": 0,
        "type": "string"
      }
    },
    "additionalProperties": false
  }

but currently, I get

"Foo": {
    "type": "object",
    "properties": {
      "bar": {
        "type": "string"
      }
    },
    "additionalProperties": false
  }

Targeting the attribute to the underlying property (by adding property: to the attribute) fixes the OpenAPI definition but breaks model validation (i.e. we get a 200 for models that should return a 400).

Known "workaround"s:

Is this something you would consider adding / having added (any blockers / difficulties you see) to Swashbuckle?

domaindrivendev commented 3 years ago

Def a useful feature that will become more relevant with the adoption of records for API types. It's doable but non-trivial and I don't have the bandwidth to look into it at the moment. However, I would accept a PR if you're interested? But, if it's looking like a big refactor, I would recommend discussing and aligning on the approach before knocking out the PR.

Off the top of my head, you would need to update the SchemaGenerator.GenerateSchemaForMember method here: https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/v6.2.1/src/Swashbuckle.AspNetCore.SwaggerGen/SchemaGenerator/SchemaGenerator.cs#L62

When retrieving custom attributes, you could do an additional check to determine if the member has a matching constructor parameter and if so include attributes on that parameter. Actually, we already do something similar but specifically for honoring Newtonsoft support for deserializing property values via the constructor. https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/v6.2.1/src/Swashbuckle.AspNetCore.Newtonsoft/SchemaGenerator/NewtonsoftDataContractResolver.cs#L147

CaringDev commented 3 years ago

That Newtonsoft logic seems very promising... ctor support has been added to STJ in 5.x I guess this would allow cleaning up the Newtonsoft special casing.

As I'm currently very busy as well, I will have a go starting mid-October. I hope you don't mind having an issue open that long?

spaasis commented 2 years ago

Here's how I solved another issue with getting the StringLength attribute values from records for a specific property. Maybe something like this could be of use in Swashbuckle if it was a bit more generic?

    public static int MaxLength<T>(Expression<Func<T, string?>> outExpr) {
        var expr = (MemberExpression)outExpr.Body;
        var prop = (PropertyInfo)expr.Member;
        var length = prop.GetCustomAttribute<StringLengthAttribute>()?.MaximumLength;

        if (length != null) {
            return length.Value;
        }

        //for records we need to dig the attributes out of the constructor
        var constructors = typeof(T).GetConstructors();
        foreach (var constructorInfo in constructors) {
            foreach (var parames in constructorInfo.GetParameters().Where(p => p.Name == prop.Name)) {
                length = parames.GetCustomAttribute<StringLengthAttribute>()?.MaximumLength;
                if (length != null) {
                    return length.Value;
                }
            }
        }
        throw new ArgumentException($"Attribute {nameof(StringLengthAttribute)} not found in property {prop.Name} of type {typeof(T)}");
    }
pixellos commented 1 year ago

In relation to this issue, created fix for positional record example/summary handling https://github.com/domaindrivendev/Swashbuckle.AspNetCore/pull/2546

martincostello commented 4 months ago

2546

jacobilsoe commented 4 months ago

Should this issue have been closed? #2546 does not seem to solve the data annotation issue with C# records.

martincostello commented 4 months ago

My understanding was that it did - if it doesn't, please open a new issue and we can look into it.

jacobilsoe commented 4 months ago

Not possible to just reopen?

martincostello commented 4 months ago

Either way, we'd like a repro so we can see exactly what doesn't seem to be working.

jacobilsoe commented 4 months ago

The repro is already stated in this issue.

Eirenarch commented 2 months ago

I needed this even thought about contributing a fix. However after digging deeper it seems to me that this issue should be fixed in ASP.NET Core's OpenAPI support and not in Swashbuckle. In the meantime I use the following filter.

using System.ComponentModel.DataAnnotations;
using System.Reflection;
using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;

public class PositionalRecordsSchemaFilter : ISchemaFilter
{
    public void Apply(OpenApiSchema model, SchemaFilterContext context)
    {
        ConstructorInfo[] constructors = context.Type.GetConstructors();

        if (constructors.Length == 1)
        {
            ParameterInfo[] parameters = constructors[0].GetParameters();

            foreach (KeyValuePair<string, OpenApiSchema> modelProperty in model.Properties)
            {
                ParameterInfo? constructorParameter = parameters.FirstOrDefault(p => String.Equals(p.Name, modelProperty.Key, StringComparison.OrdinalIgnoreCase));

                if (constructorParameter is not null)
                {
                    if (constructorParameter.ParameterType == typeof(string))
                    {
                        foreach (Attribute attribute in constructorParameter.GetCustomAttributes())
                        {
                            if (attribute is MinLengthAttribute minLength)
                            {
                                modelProperty.Value.MinLength = minLength.Length;
                            }
                            else if (attribute is MaxLengthAttribute maxLength)
                            {
                                modelProperty.Value.MaxLength = maxLength.Length;
                            }
                            else if (attribute is StringLengthAttribute stringLength)
                            {
                                modelProperty.Value.MinLength = stringLength.MinimumLength;
                                modelProperty.Value.MaxLength = stringLength.MaximumLength;
                            }
                            else if (attribute is RegularExpressionAttribute regex)
                            {
                                modelProperty.Value.Pattern = regex.Pattern;
                            }
                        }
                    }

                    if (IsNumeric(constructorParameter.ParameterType))
                    {
                        RangeAttribute? rangeAttribute = constructorParameter.GetCustomAttribute<RangeAttribute>();

                        if (rangeAttribute is not null)
                        {
                            decimal? minValue = GetDecimalValue(rangeAttribute.Minimum);
                            decimal? maxValue = GetDecimalValue(rangeAttribute.Maximum);

                            if (minValue is not null)
                            {
                                modelProperty.Value.ExclusiveMinimum = rangeAttribute.MinimumIsExclusive;
                                modelProperty.Value.Minimum = minValue;
                            }

                            if (maxValue is not null)
                            {
                                modelProperty.Value.ExclusiveMaximum = rangeAttribute.MaximumIsExclusive;
                                modelProperty.Value.Maximum = maxValue;
                            }
                        }
                    }
                }
            }
        }
    }

    private static bool IsNumeric(Type? type)
    {
        if (type is null)
        {
            return false;
        }

        return Type.GetTypeCode(type) switch
        {
            TypeCode.Byte => true,
            TypeCode.SByte => true,
            TypeCode.UInt16 => true,
            TypeCode.UInt32 => true,
            TypeCode.UInt64 => true,
            TypeCode.Int16 => true,
            TypeCode.Int32 => true,
            TypeCode.Int64 => true,
            TypeCode.Decimal => true,
            TypeCode.Double => true,
            TypeCode.Single => true,
            //Support for int?, double?, decimal? etc.
            TypeCode.Object => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) && IsNumeric(Nullable.GetUnderlyingType(type)),
            _ => false,
        };
    }

    private static decimal? GetDecimalValue(object? value)
    {
        if (value is null)
        {
            return null;
        }

        return Convert.ToDecimal(value);
    }
}

Note that it works for my API, it might need fixing for some cases and it lacks support for some things for example BigInteger properties, AllowedValues and DeniedValues and probably more

jgarciadelanoceda commented 4 weeks ago

I have a similar issue with the addition of AspNetCore that I did in #2658. The issue is that the CustomAttributes are not obtained the same as Parameters or properties. @CaringDev can you please provide a minimal reproduction?

CaringDev commented 3 weeks ago

Looking at this it seems that the problem is rooted in ASP.NET Core. Even in 9 Preview 7 this does not work out-of-the-box with the new OpenAPI support.

jgarciadelanoceda commented 3 weeks ago

I think that the problem could be caused by where are the CustomAttributes obtained. If the parameter is a Property( for example a Body request), then the attributes are obtained from the property . If the parameter is just a parameter, then they are obtained from the parameter. In this sharplab example you will see the difference.

For records right now if they are part of the Body the CustomAttributes are obtained from the property, whereas if they are part of AsParameters they are obtained from the ctor.

With the changes I did for showing the XML comments for AsParameters in both cases they are obtained from the Property

Eirenarch commented 3 weeks ago

Looking at this it seems that the problem is rooted in ASP.NET Core. Even in 9 Preview 7 this does not work out-of-the-box with the new OpenAPI support.

Maybe we should open an issue on the ASP.NET Core repo

CaringDev commented 3 weeks ago

Done: https://github.com/dotnet/aspnetcore/issues/57486