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

[BUG][Swift] Decimal-Decoding not working #13410

Closed
Feuerwerk opened this issue Sep 13, 2022 · 13 comments
Closed

[BUG][Swift] Decimal-Decoding not working #13410

Feuerwerk opened this issue Sep 13, 2022 · 13 comments

Comments

@Feuerwerk
Copy link
Contributor

Feuerwerk commented Sep 13, 2022

Description

I have a service providing a list of objects with a property of type string and format number (BigDecimal on the backend side). The generated service description is fine:

"minValue": {
  "type": "string",
  "format": "number"
}

The models generated by OpenAPI-Generator also looks good:

public var minValue: Decimal?

But when my swift5 client tries to consume this service I always get the exception that the returning data is not valid and can't be decoded.
I checked the data transmitted from the server and it looks valid to me.

"minValue": "0.12345"

When sending data from the swift client to the server, it will encode the decimal as a number

"value": 0.12345
openapi-generator version

6.1.0

Generation Details

openapi-generator generate -i http://localhost:8081/v3/api-docs -g swift5 -c config.json

{
	"unwrapRequired" : true,
	"projectName" : "ApiClient",
	"podAuthors" : "XXXXX",
	"podHomepage" : "XXXX",
	"podSummary" : "Summary"
}
Suggest a fix

I can fix the problem when I add the following four methods to the generated Extensions.swift

extension KeyedEncodingContainerProtocol {
    mutating func encode(_ value: Decimal, forKey key: Self.Key) throws {
        var mutableValue = value
        let stringValue = NSDecimalString(&mutableValue, Locale(identifier: "en_US"))
        try encode(stringValue, forKey: key)
    }
    
    mutating func encodeIfPresent(_ value: Decimal?, forKey key: Self.Key) throws {
        if let value = value {
            try encode(value, forKey: key)
        }
    }
}

extension KeyedDecodingContainerProtocol {
    func decode(_ type: Decimal.Type, forKey key: Self.Key) throws -> Decimal {
        let stringValue = try decode(String.self, forKey: key)
        guard let decimalValue = Decimal(string: stringValue) else {
            let context = DecodingError.Context(codingPath: [key], debugDescription: "The key \(key) couldn't be converted to a Decimal value")
            throw DecodingError.typeMismatch(type, context)
        }
        return decimalValue
    }
    
    func decodeIfPresent(_ type: Decimal.Type, forKey key: Self.Key) throws -> Decimal? {
        guard let stringValue = try decodeIfPresent(String.self, forKey: key) else {
            return nil
        }
        guard let decimalValue = Decimal(string: stringValue) else {
            let context = DecodingError.Context(codingPath: [key], debugDescription: "The key \(key) couldn't be converted to a Decimal value")
            throw DecodingError.typeMismatch(type, context)
        }
        return decimalValue
    }
}
@otusweb
Copy link

otusweb commented Sep 19, 2022

@Feuerwerk I'm using 6.0.0 and I'm not running into this issue. Did you try with that version?

@Feuerwerk
Copy link
Contributor Author

@otusweb I downgraded openapi-generator to 6.0.0 but got exactly the same problem. Add the 4 methods for encoding/decoding also solves the problem.

@Jonas1893
Copy link
Contributor

Jonas1893 commented Sep 29, 2022

@otusweb I downgraded openapi-generator to 6.0.0 but got exactly the same problem. Add the 4 methods for encoding/decoding also solves the problem.

Hi @Feuerwerk. Does this also work if there are attributes like this in the swagger?

"anotherValue": {
  "type": "number",
  "format": "double"
}

Because I guess with your change they would now also be decoded from a string (which it is not) and encoded to strings which will both fail

@otusweb
Copy link

otusweb commented Sep 29, 2022

@Feuerwerk
I've looked at my swagger file and I have :

        "Value": {
          "format": "decimal",   <=== important part here
          "description": "the value of the campaign.
Applies to percent, new price, discount campaign type. Other campaign type use valueX and ValueY directly on the campaign",
          "type": "string"
        }

I guess in the swagger file that you sent, there is no indication that you want a decimal, just that it should be a number.
Can you edit the service definition to see if that would fix it. On my team, i worked with the backend team to update the swagger file...

@Feuerwerk
Copy link
Contributor Author

Feuerwerk commented Sep 29, 2022

Hi @Jonas1893

Does this also work if there are attributes like this in the swagger?
Because I guess with your change they would now also be decoded from a string (which it is not) and encoded to strings which will both fail

