Change union to not "guess" based on difference

We're still using the difference, but only to see if something wrong was sent.

The new logic is to only trust the disicriminator, which means that the
behavior for non-discriminated union is now back to before. It also
means that we can't use that process anymore to normalize applied
unions. Applied unions are normalized by trusting the applied fields
exclusively: applying two fields doesn't work, applying a discriminator
and a different value doesn't work.

Author: Antoine Pelisse

merge/union_test.go

@@ -47,6 +47,7 @@ var unionFieldsParser = func() typed.ParseableType {
         scalar: string
     unions:
     - discriminator: type
+      deduceInvalidDiscriminator: true
       fields:
       - fieldName: numeric
         discriminatedBy: Numeric

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

@@ -133,23 +133,28 @@ type UnionField struct {
 }
 
 // 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

@@ -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 {
@@ -183,6 +206,7 @@ func newUnion(su *schema.Union) *union {
 		f2d[field(f.FieldName)] = discriminated(f.DiscriminatedBy)
 	}
 	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
 }