Fix bugs in OneOf to Object conversion (#4555)

* Properly implement OneOf Equality

* Avoid loss of type updates

* Merge objects into named oneof leaves

* Update docs

* Address PR feedback

docs/hugo/content/reference/_index.md

@@ -202,10 +202,10 @@ To install the CRDs for these resources, your ASO configuration must include `co
 
 Development of these new resources is complete and they will be available in the next release of ASO.
 
-| Resource            | ARM Version | CRD Version   | Supported From | Sample                                                                                                                                              |
-|---------------------|-------------|---------------|----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|
-| Registry            | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registry.yaml)            |
-| RegistryReplication | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registryreplication.yaml) |
+| Resource                                                                                                                                                                       | ARM Version | CRD Version   | Supported From | Sample                                                                                                                                              |
+|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------|---------------|----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|
+| [Registry](https://azure.github.io/azure-service-operator/reference/containerregistry/v1api20230701/#containerregistry.azure.com/v1api20230701.Registry)                       | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registry.yaml)            |
+| [RegistryReplication](https://azure.github.io/azure-service-operator/reference/containerregistry/v1api20230701/#containerregistry.azure.com/v1api20230701.RegistryReplication) | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registryreplication.yaml) |
 
 ### Released
 

docs/hugo/content/reference/containerregistry/_index.md

@@ -9,10 +9,10 @@ To install the CRDs for these resources, your ASO configuration must include `co
 
 Development of these new resources is complete and they will be available in the next release of ASO.
 
-| Resource            | ARM Version | CRD Version   | Supported From | Sample                                                                                                                                              |
-|---------------------|-------------|---------------|----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|
-| Registry            | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registry.yaml)            |
-| RegistryReplication | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registryreplication.yaml) |
+| Resource                                                                                                                                                                       | ARM Version | CRD Version   | Supported From | Sample                                                                                                                                              |
+|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------------|---------------|----------------|-----------------------------------------------------------------------------------------------------------------------------------------------------|
+| [Registry](https://azure.github.io/azure-service-operator/reference/containerregistry/v1api20230701/#containerregistry.azure.com/v1api20230701.Registry)                       | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registry.yaml)            |
+| [RegistryReplication](https://azure.github.io/azure-service-operator/reference/containerregistry/v1api20230701/#containerregistry.azure.com/v1api20230701.RegistryReplication) | 2023-07-01  | v1api20230701 | v2.12.0        | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/containerregistry/v1api20230701/v1api20230701_registryreplication.yaml) |
 
 ### Released
 

v2/tools/generator/internal/astmodel/oneof_type.go

@@ -187,7 +187,42 @@ func (oneOf *OneOfType) Equals(t Type, overrides EqualityOverrides) bool {
 		return false
 	}
 
-	return oneOf.types.Equals(other.types, overrides)
+	// Check for different properties
+	if oneOf.swaggerName != other.swaggerName {
+		return false
+	}
+
+	if oneOf.discriminatorProperty != other.discriminatorProperty {
+		return false
+	}
+
+	if oneOf.discriminatorValue != other.discriminatorValue {
+		return false
+	}
+
+	// Check for different options to select from
+	if !oneOf.types.Equals(other.types, overrides) {
+		return false
+	}
+
+	// Check for different common properties
+	if len(oneOf.propertyObjects) != len(other.propertyObjects) {
+		return false
+	}
+
+	// Requiring exactly the same property objects in the same order is overly
+	// strict as they're actually all merged together into a single object
+	// and the order is not significant. Moreover, two one-of types would be the
+	// same if the merge is the same, regardless of how many object types were there
+	// to start with. This is all too complex to handle here though, so we'll just
+	// use the strict check.
+	for i := range oneOf.propertyObjects {
+		if !oneOf.propertyObjects[i].Equals(other.propertyObjects[i], overrides) {
+			return false
+		}
+	}
+
+	return true
 }
 
 // String implements fmt.Stringer

v2/tools/generator/internal/astmodel/oneof_type_test.go

@@ -15,33 +15,33 @@ func TestOneOfEqualityDoesNotCareAboutOrder(t *testing.T) {
 	t.Parallel()
 	g := NewGomegaWithT(t)
 
-	x := NewOneOfType("x", StringType, BoolType)
-	y := NewOneOfType("y", BoolType, StringType)
+	version1 := NewOneOfType("one", StringType, BoolType)
+	version2 := NewOneOfType("one", BoolType, StringType)
 
-	g.Expect(TypeEquals(x, y)).To(BeTrue())
-	g.Expect(TypeEquals(y, x)).To(BeTrue())
+	g.Expect(TypeEquals(version1, version2)).To(BeTrue())
+	g.Expect(TypeEquals(version2, version1)).To(BeTrue())
 }
 
 func TestOneOfMustHaveAllTypesToBeEqual(t *testing.T) {
 	t.Parallel()
 	g := NewGomegaWithT(t)
 
-	x := NewOneOfType("x", StringType, BoolType, FloatType)
-	y := NewOneOfType("y", BoolType, StringType)
+	version1 := NewOneOfType("one", StringType, BoolType, FloatType)
+	version2 := NewOneOfType("one", BoolType, StringType)
 
-	g.Expect(TypeEquals(x, y)).To(BeFalse())
-	g.Expect(TypeEquals(y, x)).To(BeFalse())
+	g.Expect(TypeEquals(version1, version2)).To(BeFalse())
+	g.Expect(TypeEquals(version2, version1)).To(BeFalse())
 }
 
 func TestOneOfsWithDifferentTypesAreNotEqual(t *testing.T) {
 	t.Parallel()
 	g := NewGomegaWithT(t)
 
-	x := NewOneOfType("x", StringType, FloatType)
-	y := NewOneOfType("y", BoolType, StringType)
+	version1 := NewOneOfType("one", StringType, FloatType)
+	version2 := NewOneOfType("one", BoolType, StringType)
 
-	g.Expect(TypeEquals(x, y)).To(BeFalse())
-	g.Expect(TypeEquals(y, x)).To(BeFalse())
+	g.Expect(TypeEquals(version1, version2)).To(BeFalse())
+	g.Expect(TypeEquals(version2, version1)).To(BeFalse())
 }
 
 var expectedOneOfPanic = "OneOfType should have been replaced by generation time by 'convertAllOfAndOneOf' phase"
@@ -111,3 +111,63 @@ func TestOneOfType_WithoutAnyPropertyObjects_GivenProperties_ReturnsOneOfWithNon
 	g.Expect(result).NotTo(BeNil())
 	g.Expect(result.PropertyObjects()).To(HaveLen(0))
 }
