From 164ff8c5a5172763ddf41137b7d4dd4ffcb90ee7 Mon Sep 17 00:00:00 2001 From: Thomas Kappler Date: Fri, 12 Jan 2024 07:45:06 -0800 Subject: [PATCH 1/3] Skip flattening if properties would clash --- provider/pkg/gen/properties.go | 75 +++++++++++++++++------------ provider/pkg/gen/properties_test.go | 54 +++++++++++++++++++++ 2 files changed, 97 insertions(+), 32 deletions(-) diff --git a/provider/pkg/gen/properties.go b/provider/pkg/gen/properties.go index 492dd3586380..a7f45c3bf5a2 100644 --- a/provider/pkg/gen/properties.go +++ b/provider/pkg/gen/properties.go @@ -20,11 +20,13 @@ import ( "strconv" "strings" + "github.com/blang/semver" "github.com/go-openapi/spec" "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" @@ -100,11 +102,13 @@ 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 @@ -112,8 +116,8 @@ 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") { + // Prevent a property collision due to flatterning. From v3, we detect and avoid this case automatically. + if version.GetVersion().Major < 3 && m.resourceName == "DefenderForStorage" && (name == "malwareScanning" || name == "sensitiveDataDiscovery") { flatten = false } // See #3556 and https://github.com/Azure/azure-rest-api-specs/issues/30443 @@ -128,36 +132,32 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants 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 bag.properties { - if _, has := result.properties[propName]; has { - m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", name, propName)] = struct{}{} + // Check that none of the inner properties already exists on the outer type. + hasConflict := m.hasConflictingPropertiesForFlattening(result, bag, name, version.GetVersion()) + if !hasConflict { + // 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...) } - } - bag.requiredContainers = newRequiredContainers + for _, requiredName := range resolvedSchema.Required { + if requiredName == name { + newRequiredContainers = append(newRequiredContainers, []string{name}) + } + } + bag.requiredContainers = newRequiredContainers - result.merge(bag) - continue + result.merge(bag) + continue + } } // Skip read-only properties for input types and write-only properties for output types. @@ -260,6 +260,17 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants return result, nil } +func (m *moduleGenerator) hasConflictingPropertiesForFlattening(outerBag, innerBag *propertyBag, propertyName string, providerVersion semver.Version) bool { + hasConflict := false + for propName := range innerBag.properties { + if _, ok := outerBag.properties[propName]; ok { + m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", propertyName, propName)] = struct{}{} + hasConflict = true + } + } + return hasConflict && providerVersion.Major >= 3 +} + 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 diff --git a/provider/pkg/gen/properties_test.go b/provider/pkg/gen/properties_test.go index 61c3ef246fb6..27dff2f01845 100644 --- a/provider/pkg/gen/properties_test.go +++ b/provider/pkg/gen/properties_test.go @@ -3,6 +3,7 @@ package gen import ( "testing" + "github.com/blang/semver" "github.com/go-openapi/spec" "github.com/pulumi/pulumi-azure-native/v2/provider/pkg/openapi" "github.com/pulumi/pulumi-azure-native/v2/provider/pkg/resources" @@ -194,3 +195,56 @@ func TestNonObjectInvokeResponses(t *testing.T) { assert.NotContains(t, props.properties, resources.SingleValueProperty) }) } + +func TestHasConflictingPropertiesForFlattening(t *testing.T) { + hasConflict := func(providerVersion string, outer, inner map[string]resources.AzureAPIProperty) bool { + m := moduleGenerator{ + flattenedPropertyConflicts: map[string]struct{}{}, + } + + outerBag := &propertyBag{properties: outer} + innerBag := &propertyBag{properties: inner} + + return m.hasConflictingPropertiesForFlattening(outerBag, innerBag, "foo", semver.MustParse(providerVersion)) + } + + t.Run("no conflict v2", func(t *testing.T) { + outer := map[string]resources.AzureAPIProperty{ + "foo": {}, + } + inner := map[string]resources.AzureAPIProperty{ + "bar": {}, + } + assert.False(t, hasConflict("2.0.0", outer, inner)) + }) + + t.Run("no conflict v3", func(t *testing.T) { + outer := map[string]resources.AzureAPIProperty{ + "foo": {}, + } + inner := map[string]resources.AzureAPIProperty{ + "bar": {}, + } + assert.False(t, hasConflict("3.0.0", outer, inner)) + }) + + t.Run("conflict v2", func(t *testing.T) { + outer := map[string]resources.AzureAPIProperty{ + "foo": {}, + "bla": {}} + inner := map[string]resources.AzureAPIProperty{ + "foo": {}, + "bar": {}} + assert.False(t, hasConflict("2.0.0", outer, inner)) + }) + + t.Run("conflict v3", func(t *testing.T) { + outer := map[string]resources.AzureAPIProperty{ + "foo": {}, + "bla": {}} + inner := map[string]resources.AzureAPIProperty{ + "foo": {}, + "bar": {}} + assert.True(t, hasConflict("3.0.0", outer, inner)) + }) +} From 20be23e63855372ca754765540f6ea82fd8b63c2 Mon Sep 17 00:00:00 2001 From: Thomas Kappler Date: Wed, 18 Dec 2024 17:25:01 +0100 Subject: [PATCH 2/3] Fully separate the v2 and v3 code paths for readability and easy cleanup --- provider/pkg/gen/properties.go | 76 ++++++++++++++++++++++------- provider/pkg/gen/properties_test.go | 50 +++++-------------- 2 files changed, 70 insertions(+), 56 deletions(-) diff --git a/provider/pkg/gen/properties.go b/provider/pkg/gen/properties.go index a7f45c3bf5a2..6c4d607b59d8 100644 --- a/provider/pkg/gen/properties.go +++ b/provider/pkg/gen/properties.go @@ -20,7 +20,6 @@ import ( "strconv" "strings" - "github.com/blang/semver" "github.com/go-openapi/spec" "github.com/pulumi/pulumi-azure-native/v2/provider/pkg/convert" @@ -116,25 +115,30 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants property.Ref.String() == "#/definitions/DelegatedSubnetProperties" || property.Ref.String() == "#/definitions/DelegatedControllerProperties" - // Prevent a property collision due to flatterning. From v3, we detect and avoid this case automatically. - if version.GetVersion().Major < 3 && 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 flatterning. 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. - hasConflict := m.hasConflictingPropertiesForFlattening(result, bag, name, version.GetVersion()) - if !hasConflict { + 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 propertyUnion(result, 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 { @@ -158,6 +162,44 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants result.merge(bag) continue } + } else { + 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. + conflictingProperties := propertyUnion(result, 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. @@ -260,15 +302,15 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants return result, nil } -func (m *moduleGenerator) hasConflictingPropertiesForFlattening(outerBag, innerBag *propertyBag, propertyName string, providerVersion semver.Version) bool { - hasConflict := false +// propertyUnion returns the property names that are present in both bags. +func propertyUnion(outerBag, innerBag *propertyBag) []string { + result := []string{} for propName := range innerBag.properties { if _, ok := outerBag.properties[propName]; ok { - m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", propertyName, propName)] = struct{}{} - hasConflict = true + result = append(result, propName) } } - return hasConflict && providerVersion.Major >= 3 + return result } func (m *moduleGenerator) genProperty(name string, schema *spec.Schema, context *openapi.ReferenceContext, resolvedProperty *openapi.Schema, variants genPropertiesVariant) (*pschema.PropertySpec, *resources.AzureAPIProperty, error) { diff --git a/provider/pkg/gen/properties_test.go b/provider/pkg/gen/properties_test.go index 27dff2f01845..ac502c8d1416 100644 --- a/provider/pkg/gen/properties_test.go +++ b/provider/pkg/gen/properties_test.go @@ -3,7 +3,6 @@ package gen import ( "testing" - "github.com/blang/semver" "github.com/go-openapi/spec" "github.com/pulumi/pulumi-azure-native/v2/provider/pkg/openapi" "github.com/pulumi/pulumi-azure-native/v2/provider/pkg/resources" @@ -196,55 +195,28 @@ func TestNonObjectInvokeResponses(t *testing.T) { }) } -func TestHasConflictingPropertiesForFlattening(t *testing.T) { - hasConflict := func(providerVersion string, outer, inner map[string]resources.AzureAPIProperty) bool { - m := moduleGenerator{ - flattenedPropertyConflicts: map[string]struct{}{}, - } - - outerBag := &propertyBag{properties: outer} - innerBag := &propertyBag{properties: inner} - - return m.hasConflictingPropertiesForFlattening(outerBag, innerBag, "foo", semver.MustParse(providerVersion)) - } - - t.Run("no conflict v2", func(t *testing.T) { +func TestPropertyUnion(t *testing.T) { + t.Run("no conflict", func(t *testing.T) { outer := map[string]resources.AzureAPIProperty{ "foo": {}, } inner := map[string]resources.AzureAPIProperty{ - "bar": {}, + "bar": {}, + "foo2": {}, } - assert.False(t, hasConflict("2.0.0", outer, inner)) + assert.Empty(t, propertyUnion(&propertyBag{properties: outer}, &propertyBag{properties: inner})) }) - t.Run("no conflict v3", func(t *testing.T) { + t.Run("conflict", func(t *testing.T) { outer := map[string]resources.AzureAPIProperty{ - "foo": {}, + "foo": {}, + "foo2": {}, + "bla": {}, } inner := map[string]resources.AzureAPIProperty{ + "foo": {}, "bar": {}, } - assert.False(t, hasConflict("3.0.0", outer, inner)) - }) - - t.Run("conflict v2", func(t *testing.T) { - outer := map[string]resources.AzureAPIProperty{ - "foo": {}, - "bla": {}} - inner := map[string]resources.AzureAPIProperty{ - "foo": {}, - "bar": {}} - assert.False(t, hasConflict("2.0.0", outer, inner)) - }) - - t.Run("conflict v3", func(t *testing.T) { - outer := map[string]resources.AzureAPIProperty{ - "foo": {}, - "bla": {}} - inner := map[string]resources.AzureAPIProperty{ - "foo": {}, - "bar": {}} - assert.True(t, hasConflict("3.0.0", outer, inner)) + assert.Equal(t, []string{"foo"}, propertyUnion(&propertyBag{properties: outer}, &propertyBag{properties: inner})) }) } From 0bc1091fe2c93d129ca9d452cd9b2138d5e8cd4e Mon Sep 17 00:00:00 2001 From: Thomas Kappler Date: Wed, 8 Jan 2025 09:51:57 +0100 Subject: [PATCH 3/3] PR comments --- provider/pkg/gen/properties.go | 28 ++++++++++++++-------------- provider/pkg/gen/properties_test.go | 20 ++++++++++---------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/provider/pkg/gen/properties.go b/provider/pkg/gen/properties.go index 6c4d607b59d8..c096433b35e6 100644 --- a/provider/pkg/gen/properties.go +++ b/provider/pkg/gen/properties.go @@ -42,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 @@ -121,7 +132,7 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants flatten = false } - // Prevent a property collision due to flatterning. From v3, we detect and avoid this case automatically. + // 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 @@ -135,7 +146,7 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants // 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 propertyUnion(result, bag) { + for _, propName := range result.propertyIntersection(bag) { m.flattenedPropertyConflicts[fmt.Sprintf("%s.%s", name, propName)] = struct{}{} } @@ -170,7 +181,7 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants } // Check that none of the inner properties already exists on the outer type. - conflictingProperties := propertyUnion(result, bag) + conflictingProperties := result.propertyIntersection(bag) if len(conflictingProperties) == 0 { // Adjust every property to mark them as flattened. newProperties := map[string]resources.AzureAPIProperty{} @@ -302,17 +313,6 @@ func (m *moduleGenerator) genProperties(resolvedSchema *openapi.Schema, variants return result, nil } -// propertyUnion returns the property names that are present in both bags. -func propertyUnion(outerBag, innerBag *propertyBag) []string { - result := []string{} - for propName := range innerBag.properties { - if _, ok := outerBag.properties[propName]; ok { - result = append(result, propName) - } - } - return result -} - 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 diff --git a/provider/pkg/gen/properties_test.go b/provider/pkg/gen/properties_test.go index ac502c8d1416..41f1cdc9f40a 100644 --- a/provider/pkg/gen/properties_test.go +++ b/provider/pkg/gen/properties_test.go @@ -197,26 +197,26 @@ func TestNonObjectInvokeResponses(t *testing.T) { func TestPropertyUnion(t *testing.T) { t.Run("no conflict", func(t *testing.T) { - outer := map[string]resources.AzureAPIProperty{ + outer := propertyBag{properties: map[string]resources.AzureAPIProperty{ "foo": {}, - } - inner := map[string]resources.AzureAPIProperty{ + }} + inner := propertyBag{properties: map[string]resources.AzureAPIProperty{ "bar": {}, "foo2": {}, - } - assert.Empty(t, propertyUnion(&propertyBag{properties: outer}, &propertyBag{properties: inner})) + }} + assert.Empty(t, outer.propertyIntersection(&inner)) }) t.Run("conflict", func(t *testing.T) { - outer := map[string]resources.AzureAPIProperty{ + outer := propertyBag{properties: map[string]resources.AzureAPIProperty{ "foo": {}, "foo2": {}, "bla": {}, - } - inner := map[string]resources.AzureAPIProperty{ + }} + inner := propertyBag{properties: map[string]resources.AzureAPIProperty{ "foo": {}, "bar": {}, - } - assert.Equal(t, []string{"foo"}, propertyUnion(&propertyBag{properties: outer}, &propertyBag{properties: inner})) + }} + assert.Equal(t, []string{"foo"}, outer.propertyIntersection(&inner)) }) }