Skip to content

Commit eb9ffc2

Browse files
Matovidloclaude
andcommitted
fix: improve test coverage and docs for shared-variables guard
- Extend TestRelationsMapperVariablesSharedAcrossConsumers with a values row loaded after both consumers, exercising the > 1 guard in VariablesValuesForRelation.NewOtherSideRelation - Replace fragile strings.Count(WarnAndErrorMessages, "variablesFor") assertion with AllMessagesTxt check for the exact formatted message - Fix grammar: "create" → "creates" in linkRelations comment - Document the > 1 guard mechanism and the remaining edge case where values_row is iterated before consumers in docs/relations-validation.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 3336901 commit eb9ffc2

3 files changed

Lines changed: 33 additions & 7 deletions

File tree

docs/relations-validation.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,13 @@ A values row's `variablesValuesFor` relation is linked by looking up the parent
6565

6666
Calling `GetOneByType(VariablesForRelType)` on Y at this point returns an error ("only one expected, but found 2"). If that error were propagated, it would produce a duplicate "invalid config Y" message alongside the one already generated by Pass 2 validation of Y itself.
6767

68-
To avoid the duplicate, `NewOtherSideRelation` silently returns `(nil, nil, nil)` when `GetOneByType` reports multiple relations — deferring to Pass 2 to emit the canonical warning. The `variablesValuesFor` relation on the values row is left in place; the values row is then transitively excluded from the pull output by the ignore mapper (see below).
68+
To avoid the duplicate, `NewOtherSideRelation` checks `len(GetByType(VariablesForRelType)) > 1` before calling `GetOneByType`. When multiple relations exist, it returns `(nil, nil, nil)` — deferring to Pass 2 to emit the canonical warning. The `variablesValuesFor` relation on the values row is left in place; the values row is then transitively excluded from the pull output by the ignore mapper (see below).
69+
70+
### Edge case: values row iterated before consumers
71+
72+
If the Loaded() order is `vars_config → values_row → consumer1 → consumer2`, then when `linkRelations(values_row)` runs in Pass 1, Y has zero `variablesFor` entries (consumers have not been processed yet). The `> 1` guard does not fire. `GetOneByType` returns `nil` (no relation found), and `NewOtherSideRelation` returns the "missing relation variablesFor" error — producing an extra warning in the output alongside the "found 2" warning from Pass 2.
73+
74+
This double-warning is a pre-existing edge case. In practice it does not arise because consumer configs always appear in Loaded() before variables values rows (the Storage API returns consumers, and values rows are discovered via the consumer's `variablesValuesFrom` relations). The `> 1` guard eliminates the more common ordering issue; the remaining edge case is benign — both warnings correctly describe the malformed setup.
6975

7076
## Why the ignore mapper excludes orphaned variables configs
7177

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-
// linkRelations 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 creates 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: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,9 @@ func TestRelationsMapperLinkRelations(t *testing.T) {
7070
// TestRelationsMapperVariablesSharedAcrossConsumers verifies that when a variables config
7171
// is loaded before its consumers in changes.Loaded(), the two-pass approach still produces
7272
// exactly one warning instead of crashing with "multiple parents defined by relations".
73+
// It also exercises the > 1 guard in VariablesValuesForRelation.NewOtherSideRelation by
74+
// including a values row loaded after both consumers so that, during Pass 1, the variables
75+
// config already holds two variablesFor relations when linkRelations(valuesRow) runs.
7376
func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
7477
t.Parallel()
7578
state, d := createStateWithMapper(t)
@@ -87,14 +90,19 @@ func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
8790
}
8891
require.NoError(t, state.Set(varsConfig))
8992

90-
// Consumer 1 — variablesFrom relation pointing to varsKey.
93+
// Consumer 1 — variablesFrom and variablesValuesFrom relations pointing to varsKey.
94+
// VariablesValuesFromRelation causes linkRelations(consumer1) to add a VariablesValuesFor
95+
// relation to the values row, which in turn exercises the > 1 guard when
96+
// linkRelations(valuesRow) runs later in Pass 1.
97+
valuesRowID := keboola.RowID("val1")
9198
consumer1Key := model.ConfigKey{BranchID: branchID, ComponentID: consumerCompID, ID: "consumer1"}
9299
consumer1 := &model.ConfigState{
93100
ConfigManifest: &model.ConfigManifest{ConfigKey: consumer1Key},
94101
Remote: &model.Config{
95102
ConfigKey: consumer1Key,
96103
Relations: model.Relations{
97104
&model.VariablesFromRelation{VariablesID: varsKey.ID},
105+
&model.VariablesValuesFromRelation{VariablesValuesID: valuesRowID},
98106
},
99107
},
100108
}
@@ -113,19 +121,31 @@ func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
113121
}
114122
require.NoError(t, state.Set(consumer2))
115123

116-
// Variables config is first in Loaded() — the ordering that previously caused the crash.
124+
// Values row — loaded LAST so that when linkRelations(valuesRow) runs in Pass 1, the
125+
// variables config already holds 2 variablesFor relations (added by consumer1 and
126+
// consumer2). The > 1 guard in VariablesValuesForRelation.NewOtherSideRelation fires
127+
// and returns (nil, nil, nil), preventing a duplicate "invalid config Y" error.
128+
valuesRowKey := model.ConfigRowKey{BranchID: branchID, ComponentID: keboola.VariablesComponentID, ConfigID: varsKey.ID, ID: valuesRowID}
129+
valuesRow := &model.ConfigRowState{
130+
ConfigRowManifest: &model.ConfigRowManifest{ConfigRowKey: valuesRowKey},
131+
Remote: &model.ConfigRow{ConfigRowKey: valuesRowKey},
132+
}
133+
require.NoError(t, state.Set(valuesRow))
134+
135+
// Variables config is first in Loaded(), values row is last — both orderings that
136+
// previously caused problems are exercised in a single pass.
117137
changes := model.NewRemoteChanges()
118138
changes.AddLoaded(varsConfig)
119139
changes.AddLoaded(consumer1)
120140
changes.AddLoaded(consumer2)
141+
changes.AddLoaded(valuesRow)
121142

122143
// Must return nil — the duplicate is a warning, not a fatal error.
123144
require.NoError(t, state.Mapper().AfterRemoteOperation(t.Context(), changes))
124145

125146
// 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"))
147+
allTxt := logger.AllMessagesTxt()
148+
assert.Equal(t, 1, strings.Count(allTxt, `Only one relation "variablesFor" expected, but found 2`))
129149
assert.Empty(t, logger.ErrorMessages())
130150
}
131151

0 commit comments

Comments
 (0)