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 index method name collisions #2335

Merged
merged 13 commits into from
Jun 10, 2022
Merged
Prev Previous commit
Next Next commit
Extract indexPropertyPath to simplify testing
  • Loading branch information
theunrepentantgeek committed Jun 10, 2022
commit 0704d18253f2f7f243233f4813790c7cd902f2cd
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"strings"

"github.com/Azure/azure-service-operator/v2/internal/set"
"github.com/pkg/errors"

"github.com/Azure/azure-service-operator/v2/tools/generator/internal/astmodel"
Expand Down Expand Up @@ -93,9 +94,14 @@ func handleSecretPropertyChains(
return indexFunctions, secretPropertyKeys
}

// ensureIndexMethodNamesUnique looks for conflicting index method names generated by the passed chains and ensures
// ensureIndexPropertyPathsUnique looks for conflicting index method names generated by the passed chains and ensures
// they all produce unique names.
func ensureIndexMethodNamesUnique(chains []*propertyChain) {
func ensureIndexPropertyPathsUnique(chains []*propertyChain) {
// First mark all the properties at the end of each chain as required
for _, chain := range chains {
chain.requiredForPropertyPath = true
}

for {
collisions := make(map[string][]*propertyChain)
matthchr marked this conversation as resolved.
Show resolved Hide resolved

Expand All @@ -105,11 +111,60 @@ func ensureIndexMethodNamesUnique(chains []*propertyChain) {
collisions[methodName] = append(collisions[methodName], chain)
}

//TODO: Resolve collisions
break
changedPaths := false
for _, collision := range collisions {
if len(collision) == 1 {
// No collision, nothing to do
continue
}

if tryResolvePropertyPathCollision(collision) {
changedPaths = true
}
}

if !changedPaths {
break
matthchr marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

// tryResolvePropertyPathCollision tries to resolve a collision between multiple chains, returning true if it was able
// to make a change (this allows us to terminate if no change is made, ensuring we don't end up in an infinite loop).
func tryResolvePropertyPathCollision(chains []*propertyChain) bool {
// Isolate all unique parents
parents := set.Make[*propertyChain]()
for _, chain := range chains {
if chain.root != nil {
parents.Add(chain.root)
}
}

if len(parents) == 0 {
// No parents, nothing to do
return false
}

// Check for parents with different names
names := set.Make[string]()
for _, parent := range parents.Values() {
name := string(parent.prop.PropertyName())
names.Add(name)
}

if len(names) == 1 {
// All parents have the same name, try resolving with their parents instead
return tryResolvePropertyPathCollision(parents.Values())
}

// We have parents and their names differ, use those names in the property path
for _, parent := range parents.Values() {
parent.requiredForPropertyPath = true
}

return true
}

func catalogSecretPropertyChains(def astmodel.TypeDefinition, definitions astmodel.TypeDefinitionSet) ([]*propertyChain, error) {
indexBuilder := &indexFunctionBuilder{}

Expand Down Expand Up @@ -139,8 +194,9 @@ type indexFunctionBuilder struct {
}

type propertyChain struct {
root *propertyChain
prop *astmodel.PropertyDefinition
root *propertyChain
prop *astmodel.PropertyDefinition
requiredForPropertyPath bool
}

// newPropertyChain returns a new chain with no properties.
Expand Down Expand Up @@ -207,8 +263,18 @@ func (chain *propertyChain) indexMethodName(
chain.indexPropertyPath())
}

// indexPropertyPath returns the path of the property in the chain, using only those properties that have been flagged
func (chain *propertyChain) indexPropertyPath() string {
return chain.prop.PropertyName().String()
var result string
if chain.root != nil {
result = chain.root.indexPropertyPath()
}

if chain.requiredForPropertyPath {
result += chain.prop.PropertyName().String()
}

return result
}

// indexPropertyKey makes an indexable key for this property chain. Note that this key is just a string. The fact
Expand Down