Merge lp:~hduran-8/goose/json_encoded_errors into lp:goose

Proposed by Horacio Durán on 2014-05-01
Status: Merged
Approved by: Horacio Durán on 2014-05-02
Approved revision: 128
Merged at revision: 122
Proposed branch: lp:~hduran-8/goose/json_encoded_errors
Merge into: lp:goose
Prerequisite: lp:~hduran-8/goose/testservice_errors
Diff against target: 188 lines (+88/-9)
6 files modified
http/client.go (+21/-5)
http/client_test.go (+41/-0)
testservices/errors.go (+12/-0)
testservices/novaservice/service.go (+8/-0)
testservices/novaservice/service_http.go (+5/-3)
testservices/novaservice/service_http_test.go (+1/-1)
To merge this branch: bzr merge lp:~hduran-8/goose/json_encoded_errors
Reviewer Review Type Date Requested Status
Juju Engineering 2014-05-01 Pending
Review via email: mp+217860@code.launchpad.net

Commit message

Added json encoded for ServerErrors

added jsonEncode function to novaservice that
tries to convert the given error into a
ServerError and call its AsJSON method or
wrap the given error into a 500 ServerError
which will contain the original error message.

https://codereview.appspot.com/97870044/

R=gz

Description of the change

Added json encoded for ServerErrors

added jsonEncode function to novaservice that
tries to convert the given error into a
ServerError and call its AsJSON method or
wrap the given error into a 500 ServerError
which will contain the original error message.

https://codereview.appspot.com/97870044/

To post a comment you must log in.
Horacio Durán (hduran-8) wrote :

Reviewers: mp+217860_code.launchpad.net,

Message:
Please take a look.

Description:
Added json encoded for ServerErrors

added jsonEncode function to novaservice that
tries to convert the given error into a
ServerError and call its AsJSON method or
wrap the given error into a 500 ServerError
which will contain the original error message.

https://code.launchpad.net/~hduran-8/goose/json_encoded_errors/+merge/217860

Requires:
https://code.launchpad.net/~hduran-8/goose/testservice_errors/+merge/217818

(do not edit description out of merge proposal)

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

Affected files (+18, -0 lines):
   A [revision details]
   M testservices/errors.go
   M testservices/novaservice/service.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: testservices/errors.go
=== modified file 'testservices/errors.go'
--- testservices/errors.go 2014-04-30 23:15:55 +0000
+++ testservices/errors.go 2014-05-01 00:43:30 +0000
@@ -27,6 +27,10 @@
   return &ServerError{code: code, message: fmt.Sprintf(message, args...)}
  }

+func (n *ServerError) AsJSON() string {
+ return fmt.Sprintf(`{%q:{"message":%q, "code":%d}}`, n.Name(), n.message,
n.code)
+}
+
  func (n *ServerError) Error() string {
   return fmt.Sprintf("%s: %s", n.Name(), n.message)
  }
@@ -39,6 +43,10 @@
   return name
  }

+func NewInternalServerError(message string) *ServerError {
+ return serverErrorf(500, message)
+}
+
  func NewNotFoundError(message string) *ServerError {
   return serverErrorf(404, message)
  }

Index: testservices/novaservice/service.go
=== modified file 'testservices/novaservice/service.go'
--- testservices/novaservice/service.go 2014-04-30 19:34:31 +0000
+++ testservices/novaservice/service.go 2014-05-01 00:43:30 +0000
@@ -33,6 +33,14 @@
   nextIPId int
  }

