Skip to content

Commit 485ae67

Browse files
Matovidloclaude
andcommitted
fix: address round-3 PR review comments
- variables.go: clarify that variablesValuesFor is left in place intentionally and relies on the ignore mapper for cleanup - link_test.go: assert that consumer1 has exactly one variablesValuesFrom (no duplicate added) and consumer2 has none, confirming the > 1 guard; add note explaining AfterLocalOperation deliberate omission - docs: reword edge case as "known limitation" rather than "pre-existing bug" - expected-stderr: remove extra trailing blank line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent a63283a commit 485ae67

4 files changed

Lines changed: 15 additions & 3 deletions

File tree

docs/relations-validation.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ To avoid the duplicate, `NewOtherSideRelation` checks `len(GetByType(VariablesFo
7171

7272
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.
7373

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.
74+
This double-warning is a **known limitation of this fix**, not a pre-existing bug. Before this PR, the values-row-first ordering would have led to a different failure mode depending on whether consumers appeared later. The `> 1` guard eliminates the crash and the more common duplicate-warning case; this remaining ordering is accepted as a minor trade-off. In practice it does not arise because consumer configs always appear in `Loaded()` before their variables values rows (the Storage API returns consumers first, and values rows are discovered via the consumer's `variablesValuesFrom` relations). Both warnings correctly describe the malformed setup, so no data is lost.
7575

7676
## Why the ignore mapper excludes orphaned variables configs
7777

internal/pkg/mapper/relations/link_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,17 @@ func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
147147
allTxt := logger.AllMessagesTxt()
148148
assert.Equal(t, 1, strings.Count(allTxt, `Only one relation "variablesFor" expected, but found 2`))
149149
assert.Empty(t, logger.ErrorMessages())
150+
151+
// The > 1 guard in VariablesValuesForRelation.NewOtherSideRelation prevented
152+
// linkRelations(valuesRow) from adding a VariablesValuesFromRelation to any consumer.
153+
// consumer1 retains its original single VariablesValuesFromRelation (no duplicate was
154+
// added from the values-row side), and consumer2 has none at all.
155+
assert.Equal(t, 1, len(consumer1.Remote.Relations.GetByType(model.VariablesValuesFromRelType)))
156+
assert.Empty(t, consumer2.Remote.Relations.GetByType(model.VariablesValuesFromRelType))
157+
158+
// AfterLocalOperation is structurally identical to AfterRemoteOperation (same two-pass
159+
// logic). It is not re-tested here because the shared-variables scenario only arises
160+
// from the remote API path, where object order is non-deterministic.
150161
}
151162

152163
func TestRelationsMapperOtherSideMissing(t *testing.T) {

internal/pkg/model/variables.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,9 @@ func (t *VariablesValuesForRelation) NewOtherSideRelation(relationDefinedOn Obje
180180
variablesConfig := variablesConfigRaw.(*Config)
181181
// When the variables config temporarily holds multiple variablesFor relations (shared
182182
// across consumers), skip linking here — Pass 2 validation will detect and warn about
183-
// the duplicates and remove them.
183+
// the duplicates and remove them. The variablesValuesFor relation on the row is
184+
// intentionally left in place; the ignore mapper excludes the parent variables config
185+
// (and this row transitively) after Pass 2 removes all variablesFor entries.
184186
if len(variablesConfig.Relations.GetByType(VariablesForRelType)) > 1 {
185187
return nil, nil, nil
186188
}

test/cli/pull/variables-used-twice/expected-stderr

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,3 @@ Warning:
33
- Only one relation "variablesFor" expected, but found 2:
44
- {"componentId":"ex-generic-v2","configId":"%s"}
55
- {"componentId":"ex-generic-v2","configId":"%s"}
6-

0 commit comments

Comments
 (0)