Ensure we prune empty types

Fixes Azure#1474
matthchr · Jan 24, 2023 · 13dca36 · 13dca36
1 parent e8a0ade
commit 13dca36
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 34 deletions.
diff --git a/v2/azure-arm.yaml b/v2/azure-arm.yaml
@@ -136,63 +136,55 @@ typeTransformers:
     remove: true
     because: AccessKeys is only set on response to PUT/CREATE, but we fill out Status via GET so this field is always empty. It also contains secrets we wouldn't want to expose in status anyway.
 
-  # Deal with readonly properties that were not properly pruned in the JSON schema
+  # Deal with UserAssignedIdentities, which we don't currently handle correctly
+  # TODO: For now we prune these, see https://github.com/Azure/azure-service-operator/issues/2528 for supporting them better
   - name: BatchAccountIdentity
     group: batch
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - name: Identity
     group: servicebus
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - name: Identity
     group: storage
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - name: ManagedServiceIdentity
     group: documentdb
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
-  - name: PrivateEndpointConnectionProperties
-    group: storage
-    property: PrivateEndpoint
-    remove: true
-    because: The PrivateEndpoint property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - group: compute
     name: VirtualMachineScaleSetIdentity;VirtualMachineScaleSetIdentity_Status;VirtualMachineIdentity;VirtualMachineIdentity_Status
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - group: eventhub
     name: Identity
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - group: dbformysql
     name: Identity
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
-
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - group: containerinstance
     name: ContainerGroupIdentity
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
   - group: appconfiguration
     name: ResourceIdentity
     property: UserAssignedIdentities
     remove: true
-    because: The UserAssignedIdentities property is entirely readOnly but is modelled poorly in the JSON schemas. See discussion on https://github.com/Azure/azure-resource-manager-schemas/issues/835
+    because: We don't handle UserAssignedIdentities properly currently. See https://github.com/Azure/azure-service-operator/issues/2528
+
+  # Properties that should have been marked as ReadOnly in the Swagger
   - name: Namespace_Properties_Spec
     group: eventhub
     property: PrivateEndpointConnections

diff --git a/v2/tools/generator/internal/codegen/code_generator.go b/v2/tools/generator/internal/codegen/code_generator.go
@@ -125,7 +125,6 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration
 		// Strip out redundant type aliases
 		pipeline.RemoveTypeAliases(),
 
-		// Collapse cross group references
 		pipeline.CollapseCrossGroupReferences(),
 
 		pipeline.StripUnreferencedTypeDefinitions(),
@@ -134,6 +133,10 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration
 
 		pipeline.RemoveEmbeddedResources(configuration).UsedFor(pipeline.ARMTarget),
 
+		// This is currently also run as part of RemoveEmbeddedResources and so is technically not needed here,
+		// but we include it to hedge against future changes
+		pipeline.RemoveEmptyObjects(),
+
 		// Apply export filters before generating
 		// ARM types for resources etc:
 		pipeline.ApplyExportFilters(configuration),

