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

Nswag, c# Required = Newtonsoft.Json.Required.DisallowNull, how to handle #1991

Open
Unders0n opened this issue Mar 2, 2019 · 9 comments
Open

Comments

@Unders0n
Copy link

Unders0n commented Mar 2, 2019

Copy of https://stackoverflow.com/questions/54656603/swagger-nswagstudio-c-required-newtonsoft-json-required-disallownull-how , which got no attention, please help.

Given: api that i have limited influence to in terms of changing, built on net core 2.2. Standart netCore swagger used. Some classes of DTO have fields in it marked with [System.ComponentModel.DataAnnotations.Required] But for some reasons (which are also discussable) some methods return objects of this classes with nulls in this fields. Annotation resulted in

"required": [
"given", - this field for example
"family",
"email",
"postCode"
],
"type": "object",
...

in swagger spec which then results in

[Newtonsoft.Json.JsonProperty("given", Required = Newtonsoft.Json.Required.DisallowNull, NullValueHandling = Newtonsoft.Json.NullValueHandling.Ignore)]

in c# generated code (i'm using nswag studio and c# client with pretty standart settings). And then when i'm trying to get list of such objects from api using generated c# client, if some of such properties have null it obviosly throws newtonsoft deserialization exception. so how we can handle that? I thought of both client side and server side solutions:

1)on server we can configure not to expose info about required to swagger spec.

  1. on client we can possibly configure behavior of translating that Required block to Required = Newtonsoft.Json.Required.Default

  2. forget about all that and insist so api will not return object with null vaues which properties market Required.

@RicoSuter
Copy link
Owner

Is this swagger 2.0 or openapi 3.0?

@Unders0n
Copy link
Author

Unders0n commented Mar 2, 2019

@RSuter as from swagger spec: "swagger": "2.0",

@RicoSuter
Copy link
Owner

RicoSuter commented Mar 2, 2019

To clarify the generation of this:

image

image

base.IsNullable:

image

The biggest problem is that Swagger 2.0 does not have a way to express nullability - so we need to use required for nullability which is technically wrong - in JSON Schema this just specifies whether the the value must be set or not...

@RicoSuter
Copy link
Owner

Another option is to transform the swagger spec before generating the clients, sample here:
https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/src/AutoRestTransformer/Program.cs

@VitalickS
Copy link

As alternative, you can manually override behavior for JsonSerializerSettings in partial class method (Client). Just add this to partial class

partial void UpdateJsonSerializerSettings(JsonSerializerSettings settings)
{
    settings.ContractResolver = new SafeContractResolver();
}

class SafeContractResolver : DefaultContractResolver
{
    protected override JsonProperty CreateProperty(MemberInfo member, MemberSerialization memberSerialization)
    {
        var jsonProp = base.CreateProperty(member, memberSerialization);
        jsonProp.Required = Required.Default;
        return jsonProp;
    }
}

@kjkrum
Copy link

kjkrum commented Feb 27, 2020

@RicoSuter Given that this is a limitation of Swagger 2.0, is there an official position on where the fault lies if a 2.0 API describes a property as required but then sends null for that property? Is it unequivocally a bug in the API, or is it a matter of there being no good general solution for NSwag and therefore up to the client to implement a workaround?

@kjkrum
Copy link

kjkrum commented Feb 27, 2020

To shed some light on my own question, this SO Q&A suggests that an API sending a null value for a string property is wrong, not because the property is required, but because null isn't a string. It sounds like Swagger 2.0 having no means to express nullability strictly means that nothing is nullable. Any way of representing nullability is just a hack, whether it's an API sending nulls, or NSwag's treatment of required. Is that correct?

@kyleb4020
Copy link

As alternative, you can manually override behavior for JsonSerializerSettings in partial class method (Client). Just add this to partial class

partial void UpdateJsonSerializerSettings(JsonSerializerSettings settings)
{
    settings.ContractResolver = new SafeContractResolver();
}

class SafeContractResolver : DefaultContractResolver
{
    protected override JsonProperty CreateProperty(MemberInfo member, MemberSerialization memberSerialization)
    {
        var jsonProp = base.CreateProperty(member, memberSerialization);
        jsonProp.Required = Required.Default;
        return jsonProp;
    }
}

This answer helped me to create a more nuanced approach to intelligently adjust the Required property of the JsonProperty. Here is my code:

internal class SafeContractResolver : DefaultContractResolver
{
    protected override JsonProperty CreateProperty(MemberInfo member, MemberSerialization memberSerialization)
    {
        var jsonProp = base.CreateProperty(member, memberSerialization);
        jsonProp.Required = member.MemberType switch
        {
            MemberTypes.Property when member is PropertyInfo info && IsNullable(info) => Required.Default,
            MemberTypes.Field when member is FieldInfo fieldInfo && IsNullable(fieldInfo) => Required.Default,
            _ => jsonProp.Required
        };

        return jsonProp;
    }
}

private static bool IsNullable(PropertyInfo property) => IsNullableHelper(property.PropertyType, property.DeclaringType, property.CustomAttributes);

private static bool IsNullable(FieldInfo field) => IsNullableHelper(field.FieldType, field.DeclaringType, field.CustomAttributes);

private static bool IsNullableHelper(Type memberType, MemberInfo? declaringType, IEnumerable<CustomAttributeData> customAttributes)
{
    if (memberType.IsValueType)
        return Nullable.GetUnderlyingType(memberType) != null;

    var nullable = customAttributes
        .FirstOrDefault(x => x.AttributeType.FullName == "System.Runtime.CompilerServices.NullableAttribute");
    if (nullable is { ConstructorArguments.Count: 1 })
    {
        var attributeArgument = nullable.ConstructorArguments[0];
        if (attributeArgument.ArgumentType == typeof(byte[]))
        {
            var args = (ReadOnlyCollection<CustomAttributeTypedArgument>)attributeArgument.Value!;
            if (args.Count > 0 && args[0].ArgumentType == typeof(byte))
            {
                return (byte)args[0].Value! == 2;
            }
        }
        else if (attributeArgument.ArgumentType == typeof(byte))
        {
            return (byte)attributeArgument.Value! == 2;
        }
    }

    for (var type = declaringType; type != null; type = type.DeclaringType)
    {
        var context = type.CustomAttributes
            .FirstOrDefault(x => x.AttributeType.FullName == "System.Runtime.CompilerServices.NullableContextAttribute");
        if (context is { ConstructorArguments.Count: 1 } &&
            context.ConstructorArguments[0].ArgumentType == typeof(byte))
        {
            return (byte)context.ConstructorArguments[0].Value! == 2;
        }
    }

    // Couldn't find a suitable attribute
    return false;
}

With the introduction of C# 8 we also need to account for nullable reference types. This handy IsNullableHelper() method (found here: https://stackoverflow.com/a/58454489/7546999) uses reflection to determine if the property is nullable and should therefore have the Required property of the JsonProperty set to Required.Default.

I hope this helps the next person who comes here looking for a good workaround.

@rfcdejong
Copy link

For someone who isn't using the JsonConvert himself. You can set the default DataContractSerializer to the Safe one:

JsonConvert.DefaultSettings = () => new JsonSerializerSettings { ContractResolver = new SafeContractResolver() };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants