merge: handle reflect.Array

[tamird]

@awalterschulze this currently falls down if you use a customtype which contains an array.

[tamird]

This isn't totally right yet.

[awalterschulze]

gogoprotobuf does not currently support Clone
#14
There is no reason that I can immediately think of that it can't support Clone.
I just think it will take a bit of effort to fully support. Mostly just a lot of testing.

But I guess we can start with a test for something that does not work and a fix for that specific issue.
A small improvement in this case should also be a fine contribution.

[tamird]

@awalterschulze ok, so you need me to add a test? Can do.

[tamird]

@awalterschulze I changed the Uuid type to a [16]byte to try to test this, but it turns out that causes a huge number of panics. Could you take a look? Your help is greatly appreciated.

[awalterschulze]

What is this?

[tamird]

I replaced this file with a symlink to the canonical one.

[awalterschulze]

Ok so what happens when someone checks out this repo in windows?

[awalterschulze]

I will try to check out this pull request and check out which panics you are getting asap.

[awalterschulze]

I found the panics, I suspect there is just something wrong your customtype implementation of Uuid.
Have you tried the test/custom/Uint128 type its an array [2]uint64
Its probably a better template to create a new custom array type from if you require it to be an array of bytes.
If you still get stuck I'll take a deeper look.

[tamird]

Yeah, I'm stuck. I tried using a different type, and I no longer get panics, but JSON tests are failing because the jsonpb code is unable to handle non-generated structs. Here's my branch https://github.com/tamird/gogoprotobuf/commits/customtype-array

I'd appreciate some help here :(

I also noticed that there are no tests exercising Clone in a general way (there are a few tests from upstream), but that can come later.

[awalterschulze]

Yes the reason I say I don't support proto.Clone is because I did not write the tests.
Same reason I don't support proto.Equal.
Its basically just a time issue.

[awalterschulze]

I am just trying to make and it breaks.

[awalterschulze]

I checked out the wrong branch, my bad.

[awalterschulze]

This is one way to make it work

type Array [32]byte

func (b Array) Marshal() ([]byte, error) {
    buffer := make([]byte, b.Size())
    _, err := b.MarshalTo(buffer)
    return buffer, err
}

func (u Array) MarshalTo(data []byte) (int, error) {
    return copy(data, u[:]), nil
}

func (u *Array) Unmarshal(data []byte) error {
    if copy(u[:], data) != 32 {
        return errors.New("Array: invalid length")
    }
    return nil
}

func (b Array) MarshalJSON() ([]byte, error) {
    data, err := b.Marshal()
    if err != nil {
        return nil, err
    }
    return json.Marshal(data)
}

func (u Array) Size() int {
    return 32
}

func (b *Array) UnmarshalJSON(data []byte) error {
    v := new([]byte)
    err := json.Unmarshal(data, v)
    if err != nil {
        return err
    }
    return b.Unmarshal(*v)
}

func (this Array) Equal(that Array) bool {
    return this == that
}

func NewPopulatedArray(r randy) *Array {
    array := Array{}
    for i := range array {
        array[i] = byte(r.Intn(255))
    }
    return &array
}

[awalterschulze]

But it looks like there is no code to handle a struct as a customtype in the json marshal code.

[awalterschulze]

Ok there is quite a bit of work to do to make this work :(
This is only the start.

@@ -312,11 +308,24 @@ func (m *Marshaler) marshalValue(out *errWriter, prop *proto.Properties, v refle
                        if prop.CustomType == "" {
                                return fmt.Errorf("%v does not implement proto.Message", v.Type())
                        }
-                       t := proto.MessageType(prop.CustomType)
-                       if t == nil || !i.Type().ConvertibleTo(t) {
-                               return fmt.Errorf("%v declared custom type %s but it is not convertible to %v", v.Type(), prop.CustomType, t)
+                       log.Printf("handling customType %v", prop.CustomType)
+                       m, ok := iface.(interface {
+                               MarshalJSON() ([]byte, error)
+                       })
+                       if !ok {
+                               log.Printf("handling customType does not implememt json.Marshal %v", prop.CustomType)
+                               return fmt.Errorf("customType %v does not implement json.Marshal", prop.CustomType)
                        }
-                       pm = i.Convert(t).Interface().(proto.Message)
+                       data, err := m.MarshalJSON()
+                       if err != nil {
+                               log.Printf("handling customType marshalJson error %v", err)
+                               return err
+                       }
+                       customStr := string(data)
+                       log.Printf("customType customStr = %v", customStr)
+                       out.write(customStr)
+                       log.Printf("handling customType out error %v", out.err)
+                       return out.err
                }
                return m.marshalObject(out, pm, indent+m.Indent)
        }
@@ -420,16 +429,19 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
        if targetType.Kind() == reflect.Struct {
                var jsonFields map[string]json.RawMessage
                if err := json.Unmarshal(inputValue, &jsonFields); err != nil {
+                       panic(err)
                        return err
                }

                sprops := proto.GetProperties(targetType)
                for i := 0; i < target.NumField(); i++ {
+
                        ft := target.Type().Field(i)
                        if strings.HasPrefix(ft.Name, "XXX_") {
                                continue
                        }
-                       fieldName := jsonProperties(ft).OrigName
+                       prop := jsonProperties(ft)
+                       fieldName := prop.OrigName

                        valueForField, ok := jsonFields[fieldName]
                        if !ok {
@@ -459,6 +471,24 @@ func unmarshalValue(target reflect.Value, inputValue json.RawMessage) error {
                                f.SetInt(int64(n))
                                continue
                        }
+                       if prop.CustomType != "" {
+                               f := target.Field(i)
+                               if f.Kind() == reflect.Ptr {
+                                       f.Set(reflect.New(f.Type().Elem()))
+                               }
+                               field := f.Interface()
+                               m, ok := field.(interface {
+                                       UnmarshalJSON([]byte) error
+                               })
+                               if !ok {
+                                       return fmt.Errorf("customType %v does not implement UnmarshalJSON", prop.CustomType)
+                               }
+                               log.Printf("unmarshaling customType %v", prop.CustomType)
+                               if err := m.UnmarshalJSON(valueForField); err != nil {
+                                       return err
+                               }
+                               continue
+                       }

                        if err := unmarshalValue(target.Field(i), valueForField); err != nil {
                                return err

[awalterschulze]

So this is definitely worth logging an issue.

[tamird]

Logged #147

[awalterschulze]

Cool Thank you
So does the type Array [32]byte solution work for you in the mean time?

[tamird]

For now I've just removed all the problematic calls to proto.Clone(<message that contains a customtype struct>), and that seems to work. See cockroachdb/cockroach#4383.

[awalterschulze]

Ok cool, then I can properly fix this when I have more time.
Btw another very hacky way to properly Clone is to Marshal and Unmarshal.

[tamird]

Ha, yeah, we're going to avoid that for now :)

[awalterschulze]

I am guessing you found another solution. I am closing this, but please comment if I should reopen.

[tamird]

We wrote a wrapper that panics if you try to clone a structure that contains an array. Not ideal!

[awalterschulze]

Ok well the issue #147 is still open. So we are both still waiting for a proper fix for proto.Clone