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

Decode ID to int64 #74

Closed
mbn18 opened this issue Sep 6, 2017 · 7 comments
Closed

Decode ID to int64 #74

mbn18 opened this issue Sep 6, 2017 · 7 comments

Comments

@mbn18
Copy link

mbn18 commented Sep 6, 2017

Hi, this didnt worked for me. The ID was always 0

type FacebookPassport struct {
	ID    int64
	Name  string
}

var pass FacebookPassport
r.DecodeField("id", &pass.ID)

I had to revert to:

id := r.Get("id").(string)
nid, err := strconv.ParseInt(id, 10, 64)

Thanks

@huandu
Copy link
Owner

huandu commented Sep 8, 2017

Please provide the json string so that I can take a look at this specific case. Thanks.

@mbn18
Copy link
Author

mbn18 commented Sep 8, 2017

@huandu ,
Ran fmt.printf("%v", res)
Got map[name:Daniel Ben Nes id:287840725030539]

Not sure how to get the JSON itself.

@huandu
Copy link
Owner

huandu commented Sep 10, 2017

I see. It seems that id is a string instead of a number in JSON. Result#DecodeField doesn't parse string to number automatically. It's by design and safer. Such error is a way to let you know the JSON struct is not defined as what you think.

My recommendation is to decode id as string explicitly and call strconv.ParseInt. It's very similar to your code but a bit safer.

var id string
err := r.DecodeField("id", &id)
// TODO: remember to check err.
var pass FacebookPassport
pass.ID, err = strconv.ParseInt(id, 10, 64)

@mbn18
Copy link
Author

mbn18 commented Sep 12, 2017

Great, used it.

I do wonder if it will be beneficial to use switch like in the fmt package to convert the type.

@huandu
Copy link
Owner

huandu commented Sep 13, 2017

There are lots of switch..case in result.go. I choose not to automatically parse string to number just because I want to make such potential error explicit.

I know it's quite bothering if server returns a JSON in which number is encoded as string by mistake for some uncontrollable reason. I think it's reasonable to implement a new type like type fb.Number int64 and special decoding logic to parse number-like string automatically. I can do it in a week.

@Tri125
Copy link

Tri125 commented Sep 26, 2017

I would advice against converting the ID to int64. The documentation explicitly state that the ID is a type string with numeric content. It's not an encoding mistake by facebook, it's by design so that they can change the ID to what they want without breaking clients.

At the moment the content of the ID can be converted to a 64bit int but there's no guarantee that it will remain like that.

If a new graphAPI version change how ID are represented and your application still expect an int64, it will break.

I would argue that the introduction of a type fb.Number would encourage bad practice. If clients really want to treat them as int64, they can define their struct like so:

type FacebookPassport struct {
	ID   int64 `json:",string"`
	Name string
}

Playground example

@huandu
Copy link
Owner

huandu commented Sep 27, 2017

The type fb.Number or json tag works almost the same for developers who understand the risk and intentional convert id to int64. Considering the cost for these two options, it's much easier to implement fb.Number.

I can add some comments to refer to this discussion and state the risk clearly. It's up to developers to choose use it or not.

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