Change Properties to a map (#1655)

hack/generator/pkg/astmodel/armconversion/convert_to_arm_function_builder.go

@@ -207,7 +207,7 @@ func (builder *convertToARMBuilder) flattenedPropertyHandler(
 
 	// collect any fromProps that were flattened from the to-prop
 	var fromProps []*astmodel.PropertyDefinition
-	for _, prop := range fromType.Properties() {
+	for _, prop := range fromType.Properties().AsSlice() {
 		if prop.WasFlattenedFrom(toPropName) {
 			fromProps = append(fromProps, prop)
 		}

hack/generator/pkg/astmodel/armconversion/shared.go

@@ -107,7 +107,7 @@ func generateTypeConversionAssignments(
 	propertyHandler func(toProp *astmodel.PropertyDefinition, fromType *astmodel.ObjectType) []dst.Stmt) []dst.Stmt {
 
 	var result []dst.Stmt
-	for _, toField := range toType.Properties() {
+	for _, toField := range toType.Properties().AsSlice() {
 		fieldConversionStmts := propertyHandler(toField, fromType)
 		if len(fieldConversionStmts) > 0 {
 			result = append(result, &dst.EmptyStmt{

hack/generator/pkg/astmodel/object_type.go

@@ -19,7 +19,7 @@ import (
 // ObjectType represents an (unnamed) object type
 type ObjectType struct {
 	embedded   map[TypeName]*PropertyDefinition
-	properties map[PropertyName]*PropertyDefinition
+	properties PropertySet
 	functions  map[string]Function
 	testcases  map[string]TestCase
 	InterfaceImplementer
@@ -44,7 +44,7 @@ var _ TestCaseContainer = &ObjectType{}
 func NewObjectType() *ObjectType {
 	return &ObjectType{
 		embedded:             make(map[TypeName]*PropertyDefinition),
-		properties:           make(map[PropertyName]*PropertyDefinition),
+		properties:           make(PropertySet),
 		functions:            make(map[string]Function),
 		testcases:            make(map[string]TestCase),
 		InterfaceImplementer: MakeInterfaceImplementer(),
@@ -102,19 +102,8 @@ func defineField(fieldName string, fieldType dst.Expr, tag string) *dst.Field {
 }
 
 // Properties returns all the property definitions
-// A sorted slice is returned to preserve immutability and provide determinism
-func (objectType *ObjectType) Properties() []*PropertyDefinition {
-	var result []*PropertyDefinition
-	for _, property := range objectType.properties {
-		result = append(result, property)
-	}
-
-	// Sorted so that it's always consistent
-	sort.Slice(result, func(left int, right int) bool {
-		return result[left].propertyName < result[right].propertyName
-	})
-
-	return result
+func (objectType *ObjectType) Properties() PropertySet {
+	return objectType.properties.Copy()
 }
 
 // Property returns the details of a specific property based on its unique case sensitive name
@@ -178,7 +167,7 @@ func (objectType *ObjectType) AsType(codeGenerationContext *CodeGenerationContex
 		fields = append(fields, f.AsField(codeGenerationContext))
 	}
 
-	for _, f := range objectType.Properties() {
+	for _, f := range objectType.properties.AsSlice() {
 		fields = append(fields, f.AsField(codeGenerationContext))
 	}
 
@@ -560,7 +549,7 @@ func (objectType *ObjectType) WriteDebugDescription(builder *strings.Builder, _
 // FindPropertyWithTagValue finds the property with the given tag and value if it exists. The boolean return
 // is false if no match can be found.
 func (objectType *ObjectType) FindPropertyWithTagValue(tag string, value string) (*PropertyDefinition, bool) {
-	for _, prop := range objectType.Properties() {
+	for _, prop := range objectType.properties {
 		values, ok := prop.Tag(tag)
 		if !ok {
 			continue

hack/generator/pkg/astmodel/object_type_test.go

@@ -50,7 +50,7 @@ func Test_Properties_GivenEmptyObject_ReturnsEmptySlice(t *testing.T) {
 func Test_Properties_GivenObjectWithProperties_ReturnsExpectedSortedSlice(t *testing.T) {
 	g := NewGomegaWithT(t)
 	object := EmptyObjectType.WithProperties(fullName, familyName, knownAs, gender)
-	properties := object.Properties()
+	properties := object.Properties().AsSlice()
 	g.Expect(properties).To(HaveLen(4))
 	g.Expect(properties[0]).To(Equal(familyName))
 	g.Expect(properties[1]).To(Equal(fullName))
@@ -168,8 +168,9 @@ func Test_WithProperty_GivenEmptyObject_ReturnsPopulatedObject(t *testing.T) {
 	empty := EmptyObjectType
 	object := empty.WithProperty(fullName)
 	g.Expect(empty).NotTo(Equal(object)) // Ensure the original wasn't modified
-	g.Expect(object.properties).To(HaveLen(1))
-	g.Expect(object.Properties()[0]).To(Equal(fullName))
+	properties := object.Properties().AsSlice()
+	g.Expect(properties).To(HaveLen(1))
+	g.Expect(properties[0]).To(Equal(fullName))
 }
 
 func Test_WithProperty_GivenPopulatedObjectAndNewProperty_ReturnsExpectedObject(t *testing.T) {
@@ -181,11 +182,13 @@ func Test_WithProperty_GivenPopulatedObjectAndNewProperty_ReturnsExpectedObject(
 	modified := original.WithProperty(nickname)
 
 	g.Expect(original).NotTo(Equal(modified))
-	g.Expect(modified.Properties()).To(ContainElement(fullName))
-	g.Expect(modified.Properties()).To(ContainElement(familyName))
-	g.Expect(modified.Properties()).To(ContainElement(knownAs))
-	g.Expect(modified.Properties()).To(ContainElement(gender))
-	g.Expect(modified.Properties()).To(ContainElement(nickname))
+
+	modifiedProperties := modified.Properties()
+	g.Expect(modifiedProperties).To(ContainElement(fullName))
+	g.Expect(modifiedProperties).To(ContainElement(familyName))
+	g.Expect(modifiedProperties).To(ContainElement(knownAs))
+	g.Expect(modifiedProperties).To(ContainElement(gender))
+	g.Expect(modifiedProperties).To(ContainElement(nickname))
 }
 
 func Test_WithProperty_GivenPopulatedObjectAndReplacementProperty_ReturnsExpectedObject(t *testing.T) {

hack/generator/pkg/astmodel/one_of_json_marshal_function.go

@@ -61,7 +61,7 @@ func (f *OneOfJSONMarshalFunction) AsFunc(
 
 	var statements []dst.Stmt
 
-	for _, property := range f.oneOfObject.Properties() {
+	for _, property := range f.oneOfObject.Properties().AsSlice() {
 
 		ifStatement := dst.IfStmt{
 			Cond: astbuilder.NotNil(

hack/generator/pkg/astmodel/property_container.go

@@ -9,8 +9,7 @@ package astmodel
 // Provides readonly access as we need to use a TypeVisitor for modifications to preserve type wrapping
 type PropertyContainer interface {
 	// Properties returns all the properties from this container
-	// A sorted slice is returned to preserve immutability and provide determinism
-	Properties() []*PropertyDefinition
+	Properties() PropertySet
 
 	// Property returns the property and true if the named property is found, nil and false otherwise
 	Property(name PropertyName) (*PropertyDefinition, bool)

hack/generator/pkg/astmodel/property_set.go

@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package astmodel
+
+import (
+	"sort"
+)
+
+// PropertySet wraps a set of property definitions, indexed by name, along with some convenience methods
+type PropertySet map[PropertyName]*PropertyDefinition
+
+// NewPropertySet creates a new set of properties
+func NewPropertySet(properties ...*PropertyDefinition) PropertySet {
+	result := make(PropertySet)
+	for _, prop := range properties {
+		result[prop.PropertyName()] = prop
+	}
+
+	return result
+}
+
+// AsSlice returns all the properties in a slice, sorted alphabetically by name
+func (p PropertySet) AsSlice() []*PropertyDefinition {
+	var result []*PropertyDefinition
+	for _, prop := range p {
+		result = append(result, prop)
+	}
+
+	// Sort it so that it's always consistent
+	sort.Slice(result, func(left int, right int) bool {
+		return result[left].propertyName < result[right].propertyName
+	})
+
+	return result
+}
+
+// Add updates the set by including the provided property
+// Any existing definition by that name will be overwritten if present
+func (p PropertySet) Add(property *PropertyDefinition) {
+	p[property.propertyName] = property
+}
+
+// Copy returns a new property set with the same properties as this one
+func (p PropertySet) Copy() PropertySet {
+	result := make(PropertySet, len(p))
+	for name, prop := range p {
+		result[name] = prop
+	}
+
+	return result
+}

hack/generator/pkg/astmodel/property_set_test.go

@@ -0,0 +1,50 @@
+/*
+ * Copyright (c) Microsoft Corporation.
+ * Licensed under the MIT license.
+ */
+
+package astmodel
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestPropertySet_NewPropertySet_ReturnsEmptySet(t *testing.T) {
+	g := NewGomegaWithT(t)
+	set := NewPropertySet()
+	g.Expect(set).To(HaveLen(0))
+}
+
+func TestPropertySet_NewPropertySetWithProperties_ReturnsSetContainingProperties(t *testing.T) {
+	g := NewGomegaWithT(t)
+	set := NewPropertySet(fullName, familyName, knownAs, gender)
+	g.Expect(set).To(ContainElements(fullName, familyName, knownAs, gender))
+	g.Expect(set).To(HaveLen(4))
+}
+
+func TestPropertySet_AsSlice_ReturnsSliceContainingProperties(t *testing.T) {
+	g := NewGomegaWithT(t)
+	slice := NewPropertySet(fullName, familyName, knownAs, gender).AsSlice()
+	g.Expect(slice).To(ContainElements(fullName, familyName, knownAs, gender))
+	g.Expect(slice).To(HaveLen(4))
+}
+
+func TestPropertySet_Add_ReturnsExpectedSet(t *testing.T) {
+	g := NewGomegaWithT(t)
+	set := NewPropertySet()
+	set.Add(fullName)
+	set.Add(familyName)
+	set.Add(knownAs)
+	set.Add(gender)
+	g.Expect(set).To(ContainElements(fullName, familyName, knownAs, gender))
+	g.Expect(set).To(HaveLen(4))
+}
+
+func TestPropertySet_Copy_ReturnsExpectedSet(t *testing.T) {
+	g := NewGomegaWithT(t)
+	clone := NewPropertySet(fullName, familyName, knownAs, gender).Copy()
+	g.Expect(clone).To(ContainElements(fullName, familyName, knownAs, gender))
+	g.Expect(clone).To(HaveLen(4))
+}

hack/generator/pkg/astmodel/resource_type.go

@@ -24,7 +24,7 @@ type ResourceType struct {
 	status           Type
 	isStorageVersion bool
 	owner            *TypeName
-	properties       map[PropertyName]*PropertyDefinition
+	properties       PropertySet
 	functions        map[string]Function
 	testcases        map[string]TestCase
 	InterfaceImplementer
@@ -35,7 +35,7 @@ func NewResourceType(specType Type, statusType Type) *ResourceType {
 	result := &ResourceType{
 		isStorageVersion:     false,
 		owner:                nil,
-		properties:           make(map[PropertyName]*PropertyDefinition),
+		properties:           make(PropertySet),
 		functions:            make(map[string]Function),
 		testcases:            make(map[string]TestCase),
 		InterfaceImplementer: MakeInterfaceImplementer(),
@@ -334,25 +334,14 @@ func (resource *ResourceType) WithoutProperty(name PropertyName) *ResourceType {
 }
 
 // Properties returns all the properties from this resource type
-// An ordered slice is returned to preserve immutability and provide determinism
-func (resource *ResourceType) Properties() []*PropertyDefinition {
-	result := []*PropertyDefinition{
-		resource.createSpecProperty(),
-	}
+func (resource *ResourceType) Properties() PropertySet {
+	result := resource.properties.Copy()
 
+	result.Add(resource.createSpecProperty())
 	if resource.status != nil {
-		result = append(result, resource.createStatusProperty())
+		result.Add(resource.createStatusProperty())
 	}
 
-	for _, property := range resource.properties {
-		result = append(result, property)
-	}
-
-	// Sorted so that it's always consistent
-	sort.Slice(result, func(left int, right int) bool {
-		return result[left].propertyName < result[right].propertyName
-	})
-
 	return result
 }
 
@@ -469,7 +458,7 @@ func (resource *ResourceType) AsDeclarations(codeGenerationContext *CodeGenerati
 		}
 	}
 
-	for _, property := range resource.Properties() {
+	for _, property := range resource.Properties().AsSlice() {
 		f := property.AsField(codeGenerationContext)
 		if f != nil {
 			fields = append(fields, f)

hack/generator/pkg/astmodel/type_visitor.go

@@ -132,7 +132,7 @@ func IdentityVisitOfObjectType(this *TypeVisitor, it *ObjectType, ctx interface{
 	// just map the property types
 	var errs []error
 	var newProps []*PropertyDefinition
-	for _, prop := range it.Properties() {
+	for _, prop := range it.Properties().AsSlice() {
 		p, err := this.Visit(prop.propertyType, ctx)
 		if err != nil {
 			errs = append(errs, err)

hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects_test.go

@@ -346,6 +346,6 @@ func TestOneOfResourceSpec(t *testing.T) {
 	result := synth.oneOfObject(oneOf, names)
 	result, ok := astmodel.AsObjectType(result)
 	g.Expect(ok).To(BeTrue())
-	result = astmodel.NewObjectType().WithProperties(result.(*astmodel.ObjectType).Properties()...)
+	result = astmodel.NewObjectType().WithProperties(result.(*astmodel.ObjectType).Properties().AsSlice()...)
 	g.Expect(result).To(Equal(expected))
 }

hack/generator/pkg/codegen/pipeline/status_augment.go

@@ -91,7 +91,7 @@ func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter {
 		// based on their properties and copy across “flatten” when present, and
 		// also invoke the merger on the (Spec, Status) types of each property recursively
 		merger.Add(func(spec, status *astmodel.ObjectType) (astmodel.Type, error) {
-			props := spec.Properties()
+			props := spec.Properties().AsSlice()
 
 			changed := false
 			for ix, specProp := range props {

hack/generator/pkg/codegen/storage/type_converter.go

@@ -83,7 +83,7 @@ func (t *TypeConverter) convertObjectType(
 	_ *astmodel.TypeVisitor, object *astmodel.ObjectType, _ interface{}) (astmodel.Type, error) {
 
 	var errs []error
-	properties := object.Properties()
+	properties := object.Properties().AsSlice()
 	for i, prop := range properties {
 		p, err := t.propertyConverter.ConvertProperty(prop)
 		if err != nil {

hack/generator/pkg/testcases/object_type_serialization_test_case.go

@@ -59,7 +59,7 @@ func (o ObjectSerializationTestCase) References() astmodel.TypeNameSet {
 // codeGenerationContext contains reference material to use when generating
 func (o ObjectSerializationTestCase) AsFuncs(name astmodel.TypeName, genContext *astmodel.CodeGenerationContext) []dst.Decl {
 	var errs []error
-	properties := o.makePropertyMap()
+	properties := o.objectType.Properties()
 
 	// Find all the simple generators (those with no external dependencies)
 	simpleGenerators := o.createGenerators(properties, genContext, o.createIndependentGenerator)
@@ -668,14 +668,6 @@ func (o *ObjectSerializationTestCase) removeByPackage(
 	}
 }
 
-func (o *ObjectSerializationTestCase) makePropertyMap() map[astmodel.PropertyName]*astmodel.PropertyDefinition {
-	result := make(map[astmodel.PropertyName]*astmodel.PropertyDefinition)
-	for _, prop := range o.objectType.Properties() {
-		result[prop.PropertyName()] = prop
-	}
-	return result
-}
-
 func (o ObjectSerializationTestCase) idOfSubjectGeneratorGlobal() string {
 	return o.idOfGeneratorGlobal(o.subject)
 }