Skip to content

Commit 3336901

Browse files
Matovidloclaude
andcommitted
fix: address PR review comments for shared variables fix
- Explicitly check relation count before GetOneByType in variables.go to make the multiple-parents guard more readable and future-safe - Add unit test for order-dependent scenario (variables config first in Loaded) that previously caused the fatal crash - Fix typo: lintRelations → linkRelations in link.go comment - Fix doc directory layout: variables/variables/ → variables/values/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2d30067 commit 3336901

4 files changed

Lines changed: 72 additions & 6 deletions

File tree

docs/relations-validation.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ In the Keboola platform, variable configs (`keboola.variables` component) belong
88
my-transformation/
99
variables/
1010
config.json ← keboola.variables config
11-
variables/
11+
values/
1212
default/ ← variables values row
1313
```
1414

internal/pkg/mapper/relations/link.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func (m *relationsMapper) AfterRemoteOperation(ctx context.Context, changes *mod
7777
return nil
7878
}
7979

80-
// lintRelations finds the other side of the relation and create a corresponding relation on the other side.
80+
// linkRelations finds the other side of the relation and create a corresponding relation on the other side.
8181
func (m *relationsMapper) linkRelations(object model.ObjectWithRelations, allObjects model.Objects) error {
8282
errs := errors.NewMultiError()
8383
relations := object.GetRelations()

internal/pkg/mapper/relations/link_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"strings"
55
"testing"
66

7+
"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
78
"github.com/stretchr/testify/assert"
89
"github.com/stretchr/testify/require"
910

@@ -66,6 +67,68 @@ func TestRelationsMapperLinkRelations(t *testing.T) {
6667
}, object2.Local.Relations)
6768
}
6869

70+
// TestRelationsMapperVariablesSharedAcrossConsumers verifies that when a variables config
71+
// is loaded before its consumers in changes.Loaded(), the two-pass approach still produces
72+
// exactly one warning instead of crashing with "multiple parents defined by relations".
73+
func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
74+
t.Parallel()
75+
state, d := createStateWithMapper(t)
76+
logger := d.DebugLogger()
77+
78+
branchID := keboola.BranchID(1)
79+
consumerCompID := keboola.ComponentID("ex-generic-v2")
80+
81+
// Variables config (Y) — added to state and loaded FIRST to exercise the ordering
82+
// that previously caused a fatal "multiple parents" crash in PathsGenerator.
83+
varsKey := model.ConfigKey{BranchID: branchID, ComponentID: keboola.VariablesComponentID, ID: "vars"}
84+
varsConfig := &model.ConfigState{
85+
ConfigManifest: &model.ConfigManifest{ConfigKey: varsKey},
86+
Remote: &model.Config{ConfigKey: varsKey},
87+
}
88+
require.NoError(t, state.Set(varsConfig))
89+
90+
// Consumer 1 — variablesFrom relation pointing to varsKey.
91+
consumer1Key := model.ConfigKey{BranchID: branchID, ComponentID: consumerCompID, ID: "consumer1"}
92+
consumer1 := &model.ConfigState{
93+
ConfigManifest: &model.ConfigManifest{ConfigKey: consumer1Key},
94+
Remote: &model.Config{
95+
ConfigKey: consumer1Key,
96+
Relations: model.Relations{
97+
&model.VariablesFromRelation{VariablesID: varsKey.ID},
98+
},
99+
},
100+
}
101+
require.NoError(t, state.Set(consumer1))
102+
103+
// Consumer 2 — also variablesFrom relation pointing to the same varsKey.
104+
consumer2Key := model.ConfigKey{BranchID: branchID, ComponentID: consumerCompID, ID: "consumer2"}
105+
consumer2 := &model.ConfigState{
106+
ConfigManifest: &model.ConfigManifest{ConfigKey: consumer2Key},
107+
Remote: &model.Config{
108+
ConfigKey: consumer2Key,
109+
Relations: model.Relations{
110+
&model.VariablesFromRelation{VariablesID: varsKey.ID},
111+
},
112+
},
113+
}
114+
require.NoError(t, state.Set(consumer2))
115+
116+
// Variables config is first in Loaded() — the ordering that previously caused the crash.
117+
changes := model.NewRemoteChanges()
118+
changes.AddLoaded(varsConfig)
119+
changes.AddLoaded(consumer1)
120+
changes.AddLoaded(consumer2)
121+
122+
// Must return nil — the duplicate is a warning, not a fatal error.
123+
require.NoError(t, state.Mapper().AfterRemoteOperation(t.Context(), changes))
124+
125+
// Exactly one warning about the duplicate variablesFor relation.
126+
// WarnAndErrorMessages() returns raw JSON; the formatter capitalises sentence starts.
127+
msgs := logger.WarnAndErrorMessages()
128+
assert.Equal(t, 1, strings.Count(msgs, "variablesFor"))
129+
assert.Empty(t, logger.ErrorMessages())
130+
}
131+
69132
func TestRelationsMapperOtherSideMissing(t *testing.T) {
70133
t.Parallel()
71134
state, d := createStateWithMapper(t)

internal/pkg/model/variables.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,12 +178,15 @@ func (t *VariablesValuesForRelation) NewOtherSideRelation(relationDefinedOn Obje
178178
return nil, nil, errors.Errorf(`%s not found, referenced from %s, by relation "%s"`, variablesConfigKey.Desc(), relationDefinedOn.Desc(), t.Type())
179179
}
180180
variablesConfig := variablesConfigRaw.(*Config)
181+
// When the variables config temporarily holds multiple variablesFor relations (shared
182+
// across consumers), skip linking here — Pass 2 validation will detect and warn about
183+
// the duplicates and remove them.
184+
if len(variablesConfig.Relations.GetByType(VariablesForRelType)) > 1 {
185+
return nil, nil, nil
186+
}
181187
variablesForRaw, err := variablesConfig.Relations.GetOneByType(VariablesForRelType)
182188
if err != nil {
183-
// Multiple variablesFor relations exist on the variables config (shared across consumers).
184-
// Pass 2 validation will detect and warn about the duplicates; skip linking here to avoid
185-
// a redundant error before the cleanup runs.
186-
return nil, nil, nil //nolint:nilerr
189+
return nil, nil, errors.PrefixErrorf(err, "invalid %s", variablesConfig.Desc())
187190
}
188191
if variablesForRaw == nil {
189192
return nil, nil, errors.NewNestedError(

0 commit comments

Comments
 (0)