Skip to content

Commit 40bfad5

Browse files
sapphirewsapphirew
authored
fix(compare): support scalar slice element types in compareSlice (#697)
 Issue #, if available: N/A Description of changes: Add long, integer, double, float, and boolean to the existing list/map case in compareSlice(). These scalar pointer slice types (e.g. []*int64) previously hit the default error case with 'unsupported element type in compareSlice: long', forcing service controllers to use compare.is_ignored workarounds. Uses DeepEqual (same as list/map/structure cases) since there is no typed slice comparison helper in the runtime for these types. Add s3files test fixture with AccessPoint resource containing a PosixUser.SecondaryGIDs field ([]*int64) to exercise the new code path. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent ece451c commit 40bfad5

4 files changed

Lines changed: 200 additions & 2 deletions

File tree

‎pkg/generate/code/compare.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -525,8 +525,9 @@ func compareSlice(
525525
"%sif !equality.Semantic.Equalities.DeepEqual(%s, %s) {\n",
526526
indent, firstResVarName, secondResVarName,
527527
)
528-
case "list", "map":
529-
// For nested collection types (e.g. [][]*string or []map[string]*string),
528+
case "list", "map", "long", "integer", "double", "float", "boolean":
529+
// For nested collection types (e.g. [][]*string, []map[string]*string)
530+
// and scalar pointer slices (e.g. []*int64, []*float64, []*bool),
530531
// use DeepEqual since there's no simple element-wise comparison available.
531532
out += fmt.Sprintf(
532533
"%sif !equality.Semantic.Equalities.DeepEqual(%s, %s) {\n",

‎pkg/generate/code/compare_test.go‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -754,3 +754,63 @@ func TestCompareResource_QuickSight_DataSet(t *testing.T) {
754754
require.NoError(err)
755755
assert.Equal(expected, got)
756756
}
757+
758+
// TestCompareResource_S3Files_AccessPoint verifies that the code generator
759+
// correctly handles the S3 Files AccessPoint resource, specifically the
760+
// PosixUser.SecondaryGids field which is a list of longs ([]*int64).
761+
// This was previously unsupported and caused "unsupported element type in
762+
// compareSlice: long" errors.
763+
func TestCompareResource_S3Files_AccessPoint(t *testing.T) {
764+
assert := assert.New(t)
765+
require := require.New(t)
766+
767+
g := testutil.NewModelForService(t, "s3files")
768+
769+
crd := testutil.GetCRDByName(t, g, "AccessPoint")
770+
require.NotNil(crd)
771+
772+
expected := `
773+
if ackcompare.HasNilDifference(a.ko.Spec.FileSystemID, b.ko.Spec.FileSystemID) {
774+
delta.Add("Spec.FileSystemID", a.ko.Spec.FileSystemID, b.ko.Spec.FileSystemID)
775+
} else if a.ko.Spec.FileSystemID != nil && b.ko.Spec.FileSystemID != nil {
776+
if *a.ko.Spec.FileSystemID != *b.ko.Spec.FileSystemID {
777+
delta.Add("Spec.FileSystemID", a.ko.Spec.FileSystemID, b.ko.Spec.FileSystemID)
778+
}
779+
}
780+
if ackcompare.HasNilDifference(a.ko.Spec.PosixUser, b.ko.Spec.PosixUser) {
781+
delta.Add("Spec.PosixUser", a.ko.Spec.PosixUser, b.ko.Spec.PosixUser)
782+
} else if a.ko.Spec.PosixUser != nil && b.ko.Spec.PosixUser != nil {
783+
if ackcompare.HasNilDifference(a.ko.Spec.PosixUser.GID, b.ko.Spec.PosixUser.GID) {
784+
delta.Add("Spec.PosixUser.GID", a.ko.Spec.PosixUser.GID, b.ko.Spec.PosixUser.GID)
785+
} else if a.ko.Spec.PosixUser.GID != nil && b.ko.Spec.PosixUser.GID != nil {
786+
if *a.ko.Spec.PosixUser.GID != *b.ko.Spec.PosixUser.GID {
787+
delta.Add("Spec.PosixUser.GID", a.ko.Spec.PosixUser.GID, b.ko.Spec.PosixUser.GID)
788+
}
789+
}
790+
if len(a.ko.Spec.PosixUser.SecondaryGIDs) != len(b.ko.Spec.PosixUser.SecondaryGIDs) {
791+
delta.Add("Spec.PosixUser.SecondaryGIDs", a.ko.Spec.PosixUser.SecondaryGIDs, b.ko.Spec.PosixUser.SecondaryGIDs)
792+
} else if len(a.ko.Spec.PosixUser.SecondaryGIDs) > 0 {
793+
if !equality.Semantic.Equalities.DeepEqual(a.ko.Spec.PosixUser.SecondaryGIDs, b.ko.Spec.PosixUser.SecondaryGIDs) {
794+
delta.Add("Spec.PosixUser.SecondaryGIDs", a.ko.Spec.PosixUser.SecondaryGIDs, b.ko.Spec.PosixUser.SecondaryGIDs)
795+
}
796+
}
797+
if ackcompare.HasNilDifference(a.ko.Spec.PosixUser.UID, b.ko.Spec.PosixUser.UID) {
798+
delta.Add("Spec.PosixUser.UID", a.ko.Spec.PosixUser.UID, b.ko.Spec.PosixUser.UID)
799+
} else if a.ko.Spec.PosixUser.UID != nil && b.ko.Spec.PosixUser.UID != nil {
800+
if *a.ko.Spec.PosixUser.UID != *b.ko.Spec.PosixUser.UID {
801+
delta.Add("Spec.PosixUser.UID", a.ko.Spec.PosixUser.UID, b.ko.Spec.PosixUser.UID)
802+
}
803+
}
804+
}
805+
desiredACKTags, _ := convertToOrderedACKTags(a.ko.Spec.Tags)
806+
latestACKTags, _ := convertToOrderedACKTags(b.ko.Spec.Tags)
807+
if !ackcompare.MapStringStringEqual(desiredACKTags, latestACKTags) {
808+
delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
809+
}
810+
`
811+
got, err := code.CompareResource(
812+
crd.Config(), crd, "delta", "a.ko", "b.ko", 1,
813+
)
814+
require.NoError(err)
815+
assert.Equal(expected, got)
816+
}

‎pkg/testdata/codegen/sdk-codegen/aws-models/s3files.json‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
{
2+
"smithy": "2.0",
3+
"metadata": {},
4+
"shapes": {
5+
"com.amazonaws.s3files#S3Files": {
6+
"type": "service",
7+
"version": "2024-08-20",
8+
"traits": {
9+
"aws.api#service": {"sdkId": "S3Files"},
10+
"smithy.api#documentation": "Amazon S3 Files"
11+
},
12+
"operations": [
13+
{"target": "com.amazonaws.s3files#CreateAccessPoint"},
14+
{"target": "com.amazonaws.s3files#GetAccessPoint"},
15+
{"target": "com.amazonaws.s3files#DeleteAccessPoint"}
16+
]
17+
},
18+
"com.amazonaws.s3files#CreateAccessPoint": {
19+
"type": "operation",
20+
"input": {"target": "com.amazonaws.s3files#CreateAccessPointInput"},
21+
"output": {"target": "com.amazonaws.s3files#CreateAccessPointOutput"}
22+
},
23+
"com.amazonaws.s3files#CreateAccessPointInput": {
24+
"type": "structure",
25+
"members": {
26+
"FileSystemId": {"target": "com.amazonaws.s3files#FileSystemId"},
27+
"PosixUser": {"target": "com.amazonaws.s3files#PosixUser"},
28+
"Tags": {"target": "com.amazonaws.s3files#Tags"}
29+
}
30+
},
31+
"com.amazonaws.s3files#CreateAccessPointOutput": {
32+
"type": "structure",
33+
"members": {
34+
"AccessPointId": {"target": "com.amazonaws.s3files#AccessPointId"},
35+
"FileSystemId": {"target": "com.amazonaws.s3files#FileSystemId"},
36+
"PosixUser": {"target": "com.amazonaws.s3files#PosixUser"},
37+
"Tags": {"target": "com.amazonaws.s3files#Tags"}
38+
}
39+
},
40+
"com.amazonaws.s3files#GetAccessPoint": {
41+
"type": "operation",
42+
"input": {"target": "com.amazonaws.s3files#GetAccessPointInput"},
43+
"output": {"target": "com.amazonaws.s3files#GetAccessPointOutput"}
44+
},
45+
"com.amazonaws.s3files#GetAccessPointInput": {
46+
"type": "structure",
47+
"members": {
48+
"AccessPointId": {"target": "com.amazonaws.s3files#AccessPointId"}
49+
}
50+
},
51+
"com.amazonaws.s3files#GetAccessPointOutput": {
52+
"type": "structure",
53+
"members": {
54+
"AccessPointId": {"target": "com.amazonaws.s3files#AccessPointId"},
55+
"FileSystemId": {"target": "com.amazonaws.s3files#FileSystemId"},
56+
"PosixUser": {"target": "com.amazonaws.s3files#PosixUser"},
57+
"Tags": {"target": "com.amazonaws.s3files#Tags"}
58+
}
59+
},
60+
"com.amazonaws.s3files#DeleteAccessPoint": {
61+
"type": "operation",
62+
"input": {"target": "com.amazonaws.s3files#DeleteAccessPointInput"},
63+
"output": {"target": "com.amazonaws.s3files#DeleteAccessPointOutput"}
64+
},
65+
"com.amazonaws.s3files#DeleteAccessPointInput": {
66+
"type": "structure",
67+
"members": {
68+
"AccessPointId": {"target": "com.amazonaws.s3files#AccessPointId"}
69+
}
70+
},
71+
"com.amazonaws.s3files#DeleteAccessPointOutput": {
72+
"type": "structure",
73+
"members": {}
74+
},
75+
"com.amazonaws.s3files#AccessPointId": {
76+
"type": "string"
77+
},
78+
"com.amazonaws.s3files#FileSystemId": {
79+
"type": "string"
80+
},
81+
"com.amazonaws.s3files#PosixUser": {
82+
"type": "structure",
83+
"members": {
84+
"Uid": {"target": "com.amazonaws.s3files#Long"},
85+
"Gid": {"target": "com.amazonaws.s3files#Long"},
86+
"SecondaryGids": {"target": "com.amazonaws.s3files#SecondaryGids"}
87+
}
88+
},
89+
"com.amazonaws.s3files#SecondaryGids": {
90+
"type": "list",
91+
"member": {"target": "com.amazonaws.s3files#Long"}
92+
},
93+
"com.amazonaws.s3files#Long": {
94+
"type": "long"
95+
},
96+
"com.amazonaws.s3files#Tags": {
97+
"type": "list",
98+
"member": {"target": "com.amazonaws.s3files#Tag"}
99+
},
100+
"com.amazonaws.s3files#Tag": {
101+
"type": "structure",
102+
"members": {
103+
"Key": {"target": "com.amazonaws.s3files#TagKey"},
104+
"Value": {"target": "com.amazonaws.s3files#TagValue"}
105+
}
106+
},
107+
"com.amazonaws.s3files#TagKey": {
108+
"type": "string"
109+
},
110+
"com.amazonaws.s3files#TagValue": {
111+
"type": "string"
112+
}
113+
}
114+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
sdk_names:
2+
model_name: s3files
3+
package_name: s3files
4+
ignore:
5+
resource_names: []
6+
resources:
7+
AccessPoint:
8+
fields:
9+
ID:
10+
is_primary_key: true
11+
renames:
12+
operations:
13+
CreateAccessPoint:
14+
output_fields:
15+
AccessPointId: ID
16+
GetAccessPoint:
17+
input_fields:
18+
AccessPointId: ID
19+
output_fields:
20+
AccessPointId: ID
21+
DeleteAccessPoint:
22+
input_fields:
23+
AccessPointId: ID

0 commit comments

Comments
 (0)