Merge pull request #82 from apelisse/change-union

Change union to not "guess" based on difference

Author: k8s-ci-robot

merge/union_test.go

@@ -47,16 +47,17 @@ var unionFieldsParser = func() typed.ParseableType {
         scalar: string
     unions:
     - discriminator: type
+      deduceInvalidDiscriminator: true
       fields:
       - fieldName: numeric
-        discriminatedBy: Numeric
+        discriminatorValue: Numeric
       - fieldName: string
-        discriminatedBy: String
+        discriminatorValue: String
     - fields:
       - fieldName: fieldA
-        discriminatedBy: FieldA
+        discriminatorValue: FieldA
       - fieldName: fieldB
-        discriminatedBy: FieldB`)
+        discriminatorValue: FieldB`)
 	if err != nil {
 		panic(err)
 	}

merge/update.go

@@ -151,15 +151,15 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
 // and return it.
 func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) {
 	managers = shallowCopyManagers(managers)
-	configObject, err := configObject.Empty().NormalizeUnions(configObject)
+	configObject, err := configObject.NormalizeUnionsApply(configObject)
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, err
 	}
 	newObject, err := liveObject.Merge(configObject)
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, fmt.Errorf("failed to merge config: %v", err)
 	}
-	newObject, err = liveObject.NormalizeUnions(newObject)
+	newObject, err = configObject.NormalizeUnionsApply(newObject)
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, err
 	}

schema/elements.go

@@ -126,30 +126,35 @@ type UnionField struct {
 	// FieldName is the name of the field that is part of the union. This
 	// is the serialized form of the field.
 	FieldName string `yaml:"fieldName"`
-	// DiscriminatedBy is the value of the discriminator to select that
-	// field. If the union doesn't have a discriminator, this field is
-	// ignored.
-	DiscriminatedBy string `yaml:"discriminatedBy"`
+	// Discriminatorvalue is the value of the discriminator to
+	// select that field. If the union doesn't have a discriminator,
+	// this field is ignored.
+	DiscriminatorValue string `yaml:"discriminatorValue"`
 }
 
 // Union, or oneof, means that only one of multiple fields of a structure can be
