Fix Storage Conversion generation bug

hack/generator/pkg/astbuilder/builder.go

@@ -271,6 +271,16 @@ func Dereference(expr dst.Expr) *dst.UnaryExpr {
 	}
 }
 
+// AsDereference converts the supplied expression into a dereference if it is one
+func AsDereference(expr dst.Expr) (*dst.UnaryExpr, bool) {
+	result, ok := expr.(*dst.UnaryExpr)
+	if ok && result.Op == token.MUL {
+		return result, true
+	}
+
+	return nil, false
+}
+
 // Returns creates a return statement with one or more expressions, of the form
 //
 //    return <expr>

hack/generator/pkg/astmodel/type_name.go

@@ -69,6 +69,18 @@ func (typeName TypeName) AsType(codeGenerationContext *CodeGenerationContext) ds
 // AsZero renders an expression for the "zero" value of the type.
 // The exact thing we need to generate depends on the actual type we reference
 func (typeName TypeName) AsZero(types Types, ctx *CodeGenerationContext) dst.Expr {
+
+	if _, isLocal := typeName.PackageReference.AsLocalPackage(); !isLocal {
+		// TypeName is external, zero value is a qualified empty struct
+		// (we might not actually use this, if the property is optional, but we still need to generate the right thing)
+
+		packageName := ctx.MustGetImportedPackageName(typeName.PackageReference)
+		return &dst.SelectorExpr{
+			X:   dst.NewIdent(packageName),
+			Sel: dst.NewIdent(fmt.Sprintf("%s{}", typeName.Name())),
+		}
+	}
+
 	actualType, err := types.FullyResolve(typeName)
 	if err != nil {
 		// This should never happen

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

@@ -7,9 +7,10 @@ package storage
 
 import (
 	"fmt"
+	"strings"
+
 	"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
 	"github.com/pkg/errors"
-	"strings"
 )
 
 // PropertyConverter is used to convert the properties of object inputTypes as required for storage variants
@@ -90,6 +91,11 @@ func (p *PropertyConverter) useBaseTypeForEnumerations(
 func (p *PropertyConverter) shortCircuitNamesOfSimpleTypes(
 	tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) {
 
+	// for nonlocal packages, preserve the name as is
+	if _, ok := tn.PackageReference.AsLocalPackage(); !ok {
+		return tn, nil
+	}
+
 	actualType, err := p.types.FullyResolve(tn)
 	if err != nil {
 		// Can't resolve to underlying type, give up

hack/generator/pkg/conversions/property_assignment_function_test.go

@@ -23,17 +23,21 @@ type StorageConversionPropertyTestCase struct {
 
 func CreatePropertyAssignmentFunctionTestCases() []*StorageConversionPropertyTestCase {
 
+	// Package References
 	vCurrent := makeTestLocalPackageReference("Verification", "vCurrent")
 	vNext := makeTestLocalPackageReference("Verification", "vNext")
 
+	// Custom Types
 	alpha := astmodel.EnumValue{Identifier: "Alpha", Value: "alpha"}
 	beta := astmodel.EnumValue{Identifier: "Beta", Value: "beta"}
 
 	enumType := astmodel.NewEnumType(astmodel.StringType, alpha, beta)
 	currentEnum := astmodel.MakeTypeDefinition(astmodel.MakeTypeName(vCurrent, "Bucket"), enumType)
 	nextEnum := astmodel.MakeTypeDefinition(astmodel.MakeTypeName(vNext, "Bucket"), enumType)
 
-	// These are aliases of primitive types
+	jsonObjectType := astmodel.NewMapType(astmodel.StringType, astmodel.JSONTypeName)
+
+	// Aliases of primitive types
 	currentAge := astmodel.MakeTypeDefinition(astmodel.MakeTypeName(vCurrent, "Age"), astmodel.IntType)
 	nextAge := astmodel.MakeTypeDefinition(astmodel.MakeTypeName(vNext, "Age"), astmodel.IntType)
 
@@ -76,7 +80,10 @@ func CreatePropertyAssignmentFunctionTestCases() []*StorageConversionPropertyTes
 
 	referenceProperty := astmodel.NewPropertyDefinition("Reference", "reference", astmodel.ResourceReferenceTypeName)
 	knownReferenceProperty := astmodel.NewPropertyDefinition("KnownReference", "known-reference", astmodel.KnownResourceReferenceTypeName)
-	jsonProperty := astmodel.NewPropertyDefinition("JSONBlob", "json-blob", astmodel.JSONTypeName)
+	jsonProperty := astmodel.NewPropertyDefinition("JSONBlob", "jsonBlob", astmodel.JSONTypeName)
+	optionalJSONProperty := astmodel.NewPropertyDefinition("JSONBlob", "jsonBlob", astmodel.NewOptionalType(astmodel.JSONTypeName))
+	jsonObjectProperty := astmodel.NewPropertyDefinition("JSONObject", "jsonObject", jsonObjectType)
+	optionalJSONObjectProperty := astmodel.NewPropertyDefinition("JSONObject", "jsonObject", astmodel.NewOptionalType(jsonObjectType))
 
 	idFactory := astmodel.NewIdentifierFactory()
 	ageFunction := test.NewFakeFunction("Age", idFactory)
@@ -190,7 +197,11 @@ func CreatePropertyAssignmentFunctionTestCases() []*StorageConversionPropertyTes
 
 		createPropertyAssignmentTest("CopyReferenceProperty", referenceProperty, referenceProperty),
 		createPropertyAssignmentTest("CopyKnownReferenceProperty", knownReferenceProperty, knownReferenceProperty),
+
 		createPropertyAssignmentTest("CopyJSONProperty", jsonProperty, jsonProperty),
+		createPropertyAssignmentTest("CopyJSONObjectProperty", jsonObjectProperty, jsonObjectProperty),
+		createPropertyAssignmentTest("CopyOptionalJSONProperty", optionalJSONProperty, jsonProperty),
+		createPropertyAssignmentTest("CopyJSONObjectProperty", optionalJSONObjectProperty, jsonObjectProperty),
 	}
 }
 

hack/generator/pkg/conversions/property_conversions.go

@@ -50,9 +50,9 @@ func init() {
 		// Complex object types
 		assignObjectFromObject,
 		// Known types
-		copyKnownType(astmodel.KnownResourceReferenceTypeName, "Copy"),
-		copyKnownType(astmodel.ResourceReferenceTypeName, "Copy"),
-		copyKnownType(astmodel.JSONTypeName, "DeepCopy"),
+		copyKnownType(astmodel.KnownResourceReferenceTypeName, "Copy", returnsValue),
+		copyKnownType(astmodel.ResourceReferenceTypeName, "Copy", returnsValue),
+		copyKnownType(astmodel.JSONTypeName, "DeepCopy", returnsReference),
 		// Meta-conversions
 		assignFromOptional, // Must go before assignToOptional so we generate the right zero values
 		assignToOptional,
@@ -932,11 +932,18 @@ func AssignKnownType(name astmodel.TypeName) func(*TypedConversionEndpoint, *Typ
 	}
 }
 
+type knownTypeMethodReturn int
+
+const (
+	returnsReference = 0
+	returnsValue     = 1
+)
+
 // copyKnownType will generate an assignment with the results of a call on the specified TypeName
 //
 // <destination> = <source>.<methodName>()
 //
-func copyKnownType(name astmodel.TypeName, methodName string) func(*TypedConversionEndpoint, *TypedConversionEndpoint, *PropertyConversionContext) PropertyConversion {
+func copyKnownType(name astmodel.TypeName, methodName string, returnKind knownTypeMethodReturn) func(*TypedConversionEndpoint, *TypedConversionEndpoint, *PropertyConversionContext) PropertyConversion {
 	return func(sourceEndpoint *TypedConversionEndpoint, destinationEndpoint *TypedConversionEndpoint, _ *PropertyConversionContext) PropertyConversion {
 		// Require source to be non-optional
 		if _, sourceIsOptional := astmodel.AsOptionalType(sourceEndpoint.Type()); sourceIsOptional {
@@ -971,6 +978,18 @@ func copyKnownType(name astmodel.TypeName, methodName string) func(*TypedConvers
 		}
 
 		return func(reader dst.Expr, writer func(dst.Expr) []dst.Stmt, generationContext *astmodel.CodeGenerationContext) []dst.Stmt {
+			// If our writer is dereferencing a value, skip that as we don't need to dereference before a method call
+			if unary, ok := astbuilder.AsDereference(reader); ok {
+				reader = unary.X
+			}
+
+			if returnKind == returnsReference {
+				// If the copy method returns a ptr, we need to dereference
+				// This dereference is always safe because we ensured that both source and destination are always
+				// non optional. The handler assignToOptional() should do the right thing when this happens.
+				return writer(astbuilder.Dereference(astbuilder.CallExpr(reader, methodName)))
+			}
+
 			return writer(astbuilder.CallExpr(reader, methodName))
 		}
 	}

hack/generator/pkg/conversions/testdata/CopyJSONObjectProperty.golden

@@ -0,0 +1,45 @@
+// Code generated by azure-service-operator-codegen. DO NOT EDIT.
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+package vCurrent
+
+import (
+	"github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+type Person struct {
+	JSONObject map[string]v1.JSON `json:"jsonObject"`
+}
+
+// AssignPropertiesFromPerson populates our Person from the provided source Person
+func (person *Person) AssignPropertiesFromPerson(source *vNext.Person) error {
+
+	// JSONObject
+	jsonObjectMap := make(map[string]v1.JSON)
+	for jsonObjectKey, jsonObjectValue := range source.JSONObject {
+		// Shadow the loop variable to avoid aliasing
+		jsonObjectValue := jsonObjectValue
+		jsonObjectMap[jsonObjectKey] = *jsonObjectValue.DeepCopy()
+	}
+	person.JSONObject = jsonObjectMap
+
+	// No error
+	return nil
+}
+
+// AssignPropertiesToPerson populates the provided destination Person from our Person
+func (person *Person) AssignPropertiesToPerson(destination *vNext.Person) error {
+
+	// JSONObject
+	jsonObjectMap := make(map[string]v1.JSON)
+	for jsonObjectKey, jsonObjectValue := range person.JSONObject {
+		// Shadow the loop variable to avoid aliasing
+		jsonObjectValue := jsonObjectValue
+		jsonObjectMap[jsonObjectKey] = *jsonObjectValue.DeepCopy()
+	}
+	destination.JSONObject = jsonObjectMap
+
+	// No error
+	return nil
+}

hack/generator/pkg/conversions/testdata/CopyJSONProperty.golden

@@ -9,14 +9,14 @@ import (
 )
 
 type Person struct {
-	JSONBlob v1.JSON `json:"json-blob"`
+	JSONBlob v1.JSON `json:"jsonBlob"`
 }
 
 // AssignPropertiesFromPerson populates our Person from the provided source Person
 func (person *Person) AssignPropertiesFromPerson(source *vNext.Person) error {
 
 	// JSONBlob
-	person.JSONBlob = source.JSONBlob.DeepCopy()
+	person.JSONBlob = *source.JSONBlob.DeepCopy()
 
 	// No error
 	return nil
@@ -26,7 +26,7 @@ func (person *Person) AssignPropertiesFromPerson(source *vNext.Person) error {
 func (person *Person) AssignPropertiesToPerson(destination *vNext.Person) error {
 
 	// JSONBlob
-	destination.JSONBlob = person.JSONBlob.DeepCopy()
+	destination.JSONBlob = *person.JSONBlob.DeepCopy()
 
 	// No error
 	return nil

hack/generator/pkg/conversions/testdata/CopyOptionalJSONProperty.golden

@@ -0,0 +1,38 @@
+// Code generated by azure-service-operator-codegen. DO NOT EDIT.
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+package vCurrent
+
+import (
+	"github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext"
+	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+)
+
+type Person struct {
+	JSONBlob *v1.JSON `json:"jsonBlob"`
+}
+
+// AssignPropertiesFromPerson populates our Person from the provided source Person
+func (person *Person) AssignPropertiesFromPerson(source *vNext.Person) error {
+
+	// JSONBlob
+	jsonBlob := *source.JSONBlob.DeepCopy()
+	person.JSONBlob = &jsonBlob
+
+	// No error
+	return nil
+}
+
+// AssignPropertiesToPerson populates the provided destination Person from our Person
+func (person *Person) AssignPropertiesToPerson(destination *vNext.Person) error {
+
+	// JSONBlob
+	if person.JSONBlob != nil {
+		destination.JSONBlob = *person.JSONBlob.DeepCopy()
+	} else {
+		destination.JSONBlob = v1.JSON{}
+	}
+
+	// No error
+	return nil
+}