RicoSuter / NSwag

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

Is there a setting to change doubles to decimals? #1814

Closed VictorioBerra closed 5 years ago

VictorioBerra commented 5 years ago

I am trying to generate a CSharp Client from a swagger URL online using the NSwagStudio.

Some of the definitions look like this:

"initialInvestment": {
     "type": "number",
    "format": "double",
    "description": "The minimum initial investment required to purchase the fund"
},

It returns money as doubles, but I know I should be using decimals for money. Is there a setting in here somewhere to generate my POCOs using decimals any time it sees a type number with format double?

RicoSuter commented 5 years ago

There is no setting for that i think. You’d need to preprocess the spec (eg with JsonSchemaVisitor in NJsonSchema)

VictorioBerra commented 5 years ago

Gotcha, I saw a lot of options for primitive types, just seemed odd that it was such an exclusive list. Why not have all the primitive types?

image

RicoSuter commented 5 years ago

Here you can only specify types which cannot be determined from the spec or are not well defined in Swagger... in your case it clearly states double and if you need decimal then the spec is “wrong”. You could also write a custom csharp type resolver but then you cannot use it via cli/studio.

VictorioBerra commented 5 years ago

if you need decimal then the spec is “wrong”.

Maybe slightly off topic, but the API I am using returns doubles even for money, and I am not sure why. It is a widely used API and the spec is almost certanly not wrong due to the way they reference and specify stuff even in the non-swagger HTML docs. Everywhere I look it is stressed that if you deal with money in your app you should used the highest precision that the language offers (in this case a decimal)

Knowing that info, I assumed I should force over all double's to decimals right away, otherwise I will have to cast everything as the deeper layers of my app start interacting with the payload from the server.

Maybe I am better off adding a mapping layer between the API deserialization classes so I get DTOs in the format I need instead? Or maybe I should keep them as doubles and continue to cast them where needed? Is it possible that casting doubles to decimals can throw everything off?

RicoSuter commented 5 years ago

You should directly generate decimals the because converting from string to double to decimal might lose precision...

Fo now the simplest solution is to write an own cli tool with NSwag.Core (SwaggerDocument.FromJson/ToJson) and write a simple schema visitor to change to decimal:

https://github.com/RSuter/NJsonSchema/blob/master/src/NJsonSchema/Visitors/JsonSchemaVisitorBase.cs

RicoSuter commented 5 years ago

Sample https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/src/AutoRestTransformer/Program.cs

VictorioBerra commented 5 years ago

@RSuter Slowly working my way through this. As a noob i found a lot of confusion on when to use chunks of NJsonSchema vs when to use NSwag. Looks like NSwag is a fancy wrapper for NJsonSchema. And the example you provided used JsonSchemaVisitor but I found that I needed JsonSchemaVisitorBase instead.

Anyways, since I am now having to drop down low level and use a visitor, I also can no longer use the GUI which sucks. Because of this I am having to go through the UI and find the options that match the items in SwaggerToCSharpClientGeneratorSettings. I found some stuff hard to find like how can I disable annotation attributes? and FromJsonToJson? There does not seem to be an option in SwaggerToCSharpClientGeneratorSettings or CSharpClientGeneratorSettings. But its here:

image

VictorioBerra commented 5 years ago

I think I found some here https://github.com/RSuter/NJsonSchema/blob/master/src/NJsonSchema.CodeGeneration.CSharp/CSharpGeneratorSettings.cs

RicoSuter commented 5 years ago

You could just write the cli so that it reads and writes a file and then use the new file in the ui

VictorioBerra commented 5 years ago

That is a good point. In the end, this is what I arrived at. (written in LinqPad)

async void Main()
{
    // Fetch the Swagger JSON
    var client = new HttpClient();
    HttpResponseMessage response = await client.GetAsync("https://apisb.etrade.com/docs/api/market/quotesjson/swagger.json");
    response.EnsureSuccessStatusCode();
    string responseBody = await response.Content.ReadAsStringAsync();

    await RunAsync(responseBody);
}

