Add support for `[JsonObject(ItemRequired = Required.Always)]`

[jeremyVignelles]

[JsonProperty(Required = Required.Always)] is supported, but not [JsonObject(ItemRequired = Required.Always)], which allows to set this on the above level.

It would be nice to add support for this one in the future (even if I have workarounds for this one).

See : https://www.newtonsoft.com/json/help/html/P_Newtonsoft_Json_JsonObjectAttribute_ItemRequired.htm

A repro : https://github.com/jeremyVignelles/TestNSwagNetCoreApp/commit/8edd64bd29901d419db22fb70b96fb609fc74c2e#diff-f5a253633e911f1649e6707660563be8R41

Open the swagger.json, see how "hello" is not a required property.

[jeremyVignelles]

For those looking into this, here is what I did:

EDIT : Support overriding with attrbutes

namespace MyNamespace
{
    using System;
    using Newtonsoft.Json;
    using Newtonsoft.Json.Serialization;

    /// <summary>
    /// The contract resolver that marks every property as required by default
    /// </summary>
    public class MyContractResolver : CamelCasePropertyNamesContractResolver
    {
        /// <summary>
        /// Creates a <see cref="T:Newtonsoft.Json.Serialization.JsonObjectContract" /> for the given type.
        /// </summary>
        /// <param name="objectType">Type of the object.</param>
        /// <returns>A <see cref="T:Newtonsoft.Json.Serialization.JsonObjectContract" /> for the given type.</returns>
        protected override JsonObjectContract CreateObjectContract(Type objectType)
        {
            var contract = base.CreateObjectContract(objectType);

            /* I'd like to be able to use this, instead of the lines below, but https://github.com/RSuter/NJsonSchema/issues/841
            if (!contract.ItemRequired.HasValue)
            {
                contract.ItemRequired = Required.Always;
            }
            /*/
            foreach (var p in contract.Properties)
            {
                if (!p.HasMemberAttribute)
                {// The JsonProperty/JsonRequired attribute has not been explicitly set, let's use this default value
                    p.Required = Required.Always;
                }
            }
            //*/
            return contract;
        }
    }
}

then

            settings.SerializerSettings = new JsonSerializerSettings
            {
                ContractResolver = new MyContractResolver ()
            };

[RicoSuter]

Because we use the contract resolver internally to reflect the serialized types i would expect that [JsonObject(ItemRequired = Required.Always)] just works... but it seems that a custom resolver is the only solution. Thanks for sharing...

[jeremyVignelles]

The custom resolver helps setting JsonProperty.Required on each property, but shows the same issue when placed on the object itself (aka JsonObject.ItemRequired)

[jeremyVignelles]

I just edited my sample contract resolver, it can now use values that are explicitly defined in a JsonPropertyAttribute

[RicoSuter]

Can we close this issue then?

[jeremyVignelles]

After thinking for a while, should it be closed or should it work out of the box? I'll let you decide if this needs to be closed then, but I have a workaround for now

[RicoSuter]

I think if the contract resolver does not respect the [JsonObject(ItemRequired = Required.Always)] then the serialization will also ignore it and then it is wrong to manually implement this out-of-the-box as it would not reflect the actual behavior of the serializer... or am I wrong?

[jeremyVignelles]

I'm not sure I get you... I expect [JsonObject(ItemRequired = Required.Always)] to work as advertised in the serializer i.e : disallow null and undefined values for all properties in this object.

Why would the default openapi spec generator interpret something else?

[RicoSuter]

Ok, I've checked this in some tests and it seems that you are right: The serializer and contract resolver do not match in this case:

So we need to handle this special case manually so that the schema reflects what the serializer expects/does.

[jeremyVignelles]

Or maybe it is an upstream bug that needs to be addressed there?

[RicoSuter]

Yes, it's really strange, in the object contract it is set as expected:

But the property does not "inherit" this:

But I'd expect that the property uses the object's value if no JsonPropertyAttribute is set which overrides this at serialization time...

[RicoSuter]

Ah shit, we have a problem, the ItemRequired is only overridden if Required is set in the attribute, e.g. [JsonProperty("abc", Required = Required.Default)] and not in this case [JsonProperty("abc")] - but to check this, we cannot check Required of JsonPropertyAttribute because it defaults to Default when Required is not set. It seems that Newtonsoft.Json internally uses the internal field directly to do this check: https://github.com/JamesNK/Newtonsoft.Json/blob/d48558b53b0a516bfa15c1af50231ea1ba7c2454/Src/Newtonsoft.Json/JsonPropertyAttribute.cs#L46

[RicoSuter]

I've created a PR: https://github.com/RSuter/NJsonSchema/pull/870

But we need to solve the open comment - maybe with a reflection hack? :-)

[RicoSuter]

@JamesNK is there a way to get the actual required value of a property? Or is there another way to actually check whether a JsonPropertyAttribute overrides the required value?

[simeyla]

