launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/gomaasapi/binary-results into lp:gomaasapi.

Commit message:
Support binary (non-JSON) results in JSONObject.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/gomaasapi/binary-results/+merge/147894

A JSONObject parsed from an API response now makes its raw binary original available as GetBytes().  It acts like just another type you can convert a JSONObject to.

This does mean that parsing basically can't fail.  I haven't been able to identify any errors that could arise from json.Unmarshal apart from the ones you see in the code.  But obviously it's not a good idea to assume that there won't be any other errors, because things can change.  The safe thing to do is to report any unexpected errors, and update the code in cases where we should tolerate the errors.

You may notice a nasty consequence of how Go's switch statement differs from C's.  Consider:

y = 0
switch (x) {
case 1:
case 2:
        y = 2
}

(I know you wouldn't parenthesize the switch expression in Go, but bear with me).

If this is C, and x == 1, then y will be 2.  But if the code is Go, and x == 1, then y will be 0.

Finally, there's a somewhat nasty case with nilness.  There may be other to represent nil, or maybe IsNil should just ignore the binary.  But what you see here was relatively easy to do, and since it's documented, maybe the caller can rule out the special case anyway.


Jeroen
-- 
https://code.launchpad.net/~jtv/gomaasapi/binary-results/+merge/147894
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/gomaasapi/binary-results into lp:gomaasapi.

=== modified file 'jsonobject.go'
--- jsonobject.go	2013-02-12 07:17:17 +0000
+++ jsonobject.go	2013-02-12 11:31:20 +0000
@@ -25,8 +25,23 @@
 // Reading a null item is also an error.  So before you try obj.Get*(),
 // first check obj.IsNil().
 type JSONObject struct {
-	value  interface{}
+	// Parsed value.  May actually be any of the types a JSONObject can
+	// wrap, except raw bytes.  If the object can only be interpreted
+	// as raw bytes, this will be nil.
+	value interface{}
+	// Raw bytes, if this object was parsed directly from an API response.
+	// Is nil for sub-objects found within other objects.  An object that
+	// was parsed directly from a response can be both raw bytes and some
+	// other value at the same  time.
+	// For example, "[]" looks like a JSON list, so you can read it as an
+	// array.  But it may also be the raw contents of a file that just
+	// happens to look like JSON, and so you can read it as raw bytes as
+	// well.
+	bytes []byte
+	// Client for further communication with the API.
 	client Client
+	// Is this a JSON null?
+	isNull bool
 }
 
 // Our JSON processor distinguishes a MAASObject from a jsonMap by the fact
@@ -41,7 +56,7 @@
 // being converted to a JSONObject type.
 func maasify(client Client, value interface{}) JSONObject {
 	if value == nil {
-		return JSONObject{}
+		return JSONObject{isNull: true}
 	}
 	switch value.(type) {
 	case string, float64, bool:
@@ -67,12 +82,26 @@
 
 // Parse a JSON blob into a JSONObject.
 func Parse(client Client, input []byte) (JSONObject, error) {
-	var obj interface{}
-	err := json.Unmarshal(input, &obj)
-	if err != nil {
-		return JSONObject{}, err
-	}
-	return maasify(client, obj), nil
+	var obj JSONObject
+	if input == nil {
+		panic(errors.New("Parse() called with nil input"))
+	}
+	var parsed interface{}
+	err := json.Unmarshal(input, &parsed)
+	if err == nil {
+		obj = maasify(client, parsed)
+		obj.bytes = input
+	} else {
+		switch err.(type) {
+		case *json.InvalidUTF8Error:
+		case *json.SyntaxError:
+			// This isn't JSON.  Treat it as raw binary data.
+		default:
+			return obj, err
+		}
+		obj = JSONObject{value: nil, client: client, bytes: input}
+	}
+	return obj, nil
 }
 
 // Return error value for failed type conversion.
@@ -81,6 +110,7 @@
 	return errors.New(msg)
 }
 
+<<<<<<< TREE
 // MarshalJSON tells the standard json package how to serialize a JSONObject
 // back to JSON.
 func (obj JSONObject) MarshalJSON() ([]byte, error) {
@@ -93,10 +123,28 @@
 // With MarshalJSON, JSONObject implements json.Marshaler.
 var _ json.Marshaler = (*JSONObject)(nil)
 
+=======
+// IsNil tells you whether a JSONObject is a JSON "null."
+// There is one irregularity.  If the original JSON blob was actually raw
+// data, not JSON, then its IsNil will return false because the object
+// contains the binary data as a non-nil value.  But, if the original JSON
+// blob consisted of a null, then IsNil returns true even though you can
+// still retrieve binary data from it.
+>>>>>>> MERGE-SOURCE
 func (obj JSONObject) IsNil() bool {
-	return obj.value == nil
+	if obj.value != nil {
+		return false
+	}
+	if obj.bytes == nil {
+		return true
+	}
+	// This may be a JSON null.  We can't expect every JSON null to look
+	// the same; there may be leading or trailing space.
+	return obj.isNull
 }
 
+// GetString retrieves the object's value as a string.  If the value wasn't
+// a JSON string, that's an error.
 func (obj JSONObject) GetString() (value string, err error) {
 	value, ok := obj.value.(string)
 	if !ok {
@@ -105,6 +153,8 @@
 	return
 }
 
+// GetFloat64 retrieves the object's value as a float64.  If the value wasn't
+// a JSON number, that's an error.
 func (obj JSONObject) GetFloat64() (value float64, err error) {
 	value, ok := obj.value.(float64)
 	if !ok {
@@ -113,6 +163,8 @@
 	return
 }
 
+// GetMap retrieves the object's value as a map.  If the value wasn't a JSON
+// object, that's an error.
 func (obj JSONObject) GetMap() (value map[string]JSONObject, err error) {
 	value, ok := obj.value.(map[string]JSONObject)
 	if !ok {
@@ -121,6 +173,8 @@
 	return
 }
 
+// GetArray retrieves the object's value as an array.  If the value wasn't a
+// JSON list, that's an error.
 func (obj JSONObject) GetArray() (value []JSONObject, err error) {
 	value, ok := obj.value.([]JSONObject)
 	if !ok {
@@ -129,6 +183,8 @@
 	return
 }
 
+// GetBool retrieves the object's value as a bool.  If the value wasn't a JSON
+// bool, that's an error.
 func (obj JSONObject) GetBool() (value bool, err error) {
 	value, ok := obj.value.(bool)
 	if !ok {
@@ -136,3 +192,18 @@
 	}
 	return
 }
+
+// GetBytes retrieves the object's value as raw bytes.  A JSONObject that was
+// parsed from the original input (as opposed to one that's embedded in
+// another JSONObject) can contain both the raw bytes and the parsed JSON
+// value, but either can be the case without the other.
+// If this object wasn't parsed directly from the original input, that's an
+// error.
+// If the object was parsed from an original input that just said "null", then
+// IsNil will return true but the raw bytes are still available from GetBytes.
+func (obj JSONObject) GetBytes() ([]byte, error) {
+	if obj.bytes == nil {
+		return nil, failConversion("bytes", obj)
+	}
+	return obj.bytes, nil
+}

=== modified file 'jsonobject_test.go'
--- jsonobject_test.go	2013-02-12 09:12:01 +0000
+++ jsonobject_test.go	2013-02-12 11:31:20 +0000
@@ -157,6 +157,89 @@
 	c.Check(out, Equals, 12.0)
 }
 
+func (suite *JSONObjectSuite) TestParseKeepsBinaryOriginal(c *C) {
+	blob := []byte(`"Hi"`)
+
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+
+	text, err := obj.GetString()
+	c.Assert(err, IsNil)
+	c.Check(text, Equals, "Hi")
+	binary, err := obj.GetBytes()
+	c.Assert(err, IsNil)
+	c.Check(binary, DeepEquals, blob)
+}
+
+func (suite *JSONObjectSuite) TestParseTreatsInvalidJSONAsBinary(c *C) {
+	blob := []byte("?x]}y![{z")
+
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+
+	c.Check(obj.IsNil(), Equals, false)
+	c.Check(obj.value, IsNil)
+	binary, err := obj.GetBytes()
+	c.Assert(err, IsNil)
+	c.Check(binary, DeepEquals, blob)
+}
+
+func (suite *JSONObjectSuite) TestParseTreatsInvalidUTF8AsBinary(c *C) {
+	// Arbitrary data that is definitely not UTF-8.
+	blob := []byte{220, 8, 129}
+
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+
+	c.Check(obj.IsNil(), Equals, false)
+	c.Check(obj.value, IsNil)
+	binary, err := obj.GetBytes()
+	c.Assert(err, IsNil)
+	c.Check(binary, DeepEquals, blob)
+}
+
+func (suite *JSONObjectSuite) TestParseTreatsEmptyJSONAsBinary(c *C) {
+	blob := []byte{}
+
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+
+	c.Check(obj.IsNil(), Equals, false)
+	data, err := obj.GetBytes()
+	c.Assert(err, IsNil)
+	c.Check(data, DeepEquals, blob)
+}
+
+func (suite *JSONObjectSuite) TestParsePanicsOnNilJSON(c *C) {
+	defer func() {
+		failure := recover()
+		c.Assert(failure, NotNil)
+		c.Check(failure.(error).Error(), Matches, ".*nil input")
+	}()
+	Parse(Client{}, nil)
+}
+
+func (suite *JSONObjectSuite) TestParseNullProducesIsNil(c *C) {
+	blob := []byte("null")
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+	c.Check(obj.IsNil(), Equals, true)
+}
+
+func (suite *JSONObjectSuite) TestParseNonNullProducesNonIsNil(c *C) {
+	blob := []byte("1")
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+	c.Check(obj.IsNil(), Equals, false)
+}
+
+func (suite *JSONObjectSuite) TestParseSpacedNullProducesIsNil(c *C) {
+	blob := []byte("      null     ")
+	obj, err := Parse(Client{}, blob)
+	c.Assert(err, IsNil)
+	c.Check(obj.IsNil(), Equals, true)
+}
+
 // String-type JSONObjects convert only to string.
 func (suite *JSONObjectSuite) TestConversionsString(c *C) {
 	obj := maasify(Client{}, "Test string")