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

resourcemanager/resourceids: adding a Match function #234

Merged
merged 2 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions resourcemanager/features/user_specified_segments.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package features

// TreatUserSpecifiedSegmentsAsCaseInsensitive is a feature-toggle which specifies whether User Specified
// Resource ID Segments should be compared case-insensitively as required.
//
// @tombuildsstuff: whilst this IS EXPOSED in the public interface - this is NOT READY FOR USE and should
// not be exposed in user-focused logic at this time, else this'll become a source of knock-on problems
tombuildsstuff marked this conversation as resolved.
Show resolved Hide resolved
// rather than being useful.
//
// There are a number of dependencies to enabling this, including completing the standardiation on the
// `ResourceId` interface and the `ResourceIDReference` schema types - and surrounding updates.
var TreatUserSpecifiedSegmentsAsCaseInsensitive = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the comment I could still see this creeping into places where it shouldn't in the provider. Would it make sense and/or be possible for this to be accompanied by a check in the provider to flag new additions of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this, do you mean a check in the Provider to ensure that we're not doing .ID() ==? If so, yes - that can be added at the same time as fixing the 6 instances above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually mean a check in the Provider to ensure we're not setting features.TreatUserSpecifiedSegmentsAsCaseInsensitive = true unless it's been explicitly added to a list of exceptions, like we do with these checks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense - we probably want both checks actually.

It's worth noting that since this feature-flag is for the behaviour as a whole (i.e. across the entire Provider) rather than being for individual Resource ID types - we'd only set this one-time rather than in multiple places anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For info: I've intentionally not included the validation changes in hashicorp/terraform-provider-azurerm#25873 since this wants to be done in a follow up PR - will send that in the morning:

61 changes: 61 additions & 0 deletions resourcemanager/resourceids/match.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package resourceids

import (
"reflect"
"strings"

"github.com/hashicorp/go-azure-helpers/resourcemanager/features"
)

// Match compares two instances of the same ResourceId and determines whether they are a match
//
// Whilst it might seem fine to compare the result of the `.ID()` function, that doesn't account
// for Resource ID Segments which need to be compared as case-insensitive.
//
// As such whilst this function is NOT exposing that functionality right now, it will when the
// centralised feature-flag for this is rolled out.
func Match(first, second ResourceId) bool {
// since we're comparing interface types, ensure the two underlying types are the same
if reflect.TypeOf(first) != reflect.TypeOf(second) {
return false
}

parser := NewParserFromResourceIdType(first)
firstParsed, err := parser.Parse(first.ID(), true)
if err != nil {
return false
}
secondParsed, err := parser.Parse(second.ID(), true)
if err != nil {
return false
}
firstVal := firstParsed.Parsed
secondVal := secondParsed.Parsed
if len(firstVal) != len(secondVal) {
return false
}
for key, val := range firstVal {
otherVal, ok := secondVal[key]
if !ok {
return false
}

segment := parser.namedSegment(key)
if segment == nil {
return false
}

if features.TreatUserSpecifiedSegmentsAsCaseInsensitive && segment.Type == UserSpecifiedSegmentType {
if !strings.EqualFold(val, otherVal) {
return false
}
} else if val != otherVal {
return false
}
}

return true
}
211 changes: 211 additions & 0 deletions resourcemanager/resourceids/match_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package resourceids

import (
"fmt"
"testing"

"github.com/hashicorp/go-azure-helpers/resourcemanager/features"
)

func TestMatch(t *testing.T) {
// force a case-sensitive comparison for this test
features.TreatUserSpecifiedSegmentsAsCaseInsensitive = false

testData := []struct {
first ResourceId
second ResourceId
expected bool
}{
{
// two instances of the same Resource ID with the same value should match
first: newPlanetID("mars"),
second: newPlanetID("mars"),
expected: true,
},
{
// two instances of the same Resource ID with the same value in differing casing
// shouldn't match
first: newPlanetID("mars"),
second: newPlanetID("mArs"),
expected: false,
},
{
// two instances of the same Resource ID with differing values shouldn't match
first: newPlanetID("earth"),
second: newPlanetID("mars"),
expected: false,
},
{
// two different Resource ID types shouldn't match - same URI
first: newPlanetID("earth"),
second: newOtherPlanetID("earth"),
expected: false,
},
{
// two different Resource ID types shouldn't match - different URIs
first: newPlanetID("earth"),
second: newSolarSystemPlanetID("milkyWay", "earth"),
expected: false,
},
}
for i, data := range testData {
t.Logf("Iteration %d", i)
actual := Match(data.first, data.second)
if actual != data.expected {
t.Fatalf("expected Match to return %t but got %t", data.expected, actual)
}
}
}