private static async Task RunAsync(string jsonString)
{
    var document = await SwaggerDocument.FromJsonAsync(jsonString);

    var visitor = new DoubleToDecimalVisitor();
    await visitor.VisitAsync(document);

    await SwaggerDocument.FromJsonAsync(jsonString);

    var settings = new SwaggerToCSharpClientGeneratorSettings()
    {
        GenerateExceptionClasses = false,
        GenerateClientClasses = false,
        ParameterArrayType = "System.Collections.Generic.List",
        CSharpGeneratorSettings =
        {
            GenerateJsonMethods = false,
            GenerateDataAnnotations = false
        }
    };

    var generator = new SwaggerToCSharpClientGenerator(document, settings);

    var code = generator.GenerateFile();

    code.Dump();

}

public class DoubleToDecimalVisitor : JsonSchemaVisitorBase
{
    protected override Task<JsonSchema4> VisitSchemaAsync(JsonSchema4 schema, string path, string typeNameHint)
    {       
        if (schema is SwaggerParameter || schema is JsonProperty)
        {
            if (schema.Type == JsonObjectType.Number)
            {
                schema.Format = JsonFormatStrings.Decimal;
            }
        }
        return Task.FromResult(schema);
    }
}
RicoSuter commented 5 years ago

Looks perfect (except an unused await SwaggerDocument.FromJsonAsync(jsonString);). When using the toolchain this way, you have also many more extension points available.

RicoSuter commented 5 years ago

As a noob i found a lot of confusion on when to use chunks of NJsonSchema vs when to use NSwag. Looks like NSwag is a fancy wrapper for NJsonSchema. And the example you provided used JsonSchemaVisitor but I found that I needed JsonSchemaVisitorBase instead.

