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.

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

Records (positional) and DataAnnotations #2212

Open
CaringDev opened this issue Sep 3, 2021 · 16 comments
Open

Records (positional) and DataAnnotations #2212

CaringDev opened this issue Sep 3, 2021 · 16 comments
Labels
help-wanted A change up for grabs for contributions from the community p2 Medium priority
Milestone

Comments

@CaringDev
Copy link

CaringDev commented Sep 3, 2021

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:

  • duplicate the attributes (using property and param) does not work in .NET 9 (Preview 7) anymore
  • use 'traditional' (optionally init) property-bag classes (read-only properties in classes with constructors do not show up in the generated OpenAPI definition).

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

@domaindrivendev
Copy link
Owner

domaindrivendev commented Sep 8, 2021

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

@domaindrivendev domaindrivendev added the p2 Medium priority label Sep 8, 2021
@CaringDev
Copy link
Author

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.

  • Is there already a way to determine which STJ version is used?
  • Would it be acceptable to add a direct STJ >= 5.x dependency? Currently for netstandard2.0 there's a transitive one >= 4.6 (through Swashbuckle.AspNetCore.SwaggerGen and Swashbuckle.AspNetCore.SwaggerUI).
  • Or would we need to #if TFM? Note that this is not safe (e.g. using STJ 5 on netcore3.1 is possible).

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
Copy link

spaasis commented Jan 20, 2022

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
Copy link
Contributor

In relation to this issue, created fix for positional record example/summary handling #2546

@martincostello
Copy link
Collaborator

#2546

@martincostello martincostello added this to the v6.6.0 milestone May 4, 2024
@jacobilsoe
Copy link

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

@martincostello
Copy link
Collaborator

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

@jacobilsoe
Copy link

Not possible to just reopen?

@martincostello
Copy link
Collaborator

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

@jacobilsoe
Copy link

The repro is already stated in this issue.

@martincostello martincostello added the help-wanted A change up for grabs for contributions from the community label May 15, 2024
@Eirenarch
Copy link

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

jgarciadelanoceda commented Aug 23, 2024

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
Copy link

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
Copy link
Author

Done: dotnet/aspnetcore#57486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted A change up for grabs for contributions from the community p2 Medium priority
Projects
None yet
Development

No branches or pull requests

8 participants