func TestMatch_Insensitive(t *testing.T) {
// force a case-sensitive comparison for this test
features.TreatUserSpecifiedSegmentsAsCaseInsensitive = true

testData := []struct {
first ResourceId
second ResourceId
expected bool
}{
{
// two instances of the same Resource ID with the same value should match
first: newPlanetID("mars"),
second: newPlanetID("mars"),
expected: true,
},
{
// two instances of the same Resource ID with the same value in differing casing
// should match since the feature-flag is enabled
first: newPlanetID("mars"),
second: newPlanetID("mArs"),
expected: true,
},
{
// two instances of the same Resource ID with differing values shouldn't match
first: newPlanetID("earth"),
second: newPlanetID("mars"),
expected: false,
},
{
// two different Resource ID types shouldn't match - same URI
first: newPlanetID("earth"),
second: newOtherPlanetID("earth"),
expected: false,
},
{
// two different Resource ID types shouldn't match - different URIs
first: newPlanetID("earth"),
second: newSolarSystemPlanetID("milkyWay", "earth"),
expected: false,
},
}
for i, data := range testData {
t.Logf("Iteration %d", i)
actual := Match(data.first, data.second)
if actual != data.expected {
t.Fatalf("expected Match to return %t but got %t", data.expected, actual)
}
}
}

// NOTE: the ResourceId implementations below are purely for test purposes and so intentionally
// incomplete implementations/panicking for unexpected/unused methods

var _ ResourceId = &planetResourceId{}

type planetResourceId struct {
planetName string
}

func newPlanetID(planetName string) *planetResourceId {
return &planetResourceId{
planetName: planetName,
}
}

func (f *planetResourceId) FromParseResult(_ ParseResult) error {
panic("not implemented since this codepath should not be used")
}

func (f *planetResourceId) ID() string {
return fmt.Sprintf("/planets/%s", f.planetName)
}

func (f *planetResourceId) String() string {
panic("not implemented since this codepath should not be used")
}

func (f *planetResourceId) Segments() []Segment {
return []Segment{
StaticSegment("planets", "planets", "planets"),
UserSpecifiedSegment("planetName", "earth"),
}
}

var _ ResourceId = &otherPlanetResourceId{}

type otherPlanetResourceId struct {
planetName string
}

func newOtherPlanetID(planetName string) *otherPlanetResourceId {
return &otherPlanetResourceId{
planetName: planetName,
}
}

func (f *otherPlanetResourceId) FromParseResult(_ ParseResult) error {
panic("not implemented since this codepath should not be used")
}

func (f *otherPlanetResourceId) ID() string {
return fmt.Sprintf("/planets/%s", f.planetName)
}

func (f *otherPlanetResourceId) String() string {
panic("not implemented since this codepath should not be used")
}

func (f *otherPlanetResourceId) Segments() []Segment {
return []Segment{
StaticSegment("planets", "planets", "planets"),
UserSpecifiedSegment("planetName", "earth"),
}
}

var _ ResourceId = &solarSystemPlanetResourceId{}

type solarSystemPlanetResourceId struct {
planetName string
solarSystemName string
}

func newSolarSystemPlanetID(solarSystemName, planetName string) *solarSystemPlanetResourceId {
return &solarSystemPlanetResourceId{
planetName: planetName,
solarSystemName: solarSystemName,
}
}

func (f *solarSystemPlanetResourceId) FromParseResult(_ ParseResult) error {
panic("not implemented since this codepath should not be used")
}

func (f *solarSystemPlanetResourceId) ID() string {
return fmt.Sprintf("/solarSystems/%s/planets/%s", f.solarSystemName, f.planetName)
}

func (f *solarSystemPlanetResourceId) String() string {
panic("not implemented since this codepath should not be used")
}

func (f *solarSystemPlanetResourceId) Segments() []Segment {
return []Segment{
StaticSegment("solarSystems", "solarSystems", "solarSystems"),
UserSpecifiedSegment("solarSystemName", "milkyWay"),
StaticSegment("planets", "planets", "planets"),
UserSpecifiedSegment("planetName", "earth"),
}
}
13 changes: 13 additions & 0 deletions resourcemanager/resourceids/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"fmt"
"regexp"
"strings"

"github.com/hashicorp/go-azure-helpers/lang/pointer"
)

type Parser struct {
Expand Down Expand Up @@ -205,6 +207,17 @@ func (p Parser) Parse(input string, insensitively bool) (*ParseResult, error) {
return &parseResult, nil
}

// namedSegment returns the named Segment for a ResourceId, if it exists
func (p Parser) namedSegment(name string) *Segment {
for _, item := range p.segments {
if item.Name == name {
return pointer.To(item)
}
}

return nil
}

func (p Parser) parseScopePrefix(input, regexForNonScopeSegments string, insensitively bool) (*string, error) {
regexToUse := fmt.Sprintf("^((.){1,})%s", regexForNonScopeSegments)
if insensitively {
Expand Down