A Swagger document mainly contains HTTP operation descriptions and DTO descriptions. The DTOs are described as some variant of JSON Schema. All JSON Schema/DTO handling (from .NET and to C#/TypeScript) is implemented in NJS and only the Swagger specific stuff in NSwag. So a lot of low level stuff (e.g. $ref handling, etc.) is handled in NJS because it is not Swagger but more JSON (Schema) specific.

Anyways, since I am now having to drop down low level and use a visitor, I also can no longer use the GUI which sucks.

Yes, for the Swagger generators it is already possible to load such additional extensions in NSwagStudio/CLI (like plugins). This is not yet possible with the C#/TypeScript generator commands and you can always easily build your own CLI.

VictorioBerra commented 5 years ago

Thanks for taking the time to help me with this.

RicoSuter commented 5 years ago

Sure, you're welcome.

I'd really like some concept of preprocessors/transformers which can be added to the pipeline (and e.g. loaded from a DLL or e.g. yours prepackaged). This one is also a possible candidate: https://github.com/RSuter/NJsonSchema/issues/17

adamjones1 commented 5 years ago

Given that numbers are already converted to a decimal string format when serialising them as JSON across the wire, it would be preferable to have decimal as the default real number type in clients (or at the very least this should be easily configurable without custom code).

That is, although the spec format name is "double", in JSON or XML this necessarily means "decimal approximation of double", so by converting on the client side back to double you're only approximating an approximation. Having only one level of approximation, taking the decimal conversion, would be better. At the moment I'm using just decimals on the server side anyway (no approximation) and I'm having to add in a custom pre-processor in every NSwag client to avoid the imprecision.

RicoSuter commented 5 years ago

If you have decimals in your server models and generate schemas from it with nswag/njs they are described as number+decimal and generated as decimals in the code generator

VictorioBerra commented 5 years ago

@RicoSuter I think he means when creating a Client from an existing Swagger document that describes the number type as number only (no +decimal).

adamjones1 commented 5 years ago

Yes sorry this is given existing server-side specs which aren't generated by NSwag. Did you say NSwag can generate specs with a 'number' type and 'decimal' format? I wasn't aware that was a supported format in OpenApi?

Also just to clarify on my previous comment, this only applies to media formats where number types are necessarily decimal, eg. JSON and XML. So to refine the proposal, it would be good if there was either an option for NSwag to infer the number type by the media type (something like UseMediaNativeNumberType), or a global override option for the number type like the primitive types above. (The latter would be less preferable though as then services which can return either JSON or a media format supporting native doubles would have one coerced to the wrong type.)

drdamour commented 3 years ago

i don't think open api defines a decimal format https://swagger.io/docs/specification/data-models/data-types/#numbers

it does seem like a prudent thing to turn open-api number double formats into decimal type in .net as needed

renenucci commented 3 years ago

i don't think open api defines a decimal format https://swagger.io/docs/specification/data-models/data-types/#numbers

it does seem like a prudent thing to turn open-api number double formats into decimal type in .net as needed

I Agree.

There's most common use to map double to decimal in .Net.

drdamour commented 3 years ago

i don't think open api defines a decimal format https://swagger.io/docs/specification/data-models/data-types/#numbers it does seem like a prudent thing to turn open-api number double formats into decimal type in .net as needed

I Agree.

There's most common use to map double to decimal in .Net.

But by that point precision can already be lost.

renenucci commented 3 years ago

i don't think open api defines a decimal format https://swagger.io/docs/specification/data-models/data-types/#numbers it does seem like a prudent thing to turn open-api number double formats into decimal type in .net as needed

I Agree. There's most common use to map double to decimal in .Net.

But by that point precision can already be lost.

Sorry but, why? If I have a decimal request/response, it will not be lost in request/response's .NET pipeline. Am I missing some point here?

The point is: I have an Request with require a decimal input, wich open API translate as double in swagger.json, but, when nSwag generates the c# client's code, it put as double, not decimal, and in that way, yes, I'll loose precision because I'm handling as double until TCP transport.

So, how can It already be lost in that case?

drdamour commented 3 years ago

i don't think open api defines a decimal format https://swagger.io/docs/specification/data-models/data-types/#numbers it does seem like a prudent thing to turn open-api number double formats into decimal type in .net as needed

I Agree. There's most common use to map double to decimal in .Net.

But by that point precision can already be lost.

Sorry but, why? If I have a decimal request/response, it will not be lost in request/response's .NET pipeline. Am I missing some point here?

The point is: I have an Request with require a decimal input, wich open API translate as double in swagger.json, but, when nSwag generates the c# client's code, it put as double, not decimal, and in that way, yes, I'll loose precision because I'm handling as double until TCP transport.

So, how can It already be lost in that case?

on read from wire

more-urgent-jest commented 2 years ago

I am using stoplight studio to create the spec for a new API and if I edit the json to define a property as:

          "amount": {
            "type": "number+decimal"
          },

then NSwagStudio v13.16.1.0 generates:

        public object Amount { get; set; }

unless you mean:

          "amount": {
            "type": "number",
            "format": "decimal"
          },

then NSwagStudio v13.16.1.0 generates:

        public decimal Amount { get; set; }
sdktraceur commented 1 year ago

Please check your swagger document, I had the same problem,

"qty": { "type": "number", "format": "double", "nullable": true },

I am using .NET, so I just configured Swagger generator:

builder.Services.AddSwaggerGen(options => { options.MapType<decimal>(() => new OpenApiSchema { Type = "number", Format = "decimal" }); });

and then output swagger document was like:

"qty": { "type": "number", "format": "decimal", "nullable": true }, so NSwag was able to generate proper type:

[System.Text.Json.Serialization.JsonIgnore(Condition = System.Text.Json.Serialization.JsonIgnoreCondition.WhenWritingDefault)] public decimal? Qty { get; set; }

adamjones2 commented 10 months ago

Has there been any progress with this issue yet? The above ("format": "decimal") isn't an acceptable solution because in general clients don't have control of the OpenAPI spec they're consuming. For the overwhelmingly common case of an external API which is agnostic to NSwag (and thus doesn't use NSwag's own special "decimal" number format) eg. if it's not implemented in .Net, and which uses JSON transport, there has to be a way to generate a client with number types that match their actual format in the transport - decimals. Without going through all the faff of building a bespoke NSwag CLI. IMO we need something like a UseMediaNativeNumberType option.

(To clarify by the way I'm @adamjones1 above with a new account.)