[v3] Skip flattening if properties would clash

provider/pkg/gen/properties.go

@@ -25,6 +25,7 @@ import (
 	"github.com/pulumi/pulumi-azure-native/v2/provider/pkg/convert"
 	"github.com/pulumi/pulumi-azure-native/v2/provider/pkg/openapi"
 	"github.com/pulumi/pulumi-azure-native/v2/provider/pkg/resources"
+	"github.com/pulumi/pulumi-azure-native/v2/provider/pkg/version"
 	"github.com/pulumi/pulumi/pkg/v3/codegen"
 	"github.com/pulumi/pulumi/pkg/v3/codegen/schema"
 	pschema "github.com/pulumi/pulumi/pkg/v3/codegen/schema"
@@ -41,6 +42,17 @@ type propertyBag struct {
 	requiredContainers RequiredContainers
 }
 
+// propertyIntersection returns the property names that are present in both bags.
+func (p *propertyBag) propertyIntersection(other *propertyBag) []string {
+	result := []string{}
+	for propName := range p.properties {
+		if _, ok := other.properties[propName]; ok {
+			result = append(result, propName)
+		}
+	}
+	return result
+}
+
 type RequiredContainers [][]string
 
 // genPropertiesVariant is a set of flags that control the behavior of property generation
@@ -100,64 +112,105 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants
 		// Change the name to lowerCamelCase.
 		sdkName = ToLowerCamel(sdkName)
 
-		flatten, ok := property.Extensions.GetBool(extensionClientFlatten)
-
 		// Flattened properties aren't modelled in the SDK explicitly: their sub-properties are merged directly to the parent.
-		// If the type is marked as a dictionary, ignore the extension and proceed with modeling this property explicitly.
-		// We can't flatten dictionaries in a type-safe manner.
+		// There are two exceptions to this rule, for which we skip the merging:
+		// 1. If the type is marked as a dictionary, ignore the extension and proceed with modeling this property explicitly.
+		//    We can't flatten dictionaries in a type-safe manner.
+		// 2. Sometimes the Azure spec misuses the "x-ms-client-flatten" extension in cases where flattening would cause two
+		//    properties to conflict because the outer and the inner type both have a property with the same name.
+		flatten, ok := property.Extensions.GetBool(extensionClientFlatten)
 		isDict := resolvedProperty.AdditionalProperties != nil
 
 		// TODO: Remove when https://github.com/Azure/azure-rest-api-specs/pull/14550 is rolled back
 		workaroundDelegatedNetworkBreakingChange := property.Ref.String() == "#/definitions/OrchestratorResourceProperties" ||
 			property.Ref.String() == "#/definitions/DelegatedSubnetProperties" ||
 			property.Ref.String() == "#/definitions/DelegatedControllerProperties"
 
-		// TODO: can be removed when https://github.com/pulumi/pulumi-azure-native/pull/3016 is merged
-		if m.resourceName == "DefenderForStorage" && (name == "malwareScanning" || name == "sensitiveDataDiscovery") {
-			flatten = false
-		}
 		// See #3556 and https://github.com/Azure/azure-rest-api-specs/issues/30443
 		// It's ok if a new API version is fixed, this is a no-op then.
 		if m.resourceName == "CIAMTenant" && name == "tier" && strings.HasPrefix(m.module, "azureactivedirectory") {
 			flatten = false
 		}
 
-		if (ok && flatten && !isDict) || workaroundDelegatedNetworkBreakingChange {
-			bag, err := m.genProperties(resolvedProperty, variants.noResponse())
-			if err != nil {
-				return nil, err
+		// Prevent a property collision due to flattening. From v3, we detect and avoid this case automatically.
+		if version.GetVersion().Major < 3 {
+			if m.resourceName == "DefenderForStorage" && (name == "malwareScanning" || name == "sensitiveDataDiscovery") {
+				flatten = false
 			}
 
-			// Check that none of the inner properties already exists on the outer type. This
-			// causes a conflict when flattening, and will probably need to be handled in v3.
-			for propName := range bag.properties {
-				if _, has := result.properties[propName]; has {
+			if (ok && flatten && !isDict) || workaroundDelegatedNetworkBreakingChange {
+				bag, err := m.genProperties(resolvedProperty, variants.noResponse())
+				if err != nil {
+					return nil, err
+				}
+
+				// Check that none of the inner properties already exists on the outer type. This
+				// causes a conflict when flattening, and will be handled in v3.
+				for _, propName := range result.propertyIntersection(bag) {
 					m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", name, propName)] = struct{}{}
 				}
-			}
 
-			// Adjust every property to mark them as flattened.
-			newProperties := map[string]resources.AzureAPIProperty{}
-			for n, value := range bag.properties {
-				// The order of containers is important, so we prepend the outermost name.
-				value.Containers = append([]string{name}, value.Containers...)
-				newProperties[n] = value
-			}
-			bag.properties = newProperties
+				// Adjust every property to mark them as flattened.
+				newProperties := map[string]resources.AzureAPIProperty{}
+				for n, value := range bag.properties {
+					// The order of containers is important, so we prepend the outermost name.
+					value.Containers = append([]string{name}, value.Containers...)
+					newProperties[n] = value
+				}
+				bag.properties = newProperties
 
-			newRequiredContainers := make(RequiredContainers, len(bag.requiredContainers))
-			for i, containers := range bag.requiredContainers {
-				newRequiredContainers[i] = append([]string{name}, containers...)
-			}
-			for _, requiredName := range resolvedSchema.Required {
-				if requiredName == name {
-					newRequiredContainers = append(newRequiredContainers, []string{name})
+				newRequiredContainers := make(RequiredContainers, len(bag.requiredContainers))
+				for i, containers := range bag.requiredContainers {
+					newRequiredContainers[i] = append([]string{name}, containers...)
+				}
+				for _, requiredName := range resolvedSchema.Required {
+					if requiredName == name {
+						newRequiredContainers = append(newRequiredContainers, []string{name})
+					}
 				}
+				bag.requiredContainers = newRequiredContainers
+
+				result.merge(bag)
+				continue
 			}
-			bag.requiredContainers = newRequiredContainers
+		} else {
+			if (ok && flatten && !isDict) || workaroundDelegatedNetworkBreakingChange {
+				bag, err := m.genProperties(resolvedProperty, variants.noResponse())
+				if err != nil {
+					return nil, err
+				}
 
-			result.merge(bag)
-			continue
+				// Check that none of the inner properties already exists on the outer type.
+				conflictingProperties := result.propertyIntersection(bag)
+				if len(conflictingProperties) == 0 {
+					// Adjust every property to mark them as flattened.
+					newProperties := map[string]resources.AzureAPIProperty{}
+					for n, value := range bag.properties {
+						// The order of containers is important, so we prepend the outermost name.
+						value.Containers = append([]string{name}, value.Containers...)
+						newProperties[n] = value
+					}
+					bag.properties = newProperties
+
+					newRequiredContainers := make(RequiredContainers, len(bag.requiredContainers))
+					for i, containers := range bag.requiredContainers {
+						newRequiredContainers[i] = append([]string{name}, containers...)
+					}
+					for _, requiredName := range resolvedSchema.Required {
+						if requiredName == name {
+							newRequiredContainers = append(newRequiredContainers, []string{name})
+						}
+					}
+					bag.requiredContainers = newRequiredContainers
+
+					result.merge(bag)
+					continue
+				} else {
+					for _, propName := range conflictingProperties {
+						m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", name, propName)] = struct{}{}
+					}
+				}
+			}
 		}
 
 		// Skip read-only properties for input types and write-only properties for output types.

provider/pkg/gen/properties_test.go

@@ -194,3 +194,29 @@ func TestNonObjectInvokeResponses(t *testing.T) {
 		assert.NotContains(t, props.properties, resources.SingleValueProperty)
 	})
 }
+
+func TestPropertyUnion(t *testing.T) {
+	t.Run("no conflict", func(t *testing.T) {
+		outer := propertyBag{properties: map[string]resources.AzureAPIProperty{
+			"foo": {},
+		}}
+		inner := propertyBag{properties: map[string]resources.AzureAPIProperty{
+			"bar":  {},
+			"foo2": {},
+		}}
+		assert.Empty(t, outer.propertyIntersection(&inner))
+	})
+
+	t.Run("conflict", func(t *testing.T) {
+		outer := propertyBag{properties: map[string]resources.AzureAPIProperty{
+			"foo":  {},
+			"foo2": {},
+			"bla":  {},
+		}}
+		inner := propertyBag{properties: map[string]resources.AzureAPIProperty{
+			"foo": {},
+			"bar": {},
+		}}
+		assert.Equal(t, []string{"foo"}, outer.propertyIntersection(&inner))
+	})
+}