Go OpenStack Exchange

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

  1. fix-nil-string-unmarshalling
  2. Merge into trunk
Proposed by Ian Booth
Status: Merged
Approved by: Ian Booth
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 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.
Revision history for this message
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

Revision history for this message
Andrew Wilkins (axwalk) wrote :

LGTM

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'nova/json.go'
--- nova/json.go 2014-05-16 06:16:12 +0000
+++ nova/json.go 2014-06-13 06:42:21 +0000
@@ -36,22 +36,38 @@
36 return result36 return result
37}37}
3838
39// getIdAsStringPtr extracts the field with the specified tag from the json data
40// and returns it converted to a string pointer.
41func getIdAsStringPtr(b []byte, tag string) (*string, error) {
42 var out map[string]interface{}
43 if err := json.Unmarshal(b, &out); err != nil {
44 return nil, err
45 }
46 val, ok := out[tag]
47 if !ok || val == nil {
48 return nil, nil
49 }
50 floatVal, ok := val.(float64)
51 var strVal string
52 if ok {
53 strVal = fmt.Sprint(int(floatVal))
54 } else {
55 strVal = fmt.Sprint(val)
56 }
57 return &strVal, nil
58}
59
39// getIdAsString extracts the field with the specified tag from the json data60// getIdAsString extracts the field with the specified tag from the json data
40// and returns it converted to a string.61// and returns it converted to a string.
41func getIdAsString(b []byte, tag string) (string, error) {62func getIdAsString(b []byte, tag string) (string, error) {
42 var out map[string]interface{}63 strPtr, err := getIdAsStringPtr(b, tag)
43 if err := json.Unmarshal(b, &out); err != nil {64 if err != nil {
44 return "", err65 return "", err
45 }66 }
46 val, ok := out[tag]67 if strPtr == nil {
47 if !ok {
48 return "", nil68 return "", nil
49 }69 }
50 floatVal, ok := val.(float64)70 return *strPtr, nil
51 if ok {
52 return fmt.Sprint(int(floatVal)), nil
53 }
54 return fmt.Sprint(val), nil
55}71}
5672
57// appendJSON marshals the given attribute value and appends it as an encoded value to the given json data.73// appendJSON marshals the given attribute value and appends it as an encoded value to the given json data.
@@ -149,11 +165,10 @@
149 if err = json.Unmarshal(b, &jfip); err != nil {165 if err = json.Unmarshal(b, &jfip); err != nil {
150 return err166 return err
151 }167 }
152 if instIdStr, err := getIdAsString(b, instanceIdTag); err != nil {168 if instIdPtr, err := getIdAsStringPtr(b, instanceIdTag); err != nil {
153 return err169 return err
154 } else if instIdStr != "" {170 } else {
155 strId := instIdStr171 jfip.InstanceId = instIdPtr
156 jfip.InstanceId = &strId
157 }172 }
158 if jfip.Id, err = getIdAsString(b, idTag); err != nil {173 if jfip.Id, err = getIdAsString(b, idTag); err != nil {
159 return err174 return err
@@ -252,11 +267,10 @@
252 if jri.ParentGroupId, err = getIdAsString(b, parentGroupIdTag); err != nil {267 if jri.ParentGroupId, err = getIdAsString(b, parentGroupIdTag); err != nil {
253 return err268 return err
254 }269 }
255 if groupId, err := getIdAsString(b, groupIdTag); err != nil {270 if groupIdPtr, err := getIdAsStringPtr(b, groupIdTag); err != nil {
256 return err271 return err
257 } else if groupId != "" {272 } else {
258 strId := groupId273 jri.GroupId = groupIdPtr
259 jri.GroupId = &strId
260 }274 }
261 *ruleInfo = RuleInfo(jri)275 *ruleInfo = RuleInfo(jri)
262 return nil276 return nil
263277
=== modified file 'nova/json_test.go'
--- nova/json_test.go 2013-06-22 11:39:35 +0000
+++ nova/json_test.go 2014-06-13 06:42:21 +0000
@@ -2,47 +2,43 @@
22
3import (3import (
4 "encoding/json"4 "encoding/json"
5 . "launchpad.net/gocheck"5 gc "launchpad.net/gocheck"
6 "launchpad.net/goose/nova"6 "launchpad.net/goose/nova"
7)7)
88
9type JsonSuite struct {9type JsonSuite struct {
10}10}
1111
12var _ = Suite(&JsonSuite{})12var _ = gc.Suite(&JsonSuite{})
1313
14func (s *JsonSuite) SetUpSuite(c *C) {14func (s *JsonSuite) SetUpSuite(c *gc.C) {
15 nova.UseNumericIds(true)15 nova.UseNumericIds(true)
16}16}
1717
18func (s *JsonSuite) assertMarshallRoundtrip(c *C, value interface{}, unmarshalled interface{}) {18func (s *JsonSuite) assertMarshallRoundtrip(c *gc.C, value interface{}, unmarshalled interface{}) {
19 data, err := json.Marshal(value)19 data, err := json.Marshal(value)
20 if err != nil {20 c.Assert(err, gc.IsNil)
21 panic(err)
22 }
23 err = json.Unmarshal(data, &unmarshalled)21 err = json.Unmarshal(data, &unmarshalled)
24 if err != nil {22 c.Assert(err, gc.IsNil)
25 panic(err)23 c.Assert(unmarshalled, gc.DeepEquals, value)
26 }
27 c.Assert(unmarshalled, DeepEquals, value)
28}24}
2925
30// The following tests all check that unmarshalling of Ids with values > 1E626// The following tests all check that unmarshalling of Ids with values > 1E6
31// works properly.27// works properly.
3228
33func (s *JsonSuite) TestMarshallEntityLargeIntId(c *C) {29func (s *JsonSuite) TestMarshallEntityLargeIntId(c *gc.C) {
34 entity := nova.Entity{Id: "2000000", Name: "test"}30 entity := nova.Entity{Id: "2000000", Name: "test"}
35 var unmarshalled nova.Entity31 var unmarshalled nova.Entity
36 s.assertMarshallRoundtrip(c, &entity, &unmarshalled)32 s.assertMarshallRoundtrip(c, &entity, &unmarshalled)
37}33}
3834
39func (s *JsonSuite) TestMarshallFlavorDetailLargeIntId(c *C) {35func (s *JsonSuite) TestMarshallFlavorDetailLargeIntId(c *gc.C) {
40 fd := nova.FlavorDetail{Id: "2000000", Name: "test"}36 fd := nova.FlavorDetail{Id: "2000000", Name: "test"}
41 var unmarshalled nova.FlavorDetail37 var unmarshalled nova.FlavorDetail
42 s.assertMarshallRoundtrip(c, &fd, &unmarshalled)38 s.assertMarshallRoundtrip(c, &fd, &unmarshalled)
43}39}
4440
45func (s *JsonSuite) TestMarshallServerDetailLargeIntId(c *C) {41func (s *JsonSuite) TestMarshallServerDetailLargeIntId(c *gc.C) {
46 fd := nova.Entity{Id: "2000000", Name: "test"}42 fd := nova.Entity{Id: "2000000", Name: "test"}
47 im := nova.Entity{Id: "2000000", Name: "test"}43 im := nova.Entity{Id: "2000000", Name: "test"}
48 sd := nova.ServerDetail{Id: "2000000", Name: "test", Flavor: fd, Image: im}44 sd := nova.ServerDetail{Id: "2000000", Name: "test", Flavor: fd, Image: im}
@@ -50,9 +46,36 @@
50 s.assertMarshallRoundtrip(c, &sd, &unmarshalled)46 s.assertMarshallRoundtrip(c, &sd, &unmarshalled)
51}47}
5248
53func (s *JsonSuite) TestMarshallFloatingIPLargeIntId(c *C) {49func (s *JsonSuite) TestMarshallFloatingIPLargeIntId(c *gc.C) {
54 id := "3000000"50 id := "3000000"
55 fip := nova.FloatingIP{Id: "2000000", InstanceId: &id}51 fip := nova.FloatingIP{Id: "2000000", InstanceId: &id}
56 var unmarshalled nova.FloatingIP52 var unmarshalled nova.FloatingIP
57 s.assertMarshallRoundtrip(c, &fip, &unmarshalled)53 s.assertMarshallRoundtrip(c, &fip, &unmarshalled)
58}54}
55
56func (s *JsonSuite) TestUnmarshallFloatingIPNilStrings(c *gc.C) {
57 var fip nova.FloatingIP
58 data := []byte(`{"instance_id": null, "ip": "10.1.1.1", "fixed_ip": null, "id": "12345", "pool": "Ext-Net"}`)
59 err := json.Unmarshal(data, &fip)
60 c.Assert(err, gc.IsNil)
61 expected := nova.FloatingIP{
62 Id: "12345",
63 IP: "10.1.1.1",
64 Pool: "Ext-Net",
65 FixedIP: nil,
66 InstanceId: nil,
67 }
68 c.Assert(fip, gc.DeepEquals, expected)
69}
70
71func (s *JsonSuite) TestUnmarshallRuleInfoNilStrings(c *gc.C) {
72 var ri nova.RuleInfo
73 data := []byte(`{"group_id": null, "parent_group_id": "12345"}`)
74 err := json.Unmarshal(data, &ri)
75 c.Assert(err, gc.IsNil)
76 expected := nova.RuleInfo{
77 GroupId: nil,
78 ParentGroupId: "12345",
79 }
80 c.Assert(ri, gc.DeepEquals, expected)
81}

Subscribers

People subscribed via source and target branches