Skip to content

Commit 594a575

Browse files
MatovidloMatovidlo
authored
Merge pull request #2584 from keboola/mvasko/fix-shared-variables-templates-api
fix(templates): ignore shared variables configs with multiple consumers
2 parents ee88991 + caeccbd commit 594a575

11 files changed

Lines changed: 262 additions & 8 deletions

File tree

‎internal/pkg/fixtures/configs/with-variables-from-template.json‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
{
2+
"componentId": "ex-generic-v2",
3+
"name": "With Variables From Template",
4+
"configuration": {
5+
"variables_id": "%%TEST_CONFIG_WITH_VARIABLES_FROM_TEMPLATE_VARIABLES_ID%%",
6+
"variables_values_id": "%%TEST_CONFIG_WITH_VARIABLES_FROM_TEMPLATE_VARIABLES_VALUES_ID%%"
7+
},
8+
"rows": [],
9+
"metadata": {
10+
"KBC.KAC.templates.configId": "{\"idInTemplate\":\"with-variables-from-template\"}",
11+
"KBC.KAC.templates.instanceId": "inst-001",
12+
"KBC.KAC.templates.repository": "keboola",
13+
"KBC.KAC.templates.templateId": "my-template-id"
14+
},
15+
"isDisabled": false
16+
}

