Skip to content

Commit 91ec60b

Browse files
erezrokaherezrokah
authored
fix(table-diff): Support PK changes (#5638)
<!-- 🎉 Thank you for making CloudQuery awesome by submitting a PR 🎉 --> #### Summary This PR fixes the table diff logic to support PK changes. For example #5636 (comment) is wrong. <!-- Use the following steps to ensure your PR is ready to be reviewed - [ ] Read the [contribution guidelines](../blob/main/CONTRIBUTING.md) 🧑‍🎓 - [ ] Test locally on your own infrastructure - [ ] Run `go fmt` to format your code 🖊 - [ ] Lint your changes via `golangci-lint run` 🚨 (install golangci-lint [here](https://golangci-lint.run/usage/install/#local-installation)) - [ ] Update or add tests 🧪 - [ ] Ensure the status checks below are successful ✅ --->
1 parent 8e2663c commit 91ec60b

3 files changed

Lines changed: 90 additions & 18 deletions

File tree

‎scripts/table_diff/changes/changes.go‎

Lines changed: 41 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@ type change struct {
1919
Breaking bool `json:"breaking"`
2020
}
2121

22+
type column struct {
23+
dataType string
24+
pk bool
25+
}
26+
2227
func backtickStrings(strings ...string) []interface{} {
2328
backticked := make([]interface{}, len(strings))
2429
for i, s := range strings {
@@ -27,47 +32,66 @@ func backtickStrings(strings ...string) []interface{} {
2732
return backticked
2833
}
2934

30-
func parseColumnChange(line string) (name string, dataType string) {
35+
func parseColumnChange(line string) (name string, dataType string, pk bool) {
3136
match := columnRegex.FindStringSubmatch(line)
3237
if match == nil {
33-
return "", ""
38+
return "", "", false
3439
}
3540
result := make(map[string]string)
3641
for i, name := range columnRegex.SubexpNames() {
3742
if i != 0 && name != "" {
3843
result[name] = match[i]
3944
}
4045
}
41-
return result["name"], result["dataType"]
46+
cleanName := strings.TrimSuffix(result["name"], " (PK)")
47+
return cleanName, result["dataType"], result["name"] != cleanName
4248
}
4349

4450
func getColumnChanges(file *gitdiff.File, table string) (changes []change) {
45-
addedColumns := make(map[string]string)
46-
deletedColumns := make(map[string]string)
51+
addedColumns := make(map[string]column)
52+
deletedColumns := make(map[string]column)
4753
for _, fragment := range file.TextFragments {
4854
for _, line := range fragment.Lines {
49-
name, dataType := parseColumnChange(line.Line)
55+
name, dataType, pk := parseColumnChange(line.Line)
5056
if name == "" || dataType == "" {
5157
continue
5258
}
59+
column := column{dataType: dataType, pk: pk}
5360
switch line.Op {
5461
case gitdiff.OpAdd:
55-
addedColumns[name] = dataType
62+
addedColumns[name] = column
5663
case gitdiff.OpDelete:
57-
deletedColumns[name] = dataType
64+
deletedColumns[name] = column
5865
}
5966
}
6067
}
61-
for deletedName, deletedDataType := range deletedColumns {
62-
if addedDataType, ok := addedColumns[deletedName]; ok {
63-
if addedDataType != deletedDataType {
68+
for deletedName, deletedColumn := range deletedColumns {
69+
if addedColumn, ok := addedColumns[deletedName]; ok {
70+
if deletedColumn.dataType == addedColumn.dataType && deletedColumn.pk == addedColumn.pk {
6471
changes = append(changes, change{
65-
Text: fmt.Sprintf("Table %s: column type changed from %s to %s for %s", backtickStrings(table, deletedDataType, addedDataType, deletedName)...),
66-
Breaking: true,
72+
Text: fmt.Sprintf("Table %s: column order changed for %s", backtickStrings(table, deletedName)...),
73+
Breaking: false,
6774
})
68-
} else {
75+
continue
76+
}
77+
78+
if addedColumn.dataType != deletedColumn.dataType {
6979
changes = append(changes, change{
70-
Text: fmt.Sprintf("Table %s: column order changed for %s", backtickStrings(table, deletedName)...),
80+
Text: fmt.Sprintf("Table %s: column type changed from %s to %s for %s", backtickStrings(table, deletedColumn.dataType, addedColumn.dataType, deletedName)...),
81+
Breaking: false,
82+
})
83+
}
84+
85+
if addedColumn.pk && !deletedColumn.pk {
86+
changes = append(changes, change{
87+
Text: fmt.Sprintf("Table %s: column primary key %s added", backtickStrings(table, deletedName)...),
88+
Breaking: false,
89+
})
90+
}
91+
92+
if !addedColumn.pk && deletedColumn.pk {
93+
changes = append(changes, change{
94+
Text: fmt.Sprintf("Table %s: column primary key %s removed", backtickStrings(table, deletedName)...),
7195
Breaking: false,
7296
})
7397
}
@@ -78,10 +102,10 @@ func getColumnChanges(file *gitdiff.File, table string) (changes []change) {
78102
})
79103
}
80104
}
81-
for addedName, addedDataType := range addedColumns {
105+
for addedName, addedColumn := range addedColumns {
82106
if _, ok := deletedColumns[addedName]; !ok {
83107
changes = append(changes, change{
84-
Text: fmt.Sprintf("Table %s: column added with name %s and type %s", backtickStrings(table, addedName, addedDataType)...),
108+
Text: fmt.Sprintf("Table %s: column added with name %s and type %s", backtickStrings(table, addedName, addedColumn.dataType)...),
85109
Breaking: false,
86110
})
87111
}

‎scripts/table_diff/changes/changes_test.go‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,18 @@ func Test_parseColumnChange(t *testing.T) {
3131
args args
3232
wantName string
3333
wantDataType string
34+
wantPk bool
3435
}{
3536
{name: "Should parse name and data type when change is a column", args: args{line: "|name|String|"}, wantName: "name", wantDataType: "String"},
37+
{name: "Should parse name, pk and data type when a column is a primary key", args: args{line: "|name (PK)|String|"}, wantName: "name", wantDataType: "String", wantPk: true},
3638
{name: "Should return empty strings when change is not a column", args: args{line: "# Table: azure_appservice_site_auth_settings"}, wantName: "", wantDataType: ""},
3739
}
3840
for _, tt := range tests {
3941
t.Run(tt.name, func(t *testing.T) {
40-
gotName, gotDataType := parseColumnChange(tt.args.line)
42+
gotName, gotDataType, pk := parseColumnChange(tt.args.line)
4143
require.Equal(t, tt.wantName, gotName)
4244
require.Equal(t, tt.wantDataType, gotDataType)
45+
require.Equal(t, tt.wantPk, pk)
4346
})
4447
}
4548
}
@@ -158,6 +161,24 @@ func Test_getChanges(t *testing.T) {
158161
},
159162
wantErr: false,
160163
},
164+
{
165+
name: "Should handle PK changes",
166+
diffDataFile: "testdata/pr_5636_diff.txt",
167+
wantChanges: []change{
168+
{
169+
Text: "Table `gcp_resourcemanager_projects`: column primary key `_cq_id` removed",
170+
Breaking: false,
171+
},
172+
{
173+
Text: "Table `gcp_resourcemanager_projects`: column primary key `name` added",
174+
Breaking: false,
175+
},
176+
{
177+
Text: "Table `gcp_resourcemanager_projects`: column primary key `project_id` added",
178+
Breaking: false,
179+
},
180+
},
181+
},
161182
}
162183
for _, tt := range tests {
163184
t.Run(tt.name, func(t *testing.T) {

‎scripts/table_diff/changes/testdata/pr_5636_diff.txt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
diff --git a/plugins/source/gcp/docs/tables/gcp_resourcemanager_projects.md b/plugins/source/gcp/docs/tables/gcp_resourcemanager_projects.md
2+
index a39e930ae27..5c8b8b5ea39 100644
3+
--- a/plugins/source/gcp/docs/tables/gcp_resourcemanager_projects.md
4+
+++ b/plugins/source/gcp/docs/tables/gcp_resourcemanager_projects.md
5+
@@ -2,7 +2,7 @@
6+
7+
8+
9+
-The primary key for this table is **_cq_id**.
10+
+The composite primary key for this table is (**project_id**, **name**).
11+
12+
13+
14+
@@ -11,10 +11,10 @@ The primary key for this table is **_cq_id**.
15+
| ------------- | ------------- |
16+
|_cq_source_name|String|
17+
|_cq_sync_time|Timestamp|
18+
-|_cq_id (PK)|UUID|
19+
+|_cq_id|UUID|
20+
|_cq_parent_id|UUID|
21+
-|project_id|String|
22+
-|name|String|
23+
+|project_id (PK)|String|
24+
+|name (PK)|String|
25+
|parent|String|
26+
|state|String|
27+
|display_name|String|

0 commit comments

Comments
 (0)