mlab-lattice / lattice

Apache License 2.0
1 stars 3 forks source link

don't panic on DeepCopy errors #239

Open kevindrosendahl opened 5 years ago

kevindrosendahl commented 5 years ago

Currently, for some structs that contain fields that use interface{}, we've had to implement DeepCopyInto via a json marshal -> unmarshal DeepCopyInto does not return an error, so currently to be safe we panic on errors.

We should either:

  1. figure out a way to reliably copy interface{} without using something that can error like marshal/unmarshalling
  2. add a way to check if DeepCopyInto failed for structs using this technique rather than panicking

Attempts to come up with a good solution for copying interface{} without involving errors have come up short. One attempted design was to create a pkg/utils/deepcopy.Interface

type Interface interface {
    DeepCopyInterface()
}

and replace references to interface{} with deepcopy.Interface. However, this means that primitive/builtin types cannot be used as we cannot implement this interface for them. This is a non-starter for things like definitionv1.Reference.Parameters.

The other option then is to add a way to check if DeepCopyInto failed. One option is to have structs that need to use a fallible copy process have a deepCopyCanary error member, and set it to the error if their DeepCopyInto failed. For example:

type T1 struct {
    Vals           map[string]interface{} `json:"vals"`
    deepCopyCanary error
}

func (in *T1) DeepCopyInto(out *T1) {
    data, err := json.Marshal(&in)
    if err != nil {
        in.deepCopyCanary = err
        return
    }

    if err := json.Unmarshal(data, &out); err != nil {
        // important: deepCopyCanary set on out
        out.deepCopyCanary = err
    }
}

This gets tricky if you have:

type T2 struct {
    T1
}

type T3 struct {
    *T2
}

Deeply nested types with fallible DeepCopyInto implementations make it infeasible to implement DeepCopyInto this way.

One possible solution would be to be able to tag types with fallible DeepCopyIntos, or even autogenerate them:

// +lattice:deepcopy-canary:json

type T1 struct {
    Vals           map[string]interface{} `json:"vals"`
    deepCopyCanary error
}

Probably the simplest way to integrate this with T1 being nested would be to generate DeepCopyCanary for everything through a package annotation:

// +lattice:deepcopy-canary:package

This would generate:

func (in *T2) DeepCopyCanary() error {
    if err := in.T1.DeepCopyCanary(); err != nil {
        return err
    }

    return nil
}

func (in *T3) DeepCopyCanary() error {
    if in.T2 != nil {
        if err := in.T2.DeepCopyCanary(); err != nil {
             return err
        }
    }

    return nil
}

Then, to safely DeepCopy a T3, you would call:

t3 = t3.DeepCopy()
if err := t3.DeepCopyCanary(); err != nil {
   // handle err
}

Another possible implementation of DeepCopyCanary would wrap DeepCopyInto and return (<val>, error), so instead you would call:

t3, err := t3.DeepCopyCanary()
if err != nil {
    // handle err
}

The generated code for this would probably be a bit trickier, but it could be a nicer experience.