Swashbuckle.aspnetcore: Generation failure involving JsonProperty and JsonConstructor

[felpel]

Original issue: https://github.com/Azure/autorest.typescript/issues/235

Swashbuckle.AspNetCore version: 2.4.0

Issue

I tried to use Microsoft's Autorest to generate the client code for one of my web services where the Swagger definition is generated by this tool. However, since the document is incoherent, Autorest fails and shows this error:

Property 'foo' in Model 'BarModel' is marked readOnly and is also required. This is not allowed.

No errors is shown when we navigate to the web server's Swagger UI route, but it is faulty when I copy the generated Swagger file in Swagger's online editor:

BarModel:
    required:
      - foo
    type: object
        properties:
           foo:
           $ref: '#/definitions/FooModel'
           readOnly: true

The readonly: true property might be wrong since the foo property is set in the constructor decorated with the JsonConstructor attribute, as shown here:

[JsonObject(MemberSerialization.OptIn)]
public sealed class BarModel
{
        #region Properties

        [JsonProperty(PropertyName = "foo", Required = Required.Always)]
        public FooModel Foo { get; }

        // Other properties omitted

       #endregion

        #region Constructors

        [JsonConstructor]
        public AgentModel(FooModel foo)
        {
            Foo = foo;
            // Other constructor parameters omitted
        }

        #endregion
}

Should it really be read-only in this case since it will be set when the JSON is deserialized in the C# object inside the constructor? It will become read-only after the object is constructed, but not before. If we remove the Required = Required.Always in the JsonProperty attribute, the value could be nullable, which is not the case.

[domaindrivendev]

How are you using this model - is it being passed as a body parameter (i.e. [FromBody])?

If so, if you submit a JSON request, including a value for the foo field, the serializer WILL NOT populate it as the setter is private. Therefore, from an API perspective - it's readonly

[domaindrivendev]

Ahhh - I just saw your use of JsonConstructor for deserialization as opposed to public setters. Swashbuckle doesn't currently handle that approach. Is there any reason why you wouldn't just switch to a public setter?

It's also worth noting that Swashbuckle uses the metadata that's provided by the serializer (i.e. JsonProperty.Writeable) to assign the readonly flag. So, from a root cause perspective, this issue might run down a level to JSON.NET.

https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/v3.0.0/src/Swashbuckle.AspNetCore.SwaggerGen/Generator/SchemaRegistry.cs#L250

[RikkiGibson]

Is support for the getter-only+JsonConstructor pattern on the roadmap anywhere? Or something you'd be open to contributions for?

[kzryzstof]

The nice thing of using read-only properties allows us to have immutable instances of our models. Coupled with the JsonConstructorattribute, additional policies are added in our constructors to prevent the rest of the application from dealing directly with any sort of garbage, mostly the null.

Adding the support for read-only properties and JsonContructor would be a great addition to the tool.

[felpel]

@domaindrivendev should we still see those errors when we navigate to the Swagger endpoint like we would see in Swagger's online editor? I could file another issue for this separately.

[domaindrivendev]

@RikkiGibson - nothing on the roadmap right now but always open to contributions.

One thing to note if you do take a look at this, Swashbuckle delegates to the JSON.NET ContractResolver for most of the schema metadata. This is by design because it keeps API descriptions closely in line with "actual" serialization behavior.

Ideally, there would be a single field on JsonProperty (provided by the ContractResolver) that automatically takes read-only+JsonConstructor usage into account. It seems to me that JsonProperty.Writable _should_ be doing this but clearly doesn't right now. So, before refactoring the Swashbuckle code, I would ask that you investigate this avenue a little further - perhaps the best place for the change is in here rather than Swashbuckle?

[RikkiGibson]

Good idea :) I went ahead and created JamesNK/Newtonsoft.Json#1800. I'm sure it'll be valuable to hear from a maintainer on JSON.NET about the intended meaning of the property.

My suspicion on reading the code is that the burden is going to wind up on the Swashbuckle side-- in addition to checking jsonContract.Properties, it will be necessary to check jsonContract.CreatorParameters.

[domaindrivendev]

@felpel @RikkiGibson @kzryzstof - I've finally gotten around to adding this functionality and it should be available as of 5.2.1. One thing to note - it will only work if you're using the Newtonsoft serializer as opposed to System.Text.Json as the latter doesn't support this feature yet either. Furthermore, from v5.0.0 of Swashbuckle and onwards, you need to explicitly tell it to honor Newtonsoft behavior rather than STJ behavior. See this readme section for full details.