+
+func TestOneOfType_Equals_GivenChange_RecognisesDifference(t *testing.T) {
+	t.Parallel()
+
+	nameMixin := NewObjectType().WithProperties(
+		NewPropertyDefinition("Name", "name", StringType))
+	locationMixin := NewObjectType().WithProperties(
+		NewPropertyDefinition("Location", "location", StringType))
+
+	cases := map[string]struct {
+		change         func(*OneOfType) *OneOfType
+		expectedEquals bool
+	}{
+		"adding a property object": {
+			change: func(oneOf *OneOfType) *OneOfType {
+				return oneOf.WithAdditionalPropertyObject(locationMixin)
+			},
+			expectedEquals: false,
+		},
+		"without property objects": {
+			change: func(oneOf *OneOfType) *OneOfType {
+				return oneOf.WithoutAnyPropertyObjects()
+			},
+			expectedEquals: false,
+		},
+		"with different property object": {
+			change: func(oneOf *OneOfType) *OneOfType {
+				return oneOf.WithoutAnyPropertyObjects().WithAdditionalPropertyObject(locationMixin)
+			},
+			expectedEquals: false,
+		},
+		"with option type already present": {
+			change: func(oneOf *OneOfType) *OneOfType {
+				return oneOf.WithType(StringType)
+			},
+			expectedEquals: true,
+		},
+		"with additional option type": {
+			change: func(oneOf *OneOfType) *OneOfType {
+				return oneOf.WithType(FloatType)
+			},
+			expectedEquals: false,
+		},
+	}
+
+	for n, c := range cases {
+		t.Run(n, func(t *testing.T) {
+			t.Parallel()
+			g := NewGomegaWithT(t)
+
+			oneOf := NewOneOfType("oneOf").
+				WithType(StringType).
+				WithType(BoolType).
+				WithAdditionalPropertyObject(nameMixin)
+
+			actual := c.change(oneOf)
+			g.Expect(oneOf.Equals(actual, EqualityOverrides{})).To(Equal(c.expectedEquals))
+		})
+	}
+}