I've just updated my Angular project to use strict mode and wow are there a lot of problems where it's complaining things are null when they shouldn't be! Being able to just add [JsonObject(ItemRequired = Required.Always] would save me a LOT of lines of code.

@RSuter Curious if you are moving ahead with this PR or is it blocked because of the _required internal property issue? This feature would be extremely useful!

ALTERNATIVELY...

If I understand the complications correctly, you cannot combine the values of Required coming from [JsonProperty] and [JsonObject] because you don't know whether or not Required was explicitly set on any [JsonProperty] attributes. (because the field is private).

So then you have [JsonObject(ItemRequired = Required.Always)] but [JsonProperty('firstName')] will override it to Required.Default :-(

My concern is that if you needed to wait for JSON.NET to implement a way to inspect the property then there's a very good change it will never happen, and then likewise this new feature may never happen.

---

So maybe this would need to be a switch that you would opt-in to, and it would be understood that you must specify Required if you have a [JsonProperty] attribute, and that normal [JsonObject] rules don't apply.

It could potentially be bad to switch to the new behavior if people were relying on the current behavior - whether they realized it or not.

Right now I just have a LOT of objects with many 'naked' properties with no attributes and I'm dying to add something to the class level. Surprised how complicated this got, but this will be a very welcome feature when figured out :-)

[RicoSuter]

So maybe this would need to be a switch that you would opt-in to, and it would be understood that you must specify Required if you have a [JsonProperty] attribute, and that normal [JsonObject] rules don't apply

You can already do that on the global SerializerSettings (in the settings) object by providing a custom contract resolver which sets them to Always... see @jeremyVignelles

I don't know if I can resolve the problems in the PR - my only option is to use reflection to read the internal field - but because this library is .net standard > 1.0 I cannot access internal fields via reflection...

[jeremyVignelles]

I just updated my ContractResolver to add support for C#8 nullables. Now, if the property is not nullable, it will be Required.Always by default, whereas when it is nullable (string? for example), it will be Required.AllowNull.

Here is the new code:

namespace MyNamespace
{
    using Namotion.Reflection;
    using Newtonsoft.Json;
    using Newtonsoft.Json.Serialization;
    using NJsonSchema.Generation;
    using System;

    /// <summary>
    /// The contract resolver that marks every property as required by default, except the ones that are nullable
    /// </summary>
    public class ContractResolver : CamelCasePropertyNamesContractResolver
    {
        private readonly JsonSchemaGeneratorSettings _schemaGeneratorSettings;

        /// <summary>
        /// Initializes a new ContractResolver instance
        /// </summary>
        /// <param name="schemaGeneratorSettings">The settings used by NSwag's schema generation</param>
        public ContractResolver(JsonSchemaGeneratorSettings schemaGeneratorSettings)
        {
            this._schemaGeneratorSettings = schemaGeneratorSettings;
        }

        /// <summary>
        /// Creates a <see cref="T:Newtonsoft.Json.Serialization.JsonObjectContract" /> for the given type.
        /// </summary>
        /// <param name="objectType">Type of the object.</param>
        /// <returns>A <see cref="T:Newtonsoft.Json.Serialization.JsonObjectContract" /> for the given type.</returns>
        protected override JsonObjectContract CreateObjectContract(Type objectType)
        {
            var contract = base.CreateObjectContract(objectType);

            /* I'd like to be able to use this, instead of the lines below, but https://github.com/RSuter/NJsonSchema/issues/841
            if (!contract.ItemRequired.HasValue)
            {
                contract.ItemRequired = Required.Always;
            }
            /*/
            foreach (var p in contract.Properties)
            {
                if (!p.HasMemberAttribute)
                {// The JsonProperty/JsonRequired attribute has not been explicitly set, let's use this default value
                    var contextualMember = objectType.GetMember(p.UnderlyingName)[0].ToContextualMember();
                    var propertyDescription = this._schemaGeneratorSettings.ReflectionService.GetDescription(contextualMember, this._schemaGeneratorSettings);
                    p.Required = propertyDescription.IsNullable ? Required.AllowNull : Required.Always;
                }
            }
            //*/
            return contract;
        }
    }
}

Usage:

            services.AddOpenApiDocument(settings =>
            {
                settings.SerializerSettings = new JsonSerializerSettings
                {
                    ContractResolver = new ContractResolver(settings)
                };
            });

[jeremyVignelles]

Just confirmed this is still an issue, but Namotion.Reflection's API has changed it seems.

the new code now looks like this:

            foreach (var p in contract.Properties)
            {
                if (!p.HasMemberAttribute)
                {// The JsonProperty/JsonRequired attribute has not been explicitly set, let's use this default value
                    var contextualMember = objectType.ToContextualType().GetProperty(p.UnderlyingName)!;
                    var propertyDescription = this._schemaGeneratorSettings.ReflectionService.GetDescription(contextualMember.PropertyType, this._schemaGeneratorSettings);
                    p.Required = propertyDescription.IsNullable ? Required.AllowNull : Required.Always;
                }
            }