moving user ID back to an int

[myleshorton]

No description provided.

[myleshorton]

I switched this to cast it to an int @xiam, as using float64 made the tests fail. What was the thought behind float64 versus int?

[myleshorton]

If we really want it to be a float64, I think we need it to be float64 everywhere.

[xiam]

Does that casting actually work if we send a number there? I can't check
right now but, as far as I know, since data is a map of interface{} values
the value will be unmarshaled into a float64, and we need to convert that
to int.
On Thu, May 19, 2016 at 20:56 myleshorton notifications@github.com wrote:

In src/github.com/getlantern/flashlight/app/settings.go
#4345 (comment):

@@ -145,6 +145,14 @@ func (s *Settings) checkBool(data map[string]interface{}, name string, f func(bo
}
}

+func (s *Settings) checkInt(data map[string]interface{}, name string, f func(int)) {

• if v, ok := data[name].(int); ok {

I switched this to cast it to an int @xiam https://github.com/xiam, as
using float64 made the tests fail. What was the thought behind float64
versus int?

—
You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
https://github.com/getlantern/lantern/pull/4345/files/17346b9c1bbc898f0a06aeeb0bd168ed5fe363c6#r63981775

[myleshorton]

Definitely works in the tests while float64 doesn't (unless of course we actually put a float64 in the map) -- just depends what we put in. AFAIK the value will just be unmarshalled into whatever it actually is.

[myleshorton]

If the server is always using 64 bit ints, though, I believe we'll have a problem on 32 bit systems and should probably just use strings all the way through. What's the server using exactly @uaalto?

[xiam]

Do these tests consider that this is a JSON message unmarshalled into a map[string]interface{}?

lantern/src/github.com/getlantern/flashlight/app/settings.go

Line 122 in 214e282

var data map[string]interface{}

My point is that when we unmarshal a JSON Number into an interface{}, the Number becomes a float64: https://play.golang.org/p/NiHuolVKvo

Hence the type check and the subsequent cast to int.

[xiam]

Here's another reference: https://blog.golang.org/json-and-go, please see "Generic JSON with interface{}"

[uaalto]

In the server:

• The code deals with numbers as 64 bits integers.
• Redis handles everything internally as strings, but the numbers are always parsed when crossing the Lua boundary so they are properly typed.

[coveralls]

Coverage increased (+0.07%) to 71.92% when pulling 56f08c4 on user-id-back-to-int into 5218d29 on devel.

[myleshorton]

OK I just added a more explicit test for checkInt @xiam

[coveralls]

Coverage increased (+0.08%) to 71.938% when pulling 2f525b7 on user-id-back-to-int into 5218d29 on devel.

[coveralls]

Coverage increased (+0.2%) to 72.073% when pulling 214e282 on user-id-back-to-int into 5218d29 on devel.

[myleshorton]

OK @xiam good catch on this. I just changed everything to use float64 and changed the test to run with raw JSON, not a pre-generated map. We need to just make user ID a float64 and not use int because otherwise things would break on 32 bit machines AFAICT.

[myleshorton]

I must say though this all makes the string solution seem mighty appealing!

[xiam]

I think this is pretty confusing, tbh. The initial problem was that Go casts a JSON Number into a float64 (which is natural, because JavaScript does not have integers at all), and that situation was catched and taken care of by the type check (which seemed to me like a simple standard solution). Now we would have to remember that UserID is a float64 which is not really a float64 because it gets truncated into an int before sending (opening the possibility for two equal IDs with different decimal places). I mean it could work like that, but was this an actual problem? I think we could try int64 instead of int to ensure the size of the integer. But I don't understand the advantage of carrying float64 and making code confusing.

[myleshorton]

Why is float64 more confusing than int64 @xiam?

[myleshorton]

UserID is a float64 which is not really a float64 because it gets truncated into an int before sending

I don't understand what you're saying here. Where does it get truncated to an int?

[myleshorton]

Remember the biggest problem with the previous code was that it would break on 32 bit systems.

[coveralls]

Coverage increased (+0.2%) to 72.042% when pulling f50fce3 on user-id-back-to-int into 5218d29 on devel.

[xiam]

I don't understand what you're saying here. Where does it get truncated to an int?

No, you're right. We aren't, I assumed we were because an int is what we're supposed to send in the userIDHeader, but we aren't even truncating that. That should have zero effect in the server side but not sure of why we would send fractional parts if they exists, tho.

I think it's confusing because we're supposed to be using integers to represents IDs (because that's what the pro server uses), and we were at one type cast away from that, Go uses float64 to represent JSON numbers which we can convert it to int or int64 and that's all. Go has different types that ensure safety like int64, which is also safe in 32 bit systems, so to me this decision is pretty odd and does not feel necessary. But I'm fine either way as long as it works, I think we should just add a note explaining why are we using float64 as IDs, when a float64 type is clearly not suitable to hold unique values. Besides, we'll have time to revisit this at any point later if required.

[xiam]

This could send a formatted float like 12.34, which is not expected by the server, the only thing that forbids that from happening is the program's logic.

[myleshorton]

Right -- that would only happen if actually sent 12.14 somewhere as the user ID, which would cause all sorts of other problems @xiam. It would fail for invalid user IDs.

[myleshorton]

Go uses float64 gut we can convert it to int or int64 and that's all

int won't work on 32 bit systems, so it would have to be int64, but the conversion from float64 to int64 is also not a simple matter of casting. See, for example:

https://play.golang.org/p/p-BZc09w8t

when a float64 type is clearly not suitable to hold unique values

Huh? How so?

[myleshorton]

To me the solutions are either:

1. Use float64 like we are
2. Use a string
3. Convert float64 to int64

TBH I think #2 is the best, followed by #1 and then #3.

[myleshorton]

I'll add a forth option:

4. Just create our struct from the JSON and make userID int64 and hope Go does the right thing underneath!

[myleshorton]

Oh wow -- looks like we can just use UseNumber - https://golang.org/pkg/encoding/json/#Decoder.UseNumber

[xiam]

Huh? How so?

As far as I know, floating point numbers are approximates and can have different representations depending on the context, which is not a Go issue, see for instance: https://play.golang.org/p/ItmlnYAX3o or https://play.golang.org/p/qO26NnY2kj, but that was just a side note on why I think floating point values are not suitable for being IDs in the most general case, I don't believe that is really relevant to this discussion because in this particular case, that does not happen: the server never sends numbers with decimal parts and we aren't doing any arithmetic or comparisons.

https://play.golang.org/p/p-BZc09w8t

I see your point, we should take care with overflows when using floats or ints:

fmt.Println(strconv.FormatFloat(math.MaxInt64, 'f', -1, 64))

I agree using strings would be better and safer, what would be the cost of doing such change @uaalto? If you could change the userId field into a string, that would be a lot better, but I am not aware of the implications in your side.

Oh wow -- looks like we can just use UseNumber -

Gonna take a look, sounds pretty interesting

[xiam]

Using strings to represent IDs would also be future proof. We can start using letters as well as digits if we need them, no overflows, etc.

[xiam]

Btw, this does not mean we should jump and convert everything into strings and use letters in our IDs right now, the internal pro-server internals could continue using whatever it currently does. If the userID field from the JSON response you send could be a string instead of a number we could do a few fixes in our side and we would have less things (conversions, overflows, 32 bit systems, etc) to worry about.

[myleshorton]

Ha yeah I agree @xiam it seems like strings are the sanest approach overall. That said, I just went ahead and used the UseNumber approach that basically uses a json.Number (really just a string!) underneath when decoding the JSON, but then lets you convert it however you want. This now just calls Int64() on that.

The other option would be to just use strings of course, but I feel like this is pretty clean too. What do you think?

[coveralls]

Coverage increased (+0.1%) to 71.99% when pulling 4b401ed on user-id-back-to-int into 5218d29 on devel.

[xiam]

This is an awesome and clean solution @myleshorton! Thanks for looking more into it. Good finding on UseNumber too. I'd just wait for the tests to finish and I'd merge it right away :)

[myleshorton]

Oh sweet thanks @xiam! Thanks for the thorough review!

[myleshorton]

OK these finally look ok @xiam