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

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

Closed
VictorioBerra opened this issue Dec 11, 2018 · 26 comments
Closed

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

VictorioBerra opened this issue Dec 11, 2018 · 26 comments

Comments

@VictorioBerra
Copy link

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
Copy link
Owner

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

@VictorioBerra
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

VictorioBerra commented Dec 19, 2018

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
Copy link
Owner

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
Copy link
Owner

RicoSuter commented Dec 19, 2018

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

@VictorioBerra
Copy link
Author

@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
Copy link
Author

@RicoSuter
Copy link
Owner

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

@VictorioBerra
Copy link
Author

VictorioBerra commented Dec 19, 2018

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
Copy link
Owner

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

@RicoSuter
Copy link
Owner

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
Copy link
Author

Thanks for taking the time to help me with this.

@RicoSuter
Copy link
Owner

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: RicoSuter/NJsonSchema#17

@adamjones1
Copy link

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
Copy link
Owner

RicoSuter commented Jun 29, 2019

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
Copy link
Author

@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
Copy link

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
Copy link

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
Copy link

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
Copy link

drdamour commented Sep 6, 2021

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
Copy link

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
Copy link

drdamour commented Sep 6, 2021

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
Copy link

more-urgent-jest commented Aug 4, 2022

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
Copy link

sdktraceur commented Apr 30, 2023

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
Copy link

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.)

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

8 participants