Skip to content

Commit b061ffd

Browse files
michaelhtmmichaelhtm
authored
feat(compare): add is_document field config for semantic JSON/YAML comparison (#701)
Description of changes: Fields marked with `is_document: true` in generator.yaml will use ackcompare.DocumentEqual for delta comparison, preventing infinite reconciliation caused by whitespace/key-ordering differences in JSON or YAML document strings returned by AWS APIs. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 119cbe3 commit b061ffd

4 files changed

Lines changed: 213 additions & 0 deletions

File tree

‎pkg/config/field.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,11 @@ type FieldConfig struct {
428428
// string comparison. This handles IAM-specific semantics like statement
429429
// ordering independence and Action/Resource as string vs array.
430430
IsIAMPolicy bool `json:"is_iam_policy"`
431+
// IsDocument indicates that the field contains a JSON or YAML document
432+
// and should be compared using semantic document comparison rather than
433+
// string comparison. This handles differences in whitespace, key ordering,
434+
// and formatting that are semantically equivalent.
435+
IsDocument bool `json:"is_document"`
431436
// From instructs the code generator that the value of the field should
432437
// be retrieved from the specified operation and member path
433438
From *SourceFieldConfig `json:"from,omitempty"`

‎pkg/generate/code/compare.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,12 @@ func CompareResource(
166166
continue
167167
}
168168

169+
// Use semantic document comparison for fields marked as documents
170+
if fieldConfig != nil && fieldConfig.IsDocument {
171+
out += compareDocument(deltaVarName, firstResAdaptedVarName, secondResAdaptedVarName, fieldPath, indentLevel)
172+
continue
173+
}
174+
169175
memberShapeRef := specField.ShapeRef
170176
memberShape := memberShapeRef.Shape
171177

@@ -640,6 +646,67 @@ func compareIAMPolicy(
640646
return out
641647
}
642648

649+
// compareDocument outputs Go code that compares two JSON or YAML document
650+
// strings using semantic comparison that handles differences in whitespace,
651+
// key ordering, and formatting.
652+
//
653+
// Output code will look something like this:
654+
//
655+
// if ackcompare.HasNilDifference(a.ko.Spec.Configuration, b.ko.Spec.Configuration) {
656+
// delta.Add("Spec.Configuration", a.ko.Spec.Configuration, b.ko.Spec.Configuration)
657+
// } else if a.ko.Spec.Configuration != nil && b.ko.Spec.Configuration != nil {
658+
// if equal, err := ackcompare.DocumentEqual(*a.ko.Spec.Configuration, *b.ko.Spec.Configuration); err != nil || !equal {
659+
// delta.Add("Spec.Configuration", a.ko.Spec.Configuration, b.ko.Spec.Configuration)
660+
// }
661+
// }
662+
func compareDocument(
663+
// String representing the name of the variable that is of type
664+
// `*ackcompare.Delta`. We will generate Go code that calls the `Add()`
665+
// method of this variable when differences between fields are detected.
666+
deltaVarName string,
667+
// String representing the name of the variable that represents the desired
668+
// CR under comparison. This will typically be something like
669+
// "a.ko.Spec.Configuration". See `templates/pkg/resource/delta.go.tpl`.
670+
desiredResVarName string,
671+
// String representing the name of the variable that represents the latest
672+
// CR under comparison. This will typically be something like
673+
// "b.ko.Spec.Configuration". See `templates/pkg/resource/delta.go.tpl`.
674+
latestResVarName string,
675+
// String indicating the current field path being evaluated, e.g.
676+
// "Spec.Configuration".
677+
fieldPath string,
678+
// Number of levels of indentation to use
679+
indentLevel int,
680+
) string {
681+
out := ""
682+
indent := strings.Repeat("\t", indentLevel)
683+
684+
out += fmt.Sprintf(
685+
"%sif ackcompare.HasNilDifference(%s, %s) {\n",
686+
indent, desiredResVarName, latestResVarName,
687+
)
688+
out += fmt.Sprintf(
689+
"%s\t%s.Add(\"%s\", %s, %s)\n",
690+
indent, deltaVarName, fieldPath, desiredResVarName, latestResVarName,
691+
)
692+
out += fmt.Sprintf(
693+
"%s} else if %s != nil && %s != nil {\n",
694+
indent, desiredResVarName, latestResVarName,
695+
)
696+
out += fmt.Sprintf(
697+
"%s\tif equal, err := ackcompare.DocumentEqual(*%s, *%s); err != nil || !equal {\n",
698+
indent, desiredResVarName, latestResVarName,
699+
)
700+
out += fmt.Sprintf(
701+
"%s\t\t%s.Add(\"%s\", %s, %s)\n",
702+
indent, deltaVarName, fieldPath, desiredResVarName, latestResVarName,
703+
)
704+
out += fmt.Sprintf("%s\t}\n", indent)
705+
out += fmt.Sprintf("%s}\n", indent)
706+
707+
return out
708+
}
709+
643710
// compareTags outputs Go code that compares two slices of tags from two
644711
// resource fields by first converting them to the common ACK tag type and then
645712
// using a map comparison. If there is a difference, adds the difference to a

‎pkg/generate/code/compare_test.go‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -669,6 +669,91 @@ func TestCompareResource_IAM_Role_IAMPolicy(t *testing.T) {
669669
assert.Equal(expected, got)
670670
}
671671

672+
func TestCompareResource_SNS_Subscription_Document(t *testing.T) {
673+
assert := assert.New(t)
674+
require := require.New(t)
675+
676+
g := testutil.NewModelForServiceWithOptions(t, "sns", &testutil.TestingModelOptions{
677+
GeneratorConfigFile: "generator-document.yaml",
678+
})
679+
680+
crd := testutil.GetCRDByName(t, g, "Subscription")
681+
require.NotNil(crd)
682+
683+
// The FilterPolicy field is marked as is_document: true so it should use
684+
// DocumentEqual instead of string comparison (community#2872)
685+
expected := `
686+
if ackcompare.HasNilDifference(a.ko.Spec.DeliveryPolicy, b.ko.Spec.DeliveryPolicy) {
687+
delta.Add("Spec.DeliveryPolicy", a.ko.Spec.DeliveryPolicy, b.ko.Spec.DeliveryPolicy)
688+
} else if a.ko.Spec.DeliveryPolicy != nil && b.ko.Spec.DeliveryPolicy != nil {
689+
if *a.ko.Spec.DeliveryPolicy != *b.ko.Spec.DeliveryPolicy {
690+
delta.Add("Spec.DeliveryPolicy", a.ko.Spec.DeliveryPolicy, b.ko.Spec.DeliveryPolicy)
691+
}
692+
}
693+
if ackcompare.HasNilDifference(a.ko.Spec.Endpoint, b.ko.Spec.Endpoint) {
694+
delta.Add("Spec.Endpoint", a.ko.Spec.Endpoint, b.ko.Spec.Endpoint)
695+
} else if a.ko.Spec.Endpoint != nil && b.ko.Spec.Endpoint != nil {
696+
if *a.ko.Spec.Endpoint != *b.ko.Spec.Endpoint {
697+
delta.Add("Spec.Endpoint", a.ko.Spec.Endpoint, b.ko.Spec.Endpoint)
698+
}
699+
}
700+
if ackcompare.HasNilDifference(a.ko.Spec.FilterPolicy, b.ko.Spec.FilterPolicy) {
701+
delta.Add("Spec.FilterPolicy", a.ko.Spec.FilterPolicy, b.ko.Spec.FilterPolicy)
702+
} else if a.ko.Spec.FilterPolicy != nil && b.ko.Spec.FilterPolicy != nil {
703+
if equal, err := ackcompare.DocumentEqual(*a.ko.Spec.FilterPolicy, *b.ko.Spec.FilterPolicy); err != nil || !equal {
704+
delta.Add("Spec.FilterPolicy", a.ko.Spec.FilterPolicy, b.ko.Spec.FilterPolicy)
705+
}
706+
}
707+
if ackcompare.HasNilDifference(a.ko.Spec.FilterPolicyScope, b.ko.Spec.FilterPolicyScope) {
708+
delta.Add("Spec.FilterPolicyScope", a.ko.Spec.FilterPolicyScope, b.ko.Spec.FilterPolicyScope)
709+
} else if a.ko.Spec.FilterPolicyScope != nil && b.ko.Spec.FilterPolicyScope != nil {
710+
if *a.ko.Spec.FilterPolicyScope != *b.ko.Spec.FilterPolicyScope {
711+
delta.Add("Spec.FilterPolicyScope", a.ko.Spec.FilterPolicyScope, b.ko.Spec.FilterPolicyScope)
712+
}
713+
}
714+
if ackcompare.HasNilDifference(a.ko.Spec.Protocol, b.ko.Spec.Protocol) {
715+
delta.Add("Spec.Protocol", a.ko.Spec.Protocol, b.ko.Spec.Protocol)
716+
} else if a.ko.Spec.Protocol != nil && b.ko.Spec.Protocol != nil {
717+
if *a.ko.Spec.Protocol != *b.ko.Spec.Protocol {
718+
delta.Add("Spec.Protocol", a.ko.Spec.Protocol, b.ko.Spec.Protocol)
719+
}
720+
}
721+
if ackcompare.HasNilDifference(a.ko.Spec.RawMessageDelivery, b.ko.Spec.RawMessageDelivery) {
722+
delta.Add("Spec.RawMessageDelivery", a.ko.Spec.RawMessageDelivery, b.ko.Spec.RawMessageDelivery)
723+
} else if a.ko.Spec.RawMessageDelivery != nil && b.ko.Spec.RawMessageDelivery != nil {
724+
if *a.ko.Spec.RawMessageDelivery != *b.ko.Spec.RawMessageDelivery {
725+
delta.Add("Spec.RawMessageDelivery", a.ko.Spec.RawMessageDelivery, b.ko.Spec.RawMessageDelivery)
726+
}
727+
}
728+
if ackcompare.HasNilDifference(a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) {
729+
delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy)
730+
} else if a.ko.Spec.RedrivePolicy != nil && b.ko.Spec.RedrivePolicy != nil {
731+
if *a.ko.Spec.RedrivePolicy != *b.ko.Spec.RedrivePolicy {
732+
delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy)
733+
}
734+
}
735+
if ackcompare.HasNilDifference(a.ko.Spec.SubscriptionRoleARN, b.ko.Spec.SubscriptionRoleARN) {
736+
delta.Add("Spec.SubscriptionRoleARN", a.ko.Spec.SubscriptionRoleARN, b.ko.Spec.SubscriptionRoleARN)
737+
} else if a.ko.Spec.SubscriptionRoleARN != nil && b.ko.Spec.SubscriptionRoleARN != nil {
738+
if *a.ko.Spec.SubscriptionRoleARN != *b.ko.Spec.SubscriptionRoleARN {
739+
delta.Add("Spec.SubscriptionRoleARN", a.ko.Spec.SubscriptionRoleARN, b.ko.Spec.SubscriptionRoleARN)
740+
}
741+
}
742+
if ackcompare.HasNilDifference(a.ko.Spec.TopicARN, b.ko.Spec.TopicARN) {
743+
delta.Add("Spec.TopicARN", a.ko.Spec.TopicARN, b.ko.Spec.TopicARN)
744+
} else if a.ko.Spec.TopicARN != nil && b.ko.Spec.TopicARN != nil {
745+
if *a.ko.Spec.TopicARN != *b.ko.Spec.TopicARN {
746+
delta.Add("Spec.TopicARN", a.ko.Spec.TopicARN, b.ko.Spec.TopicARN)
747+
}
748+
}
749+
`
750+
got, err := code.CompareResource(
751+
crd.Config(), crd, "delta", "a.ko", "b.ko", 1,
752+
)
753+
require.NoError(err)
754+
assert.Equal(expected, got)
755+
}
756+
672757
// TestCompareResource_QuickSight_DataSet tests that the delta code generation
673758
// correctly handles the QuickSight DataSet resource, specifically the
674759
// RowLevelPermissionTagConfiguration.TagRuleConfigurations field which is a

‎pkg/testdata/models/apis/sns/0000-00-00/generator-document.yaml‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
operations:
2+
Subscribe:
3+
resource_name: Subscription
4+
operation_type: CREATE
5+
Unsubscribe:
6+
resource_name: Subscription
7+
operation_type: DELETE
8+
GetSubscriptionAttributes:
9+
resource_name: Subscription
10+
operation_type: GET_ATTRIBUTES
11+
SetSubscriptionAttributes:
12+
resource_name: Subscription
13+
operation_type: SET_ATTRIBUTES
14+
resources:
15+
Subscription:
16+
is_arn_primary_key: true
17+
update_operation:
18+
custom_method_name: customUpdate
19+
unpack_attributes_map:
20+
set_attributes_single_attribute: true
21+
fields:
22+
FilterPolicy:
23+
is_attribute: true
24+
is_document: true
25+
FilterPolicyScope:
26+
is_attribute: true
27+
Owner:
28+
is_attribute: true
29+
is_read_only: true
30+
is_owner_account_id: true
31+
PendingConfirmation:
32+
is_attribute: true
33+
is_read_only: true
34+
ConfirmationWasAuthenticated:
35+
is_attribute: true
36+
is_read_only: true
37+
EffectiveDeliveryPolicy:
38+
is_attribute: true
39+
is_read_only: true
40+
DeliveryPolicy:
41+
is_attribute: true
42+
RawMessageDelivery:
43+
is_attribute: true
44+
RedrivePolicy:
45+
is_attribute: true
46+
SubscriptionRoleArn:
47+
is_attribute: true
48+
tags:
49+
ignore: true
50+
ignore:
51+
resource_names:
52+
- PlatformApplication
53+
- Endpoint
54+
- Topic
55+
field_paths:
56+
- SubscribeInput.ReturnSubscriptionArn

0 commit comments

Comments
 (0)