Merge lp:~wallyworld/goose/fix-nil-string-unmarshalling into lp:goose

Proposed by Ian Booth on 2014-06-13
Status: Merged
Approved by: Ian Booth on 2014-06-13
Approved revision: 126
Merged at revision: 126
Proposed branch: lp:~wallyworld/goose/fix-nil-string-unmarshalling
Merge into: lp:goose
Diff against target: 181 lines (+69/-32)
2 files modified
nova/json.go (+31/-17)
nova/json_test.go (+38/-15)
To merge this branch: bzr merge lp:~wallyworld/goose/fix-nil-string-unmarshalling
Reviewer Review Type Date Requested Status
Juju Engineering 2014-06-13 Pending
Review via email: mp+223017@code.launchpad.net

Commit message

Fix unmarshalling of nil strings

In some nova structs sent and received over the
wire, serialised as json, there are id attributes
which can be string or int. We have custom marshalling
to handle that, but where the attributes were string
pointers, we were always unmarshalling as "" if the
value was nil. This broke things like floating ip
address usage.

https://codereview.appspot.com/105180043/

Description of the change

Fix unmarshalling of nil strings

In some nova structs sent and received over the
wire, serialised as json, there are id attributes
which can be string or int. We have custom marshalling
to handle that, but where the attributes were string
pointers, we were always unmarshalling as "" if the
value was nil. This broke things like floating ip
address usage.

https://codereview.appspot.com/105180043/

To post a comment you must log in.
Ian Booth (wallyworld) wrote :

Reviewers: mp+223017_code.launchpad.net,

Message:
Please take a look.

Description:
Fix unmarshalling of nil strings

In some nova structs sent and received over the
wire, serialised as json, there are id attributes
which can be string or int. We have custom marshalling
to handle that, but where the attributes were string
pointers, we were always unmarshalling as "" if the
value was nil. This broke things like floating ip
address usage.

https://code.launchpad.net/~wallyworld/goose/fix-nil-string-unmarshalling/+merge/223017

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/105180043/