diff --git a/v2/tools/generator/internal/codegen/golden_files_test.go b/v2/tools/generator/internal/codegen/golden_files_test.go
@@ -160,12 +160,10 @@ func NewTestCodeGenerator(
 			pipeline.CheckForAnyTypeStageID,
 			pipeline.CreateResourceExtensionsStageID,
 			pipeline.CreateTypesForBackwardCompatibilityID,
-			// TODO: Once the stage is enabled in the pipeline, we may need to remove it here for testing
-			// pipeline.InjectHubFunctionStageID,
-			// pipeline.ImplementConvertibleInterfaceStageId,
 			pipeline.ReportOnTypesAndVersionsStageID,
 			pipeline.ReportResourceVersionsStageID,
-			pipeline.ReportResourceStructureStageId)
+			pipeline.ReportResourceStructureStageId,
+			pipeline.RemoveEmptyObjectsStageId)
 		if !testConfig.HasARMResources {
 			codegen.RemoveStages(
 				pipeline.CreateARMTypesStageID,

diff --git a/v2/tools/generator/internal/codegen/pipeline/remove_empty_objects.go b/v2/tools/generator/internal/codegen/pipeline/remove_empty_objects.go
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package pipeline
+
+import (
+	"context"
+
+	"github.com/Azure/azure-service-operator/v2/tools/generator/internal/codegen/embeddedresources"
+)
+
+const RemoveEmptyObjectsStageId = "removeEmptyObjects"
+
+// RemoveEmptyObjects removes Definitions which are empty (an object type with no properties)
+func RemoveEmptyObjects() *Stage {
+	return NewStage(
+		RemoveEmptyObjectsStageId,
+		"Remove empty Objects",
+		func(ctx context.Context, state *State) (*State, error) {
+			updatedDefs, err := embeddedresources.RemoveEmptyObjects(state.Definitions())
+			if err != nil {
+				return nil, err
+			}
+
+			return state.WithDefinitions(updatedDefs), nil
+		})
+}
diff --git a/...gen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden b/...gen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden
@@ -43,10 +43,7 @@ type FakeResourceParameters struct {
 	APIVersion FakeResource_APIVersion_Spec `json:"apiVersion,omitempty"`
 
 	// +kubebuilder:validation:Required
-	Name string `json:"name,omitempty"`
-
-	// Properties: Generated from: https://test.test/schemas/2020-01-01/test.json#/definitions/Properties
-	Properties                *Properties         `json:"properties,omitempty"`
+	Name                      string              `json:"name,omitempty"`
 	ResourceGroupName         string              `json:"resourceGroupName,omitempty"`
 	ResourceGroupNameRef      *v1alpha1.Reference `json:"resourceGroupNameRef,omitempty"`
 	ResourceGroupNameSelector *v1alpha1.Selector  `json:"resourceGroupNameSelector,omitempty"`
@@ -65,10 +62,6 @@ type FakeResource_Type_Spec string
 
 const FakeResource_Type_Spec_MicrosoftAzureFakeResource = FakeResource_Type_Spec("Microsoft.Azure/FakeResource")
 
-// Generated from: https://test.test/schemas/2020-01-01/test.json#/definitions/Properties
-type Properties struct {
-}
-
 func init() {
 	SchemeBuilder.Register(&FakeResource{}, &FakeResourceList{})
 }
diff --git a/...rnal/codegen/testdata/TestGolden_NewARMCodeGeneratorFromConfigCreatesRightPipeline.golden b/...rnal/codegen/testdata/TestGolden_NewARMCodeGeneratorFromConfigCreatesRightPipeline.golden
@@ -22,6 +22,7 @@ collapseCrossGroupReferences                                 Find and remove cro
 stripUnreferenced                                            Strip unreferenced types
 assertTypesStructureValid                                    Verify that all local TypeNames refer to a type
 removeEmbeddedResources                           azure      Remove properties that point to embedded resources. Only removes structural aspects of embedded resources, Id/ARMId references are retained.
+removeEmptyObjects                                           Remove empty Objects
 filterTypes                                                  Apply export filters to reduce the number of generated types
 verifyNoErroredTypes                                         Verify there are no ErroredType's containing errors
 stripUnreferenced                                            Strip unreferenced types

diff --git a/...degen/testdata/TestGolden_NewCrossplaneCodeGeneratorFromConfigCreatesRightPipeline.golden b/...degen/testdata/TestGolden_NewCrossplaneCodeGeneratorFromConfigCreatesRightPipeline.golden
@@ -21,6 +21,7 @@ removeAliases                                           Remove type aliases
 collapseCrossGroupReferences                            Find and remove cross group references
 stripUnreferenced                                       Strip unreferenced types
 assertTypesStructureValid                               Verify that all local TypeNames refer to a type
+removeEmptyObjects                                      Remove empty Objects
 filterTypes                                             Apply export filters to reduce the number of generated types
 verifyNoErroredTypes                                    Verify there are no ErroredType's containing errors
 stripUnreferenced                                       Strip unreferenced types