v2/tools/generator/internal/codegen/pipeline/convert_allof_and_oneof_to_objects.go

@@ -266,7 +266,7 @@ func (s synthesizer) getOneOfName(t astmodel.Type, propIndex int) (propertyNames
 	switch concreteType := t.(type) {
 	case astmodel.InternalTypeName:
 
-		if def, ok := s.defs[concreteType]; ok {
+		if def, ok := s.lookupDefinition(concreteType); ok {
 			// TypeName represents one of our definitions; if we can get a good name from the content
 			// (say, from a OneOf discriminator), we should use that
 			names, err := s.getOneOfName(def.Type(), propIndex)
@@ -467,6 +467,7 @@ func init() {
 	i.AddUnordered(synthesizer.handleAnyType)
 	i.AddUnordered(synthesizer.handleAllOfType)
 	i.AddUnordered(synthesizer.handleTypeName)
+	i.AddUnordered(synthesizer.handleLeafOneOfWithObject)
 	i.AddUnordered(synthesizer.handleOneOf)
 	i.AddUnordered(synthesizer.handleARMIDAndString)
 	i.AddUnordered(synthesizer.handleFlaggedType)
@@ -698,6 +699,22 @@ func (s synthesizer) handleAllOfType(leftAllOf *astmodel.AllOfType, right astmod
 	return s.intersectTypes(result, right)
 }
 
+func (s synthesizer) handleLeafOneOfWithObject(
+	leaf *astmodel.OneOfType,
+	object *astmodel.ObjectType,
+) (astmodel.Type, error) {
+	if !leaf.HasDiscriminatorValue() {
+		// Not a leaf
+		return nil, nil
+	}
+
+	// We have a leaf with a discriminator value, so we need to merge the object into the leaf
+	result := leaf.WithAdditionalPropertyObject(object)
+
+	// Still have a OneOf, need to reprocess it
+	return s.oneOfToObject(result)
+}
+
 // if combining a type with a oneOf that contains that type, the result is that type
 func (s synthesizer) handleOneOf(leftOneOf *astmodel.OneOfType, right astmodel.Type) (astmodel.Type, error) {
 	// if there is an equal case, use that
@@ -730,7 +747,7 @@ func (s synthesizer) handleOneOf(leftOneOf *astmodel.OneOfType, right astmodel.T
 }
 
 func (s synthesizer) handleTypeName(leftName astmodel.InternalTypeName, right astmodel.Type) (astmodel.Type, error) {
-	found, ok := s.defs[leftName]
+	found, ok := s.lookupDefinition(leftName)
 	if !ok {
 		return nil, eris.Errorf("couldn't find type %s", leftName)
 	}
@@ -776,7 +793,7 @@ func (s synthesizer) handleTypeName(leftName astmodel.InternalTypeName, right as
 	}
 
 	// Check to see if we've already redefined this type even though there is only one references
-	//// (this can happen with nesting of typeNames and allOfs because we process things iteratively pair-by-pair).
+	// (this can happen with nesting of typeNames and allOfs because we process things iteratively pair-by-pair).
 	// If we have, we need to merge our new changes with the changes already made.
 	redefined, ok := s.updatedDefs[leftName]
 	for ok {
@@ -936,7 +953,7 @@ func (s synthesizer) simplifyAllOfTypeNames(types []astmodel.Type) []astmodel.Ty
 	result := make([]astmodel.Type, len(types))
 	for i, t := range types {
 		if tn, ok := astmodel.AsInternalTypeName(t); ok {
-			if def, ok := s.defs[tn]; ok {
+			if def, ok := s.lookupDefinition(tn); ok {
 				result[i] = def.Type()
 				foundName = true
 				continue
@@ -984,3 +1001,16 @@ func countTypeReferences(defs astmodel.TypeDefinitionSet) map[astmodel.TypeName]
 
 	return referenceCounts
 }
+
+// lookupDefinition finds a type definition by name, first checking the updated definitions and then the original definitions.
+func (s synthesizer) lookupDefinition(name astmodel.InternalTypeName) (*astmodel.TypeDefinition, bool) {
+	if def, ok := s.updatedDefs[name]; ok {
+		return &def, true
+	}
+
+	if def, ok := s.defs[name]; ok {
+		return &def, true
+	}
+
+	return nil, false
+}