Ensure we prune empty types (#2697)

Fixes #1474

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

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),

v2/tools/generator/internal/codegen/golden_files_test.go

@@ -160,9 +160,6 @@ 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)

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
+		})
+}

v2/tools/generator/internal/codegen/testdata/AdditionalProperties/Generates_nothing_if_set_to_false.golden

@@ -6,28 +6,18 @@ package v1beta20200101
 import (
 	v20200101s "github.com/Azure/azure-service-operator/testing/test/v1beta20200101storage"
 	"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
-	"github.com/pkg/errors"
 )
 
 // Generated from: https://test.test/schemas/2020-01-01/test.json
 type Test struct {
-	Tags *Test_Tags `json:"tags,omitempty"`
+	Foo *string `json:"foo,omitempty"`
 }
 
 // AssignProperties_From_Test populates our Test from the provided source Test
 func (test *Test) AssignProperties_From_Test(source *v20200101s.Test) error {
 
-	// Tags
-	if source.Tags != nil {
-		var tag Test_Tags
-		err := tag.AssignProperties_From_Test_Tags(source.Tags)
-		if err != nil {
-			return errors.Wrap(err, "calling AssignProperties_From_Test_Tags() to populate field Tags")
-		}
-		test.Tags = &tag
-	} else {
-		test.Tags = nil
-	}
+	// Foo
+	test.Foo = genruntime.ClonePointerToString(source.Foo)
 
 	// No error
 	return nil
@@ -38,43 +28,8 @@ func (test *Test) AssignProperties_To_Test(destination *v20200101s.Test) error {
 	// Create a new property bag
 	propertyBag := genruntime.NewPropertyBag()
 
-	// Tags
-	if test.Tags != nil {
-		var tag v20200101s.Test_Tags
-		err := test.Tags.AssignProperties_To_Test_Tags(&tag)
-		if err != nil {
-			return errors.Wrap(err, "calling AssignProperties_To_Test_Tags() to populate field Tags")
-		}
-		destination.Tags = &tag
-	} else {
-		destination.Tags = nil
-	}
-
-	// Update the property bag
-	if len(propertyBag) > 0 {
-		destination.PropertyBag = propertyBag
-	} else {
-		destination.PropertyBag = nil
-	}
-
-	// No error
-	return nil
-}
-
-type Test_Tags struct {
-}
-
-// AssignProperties_From_Test_Tags populates our Test_Tags from the provided source Test_Tags
-func (tags *Test_Tags) AssignProperties_From_Test_Tags(source *v20200101s.Test_Tags) error {
-
-	// No error
-	return nil
-}
-
-// AssignProperties_To_Test_Tags populates the provided destination Test_Tags from our Test_Tags
-func (tags *Test_Tags) AssignProperties_To_Test_Tags(destination *v20200101s.Test_Tags) error {
-	// Create a new property bag
-	propertyBag := genruntime.NewPropertyBag()
+	// Foo
+	destination.Foo = genruntime.ClonePointerToString(test.Foo)
 
 	// Update the property bag
 	if len(propertyBag) > 0 {

v2/tools/generator/internal/codegen/testdata/AdditionalProperties/Generates_nothing_if_set_to_false.json

@@ -1,11 +1,13 @@
 {
-    "$comment": "Here we check that if 'additionalProperties' is explicitly set to 'false' (and there are no other fields) the generated struct will be empty",
-
+    "$comment": "Here we check that if 'additionalProperties' is explicitly set to 'false' (and there are no other fields) the generated struct will be empty (and removed)",
     "id": "https://test.test/schemas/2020-01-01/test.json",
     "$schema": "http://json-schema.org/draft-04/schema#",
     "title": "Test",
     "type": "object",
     "properties": {
+        "foo": {
+            "type": "string"
+        },
         "tags": {
             "type": "object",
             "additionalProperties": false

v2/tools/generator/internal/codegen/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{})
 }

v2/tools/generator/internal/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

v2/tools/generator/internal/codegen/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

v2/tools/generator/internal/codegen/testdata/TestGolden_NewTestCodeGeneratorCreatesRightPipeline.golden

@@ -20,6 +20,7 @@ determineResourceOwnership                           Determine ARM resource rela
 removeAliases                                        Remove type aliases
 stripUnused                                          Strip unused types for test
 assertTypesStructureValid                            Verify that all local TypeNames refer to a type
+removeEmptyObjects                                   Remove empty Objects
 verifyNoErroredTypes                                 Verify there are no ErroredType's containing errors
 stripUnused                                          Strip unused types for test
 replaceAnyTypeWithJSON                               Replace properties using interface{} with arbitrary JSON