UPSTREAM: 16964: Preserve int64 data when unmarshaling#5774
UPSTREAM: 16964: Preserve int64 data when unmarshaling#5774openshift-bot merged 2 commits intoopenshift:masterfrom liggitt:json_decode
Conversation
|
[test] |
There was a problem hiding this comment.
Extract method for handling interface{} and call in both convertMapNumbers and convertSliceNumbers?
liggitt
changed the title
|
[test] |
liggitt
changed the title
|
@smarterclayton @pmorie @pweil- PTAL. I think you will be pleasantly surprised at how contained the changes to |
|
Evaluated for origin test up to 2ed17a9 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6982/) |
There was a problem hiding this comment.
This is ok for the unstructured path, but we should note this path is much slower than structured decoding. Don't want someone to switch to this path without knowing the trade off (lots more allocations).
There was a problem hiding this comment.
when you do structured decoding, json looks at the types of the destination fields, so you don't have the float issue
There was a problem hiding this comment.
That's what I mean - I don't want someone switching to this accidentally
On Nov 7, 2015, at 9:56 AM, Jordan Liggitt notifications@github.com wrote:
In Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/json/json.go
#5774 (comment):
+package json
+
+import (
- "bytes"
- "encoding/json"
+)
+
+// Marshal delegates to json.Marshal
+// It is only here so this package can be a drop-in for common encoding/json uses
+func Marshal(v interface{}) ([]byte, error) {- return json.Marshal(v)
+}
+
+// Unmarshal unmarshals the given data
+// If v is a *map[string]interface{}, numbers are converted to int64 or float64
+func Unmarshal(data []byte, v interface{}) error {
when you do structured decoding, json looks at the types of the destination
fields, so you don't have the float issue
—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5774/files#r44213680.
|
Excellent patch, good tests. One request for a comment on the method that you can do upstream. |
smarterclayton
added
priority/P1
approved
|
P1 and approved because this can result in wedged servers. [merge] and LGTM |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3962/) (Image: devenv-rhel7_2656) |
|
Evaluated for origin merge up to 2ed17a9 |
5 participants
Fixes #5557