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

Deserialize numbers from string #74

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Jan 4, 2024

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

mstyura commented Jan 11, 2024

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

Copy link
Contributor

@daveleek daveleek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! thank you for your contribution!

@daveleek daveleek merged commit a8a45f7 into Unleash:main Jan 12, 2024
9 checks passed
@sighphyre
Copy link
Member

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?

  • Is this an OSS/Pro/Enterprise instance?
  • How long have you been running your Unleash instance?
  • What version are you on?
  • Do you have custom changes built on top of the OSS or enterprise version?
  • Are you using Gitlab flags?

@mstyura
Copy link
Contributor Author

mstyura commented Jan 17, 2024

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

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants