Skip to content

Commit

Permalink
Fix spec initializing copying wrong property (#4108)
Browse files Browse the repository at this point in the history
* Add Path to TypedConversionEndpoint and initialize

* Don't generate matches for properties from different paths

* Use path to match properties for conversions

* Update generated files

* Add golden tests and fix bugs

* Tidy lint issues

* Address PR feedback

* Fix bug

* Update new resources
  • Loading branch information
theunrepentantgeek authored Jun 19, 2024
1 parent e71cd29 commit 1424557
Show file tree
Hide file tree
Showing 20 changed files with 375 additions and 32 deletions.
7 changes: 3 additions & 4 deletions v2/api/apimanagement/v1api20220801/api_types_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion v2/api/insights/v1api20220615/webtest_types_gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ func transformPropertyToConfigMapReference(prop *astmodel.PropertyDefinition, ne
return prop.WithType(newType), nil
}

func createNewConfigMapReference(prop *astmodel.PropertyDefinition, newType astmodel.Type) (*astmodel.PropertyDefinition, *astmodel.PropertyDefinition, error) {
func createNewConfigMapReference(
prop *astmodel.PropertyDefinition,
newType astmodel.Type,
) (*astmodel.PropertyDefinition, *astmodel.PropertyDefinition, error) {
// The expectation is that this is a string
propType := prop.PropertyType()
if !astmodel.TypeEquals(astmodel.Unwrap(propType), astmodel.StringType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,17 @@ var _ fmt.Stringer = &ReadableConversionEndpoint{}
// NewReadableConversionEndpointReadingProperty creates a ReadableConversionEndpoint that reads a value from a specific
// property
func NewReadableConversionEndpointReadingProperty(
propertyName astmodel.PropertyName,
propertyType astmodel.Type,
property *astmodel.PropertyDefinition,
) *ReadableConversionEndpoint {
name := string(propertyName)
name := string(property.PropertyName())
endpoint := NewTypedConversionEndpoint(property.PropertyType(), name)

if property.WasFlattened() {
endpoint = endpoint.WithPath(property.FlattenedFrom())
}

return &ReadableConversionEndpoint{
endpoint: NewTypedConversionEndpoint(propertyType, name),
endpoint: endpoint,
reader: func(source dst.Expr) dst.Expr {
return astbuilder.Selector(source, name)
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func NewReadableConversionEndpointSet() ReadableConversionEndpointSet {
func (set ReadableConversionEndpointSet) CreatePropertyEndpoints(sourceType astmodel.Type) int {
// Add an endpoint for each property we can read
return set.addForEachProperty(sourceType, func(prop *astmodel.PropertyDefinition) *ReadableConversionEndpoint {
return NewReadableConversionEndpointReadingProperty(prop.PropertyName(), prop.PropertyType())
return NewReadableConversionEndpointReadingProperty(prop)
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
package conversions

import (
"strings"

"github.com/Azure/azure-service-operator/v2/tools/generator/internal/astmodel"
)

Expand All @@ -16,6 +18,8 @@ type TypedConversionEndpoint struct {
theType astmodel.Type
// name is the name of the underlying property, used to generate useful local identifiers
name string
// path is a dot qualified path that leads to the property, used to match properties that may have been flattened
path string
}

func NewTypedConversionEndpoint(theType astmodel.Type, name string) *TypedConversionEndpoint {
Expand All @@ -30,17 +34,21 @@ func (endpoint *TypedConversionEndpoint) Name() string {
return endpoint.name
}

// Path returns a dot qualified path for this endpoint (might be empty)
func (endpoint *TypedConversionEndpoint) Path() string {
return endpoint.path
}

// Type returns the type of this endpoint
func (endpoint *TypedConversionEndpoint) Type() astmodel.Type {
return endpoint.theType
}

// WithType creates a new endpoint with a different type
func (endpoint *TypedConversionEndpoint) WithType(theType astmodel.Type) *TypedConversionEndpoint {
return &TypedConversionEndpoint{
theType: theType,
name: endpoint.name,
}
result := *endpoint
result.theType = theType
return &result
}

// IsOptional returns true if the endpoint contains an optional type, false otherwise
Expand All @@ -54,3 +62,16 @@ func (endpoint *TypedConversionEndpoint) IsBagItem() bool {
_, result := AsPropertyBagMemberType(endpoint.Type())
return result
}

func (endpoint *TypedConversionEndpoint) WithPath(
namePath []astmodel.PropertyName,
) *TypedConversionEndpoint {
path := make([]string, len(namePath))
for i, name := range namePath {
path[i] = string(name)
}

result := *endpoint
result.path = strings.Join(path, ".")
return &result
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,25 @@ var _ fmt.Stringer = &WritableConversionEndpoint{}

// NewWritableConversionEndpointWritingProperty creates a WritableConversionEndpoint for a specific property
func NewWritableConversionEndpointWritingProperty(
propertyName astmodel.PropertyName,
propertyType astmodel.Type,
property *astmodel.PropertyDefinition,
) *WritableConversionEndpoint {
name := string(propertyName)
name := string(property.PropertyName())
endpoint := NewTypedConversionEndpoint(property.PropertyType(), name)

if property.WasFlattened() {
endpoint = endpoint.WithPath(property.FlattenedFrom())
}

return &WritableConversionEndpoint{
endpoint: NewTypedConversionEndpoint(propertyType, name),
endpoint: endpoint,
writer: func(destination dst.Expr, value dst.Expr) []dst.Stmt {
return []dst.Stmt{
astbuilder.SimpleAssignment(
astbuilder.Selector(destination, name),
value),
}
},
description: fmt.Sprintf("write to property %s", propertyName),
description: fmt.Sprintf("write to property %s", name),
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func NewWritableConversionEndpointSet() WritableConversionEndpointSet {
func (set WritableConversionEndpointSet) CreatePropertyEndpoints(destinationType astmodel.Type) int {
// Add an endpoint for each property we can read
return set.addForEachProperty(destinationType, func(prop *astmodel.PropertyDefinition) *WritableConversionEndpoint {
return NewWritableConversionEndpointWritingProperty(prop.PropertyName(), prop.PropertyType())
return NewWritableConversionEndpointWritingProperty(prop)
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func NewPropertyAssignmentFunctionBuilder(
result.assignmentSelectors = []assignmentSelector{
{0, result.selectIdenticallyNamedProperties},
{1, result.selectRenamedProperties},
{2, result.selectPropertiesWithIdenticalPaths},
// High sequence numbers to ensure these are executed last
{100, result.readPropertiesFromPropertyBag},
{100, result.writePropertiesToPropertyBag},
Expand Down Expand Up @@ -258,7 +259,7 @@ func (builder *PropertyAssignmentFunctionBuilder) createConversions(
}

if conv != nil {
// A conversion was created, keep it for later
// A conversion was created, keep it for later, and make sure we don't reuse these endpoints
propertyConversions[destinationEndpoint.Name()] = conv
usedSources.Add(sourceEndpoint.Name())
usedDestinations.Add(destinationEndpoint.Name())
Expand Down Expand Up @@ -369,10 +370,87 @@ func (*PropertyAssignmentFunctionBuilder) selectIdenticallyNamedProperties(
sourceProperties conversions.ReadableConversionEndpointSet,
destinationProperties conversions.WritableConversionEndpointSet,
assign func(reader *conversions.ReadableConversionEndpoint, writer *conversions.WritableConversionEndpoint) error,
_ *conversions.PropertyConversionContext,
ctx *conversions.PropertyConversionContext,
) error {
for destinationName, destinationEndpoint := range destinationProperties {
if sourceEndpoint, ok := sourceProperties[destinationName]; ok {
// We have properties with the same name, we also need them to have the same path.
// This effectively means they were flattened from the same original property.
// If they don't, we can't match them up.
if sourceEndpoint.Endpoint().Path() != destinationEndpoint.Endpoint().Path() {
continue
}

err := assign(sourceEndpoint, destinationEndpoint)
if err != nil {
return errors.Wrapf(err, "assigning %s", destinationName)
}
}
}

return nil
}

// selectPropertiesWithIdenticalPaths matches up properties with paths for conversion.
// This serves to match up properties that were flattened from the original location,
// even if they've ended up with different names.
// sourceProperties is a set of endpoints that can be read from.
// destinationProperties is a set of endpoints that can be written to.
// assign is a function that will be called for each matching property, with the source and destination endpoints
// for that property.
// Returns an error if any of the assignments fail.
func (builder *PropertyAssignmentFunctionBuilder) selectPropertiesWithIdenticalPaths(
sourceProperties conversions.ReadableConversionEndpointSet,
destinationProperties conversions.WritableConversionEndpointSet,
assign func(reader *conversions.ReadableConversionEndpoint, writer *conversions.WritableConversionEndpoint) error,
ctx *conversions.PropertyConversionContext,
) error {
// Create a map of source properties, by path
sourceByPath := make(map[string][]*conversions.ReadableConversionEndpoint)
for _, source := range sourceProperties {
path := source.Endpoint().Path()
if path == "" {
// No path (so wasn't flattened); skip
continue
}

sourceByPath[path] = append(sourceByPath[path], source)
}

for destinationName, destinationEndpoint := range destinationProperties {
path := destinationEndpoint.Endpoint().Path()
if path == "" {
// No path (so wasn't flattened); skip
continue
}

sourceEndpoints, ok := sourceByPath[path]
if !ok {
// No match
continue
}

// Look for a unique endpoint that's compatible with the destination
var sourceEndpoint *conversions.ReadableConversionEndpoint
for _, src := range sourceEndpoints {
if !builder.typesCompatible(src.Endpoint().Type(), destinationEndpoint.Endpoint().Type(), ctx) {
// Types aren't compatible, skip
continue
}

if sourceEndpoint != nil {
// We've found multiple candidates - we can't handle this
return errors.Errorf(
"multiple source properties with path %s are compatible with destination %s, no way to select",
path,
destinationName)
}

sourceEndpoint = src
}

// If we found a compatible endpoint, use it
if sourceEndpoint != nil {
err := assign(sourceEndpoint, destinationEndpoint)
if err != nil {
return errors.Wrapf(err, "assigning %s", destinationName)
Expand Down Expand Up @@ -580,3 +658,33 @@ func (builder *PropertyAssignmentFunctionBuilder) findTypeForBag(

return t
}

// Return true if the types are generally compatible; this is used to disambiguate between properties when
// selecting property pairs for assignment, so it doesn't need to be perfect.
func (*PropertyAssignmentFunctionBuilder) typesCompatible(
left astmodel.Type,
right astmodel.Type,
_ *conversions.PropertyConversionContext,
) bool {
// if left resolves to an external type name, we're compatible if right is also an external type name
if _, ok := astmodel.AsExternalTypeName(left); ok {
_, rightIsExternal := astmodel.AsExternalTypeName(right)
return rightIsExternal
}

// if left resolves to an internal type name, we're compatible if right is also an internal type name
if _, ok := astmodel.AsInternalTypeName(left); ok {
_, rightIsInternal := astmodel.AsInternalTypeName(right)
return rightIsInternal
}

// if left resolves to a primitive type, we're compatible if right is also a primitive type
if _, ok := astmodel.AsPrimitiveType(left); ok {
_, rightIsPrimitive := astmodel.AsPrimitiveType(right)
return rightIsPrimitive
}

// TODO: Arrays, Maps, and look-through of InternalTypeName for aliases

return false
}
Loading

0 comments on commit 1424557

Please sign in to comment.