Unleash / unleash-client-rust

Unleash client SDK for Rust language projects
Apache License 2.0
23 stars 18 forks source link

Deserialize numbers from string #74

Closed mstyura closed 8 months ago

mstyura commented 8 months ago

About the changes

For unknown reason the Unleash server available to me for some of the variants among many return weight field as a string. This is allowed by java's Gson, and likely irrelevant for JS, so I've allowed serde to read numbers from string. Implementation is basically a copy of https://github.com/iddm/serde-aux/blob/3ebaa60d6e71b17fa818e0e5788aa4c771cd58e6/src/field_attributes.rs#L170 just not to introduce additional dependency, but could change to dependency instead of local copy.

Closes #None Relevant MR #73

Important files

Discussion points

mstyura commented 8 months ago

Hello @sjaanus! Could you please take a look at this PR? Thanks a lot in advance, Yury.

sighphyre commented 8 months ago

Hey @mstyura I'm so sorry to be a nuisance but I've reverted this. This isn't the correct thing to do here. Unleash absolutely should be sending this as numeric data types, not strings. I'd much rather tackle this problem at the source than patch an SDK to accept broken data. Would you mind sharing some more details about your Unleash installation so we can help debug your issue?

mstyura commented 8 months ago

Is this an OSS/Pro/Enterprise instance?

Self-managed instance of Unleash 4.22.8 (Open Source)

How long have you been running your Unleash instance?

Don't know for sure, but probably for several years.

What version are you on?

Unleash 4.22.8 (Open Source)

Do you have custom changes built on top of the OSS or enterprise version?

No

Are you using Gitlab flags?

No

I've dig a little bit deeper and found out that broken properties looks like:

{
    "name": "autotest_kseigoifpc",
    "weight": "0",
    "payload": {
        "type": "string",
        "value": "2"
    },
    "overrides": [
        {
            "values": [
                "13128454",
                "13128455",
                "13128456"
            ],
            "contextName": "userId"
        }
    ],
    "stickiness": "default",
    "weightType": "fix"
}

Which indicates that property was probably created by our automation tests by using unleash API. Maybe there is weak validation on API side.

sighphyre commented 7 months ago

@mstyura That's my gut feel as well, without more information here, that this was caused by an interaction between test code and poor validation on the Unleash side. I would generally recommend upgrading to latest if possible. There's been quite a lot of work on making the validation a lot more accurate since the 5.0.0 release