‎internal/pkg/mapper/ignore/remote.go‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,20 @@ func (m *ignoreMapper) isIgnored(ctx context.Context, object model.Object) bool
6161
func (m *ignoreMapper) isIgnoredConfig(ctx context.Context, config *model.Config) bool {
6262
// Variables config
6363
if config.ComponentID == keboola.VariablesComponentID {
64+
variablesForRels := config.Relations.GetByType(model.VariablesForRelType)
65+
66+
// Ignore if shared across multiple consumers — cannot be embedded under a single parent.
67+
// Normally the two-pass relations mapper removes all VariablesForRelType entries when
68+
// duplicates are detected (leaving len == 0, caught by the unattached check below).
69+
// This guard fires only when the variables config was absent from the loaded batch in
70+
// which consumer relations were linked, so the dedup pass never ran for it.
71+
if len(variablesForRels) > 1 {
72+
m.logger.Debugf(ctx, "Ignored %s shared by %d consumers", config.Desc(), len(variablesForRels))
73+
return true
74+
}
75+
6476
// Without target config
65-
if !config.Relations.Has(model.VariablesForRelType) && !config.Relations.Has(model.SharedCodeVariablesForRelType) {
77+
if len(variablesForRels) == 0 && !config.Relations.Has(model.SharedCodeVariablesForRelType) {
6678
m.logger.Debugf(ctx, "Ignored unattached variables %s", config.Desc())
6779
return true
6880
}

‎internal/pkg/mapper/ignore/remote_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,55 @@ func TestIgnoreMapper_AfterRemoteOperation_Variables(t *testing.T) {
6262
}, state.All())
6363
}
6464

65+
func TestIgnoreMapper_AfterRemoteOperation_Variables_SharedByMultipleConsumers(t *testing.T) {
66+
t.Parallel()
67+
state, d := createStateWithMapper(t)
68+
logger := d.DebugLogger()
69+
70+
// Consumer 1
71+
consumer1Key := model.ConfigKey{BranchID: 1, ComponentID: "ex-generic-v2", ID: "1"}
72+
consumer1 := &model.ConfigState{
73+
ConfigManifest: &model.ConfigManifest{ConfigKey: consumer1Key},
74+
Remote: &model.Config{ConfigKey: consumer1Key},
75+
}
76+
require.NoError(t, state.Set(consumer1))
77+
78+
// Consumer 2
79+
consumer2Key := model.ConfigKey{BranchID: 1, ComponentID: "ex-generic-v2", ID: "2"}
80+
consumer2 := &model.ConfigState{
81+
ConfigManifest: &model.ConfigManifest{ConfigKey: consumer2Key},
82+
Remote: &model.Config{ConfigKey: consumer2Key},
83+
}
84+
require.NoError(t, state.Set(consumer2))
85+
86+
// Shared variables config with two VariablesForRelation entries — simulates the case
87+
// where the relations mapper's two-pass dedup did not run for this config (it was absent
88+
// from the loaded batch when consumer relations were linked).
89+
sharedVarsKey := model.ConfigKey{BranchID: 1, ComponentID: keboola.VariablesComponentID, ID: "3"}
90+
sharedVars := &model.ConfigState{
91+
ConfigManifest: &model.ConfigManifest{ConfigKey: sharedVarsKey},
92+
Remote: &model.Config{
93+
ConfigKey: sharedVarsKey,
94+
Relations: model.Relations{
95+
&model.VariablesForRelation{ComponentID: consumer1Key.ComponentID, ConfigID: consumer1Key.ID},
96+
&model.VariablesForRelation{ComponentID: consumer2Key.ComponentID, ConfigID: consumer2Key.ID},
97+
},
98+
},
99+
}
100+
require.NoError(t, state.Set(sharedVars))
101+
102+
// Invoke
103+
changes := model.NewRemoteChanges()
104+
changes.AddLoaded(consumer1)
105+
changes.AddLoaded(consumer2)
106+
changes.AddLoaded(sharedVars)
107+
require.NoError(t, state.Mapper().AfterRemoteOperation(t.Context(), changes))
108+
logger.AssertJSONMessages(t, `{"level":"debug","message":"Ignored config \"branch:1/component:keboola.variables/config:3\" shared by 2 consumers"}`)
109+
110+
// Shared variables config is removed; consumers remain
111+
assert.Equal(t, []model.ObjectState{consumer1, consumer2}, state.All())
112+
}
113+
65114
func TestIgnoreMapper_AfterRemoteOperation_Scheduler(t *testing.T) {
66115
t.Parallel()
67116
state, d := createStateWithMapper(t)

‎internal/pkg/model/object.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -594,15 +594,22 @@ func (r *ConfigRow) GetContent() *orderedmap.OrderedMap {
594594
return r.Content
595595
}
596596

597-
// ParentKey - config parent can be modified via Relations, for example variables config is embedded in another config.
597+
// ParentKey - config parent can be modified via Relations, e.g. variables config embedded in another config.
598+
// When a keboola.variables config is shared by multiple consumers (ErrMultipleParents), fall back to the
599+
// structural parent (branch). All other errors are propagated.
598600
func (c *Config) ParentKey() (Key, error) {
599-
if parentKey, err := c.Relations.ParentKey(c.Key()); err != nil {
600-
return nil, err
601-
} else if parentKey != nil {
601+
parentKey, err := c.Relations.ParentKey(c.Key())
602+
if err == nil && parentKey != nil {
602603
return parentKey, nil
603604
}
604-
605-
// No parent defined via "Relations" -> parent is branch
605+
if err != nil {
606+
if errors.Is(err, ErrMultipleParents) && c.ComponentID == keboola.VariablesComponentID {
607+
// Shared variables config with multiple consumers — fall back to structural parent.
608+
return c.ConfigKey.ParentKey()
609+
}
610+
return nil, err
611+
}
612+
// No relation-defined parent — fall back to the structural parent (branch).
606613
return c.ConfigKey.ParentKey()
607614
}
608615

‎internal/pkg/model/object_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"time"
66

77
"github.com/keboola/go-utils/pkg/wildcards"
8+
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
89
"github.com/stretchr/testify/assert"
910
"github.com/stretchr/testify/require"
1011

@@ -94,6 +95,37 @@ func TestBranchMetadata_UpsertTemplateInstance_New(t *testing.T) {
9495
}, usages)
9596
}
9697

98+
func TestConfig_ParentKey_MultipleParentsFallsBackToBranch(t *testing.T) {
99+
t.Parallel()
100+
cfg := &Config{
101+
ConfigKey: ConfigKey{BranchID: 1, ComponentID: keboola.VariablesComponentID, ID: "3"},
102+
Relations: Relations{
103+
&VariablesForRelation{ComponentID: "ex-generic-v2", ConfigID: "1"},
104+
&VariablesForRelation{ComponentID: "ex-generic-v2", ConfigID: "2"},
105+
},
106+
}
107+
parentKey, err := cfg.ParentKey()
108+
require.NoError(t, err)
109+
assert.Equal(t, BranchKey{ID: 1}, parentKey)
110+
}
111+
112+
func TestConfig_ParentKey_MultipleParentsPropagatesForNonVariables(t *testing.T) {
113+
t.Parallel()
114+
// SchedulerForRelation.checkDefinedOn only requires a ConfigKey (no component-ID check),
115+
// so two entries on the same config reach the multi-parents sentinel without being
116+
// rejected by a component guard first.
117+
cfg := &Config{
118+
ConfigKey: ConfigKey{BranchID: 1, ComponentID: keboola.SchedulerComponentID, ID: "3"},
119+
Relations: Relations{
120+
&SchedulerForRelation{ComponentID: "ex-generic-v2", ConfigID: "1"},
121+
&SchedulerForRelation{ComponentID: "ex-generic-v2", ConfigID: "2"},
122+
},
123+
}
124+
_, err := cfg.ParentKey()
125+
require.Error(t, err)
126+
assert.ErrorIs(t, err, ErrMultipleParents)
127+
}
128+
97129
func TestBranchMetadata_DeleteTemplateUsage(t *testing.T) {
98130
t.Parallel()
99131

‎internal/pkg/model/relation.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@ const (
2424
UsedInRowInputMappingRelType = RelationType(`usedInRowInputMapping`)
2525
)
2626

27+
// ErrMultipleParents is returned by Relations.ParentKey when more than one relation
28+
// defines a parent key for the same object.
29+
var ErrMultipleParents = errors.New(`multiple parents defined by "relations"`)
30+
2731
// OneToXRelations gets relations that can be defined on an object only once.
2832
func OneToXRelations() []RelationType {
2933
return []RelationType{
@@ -80,7 +84,7 @@ func (v Relations) ParentKey(source Key) (Key, error) {
8084

8185
// Multiple parents are forbidden
8286
if len(parents) > 1 {
83-
return nil, errors.Errorf(`unexpected state: multiple parents defined by "relations" in %s`, source.Desc())
87+
return nil, errors.Errorf(`unexpected state in %s: %w`, source.Desc(), ErrMultipleParents)
8488
}
8589

8690
return nil, nil

‎test/templates/api/projects/project-with-shared-variables-and-instance/001-list-instances/expected-http-code‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
200

‎test/templates/api/projects/project-with-shared-variables-and-instance/001-list-instances/expected-response.json‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
{
2+
"instances": [
3+
{
4+
"instanceId": "inst-001",
5+
"templateId": "my-template-id",
6+
"version": "1.2.3",
7+
"repositoryName": "keboola",
8+
"branch": "%%TEST_BRANCH_MAIN_ID%%",
9+
"name": "Inst 001",
10+
"created": {
11+
"date": "2022-01-01T07:00:00Z",
12+
"tokenId": "123"
13+
},
14+
"updated": {
15+
"date": "2022-01-01T07:00:00Z",
16+
"tokenId": "123"
17+
},
18+
"configurations": [
19+
{
20+
"componentId": "ex-generic-v2",
21+
"configId": "%s",
22+
"name": "With Variables From Template"
23+
}
24+
]
25+
}
26+
]
27+
}

‎test/templates/api/projects/project-with-shared-variables-and-instance/001-list-instances/request.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"path": "/v1/project/%%TEST_BRANCH_MAIN_ID%%/instances",
3+
"method": "GET",
4+
"headers": {
5+
"Content-Type": "application/json",
6+
"X-StorageApi-Token": "%%TEST_KBC_STORAGE_API_TOKEN%%"
7+
},
8+
"body": {}
9+
}

‎test/templates/api/projects/project-with-shared-variables-and-instance/expected-state.json‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
{
2+
"branches": [
3+
{
4+
"branch": {
5+
"name": "Main",
6+
"description": "",
7+
"isDefault": true,
8+
"metadata": {
9+
"KBC.KAC.templates.instances": "[{\"instanceId\":\"inst-001\",\"instanceName\":\"Inst 001\",\"templateId\":\"my-template-id\",\"repositoryName\":\"keboola\",\"version\":\"1.2.3\",\"created\":{\"date\":\"2022-01-01T07:00:00Z\",\"tokenId\":\"123\"},\"updated\":{\"date\":\"2022-01-01T07:00:00Z\",\"tokenId\":\"123\"}}]"
10+
}
11+
},
12+
"configs": [
13+
{
14+
"componentId": "keboola.variables",
15+
"name": "Variables",
16+
"description": "test fixture",
17+
"configuration": {
18+
"variables": [
19+
{
20+
"name": "foo",
21+
"type": "string"
22+
}
23+
]
24+
},
25+
"rows": [
26+
{
27+
"name": "Default values",
28+
"description": "test fixture",
29+
"isDisabled": false,
30+
"configuration": {
31+
"values": [
32+
{
33+
"name": "foo",
34+
"value": "bar"
35+
}
36+
]
37+
}
38+
}
39+
],
40+
"isDisabled": false
41+
},
42+
{
43+
"componentId": "ex-generic-v2",
44+
"name": "With Variables 1",
45+
"description": "test fixture",
46+
"configuration": {
47+
"variables_id": "%s",
48+
"variables_values_id": "%s"
49+
},
50+
"rows": [],
51+
"isDisabled": false
52+
},
53+
{
54+
"componentId": "ex-generic-v2",
55+
"name": "With Variables From Template",
56+
"description": "test fixture",
57+
"configuration": {
58+
"variables_id": "%s",
59+
"variables_values_id": "%s"
60+
},
61+
"rows": [],
62+
"metadata": {
63+
"KBC.KAC.templates.configId": "{\"idInTemplate\":\"with-variables-from-template\"}",
64+
"KBC.KAC.templates.instanceId": "inst-001",
65+
"KBC.KAC.templates.repository": "keboola",
66+
"KBC.KAC.templates.templateId": "my-template-id"
67+
},
68+
"isDisabled": false
69+
}
70+
]
71+
}
72+
]
73+
}

0 commit comments

Comments
 (0)