-// set at a time. For backward compatibility reasons, and to help "dumb clients"
-// which are not aware of the union (or can't be aware of it because they
-// don't know what fields are part of the union), the code tolerates multiple
-// fields to be set but will try to detect which fields must be cleared (there
-// should never be more than two though):
-// - If there is a discriminator and its value has changed, clear all fields
-// but the one specified by the discriminator
-// - If there is no discriminator, or it hasn't changed, if new has two of the
-// fields set, remove the one that was set in old.
-// - If there is a discriminator, set it to the value we've kept (if it changed)
+// set at a time. Setting the discriminator helps clearing oher fields:
+// - If discriminator changed to non-nil, and a new field has been added
+// that doesn't match, an error is returned,
+// - If discriminator hasn't changed and two fields or more are set, an
+// error is returned,
+// - If discriminator changed to non-nil, all other fields but the
+// discriminated one will be cleared,
+// - Otherwise, If only one field is left, update discriminator to that value.
 type Union struct {
 	// Discriminator, if present, is the name of the field that
 	// discriminates fields in the union. The mapping between the value of
 	// the discriminator and the field is done by using the Fields list
 	// below.
 	Discriminator *string `yaml:"discriminator,omitempty"`
 
+	// DeduceInvalidDiscriminator indicates if the discriminator
+	// should be updated automatically based on the fields set. This
+	// typically defaults to false since we don't want to deduce by
+	// default (the behavior exists to maintain compatibility on
+	// existing types and shouldn't be used for new types).
+	DeduceInvalidDiscriminator bool `yaml:"deduceInvalidDiscriminator,omitempty"`
+
 	// This is the list of fields that belong to this union. All the
 	// fields present in here have to be part of the parent
 	// structure. Discriminator (if oneOf has one), is NOT included in

schema/schemaschema.go

@@ -96,7 +96,7 @@ var SchemaSchemaYAML = `types:
     - name: fieldName
       type:
         scalar: string
-    - name: discriminatedBy
+    - name: discriminatorValue
       type:
         scalar: string
 - name: union
@@ -105,6 +105,9 @@ var SchemaSchemaYAML = `types:
     - name: discriminator
       type:
         scalar: string
+    - name: deduceInvalidDiscriminator
+      type:
+        scalar: bool
     - name: fields
       type:
         list:

typed/typed.go

@@ -147,15 +147,13 @@ func (tv TypedValue) RemoveItems(items *fieldpath.Set) *TypedValue {
 }
 
 // NormalizeUnions takes the new object and normalizes the union:
-// - If there is a discriminator and its value has changed, clean all
-// fields but the one specified by the discriminator
-// - If there is no discriminator, or it hasn't changed, if new has two
-// of the fields set, remove the one that was set in old.
-// - If there is a discriminator, set it to the value we've kept (if it changed)
-//
-// This can fail if:
-// - Multiple new fields are set,
-// - The discriminator is changed, and at least one new field is set.
+// - If discriminator changed to non-nil, and a new field has been added
+// that doesn't match, an error is returned,
+// - If discriminator hasn't changed and two fields or more are set, an
+// error is returned,
+// - If discriminator changed to non-nil, all other fields but the
+// discriminated one will be cleared,
+// - Otherwise, If only one field is left, update discriminator to that value.
 func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
 	var errs ValidationErrors
 	var normalizeFn = func(w *mergingWalker) {
@@ -177,6 +175,30 @@ func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
 	return out, nil
 }
 
+// NormalizeUnionsApply specifically normalize unions on apply. It
+// validates that the applied union is correct (there should be no
+// ambiguity there), and clear the fields according to the sent intent.
+func (tv TypedValue) NormalizeUnionsApply(new *TypedValue) (*TypedValue, error) {
+	var errs ValidationErrors
+	var normalizeFn = func(w *mergingWalker) {
+		if w.rhs != nil {
+			v := *w.rhs
+			w.out = &v
+		}
+		if err := normalizeUnionsApply(w); err != nil {
+			errs = append(errs, w.error(err)...)
+		}
+	}
+	out, mergeErrs := merge(&tv, new, func(w *mergingWalker) {}, normalizeFn)
+	if mergeErrs != nil {
+		errs = append(errs, mergeErrs.(ValidationErrors)...)
+	}
+	if len(errs) > 0 {
+		return nil, errs
+	}
+	return out, nil
+}
+
 func (tv TypedValue) Empty() *TypedValue {
 	tv.value = value.Value{Null: true}
 	return &tv

typed/union.go

@@ -46,6 +46,28 @@ func normalizeUnions(w *mergingWalker) error {
 	return nil
 }
 
+func normalizeUnionsApply(w *mergingWalker) error {
+	atom, found := w.schema.Resolve(w.typeRef)
+	if !found {
+		panic(fmt.Sprintf("Unable to resolve schema in normalize union: %v/%v", w.schema, w.typeRef))
+	}
+	// Unions can only be in structures, and the struct must not have been removed
+	if atom.Map == nil || w.out == nil {
+		return nil
+	}
+
+	old := &value.Map{}
+	if w.lhs != nil {
+		old = w.lhs.MapValue
+	}
+	for _, union := range atom.Map.Unions {
+		if err := newUnion(&union).NormalizeApply(old, w.rhs.MapValue, w.out.MapValue); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 type discriminated string
 type field string
 
@@ -167,9 +189,10 @@ func (fs fieldsSet) String() string {
 }
 
 type union struct {
-	d  *discriminator
-	dn discriminatedNames
-	f  []field
+	deduceInvalidDiscriminator bool
+	d                          *discriminator
+	dn                         discriminatedNames
+	f                          []field
 }
 
 func newUnion(su *schema.Union) *union {
@@ -180,9 +203,10 @@ func newUnion(su *schema.Union) *union {
 	f2d := map[field]discriminated{}
 	for _, f := range su.Fields {
 		u.f = append(u.f, field(f.FieldName))
-		f2d[field(f.FieldName)] = discriminated(f.DiscriminatedBy)
+		f2d[field(f.FieldName)] = discriminated(f.DiscriminatorValue)
 	}
 	u.dn = newDiscriminatedName(f2d)
+	u.deduceInvalidDiscriminator = su.DeduceInvalidDiscriminator
 	return u
 }
 
@@ -201,29 +225,49 @@ func (u *union) Normalize(old, new, out *value.Map) error {
 	ns := newFieldsSet(new, u.f)
 	diff := ns.Difference(os)
 
-	if len(ns) > 1 && len(diff) != 1 {
-		return fmt.Errorf("unable to guess new discriminator amongst new fields: %v", diff)
+	if u.d.Get(old) != u.d.Get(new) && u.d.Get(new) != "" {
+		if len(diff) == 1 && u.d.Get(new) != u.dn.toDiscriminated(*diff.One()) {
+			return fmt.Errorf("discriminator (%v) and field changed (%v) don't match", u.d.Get(new), diff.One())
+		}
+		if len(diff) > 1 {
+			return fmt.Errorf("multiple new fields added: %v", diff)
+		}
+		u.clear(out, u.dn.toField(u.d.Get(new)))
+		return nil
 	}
 
-	discriminator := field("")
-	if len(ns) == 1 {
-		discriminator = *ns.One()
-	} else if len(diff) == 1 {
-		discriminator = *diff.One()
+	if len(ns) > 1 {
+		return fmt.Errorf("multiple fields set without discriminator change: %v", ns)
 	}
 
-	if u.d.Get(old) != u.d.Get(new) && u.d.Get(new) != "" {
-		if len(diff) == 1 && u.d.Get(new) != u.dn.toDiscriminated(discriminator) {
-			return fmt.Errorf("discriminator (%v) and field changed (%v)", u.d.Get(new), discriminator)
-		}
-		u.clear(out, u.dn.toField(u.d.Get(new)))
+	// Update discriminiator if it needs to be deduced.
+	if u.deduceInvalidDiscriminator && len(ns) == 1 {
+		u.d.Set(out, u.dn.toDiscriminated(*ns.One()))
+	}
+
+	return nil
+}
+
+func (u *union) NormalizeApply(applied, merged, out *value.Map) error {
+	as := newFieldsSet(applied, u.f)
+	if len(as) > 1 {
+		return fmt.Errorf("more than one field of union applied: %v", as)
+	}
+	if len(as) == 0 {
+		// None is set, just leave.
 		return nil
 	}
+	// We have exactly one, discriminiator must match if set
+	if u.d.Get(applied) != "" && u.d.Get(applied) != u.dn.toDiscriminated(*as.One()) {
+		return fmt.Errorf("applied discriminator (%v) doesn't match applied field (%v)", u.d.Get(applied), *as.One())
+	}
 
-	if discriminator != "" {
-		u.clear(out, discriminator)
-		u.d.Set(out, u.dn.toDiscriminated(discriminator))
+	// Update discriminiator if needed
+	if u.deduceInvalidDiscriminator {
+		u.d.Set(out, u.dn.toDiscriminated(*as.One()))
 	}
+	// Clear others fields.
+	u.clear(out, *as.One())
 
 	return nil
 }