search

launchdarkly / dotnet-sdk-common

Code shared between dotnet-server-sdk and dotnet-client-sdk
Other
3 stars 10 forks source link

The `LdValue.Convert.Long` converter returns `LdValue.AsInt` instead of `LdValue.AsLong` #32

Closed irwinb closed 4 years ago

irwinb commented 5 years ago

Is this a support request? No.

Describe the bug The LdValue.Convert.Long converter returns LdValue.AsInt instead of LdValue.AsLong.

https://github.com/launchdarkly/dotnet-sdk-common/blob/e37d5cb2e52fc5875cb56dceb06433454e457331/src/LaunchDarkly.CommonSdk/Public/LdValue.cs#L958

To reproduce Initialize an LdValue with a long list and get it back.

var val = new List<long> {long.MinValue, 10, 100, long.MaxValue};
var ldValue = LdValue.Of(val );

// This fails.
Assert.True(val, ldValue.AsLongList());

Expected behavior I expect a list with the correct values to be returned.

SDK version 4.0.1

irwinb commented 5 years ago

I'm realizing that the test I posted might not be valid. What is the maximum numeric value LaunchDarkly supports?

eli-darkly commented 4 years ago

  1. Thanks for spotting that error. Indeed, it should be using AsLong there.

  2. That's a fair question, but the answer isn't well defined. All values in LaunchDarkly flag rules and user properties are encoded as JSON-- in communication between the service and the SDKs, and also in how the service stores them internally. The JSON standard does not define any range or precision for numbers; in practice, most languages (including Go, which is what the LD service runs on) will represent them using the largest available floating-point type (which in Go is float64). Floating-point numbers do not have a maximum or minimum value, just a maximum precision.

Since the maximum floating-point precision is somewhat different from one language to another, it's not recommended to rely on being able to encode as many digits as a Go float64 can handle; for very large numbers, it'd be best to encode them as strings (assuming you don't need to use numeric comparison operators for them in a flag rule). Therefore, while the long conversion is provided here for convenience in situations where that is the type you have, its existence is somewhat misleading and it should probably have a warning that long values are not guaranteed to be encodable in JSON.

  1. I'm not entirely sure what that test code is supposed to be doing. It doesn't compile, since there is no overload of LdValue.Of() that takes List<long>, nor is there an AsLongList() method of LdValue.
eli-darkly commented 4 years ago

The fix for this is now in .NET SDK 5.9.0 and Xamarin SDK 1.1.0.