Al least from my understanding of the OpenAPI Spec 3.0 type number / format double results in a Double datatype not in a Decimal datatype. I also tried it with openapi-generator (6.2.0) and the datamodel of my Swift client now has the following property:

public var value: Double?

And in this case the workaround (solution?) I suggested for Decimal would not interfere with Double.

@Feuerwerk
Copy link
Contributor Author

Hi @otusweb

        "Value": {
          "format": "decimal",   <=== important part here
          "description": "the value of the campaign.
Applies to percent, new price, discount campaign type. Other campaign type use valueX and ValueY directly on the campaign",
          "type": "string"
        }

I guess in the swagger file that you sent, there is no indication that you want a decimal, just that it should be a number. Can you edit the service definition to see if that would fix it. On my team, i worked with the backend team to update the swagger file...

Sadly "decimal" is not specified in OpenAPI Spec 3.0 but due to #7808 i thought type string / format number is the way to go.
So I changed my server side as you suggested to type string / format decimal and generated my Swift 5 client (now using OpenAPI Generator 6.2.0) but exactly the same code is generated on client side no additional support for decimal is generated. I also gave it a test and the same exception is thrown.

The Apples Swift JsonParser can't handle Decimal from a string without additional support I think.

@Jonas1893
Copy link
Contributor

Jonas1893 commented Sep 29, 2022

Hi @Jonas1893

Does this also work if there are attributes like this in the swagger?
Because I guess with your change they would now also be decoded from a string (which it is not) and encoded to strings which will both fail

Al least from my understanding of the OpenAPI Spec 3.0 type number / format double results in a Double datatype not in a Decimal datatype.

Exactly. In the example I described the json response from the server will not be

"minValue": "0.12345"

like in your example but

"anotherValue": 0.12345

And because of your extension decoder would now try to decode from a String, fail and throw the exception, no? Maybe I am overlooking something

@Feuerwerk
Copy link
Contributor Author

@Jonas1893 That are different datatypes (Double vs. Decimal) and they can / will be treated differently by Apples Json-Decoder / -Encoder.

The code I suggested only extend encoding / decoding for swift type Decimal. But if the data model contains the swift type Double (like in your example) the code I suggested simply will not run and therefore will not interfere and cause problems.

@Jonas1893
Copy link
Contributor

Understood, thx for clarifying

@Feuerwerk
Copy link
Contributor Author

Feuerwerk commented Sep 29, 2022

@otusweb

I'm pretty sure that @wing328 who added support for Decimal in Swift-Clients in OpenAPI Generator (type string / format number) did some testing so why didn't he get errors and you don't get errors too but I'm keep getting exceptions?
According to this discussion it seems Apple changed something in the Xcode SDK.
I'm currently using 14.0.1, what version are you using?

@otusweb
Copy link

otusweb commented Sep 30, 2022

@Feuerwerk On our end, here is the json spec for a decimal number:

        "PriceIncVat": {
          "format": "decimal",
          "type": "string"
        },

and the data returned by the server for that value:
"PriceIncVat": 1.0000

I'm running 6.0.0 and still on Xcode 13, but I'm assuming that the version of Xcode should not have much impact. Maybe iOS version. I've tested on iOS 15 and lower, but not 16 yet. I don't know how the internals work though, just that this combo works for us.
Can you get your server to return the decimal number without the quotes?

@Feuerwerk
Copy link
Contributor Author

@otusweb Ok, that will explain why it's working for you. You signal in your service description that the server will send a string that should be interpreted as a decimal. Therefore OpenAPI Generator will generate a data model with the property PriceIncVat of type Decimal. But your server will send the data actually as a number. On Swift-Client side the received data will be parsed by Apples JsonDecoder which has a default implementation for Decimal and expect a number in that case (not a string). It simply works by accident for you.
But you may run into a precision problem, because according to this the buildin JsonDecoder will decode value first as a double and then call Decimal(double: myValue) to convert it into a Decimal. And this could mean that you will loose precision. This is especially dangerous for currency values like in your case.

@otusweb
Copy link

otusweb commented Sep 30, 2022

@Feuerwerk thanks for pointing that out. Sounds like a larger issue for us then :-)

Feuerwerk added a commit to Feuerwerk/openapi-generator that referenced this issue Oct 4, 2022
4brunu pushed a commit that referenced this issue Oct 5, 2022
* [swift5] fixes bug #13410

* Fixed indentation, added missing generated samples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants