~emersion/public-inbox

General catch-all for patches, questions, and discussions for emersion's projects that don't have their own mailing list.

When posting patches to this list, please edit the [PATCH] line to include the specific project you're contributing to, e.g.

[PATCH mrsh v2] kitchen: add nuggets
5 2

[drmdb PATCH] Simplify JSON unmarshalling

Silvan Jegen
Details
Message ID
<20190518170321.17152-1-s.jegen@gmail.com>
Sender timestamp
1558199001
DKIM signature
pass
Download raw message
Patch: +22 -39
Since json.Unmarshal uses reflection, it is enough to declare its
parameter as an interface{} and let it figure out the rest itself.
---
 drmtree/drmtree.go | 61 +++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

diff --git a/drmtree/drmtree.go b/drmtree/drmtree.go
index a7a56f3..1250f1b 100644
--- a/drmtree/drmtree.go
+++ b/drmtree/drmtree.go
@@ -141,81 +141,64 @@ func (prop *Property) UnmarshalJSON(b []byte) error {
 
 	if rawSpec != nil {
 		var err error
+		var spec interface{}
 		switch rawProp.Type {
 		case drm.PropertyRange:
-			var spec PropertySpecRange
-			err = json.Unmarshal(rawSpec, &spec)
-			rawProp.Spec = &spec
+			spec = PropertySpecRange{}
 		case drm.PropertyEnum, drm.PropertyBitmask:
-			var spec []PropertySpecEnum
-			err = json.Unmarshal(rawSpec, &spec)
-			rawProp.Spec = spec
+			spec = []PropertySpecEnum{}
 		case drm.PropertyObject:
-			spec := drm.ObjectType(0)
-			err = json.Unmarshal(rawSpec, &spec)
-			rawProp.Spec = spec
+			spec = drm.ObjectType(0)
 		case drm.PropertySignedRange:
-			spec := PropertySpecSignedRange{}
-			err = json.Unmarshal(rawSpec, &spec)
-			rawProp.Spec = &spec
+			spec = PropertySpecSignedRange{}
 		}
+		err = json.Unmarshal(rawSpec, &spec)
 		if err != nil {
 			return err
 		}
+		rawProp.Spec = spec
 	}
 
 	if rawValue != nil {
 		var err error
+		var value interface{}
 		switch rawProp.Type {
 		case drm.PropertyRange, drm.PropertyEnum, drm.PropertyBitmask:
-			var value uint64
-			err = json.Unmarshal(rawValue, &value)
-			rawProp.Value = value
+			value = uint64(0)
 		case drm.PropertyBlob:
-			var value []byte
-			err = json.Unmarshal(rawValue, &value)
-			rawProp.Value = value
+			value = []byte{}
 		case drm.PropertyObject:
-			var value drm.ObjectID
-			err = json.Unmarshal(rawValue, &value)
-			rawProp.Value = value
+			value = drm.ObjectID(0)
 		case drm.PropertySignedRange:
-			var value int64
-			err = json.Unmarshal(rawValue, &value)
-			rawProp.Value = value
+			value = int64(0)
 		}
+		err = json.Unmarshal(rawValue, &value)
 		if err != nil {
 			return err
 		}
+		rawProp.Value = value
 	}
 
 	if rawData != nil {
 		var err error
+		var data interface{}
 		switch prop.name {
 		case "SRC_X", "SRC_Y", "SRC_W", "SRC_H":
-			var data uint64
-			err = json.Unmarshal(rawData, &data)
-			rawProp.Data = data
+			data = uint64(0)
 		case "IN_FORMATS":
-			var data []PlaneInFormatsModifier
-			err = json.Unmarshal(rawData, &data)
-			rawProp.Data = data
+			data = []PlaneInFormatsModifier{}
 		case "MODE_ID":
-			var data Mode
-			err = json.Unmarshal(rawData, &data)
-			rawProp.Data = &data
+			data = Mode{}
 		case "WRITEBACK_PIXEL_FORMATS":
-			var data []drm.Format
-			err = json.Unmarshal(rawData, &data)
-			rawProp.Data = data
+			data = []drm.Format{}
 		case "PATH":
-			var data string
-			err = json.Unmarshal(rawData, &data)
-			rawProp.Data = data
+			data = ""
 		}
+		err = json.Unmarshal(rawData, &data)
 		if err != nil {
 			return err
 		}
+		rawProp.Data = data
 	}
 
 	*prop = Property(rawProp)
-- 
2.21.0
Details
Message ID
<jPgLLbEPA4wxvUV6Ds3b72YtMKq7RMvI0uG_k9f-b-4_PVfwDLspZmFSBHEWVttU82ZNrDVp0OHnU19XttCa34-fkravCmAC0KRs86Bx9BE=@emersion.fr>
In-Reply-To
<20190518170321.17152-1-s.jegen@gmail.com> (view parent)
Sender timestamp
1558199698
DKIM signature
pass
Download raw message
Hey, thanks for the patch!

Actually I've had a hard time getting this part right. Have you tried
adding some debug logs to the code? I've added this line after
`rawProp.Spec = spec`:

    log.Printf("%v %v %T", prop.name, spec, spec)

When piping drmdump output into itself like this (to make it actually
parse JSON):

    go run ./cmd/drmdump -j | go run ./cmd/drmdump -i

It prints lines like this:

    2019/05/18 20:09:01 rotation [map[name:rotate-0 value:0] map[name:rotate-180 value:2]] []interface {}

In other words, []PropertySpecEnum{} gets turned into a []interface{}.

I think this happens because encoding/json sees an *interface{} when
you do `json.Unmarshal(rawSpec, &spec)` (because spec is an
interface{}). So this is equivalent to:

    var v interface{}
    json.Unmarshal(b, &v)

This is pretty unfortunate, and I'm definitely interested in ways to
make the JSON unmarshalling less tedious. Let me know if you have other
ideas. :)
Silvan Jegen
Details
Message ID
<1Z36EWKV6ZP2M.35ASY3EM77CCM@homearch.localdomain>
In-Reply-To
<jPgLLbEPA4wxvUV6Ds3b72YtMKq7RMvI0uG_k9f-b-4_PVfwDLspZmFSBHEWVttU82ZNrDVp0OHnU19XttCa34-fkravCmAC0KRs86Bx9BE=@emersion.fr> (view parent)
Sender timestamp
1558204998
DKIM signature
fail
Download raw message
DKIM signature: fail
Hi Simon!

Simon Ser <contact@emersion.fr> wrote:
> Hey, thanks for the patch!
> 
> Actually I've had a hard time getting this part right. Have you tried
> adding some debug logs to the code? I've added this line after
> `rawProp.Spec = spec`:
> 
>     log.Printf("%v %v %T", prop.name, spec, spec)
> 
> When piping drmdump output into itself like this (to make it actually
> parse JSON):
> 
>     go run ./cmd/drmdump -j | go run ./cmd/drmdump -i
> 
> It prints lines like this:
> 
>     2019/05/18 20:09:01 rotation [map[name:rotate-0 value:0] map[name:rotate-180 value:2]] []interface {}
> 
> In other words, []PropertySpecEnum{} gets turned into a []interface{}.

Ah, sorry, I didn't check for that. I did the same comparing the old
and the new code and there is a difference indeed.


Old without change: scaling mode [{None 0} {Full 1} {Center 2} {Full aspect 3}] []drmtree.PropertySpecEnum
New with change:    scaling mode [map[name:None value:0] map[name:Full value:1] map[name:Center value:2] map[name:Full aspect value:3]] []interface {}


> I think this happens because encoding/json sees an *interface{} when
> you do `json.Unmarshal(rawSpec, &spec)` (because spec is an
> interface{}). So this is equivalent to:
> 
>     var v interface{}
>     json.Unmarshal(b, &v)
> 
> This is pretty unfortunate, and I'm definitely interested in ways to
> make the JSON unmarshalling less tedious. Let me know if you have other
> ideas. :)

Yeah, that makes sense.

The difference is that your code will check the well-formedness of
the JSON as well as whether or not all the expected fields for a type
contain the expected JSON types while mine will only check that the JSON
is well-formed.

I will send another patch if I come up with a working idea for this case!


Cheers,

Silvan
Details
Message ID
<JyGfMjMwSlbrrXPnYsQhsEDQBnVzMjWN_HkMZ1ug8wQ7eC82iJ-LFXh1fSOUUhX9cuwmK9e_1xRKqWRCoGwBEyX0Ag6UqUZg8SwuAjvV5Rc=@emersion.fr>
In-Reply-To
<1Z36EWKV6ZP2M.35ASY3EM77CCM@homearch.localdomain> (view parent)
Sender timestamp
1558216917
DKIM signature
pass
Download raw message
> Yeah, that makes sense.
>
> The difference is that your code will check the well-formedness of
> the JSON as well as whether or not all the expected fields for a type
> contain the expected JSON types while mine will only check that the JSON
> is well-formed.

To expand on this a bit: while the current code doesn't use the unmarshalled
data for display, in the future this will be desirable (e.g. to pretty-print the
IN_FORMATS property). It's much more convenient to work with
[]PropertySpecEnum then with []interface{} (which itself contains
map[string]interface{}, which itself contains float64). Additionally,
encoding/json parses numbers by default as float64, so we loose some bits if it
was originally an uint64 (this is important because it's used for bitfields and
enums).

> I will send another patch if I come up with a working idea for this case!

Nice!
Silvan Jegen
Details
Message ID
<2MUA2R6OW5J1I.3PLOC0ITW6M4Z@homearch.localdomain>
In-Reply-To
<JyGfMjMwSlbrrXPnYsQhsEDQBnVzMjWN_HkMZ1ug8wQ7eC82iJ-LFXh1fSOUUhX9cuwmK9e_1xRKqWRCoGwBEyX0Ag6UqUZg8SwuAjvV5Rc=@emersion.fr> (view parent)
Sender timestamp
1558218146
DKIM signature
fail
Download raw message
DKIM signature: fail
Simon Ser <contact@emersion.fr> wrote:
> > Yeah, that makes sense.
> >
> > The difference is that your code will check the well-formedness of
> > the JSON as well as whether or not all the expected fields for a type
> > contain the expected JSON types while mine will only check that the JSON
> > is well-formed.
> 
> To expand on this a bit: while the current code doesn't use the unmarshalled
> data for display, in the future this will be desirable (e.g. to pretty-print the
> IN_FORMATS property). It's much more convenient to work with
> []PropertySpecEnum then with []interface{} (which itself contains

That I agree with but since the Property's Data, Value and Spec fields
are of interface{} type, you will have to do a type assertion(s) in any
case from what I remember (I barely use the interface{} type if I can
help it). Otherwise you will not be able to use the more specific type
(i. e. []PropertySpecEnum in this case).


> map[string]interface{}, which itself contains float64). Additionally,
> encoding/json parses numbers by default as float64, so we loose some bits if it
> was originally an uint64 (this is important because it's used for bitfields and
> enums).

Ah, that's another good point.


> > I will send another patch if I come up with a working idea for this case!
> 
> Nice!

Sadly, that's an "if" not a "when" :P...


Cheers,

Silvan
Silvan Jegen
Details
Message ID
<219FWABIS3KUP.3JACS9T9QMNSB@homearch.localdomain>
In-Reply-To
<JyGfMjMwSlbrrXPnYsQhsEDQBnVzMjWN_HkMZ1ug8wQ7eC82iJ-LFXh1fSOUUhX9cuwmK9e_1xRKqWRCoGwBEyX0Ag6UqUZg8SwuAjvV5Rc=@emersion.fr> (view parent)
Sender timestamp
1558257898
DKIM signature
fail
Download raw message
DKIM signature: fail
Hi

Simon Ser <contact@emersion.fr> wrote:
> > Yeah, that makes sense.
> >
> > The difference is that your code will check the well-formedness of
> > the JSON as well as whether or not all the expected fields for a type
> > contain the expected JSON types while mine will only check that the JSON
> > is well-formed.
> 
> To expand on this a bit: while the current code doesn't use the unmarshalled
> data for display, in the future this will be desirable (e.g. to pretty-print the
> IN_FORMATS property). It's much more convenient to work with
> []PropertySpecEnum then with []interface{} (which itself contains
> map[string]interface{}, which itself contains float64). Additionally,
> encoding/json parses numbers by default as float64, so we loose some bits if it
> was originally an uint64 (this is important because it's used for bitfields and
> enums).
> 
> > I will send another patch if I come up with a working idea for this case!
> 
> Nice!

I think the last time I had to deal with a similar JSON structure
I just added all possible Go types to the type mapped in the JSON
Unmarshalling. Only the one encountered would then be populated. Something
like this (untested):


type Property struct {
	...

	Value PropertyValue `json:"value"`
	...
}

type PropertyValue struct {
			Uint     uint64
			Bytes    []byte
			ObjectID drm.ObjectID
			Int64    int64
}

Then the Unmarshaling code could then look something like:

  ...
	if rawValue != nil {
		var err error
		var value PropertyValue
		switch rawProp.Type {
		case drm.PropertyRange, drm.PropertyEnum, drm.PropertyBitmask:
			err = json.Unmarshal(rawValue, &value.Uint64)
		case drm.PropertyBlob:
			err = json.Unmarshal(rawValue, &value.Bytes)
		case drm.PropertyObject:
			err = json.Unmarshal(rawValue, &value.ObjectID)
		case drm.PropertySignedRange:
			err = json.Unmarshal(rawValue, &value.Int64)
		}
		if err != nil {
			return err
		}
		rawProp.Value = value
	}
  ...

Property.Type would then indicate which field in PorpertyValue you should
expect to be populated.

For the serialization you would have to write a MarshalJSON function
that fills the "value" JSON field with the one populated value.

This of course wastes quite a few bytes but depending on your use case
this could be acceptable.


Cheers,

Silvan