+func errorJSONEncode(err error) string {
+ serverError, ok := err.(*testservices.ServerError)
+ if !ok {
+ serverError = testservices.NewInternalServerError(err.Error())
+ }
+ return serverError.AsJSON()
+}
+
  // endpoint returns either a versioned or non-versioned service
  // endpoint URL from the given path.
  func (n *Nova) endpointURL(version bool, path string) string {

127. By Horacio Durán on 2014-05-02

Added proper handling of the json error on the client

Horacio Durán (hduran-8) wrote :
Martin Packman (gz) wrote :

LGTM.

https://codereview.appspot.com/97870044/diff/20001/http/client.go
File http/client.go (right):

https://codereview.appspot.com/97870044/diff/20001/http/client.go#newcode62
http/client.go:62: return nil, err
We seem to not be able to hit this error branch, that's fine.

https://codereview.appspot.com/97870044/diff/20001/http/client.go#newcode64
http/client.go:64: response.Title = key
Add break here. May as well exit early if we're passed something with
many top level keys.

https://codereview.appspot.com/97870044/diff/20001/http/client.go#newcode69
http/client.go:69: return nil, fmt.Errorf("Unexpected response format:
%q", jsonBytes)
"Unparsable json error body"

https://codereview.appspot.com/97870044/diff/20001/http/client_test.go
File http/client_test.go (right):

https://codereview.appspot.com/97870044/diff/20001/http/client_test.go#newcode191
http/client_test.go:191: c.Assert(unmarshalled.Message, Equals, "A
Meaningful error")
These attribute compares should be Check not Assert.

https://codereview.appspot.com/97870044/

128. By Horacio Durán on 2014-05-02

Changed error messages for more descriptive ones

Horacio Durán (hduran-8) wrote :

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'http/client.go'
2--- http/client.go 2014-04-23 06:57:55 +0000
3+++ http/client.go 2014-05-02 16:29:21 +0000
4@@ -51,8 +51,23 @@
5 return fmt.Sprintf("Failed: %d %s: %s", e.Code, e.Title, e.Message)
6 }
7
8-type ErrorWrapper struct {
9- Error ErrorResponse `json:"error"`
10+func unmarshallError(jsonBytes []byte) (*ErrorResponse, error) {
11+ var response ErrorResponse
12+ var transientObject = make(map[string]*json.RawMessage)
13+ if err := json.Unmarshal(jsonBytes, &transientObject); err != nil {
14+ return nil, err
15+ }
16+ for key, value := range transientObject {
17+ if err := json.Unmarshal(*value, &response); err != nil {
18+ return nil, err
19+ }
20+ response.Title = key
21+ break
22+ }
23+ if response.Code != 0 && response.Message != "" {
24+ return &response, nil
25+ }
26+ return nil, fmt.Errorf("Unparsable json error body: %q", jsonBytes)
27 }
28
29 type RequestData struct {
30@@ -287,9 +302,10 @@
31 errInfo := string(errBytes)
32 // Check if we have a JSON representation of the failure, if so decode it.
33 if resp.Header.Get("Content-Type") == contentTypeJSON {
34- var wrappedErr ErrorWrapper
35- if err := json.Unmarshal(errBytes, &wrappedErr); err == nil {
36- errInfo = wrappedErr.Error.Error()
37+ errorResponse, err := unmarshallError(errBytes)
38+ //TODO (hduran-8): Obtain a logger and log the error
39+ if err == nil {
40+ errInfo = errorResponse.Error()
41 }
42 }
43 httpError := &HttpError{
44
45=== modified file 'http/client_test.go'
46--- http/client_test.go 2013-10-17 23:02:16 +0000
47+++ http/client_test.go 2014-05-02 16:29:21 +0000
48@@ -181,3 +181,44 @@
49 c.Check(agent, Not(Equals), "")
50 c.Check(agent, Equals, gooseAgent())
51 }
52+
53+func (s *HTTPSClientTestSuite) TestProperlyFormattedJsonUnmarshalling(c *C) {
54+ validJSON := `{"itemNotFound": {"message": "A Meaningful error", "code": 404}}`
55+ unmarshalled, err := unmarshallError([]byte(validJSON))
56+ c.Assert(err, IsNil)
57+ c.Check(unmarshalled.Code, Equals, 404)
58+ c.Check(unmarshalled.Title, Equals, "itemNotFound")
59+ c.Check(unmarshalled.Message, Equals, "A Meaningful error")
60+}
61+
62+func (s *HTTPSClientTestSuite) TestImproperlyFormattedJSONUnmarshalling(c *C) {
63+ invalidJSON := `This string is not a valid JSON`
64+ unmarshalled, err := unmarshallError([]byte(invalidJSON))
65+ c.Assert(err, NotNil)
66+ c.Assert(unmarshalled, IsNil)
67+ c.Check(err, ErrorMatches, "invalid character 'T' looking for beginning of value")
68+}
69+
70+func (s *HTTPSClientTestSuite) TestJSONMissingCodeUnmarshalling(c *C) {
71+ missingCodeJSON := `{"itemNotFound": {"message": "A Meaningful error"}}`
72+ unmarshalled, err := unmarshallError([]byte(missingCodeJSON))
73+ c.Assert(err, NotNil)
74+ c.Assert(unmarshalled, IsNil)
75+ c.Check(err, ErrorMatches, `Unparsable json error body: "{\\"itemNotFound\\": {\\"message\\": \\"A Meaningful error\\"}}"`)
76+}
77+
78+func (s *HTTPSClientTestSuite) TestJSONMissingMessageUnmarshalling(c *C) {
79+ missingMessageJSON := `{"itemNotFound": {"code": 404}}`
80+ unmarshalled, err := unmarshallError([]byte(missingMessageJSON))
81+ c.Assert(err, NotNil)
82+ c.Assert(unmarshalled, IsNil)
83+ c.Check(err, ErrorMatches, `Unparsable json error body: "{\\"itemNotFound\\": {\\"code\\": 404}}"`)
84+}
85+
86+func (s *HTTPSClientTestSuite) TestBrokenBodyJSONUnmarshalling(c *C) {
87+ invalidBodyJSON := `{"itemNotFound": {}}`
88+ unmarshalled, err := unmarshallError([]byte(invalidBodyJSON))
89+ c.Assert(err, NotNil)
90+ c.Assert(unmarshalled, IsNil)
91+ c.Check(err, ErrorMatches, `Unparsable json error body: \"{\\\"itemNotFound\\\": {}}\"`)
92+}
93
94=== modified file 'testservices/errors.go'
95--- testservices/errors.go 2014-04-30 23:15:55 +0000
96+++ testservices/errors.go 2014-05-02 16:29:21 +0000
97@@ -27,6 +27,14 @@
98 return &ServerError{code: code, message: fmt.Sprintf(message, args...)}
99 }
100
101+func (n *ServerError) Code() int {
102+ return n.code
103+}
104+
105+func (n *ServerError) AsJSON() string {
106+ return fmt.Sprintf(`{%q:{"message":%q, "code":%d}}`, n.Name(), n.message, n.code)
107+}
108+
109 func (n *ServerError) Error() string {
110 return fmt.Sprintf("%s: %s", n.Name(), n.message)
111 }
112@@ -39,6 +47,10 @@
113 return name
114 }
115
116+func NewInternalServerError(message string) *ServerError {
117+ return serverErrorf(500, message)
118+}
119+
120 func NewNotFoundError(message string) *ServerError {
121 return serverErrorf(404, message)
122 }
123
124=== modified file 'testservices/novaservice/service.go'
125--- testservices/novaservice/service.go 2014-04-30 19:34:31 +0000
126+++ testservices/novaservice/service.go 2014-05-02 16:29:21 +0000
127@@ -33,6 +33,14 @@
128 nextIPId int
129 }
130
131+func errorJSONEncode(err error) (int, string) {
132+ serverError, ok := err.(*testservices.ServerError)
133+ if !ok {
134+ serverError = testservices.NewInternalServerError(err.Error())
135+ }
136+ return serverError.Code(), serverError.AsJSON()
137+}
138+
139 // endpoint returns either a versioned or non-versioned service
140 // endpoint URL from the given path.
141 func (n *Nova) endpointURL(version bool, path string) string {
142
143=== modified file 'testservices/novaservice/service_http.go'
144--- testservices/novaservice/service_http.go 2014-01-24 16:33:31 +0000
145+++ testservices/novaservice/service_http.go 2014-05-02 16:29:21 +0000
146@@ -297,6 +297,7 @@
147 return
148 }
149 var resp http.Handler
150+
151 if err == testservices.RateLimitExceededError {
152 resp = errRateLimitExceeded
153 } else if err == testservices.NoMoreFloatingIPs {
154@@ -306,9 +307,10 @@
155 } else {
156 resp, _ = err.(http.Handler)
157 if resp == nil {
158+ code, encodedErr := errorJSONEncode(err)
159 resp = &errorResponse{
160- http.StatusInternalServerError,
161- `{"internalServerError":{"message":"$ERROR$",code:500}}`,
162+ code,
163+ encodedErr,
164 "application/json",
165 err.Error(),
166 nil,
167@@ -651,7 +653,7 @@
168 }
169 server, err := n.server(serverId)
170 if err != nil {
171- return errNotFoundJSON
172+ return err
173 }
174 if groups {
175 srvGroups := n.allServerSecurityGroups(serverId)
176
177=== modified file 'testservices/novaservice/service_http_test.go'
178--- testservices/novaservice/service_http_test.go 2013-12-22 23:43:53 +0000
179+++ testservices/novaservice/service_http_test.go 2014-05-02 16:29:21 +0000
180@@ -261,7 +261,7 @@
181 {
182 method: "GET",
183 url: "/servers/invalid",
184- expect: errNotFoundJSON,
185+ expect: &errorResponse{code: 404, body: "{\"itemNotFound\":{\"message\":\"No such server \\\"invalid\\\"\", \"code\":404}}"},
186 },
187 {
188 method: "POST",

Subscribers

People subscribed via source and target branches