[v3] Skip flattening if properties would clash - PR #2

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
@@ -112,52 +124,54 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants
 			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
+		if version.GetVersion().Major < 3 {
+			// Prevent a property collision due to flattening. From v3, we detect and avoid this case automatically.
+			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 probably need to 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
+				flattenProperties(bag, name, resolvedSchema)
+				result.merge(bag)
+				continue
 			}
-			bag.properties = newProperties
+		} else { // v3+: don't flatten when there are conflicts
+			if (ok && flatten && !isDict) || workaroundDelegatedNetworkBreakingChange {
+				bag, err := m.genProperties(resolvedProperty, variants.noResponse())
+				if err != nil {
+					return nil, err
+				}
 
-			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})
+				// Check that none of the inner properties already exists on the outer type.
+				conflictingProperties := result.propertyIntersection(bag)
+				if len(conflictingProperties) == 0 {
+					flattenProperties(bag, name, resolvedSchema)
+
+					result.merge(bag)
+					continue
+				} else {
+					for _, propName := range conflictingProperties {
+						m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", name, propName)] = struct{}{}
+					}
 				}
 			}
-			bag.requiredContainers = newRequiredContainers
-
-			result.merge(bag)
-			continue
 		}
 
 		// Skip read-only properties for input types and write-only properties for output types.
@@ -260,6 +274,29 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants
 	return result, nil
 }
 
+// flattenProperties marks every property as flattened and updates the requiredContainers. Modifies `bag` in place.
+func flattenProperties(bag *propertyBag, name string, resolvedSchema *openapi.Schema) {
+	// 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
+}
+
 func (m *moduleGenerator) genProperty(name string, schema *spec.Schema, context *openapi.ReferenceContext, resolvedProperty *openapi.Schema, variants genPropertiesVariant) (*pschema.PropertySpec, *resources.AzureAPIProperty, error) {
 	// Identify properties which are also available as standalone resources and mark them to be maintained if not specified inline.
 	// Ignore types as we only support top-level resource properties

provider/pkg/gen/properties_test.go

@@ -194,3 +194,30 @@ func TestNonObjectInvokeResponses(t *testing.T) {
 		assert.NotContains(t, props.properties, resources.SingleValueProperty)
 	})
 }
+
+func TestPropertyIntersection(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))
+	})
+
+}