Sketch: a third take on defaulting values

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

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
+// RegisterConversionFunc registers a conversion func with the
-// three parameters: a pointer to the input type, a pointer to the output type, and
+// Converter. conversionFunc must take three parameters: a pointer to the input
-// a conversion.Scope (which should be used if recursive conversion calls are desired).
+// type, a pointer to the output type, and a conversion.Scope (which should be
-// It must return an error.
+// 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 })
+// c.RegisteConversionFuncr(
-func (c *Converter) Register(conversionFunc interface{}) error {
+//         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
 }
 

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!")

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,

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.