RicoSuter / NSwag

The Swagger/OpenAPI toolchain for .NET, ASP.NET Core and TypeScript.
http://NSwag.org
MIT License
6.76k stars 1.29k forks source link

SwaggerToCSharpClientGenerator: JSON output of JsonPatchDocument (HttpPatch) includes the operations property wrapper, causing model binding issues in the API #1410

Closed rebeccapowell closed 5 years ago

rebeccapowell commented 6 years ago

I've had some issues with a JsonPatchDocument serialisation.

The serialization of the JsonPatchDocument from the generated client sends the following:

{
    "operations: [
         { "op": "add", "path": "/name", "value": "Carly" }
    ]
}

However, Web API 2 is expecting:

[
    { "op": "add", "path": "/name", "value": "Carly" }
]

As a result the request cannot be successfully bound to the controller method parameter.

A hacky fix is to take a copy the PatchAsync method and include it in the partial class/interface. All that method does is that it serializes the object.Operations rather than the object itself. I.e. Changing from this:

var content_ = new System.Net.Http.StringContent(Newtonsoft.Json.JsonConvert.SerializeObject(customer, _settings.Value));

to this:

var content_ = new System.Net.Http.StringContent(Newtonsoft.Json.JsonConvert.SerializeObject(customer.Operations, _settings.Value));

Is this a bug, or is there some other way to work around this, since this workaround feels rather ugly?

RicoSuter commented 6 years ago

It seems that the schema of the JsonPatchDocument cannot be correctly statically determined... try add the following attribute to the operation:

[SwaggerResponse(typeof(List<JsonPatchOperation>))]

(I dont know the exact operation class)

This way the generated schema schould be correct and thus the client too...

rebeccapowell commented 6 years ago

@RSuter Hi Rico. It's the request that is a problem, not the response.

Looking at the source code for JsonPatchDocument, The ToJson method does the following:

public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
    if (value is IJsonPatchDocument)
    {
        var jsonPatchDoc = (IJsonPatchDocument)value;
        var lst = jsonPatchDoc.GetOperations();

        // write out the operations, no envelope
        serializer.Serialize(writer, lst);
    }
}

Note that it excludes the root. Is there a way to set up the JsonSerializerSetting in the client, in order to use this converter?

RicoSuter commented 6 years ago

The problem is that a converter is "procedural" and it is not possible to get a static description of what he does which is needed for the spec... but i think this would solve your problem (same functionality for params as for the response): https://github.com/RSuter/NSwag/issues/1406

what do you think?

HenryGarle commented 6 years ago

I can't seem to make any headway on this issue. Is there a way during generation time to override and force a type? I have dug around and perhaps I am missing something but I cant see a way to achieve that. Being able to override types would also solve some of the issues we have with generic wrapper classes being returned as well as the JsonPatchDocument.

RicoSuter commented 6 years ago
public void MyAction([FromBody] [JsonSchemaType(typeof(List<Marvin.JsonPatch.Operations.Operation>))] JsonPatchDocument patch)
{

}

JsonSchemaTypeAttribute from NJsonSchema NuGet package

RicoSuter commented 6 years ago

Or:

[JsonSchemaType(typeof(List<Marvin.JsonPatch.Operations.Operation>))]
public class MyJsonPatchDocument : JsonPatchDocument {  }
HenryGarle commented 6 years ago

Thank you for your speedy response! I realise now after reading through a ton of issues I was muddled with which this was and what I hope to achieve is slightly different. (And I am also using this which is similar but not a direct port). While the format is now correct what I really want it to do is to just maintain the use of JsonPatchDocument, not just pass over the list of operations. I believe after reading that this is a limitation in part due to the swagger definitions as is the issue with generic classes. Is there a way to forcibly override types at C# generation time, opposed to the swagger generation? I know that this goes against the point of a generic format but if there is a mechanic we can use to do it it would help us solve both the JsonPatchDocument as well as our issues with generic classes.

RicoSuter commented 6 years ago

The gens have a setting ExcludedTypes where you can exclude the dto and AdditionalNamespaceUsages to add addiotional using statements

HenryGarle commented 6 years ago

I think I am looking at this back to front, by the time the swagger document has been generated the damage has already been done, at that point excluding SomeType<> would do nothing as its not defined as that any longer. I will do some further digging and stop hijacking the thread. Thank you for your responses.

G4rce commented 6 years ago

@rebeccapowell @RSuter has there been any progress on this issue. Writing our own PatchAsync overload like Rebecca suggested would beat the whole point of code generation. Pls advise, regards.

rebeccapowell commented 5 years ago

@G4rce The following works:

public IActionResult PatchCustomer(string customerId, [JsonSchemaType(typeof(List<Operation>))] [FromBody]JsonPatchDocument<CustomerDto> customer)
{
    // do stuff with customer
    var customerDto = new CustomerDto();
    customer.ApplyTo(customerDto);
}