From 1ddb68d8d763470ce3978f8db74644093879770b Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 23 Nov 2014 07:44:38 +0800 Subject: [PATCH] Sketch: a third take on defaulting values --- pkg/api/v1beta1/defaults.go | 61 +++++++++++++++++++++++++ pkg/conversion/converter.go | 76 ++++++++++++++++++++++++++------ pkg/conversion/converter_test.go | 14 +++--- pkg/conversion/scheme.go | 27 +++++++++++- pkg/runtime/scheme.go | 10 ++++- 5 files changed, 164 insertions(+), 24 deletions(-) create mode 100644 pkg/api/v1beta1/defaults.go diff --git a/pkg/api/v1beta1/defaults.go b/pkg/api/v1beta1/defaults.go new file mode 100644 index 0000000000..6c400dfd45 --- /dev/null +++ b/pkg/api/v1beta1/defaults.go @@ -0,0 +1,61 @@ +/* +Copyright 2014 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1 + +import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util" +) + +func init() { + api.Scheme.AddDefaultingFuncs( + func(obj *Volume) { + if obj.Source == nil || util.AllPtrFieldsNil(obj.Source) { + obj.Source = &VolumeSource{ + EmptyDir: &EmptyDir{}, + } + } + }, + func(obj *Port) { + if obj.Protocol == "" { + obj.Protocol = ProtocolTCP + } + }, + func(obj *Container) { + // TODO: delete helper functions that touch this + if obj.ImagePullPolicy == "" { + obj.ImagePullPolicy = PullIfNotPresent + } + if obj.TerminationMessagePath == "" { + // TODO: fix other code that sets this + obj.TerminationMessagePath = api.TerminationMessagePathDefault + } + }, + func(obj *RestartPolicy) { + if util.AllPtrFieldsNil(obj) { + obj.Always = &RestartPolicyAlways{} + } + }, + func(obj *Service) { + if obj.Protocol == "" { + obj.Protocol = ProtocolTCP + } + }, + ) +} + +// TODO: remove redundant code in validation and conversion diff --git a/pkg/conversion/converter.go b/pkg/conversion/converter.go index 1e2c2c42ab..8d46f83685 100644 --- a/pkg/conversion/converter.go +++ b/pkg/conversion/converter.go @@ -40,7 +40,7 @@ type DebugLogger interface { type Converter struct { // Map from the conversion pair to a function which can // do the conversion. - funcs map[typePair]reflect.Value + conversionFuncs map[typePair]reflect.Value // This is a map from a source field type and name, to a list of destination // field type and name. @@ -51,20 +51,25 @@ type Converter struct { // source field name and type to look for. structFieldSources map[typeNamePair][]typeNamePair + // Map from a type to a function which applies defaults. + defaultingFuncs map[reflect.Type]reflect.Value + // If non-nil, will be called to print helpful debugging info. Quite verbose. Debug DebugLogger - // NameFunc is called to retrieve the name of a type; this name is used for the + // nameFunc is called to retrieve the name of a type; this name is used for the // purpose of deciding whether two types match or not (i.e., will we attempt to // do a conversion). The default returns the go type name. - NameFunc func(t reflect.Type) string + nameFunc func(t reflect.Type) string } // NewConverter creates a new Converter object. func NewConverter() *Converter { return &Converter{ + conversionFuncs: map[typePair]reflect.Value{}, + defaultingFuncs: map[reflect.Type]reflect.Value{}, funcs: map[typePair]reflect.Value{}, - NameFunc: func(t reflect.Type) string { return t.Name() }, + nameFunc: func(t reflect.Type) string { return t.Name() }, structFieldDests: map[typeNamePair][]typeNamePair{}, structFieldSources: map[typeNamePair][]typeNamePair{}, } @@ -210,14 +215,18 @@ func (s *scope) error(message string, args ...interface{}) error { return fmt.Errorf(where+message, args...) } -// Register registers a conversion func with the Converter. conversionFunc must take -// three parameters: a pointer to the input type, a pointer to the output type, and -// a conversion.Scope (which should be used if recursive conversion calls are desired). -// It must return an error. +// RegisterConversionFunc registers a conversion func with the +// Converter. conversionFunc must take three parameters: a pointer to the input +// type, a pointer to the output type, and a conversion.Scope (which should be +// used if recursive conversion calls are desired). It must return an error. // // Example: -// c.Register(func(in *Pod, out *v1beta1.Pod, s Scope) error { ... return nil }) -func (c *Converter) Register(conversionFunc interface{}) error { +// c.RegisteConversionFuncr( +// func(in *Pod, out *v1beta1.Pod, s Scope) error { +// // conversion logic... +// return nil +// }) +func (c *Converter) RegisterConversionFunc(conversionFunc interface{}) error { fv := reflect.ValueOf(conversionFunc) ft := fv.Type() if ft.Kind() != reflect.Func { @@ -246,7 +255,7 @@ func (c *Converter) Register(conversionFunc interface{}) error { if ft.Out(0) != errorType { return fmt.Errorf("expected error return, got: %v", ft) } - c.funcs[typePair{ft.In(0).Elem(), ft.In(1).Elem()}] = fv + c.conversionFuncs[typePair{ft.In(0).Elem(), ft.In(1).Elem()}] = fv return nil } @@ -266,6 +275,33 @@ func (c *Converter) SetStructFieldCopy(srcFieldType interface{}, srcFieldName st return nil } +// RegisterDefaultingFunc registers a value-defaulting func with the Converter. +// defaultingFunc must take one parameters: a pointer to the input type. +// +// Example: +// c.RegisteDefaultingFuncr( +// func(in *v1beta1.Pod) { +// // defaulting logic... +// }) +func (c *Converter) RegisterDefaultingFunc(defaultingFunc interface{}) error { + fv := reflect.ValueOf(defaultingFunc) + ft := fv.Type() + if ft.Kind() != reflect.Func { + return fmt.Errorf("expected func, got: %v", ft) + } + if ft.NumIn() != 1 { + return fmt.Errorf("expected one 'in' param, got: %v", ft) + } + if ft.NumOut() != 0 { + return fmt.Errorf("expected zero 'out' params, got: %v", ft) + } + if ft.In(0).Kind() != reflect.Ptr { + return fmt.Errorf("expected pointer arg for 'in' param 0, got: %v", ft) + } + c.defaultingFuncs[ft.In(0).Elem()] = fv + return nil +} + // FieldMatchingFlags contains a list of ways in which struct fields could be // copied. These constants may be | combined. type FieldMatchingFlags int @@ -379,7 +415,18 @@ func (c *Converter) callCustom(sv, dv, custom reflect.Value, scope *scope) error // one is registered. func (c *Converter) convert(sv, dv reflect.Value, scope *scope) error { dt, st := dv.Type(), sv.Type() - if fv, ok := c.funcs[typePair{st, dt}]; ok { + + // Apply default values. + if fv, ok := c.defaultingFuncs[st]; ok { + if c.Debug != nil { + c.Debug.Logf("Applying defaults for '%v'", st) + } + args := []reflect.Value{sv.Addr()} + fv.Call(args) + } + + // Convert sv to dv. + if fv, ok := c.conversionFuncs[typePair{st, dt}]; ok { if c.Debug != nil { c.Debug.Logf("Calling custom conversion of '%v' to '%v'", st, dt) } @@ -398,10 +445,10 @@ func (c *Converter) defaultConvert(sv, dv reflect.Value, scope *scope) error { return scope.error("Cannot set dest. (Tried to deep copy something with unexported fields?)") } - if !scope.flags.IsSet(AllowDifferentFieldTypeNames) && c.NameFunc(dt) != c.NameFunc(st) { + if !scope.flags.IsSet(AllowDifferentFieldTypeNames) && c.nameFunc(dt) != c.nameFunc(st) { return scope.error( "type names don't match (%v, %v), and no conversion 'func (%v, %v) error' registered.", - c.NameFunc(st), c.NameFunc(dt), st, dt) + c.nameFunc(st), c.nameFunc(dt), st, dt) } switch st.Kind() { @@ -498,6 +545,7 @@ func toKVValue(v reflect.Value) kvValue { case reflect.Struct: return structAdaptor(v) } + return nil } diff --git a/pkg/conversion/converter_test.go b/pkg/conversion/converter_test.go index 2ca555aac1..1b22c68593 100644 --- a/pkg/conversion/converter_test.go +++ b/pkg/conversion/converter_test.go @@ -118,14 +118,14 @@ func TestConverter_CallsRegisteredFunctions(t *testing.T) { type C struct{} c := NewConverter() c.Debug = t - err := c.Register(func(in *A, out *B, s Scope) error { + err := c.RegisterConversionFunc(func(in *A, out *B, s Scope) error { out.Bar = in.Foo return s.Convert(&in.Baz, &out.Baz, 0) }) if err != nil { t.Fatalf("unexpected error %v", err) } - err = c.Register(func(in *B, out *A, s Scope) error { + err = c.RegisterConversionFunc(func(in *B, out *A, s Scope) error { out.Foo = in.Bar return s.Convert(&in.Baz, &out.Baz, 0) }) @@ -161,7 +161,7 @@ func TestConverter_CallsRegisteredFunctions(t *testing.T) { t.Errorf("expected %v, got %v", e, a) } - err = c.Register(func(in *A, out *C, s Scope) error { + err = c.RegisterConversionFunc(func(in *A, out *C, s Scope) error { return fmt.Errorf("C can't store an A, silly") }) if err != nil { @@ -184,7 +184,7 @@ func TestConverter_fuzz(t *testing.T) { f := fuzz.New().NilChance(.5).NumElements(0, 100) c := NewConverter() - c.NameFunc = func(t reflect.Type) string { + c.nameFunc = func(t reflect.Type) string { // Hide the fact that we don't have separate packages for these things. return map[reflect.Type]string{ reflect.TypeOf(TestType1{}): "TestType1", @@ -270,7 +270,7 @@ func TestConverter_tags(t *testing.T) { } c := NewConverter() c.Debug = t - err := c.Register( + err := c.RegisterConversionFunc( func(in *string, out *string, s Scope) error { if e, a := "foo", s.SrcTag().Get("test"); e != a { t.Errorf("expected %v, got %v", e, a) @@ -296,7 +296,7 @@ func TestConverter_meta(t *testing.T) { c := NewConverter() c.Debug = t checks := 0 - err := c.Register( + err := c.RegisterConversionFunc( func(in *Foo, out *Bar, s Scope) error { if s.Meta() == nil || s.Meta().SrcVersion != "test" || s.Meta().DestVersion != "passes" { t.Errorf("Meta did not get passed!") @@ -309,7 +309,7 @@ func TestConverter_meta(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } - err = c.Register( + err = c.RegisterConversionFunc( func(in *string, out *string, s Scope) error { if s.Meta() == nil || s.Meta().SrcVersion != "test" || s.Meta().DestVersion != "passes" { t.Errorf("Meta did not get passed a second time!") diff --git a/pkg/conversion/scheme.go b/pkg/conversion/scheme.go index 7570842615..15adea4044 100644 --- a/pkg/conversion/scheme.go +++ b/pkg/conversion/scheme.go @@ -63,7 +63,7 @@ func NewScheme() *Scheme { InternalVersion: "", MetaFactory: DefaultMetaFactory, } - s.converter.NameFunc = s.nameFunc + s.converter.nameFunc = s.nameFunc return s } @@ -194,7 +194,7 @@ func (s *Scheme) NewObject(versionName, kind string) (interface{}, error) { // add conversion functions for things with changed/removed fields. func (s *Scheme) AddConversionFuncs(conversionFuncs ...interface{}) error { for _, f := range conversionFuncs { - if err := s.converter.Register(f); err != nil { + if err := s.converter.RegisterConversionFunc(f); err != nil { return err } } @@ -209,6 +209,29 @@ func (s *Scheme) AddStructFieldConversion(srcFieldType interface{}, srcFieldName return s.converter.SetStructFieldCopy(srcFieldType, srcFieldName, destFieldType, destFieldName) } +// AddDefaultingFuncs adds functions to the list of default-value functions. +// Each of the given functions is responsible for applying default values +// when converting an instance of a versioned API object into an internal +// API object. These functions do not need to handle sub-objects. We deduce +// how to call these functions from the types of their two parameters. +// +// s.AddDefaultingFuncs( +// func(obj *v1beta1.Pod) { +// if obj.OptionalField == "" { +// obj.OptionalField = "DefaultValue" +// } +// }, +// ) +func (s *Scheme) AddDefaultingFuncs(defaultingFuncs ...interface{}) error { + for _, f := range defaultingFuncs { + err := s.converter.RegisterDefaultingFunc(f) + if err != nil { + return err + } + } + return nil +} + // Convert will attempt to convert in into out. Both must be pointers. For easy // testing of conversion functions. Returns an error if the conversion isn't // possible. You can call this with types that haven't been registered (for example, diff --git a/pkg/runtime/scheme.go b/pkg/runtime/scheme.go index 08160a4852..dc713d80e8 100644 --- a/pkg/runtime/scheme.go +++ b/pkg/runtime/scheme.go @@ -265,7 +265,8 @@ func (s *Scheme) Log(l conversion.DebugLogger) { // AddConversionFuncs adds a function to the list of conversion functions. The given // function should know how to convert between two API objects. We deduce how to call -// it from the types of its two parameters; see the comment for Converter.Register. +// it from the types of its two parameters; see the comment for +// Converter.RegisterConversionFunction. // // Note that, if you need to copy sub-objects that didn't change, it's safe to call // Convert() inside your conversionFuncs, as long as you don't start a conversion @@ -287,6 +288,13 @@ func (s *Scheme) AddStructFieldConversion(srcFieldType interface{}, srcFieldName return s.raw.AddStructFieldConversion(srcFieldType, srcFieldName, destFieldType, destFieldName) } +// AddDefaultingFuncs adds a function to the list of value-defaulting functions. +// We deduce how to call it from the types of its two parameters; see the +// comment for Converter.RegisterDefaultingFunction. +func (s *Scheme) AddDefaultingFuncs(defaultingFuncs ...interface{}) error { + return s.raw.AddDefaultingFuncs(defaultingFuncs...) +} + // Convert will attempt to convert in into out. Both must be pointers. // For easy testing of conversion functions. Returns an error if the conversion isn't // possible.