Affected files (+71, -32 lines):
   A [revision details]
   M nova/json.go
   M nova/json_test.go

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/json.go'
2--- nova/json.go 2014-05-16 06:16:12 +0000
3+++ nova/json.go 2014-06-13 06:42:21 +0000
4@@ -36,22 +36,38 @@
5 return result
6 }
7
8+// getIdAsStringPtr extracts the field with the specified tag from the json data
9+// and returns it converted to a string pointer.
10+func getIdAsStringPtr(b []byte, tag string) (*string, error) {
11+ var out map[string]interface{}
12+ if err := json.Unmarshal(b, &out); err != nil {
13+ return nil, err
14+ }
15+ val, ok := out[tag]
16+ if !ok || val == nil {
17+ return nil, nil
18+ }
19+ floatVal, ok := val.(float64)
20+ var strVal string
21+ if ok {
22+ strVal = fmt.Sprint(int(floatVal))
23+ } else {
24+ strVal = fmt.Sprint(val)
25+ }
26+ return &strVal, nil
27+}
28+
29 // getIdAsString extracts the field with the specified tag from the json data
30 // and returns it converted to a string.
31 func getIdAsString(b []byte, tag string) (string, error) {
32- var out map[string]interface{}
33- if err := json.Unmarshal(b, &out); err != nil {
34+ strPtr, err := getIdAsStringPtr(b, tag)
35+ if err != nil {
36 return "", err
37 }
38- val, ok := out[tag]
39- if !ok {
40+ if strPtr == nil {
41 return "", nil
42 }
43- floatVal, ok := val.(float64)
44- if ok {
45- return fmt.Sprint(int(floatVal)), nil
46- }
47- return fmt.Sprint(val), nil
48+ return *strPtr, nil
49 }
50
51 // appendJSON marshals the given attribute value and appends it as an encoded value to the given json data.
52@@ -149,11 +165,10 @@
53 if err = json.Unmarshal(b, &jfip); err != nil {
54 return err
55 }
56- if instIdStr, err := getIdAsString(b, instanceIdTag); err != nil {
57+ if instIdPtr, err := getIdAsStringPtr(b, instanceIdTag); err != nil {
58 return err
59- } else if instIdStr != "" {
60- strId := instIdStr
61- jfip.InstanceId = &strId
62+ } else {
63+ jfip.InstanceId = instIdPtr
64 }
65 if jfip.Id, err = getIdAsString(b, idTag); err != nil {
66 return err
67@@ -252,11 +267,10 @@
68 if jri.ParentGroupId, err = getIdAsString(b, parentGroupIdTag); err != nil {
69 return err
70 }
71- if groupId, err := getIdAsString(b, groupIdTag); err != nil {
72+ if groupIdPtr, err := getIdAsStringPtr(b, groupIdTag); err != nil {
73 return err
74- } else if groupId != "" {
75- strId := groupId
76- jri.GroupId = &strId
77+ } else {
78+ jri.GroupId = groupIdPtr
79 }
80 *ruleInfo = RuleInfo(jri)
81 return nil
82
83=== modified file 'nova/json_test.go'
84--- nova/json_test.go 2013-06-22 11:39:35 +0000
85+++ nova/json_test.go 2014-06-13 06:42:21 +0000
86@@ -2,47 +2,43 @@
87
88 import (
89 "encoding/json"
90- . "launchpad.net/gocheck"
91+ gc "launchpad.net/gocheck"
92 "launchpad.net/goose/nova"
93 )
94
95 type JsonSuite struct {
96 }
97
98-var _ = Suite(&JsonSuite{})
99+var _ = gc.Suite(&JsonSuite{})
100
101-func (s *JsonSuite) SetUpSuite(c *C) {
102+func (s *JsonSuite) SetUpSuite(c *gc.C) {
103 nova.UseNumericIds(true)
104 }
105
106-func (s *JsonSuite) assertMarshallRoundtrip(c *C, value interface{}, unmarshalled interface{}) {
107+func (s *JsonSuite) assertMarshallRoundtrip(c *gc.C, value interface{}, unmarshalled interface{}) {
108 data, err := json.Marshal(value)
109- if err != nil {
110- panic(err)
111- }
112+ c.Assert(err, gc.IsNil)
113 err = json.Unmarshal(data, &unmarshalled)
114- if err != nil {
115- panic(err)
116- }
117- c.Assert(unmarshalled, DeepEquals, value)
118+ c.Assert(err, gc.IsNil)
119+ c.Assert(unmarshalled, gc.DeepEquals, value)
120 }
121
122 // The following tests all check that unmarshalling of Ids with values > 1E6
123 // works properly.
124
125-func (s *JsonSuite) TestMarshallEntityLargeIntId(c *C) {
126+func (s *JsonSuite) TestMarshallEntityLargeIntId(c *gc.C) {
127 entity := nova.Entity{Id: "2000000", Name: "test"}
128 var unmarshalled nova.Entity
129 s.assertMarshallRoundtrip(c, &entity, &unmarshalled)
130 }
131
132-func (s *JsonSuite) TestMarshallFlavorDetailLargeIntId(c *C) {
133+func (s *JsonSuite) TestMarshallFlavorDetailLargeIntId(c *gc.C) {
134 fd := nova.FlavorDetail{Id: "2000000", Name: "test"}
135 var unmarshalled nova.FlavorDetail
136 s.assertMarshallRoundtrip(c, &fd, &unmarshalled)
137 }
138
139-func (s *JsonSuite) TestMarshallServerDetailLargeIntId(c *C) {
140+func (s *JsonSuite) TestMarshallServerDetailLargeIntId(c *gc.C) {
141 fd := nova.Entity{Id: "2000000", Name: "test"}
142 im := nova.Entity{Id: "2000000", Name: "test"}
143 sd := nova.ServerDetail{Id: "2000000", Name: "test", Flavor: fd, Image: im}
144@@ -50,9 +46,36 @@
145 s.assertMarshallRoundtrip(c, &sd, &unmarshalled)
146 }
147
148-func (s *JsonSuite) TestMarshallFloatingIPLargeIntId(c *C) {
149+func (s *JsonSuite) TestMarshallFloatingIPLargeIntId(c *gc.C) {
150 id := "3000000"
151 fip := nova.FloatingIP{Id: "2000000", InstanceId: &id}
152 var unmarshalled nova.FloatingIP
153 s.assertMarshallRoundtrip(c, &fip, &unmarshalled)
154 }
155+
156+func (s *JsonSuite) TestUnmarshallFloatingIPNilStrings(c *gc.C) {
157+ var fip nova.FloatingIP
158+ data := []byte(`{"instance_id": null, "ip": "10.1.1.1", "fixed_ip": null, "id": "12345", "pool": "Ext-Net"}`)
159+ err := json.Unmarshal(data, &fip)
160+ c.Assert(err, gc.IsNil)
161+ expected := nova.FloatingIP{
162+ Id: "12345",
163+ IP: "10.1.1.1",
164+ Pool: "Ext-Net",
165+ FixedIP: nil,
166+ InstanceId: nil,
167+ }
168+ c.Assert(fip, gc.DeepEquals, expected)
169+}
170+
171+func (s *JsonSuite) TestUnmarshallRuleInfoNilStrings(c *gc.C) {
172+ var ri nova.RuleInfo
173+ data := []byte(`{"group_id": null, "parent_group_id": "12345"}`)
174+ err := json.Unmarshal(data, &ri)
175+ c.Assert(err, gc.IsNil)
176+ expected := nova.RuleInfo{
177+ GroupId: nil,
178+ ParentGroupId: "12345",
179+ }
180+ c.Assert(ri, gc.DeepEquals, expected)
181+}

Subscribers

People subscribed via source and target branches