Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spec initializing copying wrong property #4108

Merged
merged 10 commits into from
Jun 20, 2024
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
Loading