Merge pull request #2564 from keboola/PSGO-227/fix-multiple-parents-variables-templates

Matovidlo · web-flow · commit a58f30389f4b · 2026-04-13T08:19:02.000+02:00
fix(relations): two-pass link+validate to prevent order-dependent multiple-parents crash [PSGO-227]
diff --git a/docs/relations-validation.md b/docs/relations-validation.md
@@ -0,0 +1,97 @@
+# Variables Relations Validation
+
+## Background: the keboola.variables parent-child model
+
+In the Keboola platform, variable configs (`keboola.variables` component) belong to a parent config (e.g. a transformation). In the local CLI directory layout, the variables config lives *inside* the parent's folder:
+
+```
+my-transformation/
+  variables/
+    config.json         ← keboola.variables config
+    values/
+      default/          ← variables values row
+```
+
+This parent-child ownership is expressed through relations stored on the objects:
+
+| Relation type | Stored on | Direction | Storage |
+|---|---|---|---|
+| `variablesFor` | variables config | → parent config | manifest (local only) |
+| `variablesFrom` | parent config | → variables config | API |
+| `variablesValuesFor` | values row | → parent config | manifest |
+| `variablesValuesFrom` | parent config | → values row | API |
+
+The `variablesFor` relation on the variables config determines which parent folder it is placed in during sync. Because each object can have only one file-system path, **each variables config can have exactly one `variablesFor` relation** (i.e. one parent). This is enforced by `OneToXRelations()` in `internal/pkg/model/relation.go`.
+
+## Why multiple parents can appear in the API
+
+Keboola's Storage API allows a variables config to be referenced by more than one consumer via the project's configuration metadata. If a user creates such a setup through the API or the UI directly, the `keboola.variables` config will have multiple `variablesFrom` back-references, which the mapper translates into multiple `variablesFor` relations during the linking step.
+
+## The `multiple parents` guard
+
+`Relations.ParentKey()` (`internal/pkg/model/relation.go:66`) collects all relations that define a parent key. If more than one is found it returns a fatal error:
+
+```
+unexpected state: multiple parents defined by "relations" in <config desc>
+```
+
+This guard exists to protect the CLI sync engine: without it, path generation would be ambiguous and the local directory structure would be corrupted.
+
+## The ordering bug that broke Templates
+
+The relation mapper (`internal/pkg/mapper/relations/link.go`) previously processed objects in a single pass: link → validate per object. This is order-dependent:
+
+1. If the variables config Y is iterated **before** its consumers X and Z, `validateRelations(Y)` runs when Y has zero `variablesFor` relations — nothing to clean up.
+2. Later iterations for X and Z call `linkRelations`, which adds `VariablesForRelation` entries to Y.
+3. After the mapper loop, `PathsGenerator.Invoke()` (called in `remote/manager.go`) calls `Y.ParentKey()`, finds two parents, and returns the fatal error.
+
+CLI `pull` was unaffected in practice because the API happened to return consumer configs before variables configs, making consumers iterated first. The Templates service load path had a different iteration order and hit the bug.
+
+## The fix: two-pass link and validate
+
+`AfterRemoteOperation` and `AfterLocalOperation` were refactored into two explicit passes:
+
+**Pass 1 — link all objects**
+Run `linkRelations` for every loaded object. This creates all other-side relations (including adding `variablesFor` entries to every variables config) before any validation happens.
+
+**Pass 2 — validate all objects**
+Run `validateRelations` for every loaded object. With the complete relation graph now in place, duplicates are correctly detected, removed, and logged as warnings — regardless of the order objects appear in the API response.
+
+This makes behaviour order-independent: both CLI and the Templates service emit a warning and continue when a variables config has multiple parents, instead of crashing.
+
+## Silent skip in VariablesValuesForRelation.NewOtherSideRelation
+
+A values row's `variablesValuesFor` relation is linked by looking up the parent variables config's single `variablesFor` relation to determine the target consumer config. In the two-pass approach, Pass 1 runs before validation, so when `linkRelations(values_row)` runs, the parent variables config Y may already hold two `variablesFor` entries (added earlier in the same pass by the consumer configs).
+
+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.
+
+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).
+
+### Edge case: values row iterated before consumers
+
+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.
+
+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.
+
+## Why the ignore mapper excludes orphaned variables configs
+
+After `AfterRemoteOperation` completes, the ignore mapper (`internal/pkg/mapper/ignore/remote.go`) runs. It marks a `keboola.variables` config as ignored when it has no `variablesFor` relation. Config rows are **transitively ignored** when their parent config is ignored. So after Pass 2 removes Y's duplicate `variablesFor` entries (leaving Y with zero), both Y and its values rows are removed from `changes.Loaded()` and never written to the local filesystem.
+
+`PathsGenerator` only processes objects in `changes.Loaded()`, so it never encounters Y with multiple parents — making the fatal guard in `Relations.ParentKey()` safe to keep as-is.
+
+## Why this is warning-only, not a hard error
+
+The `validateRelations` function removes all duplicate relations and logs a warning. The invalid `variablesFor` entries are dropped and the variables config (along with its rows) is excluded from the local sync output. The project can still be synced — it just does not represent the shared-variables scenario in the local directory hierarchy.
+
+Templates only reads remote state to discover existing configs and apply template changes on top. It never generates or syncs a local directory hierarchy, so the "ambiguous path" concern does not apply. Making this a hard error in the Templates code path only served to block users from using templates in projects with this configuration.
+
+## Related files
+
+| File | Role |
+|---|---|
+| `internal/pkg/mapper/relations/link.go` | Two-pass AfterRemoteOperation / AfterLocalOperation |
+| `internal/pkg/model/relation.go` | `Relations.ParentKey()`, `OneToXRelations()` |
+| `internal/pkg/model/variables.go` | `VariablesValuesForRelation.NewOtherSideRelation` — silent skip for multiple variablesFor |
+| `internal/pkg/mapper/ignore/remote.go` | Transitive ignore of orphaned variables configs and their rows |
+| `internal/pkg/state/remote/manager.go` | Sequencing of AfterRemoteOperation → PathsGenerator |
+| `test/cli/pull/variables-used-twice/` | E2E test covering the shared-variables warning |
diff --git a/internal/pkg/mapper/relations/link.go b/internal/pkg/mapper/relations/link.go
@@ -12,9 +12,27 @@ import (
 func (m *relationsMapper) AfterLocalOperation(ctx context.Context, changes *model.LocalChanges) error {
 	errs := errors.NewMultiError()
 	allObjects := m.state.LocalObjects()
-	for _, objectState := range changes.Loaded() {
-		if err := m.linkAndValidateRelations(objectState.LocalState(), allObjects); err != nil {
-			errs.Append(err)
+	loaded := changes.Loaded()
+
+	// Pass 1: link all objects so every other-side relation exists before validation.
+	// Processing link and validate in a single loop would cause order-dependent behaviour:
+	// a variables config iterated before its consumers would be validated before the consumer
+	// linking steps add VariablesForRelation entries to it, leaving duplicates undetected until
+	// PathsGenerator runs and hits a fatal "multiple parents" error.
+	for _, objectState := range loaded {
+		if o, ok := objectState.LocalState().(model.ObjectWithRelations); ok {
+			if err := m.linkRelations(o, allObjects); err != nil {
+				errs.Append(err)
+			}
+		}
+	}
+
+	// Pass 2: validate all objects now that the relation graph is complete.
+	for _, objectState := range loaded {
+		if o, ok := objectState.LocalState().(model.ObjectWithRelations); ok {
+			if err := m.validateRelations(o); err != nil {
+				errs.Append(errors.PrefixErrorf(err, "invalid %s", objectState.LocalState().Desc()))
+			}
 		}
 	}
 
@@ -30,9 +48,24 @@ func (m *relationsMapper) AfterLocalOperation(ctx context.Context, changes *mode
 func (m *relationsMapper) AfterRemoteOperation(ctx context.Context, changes *model.RemoteChanges) error {
 	errs := errors.NewMultiError()
 	allObjects := m.state.RemoteObjects()
-	for _, objectState := range changes.Loaded() {
-		if err := m.linkAndValidateRelations(objectState.RemoteState(), allObjects); err != nil {
-			errs.Append(err)
+	loaded := changes.Loaded()
+
+	// Pass 1: link all objects so every other-side relation exists before validation.
+	// See AfterLocalOperation for the reasoning behind the two-pass approach.
+	for _, objectState := range loaded {
+		if o, ok := objectState.RemoteState().(model.ObjectWithRelations); ok {
+			if err := m.linkRelations(o, allObjects); err != nil {
+				errs.Append(err)
+			}
+		}
+	}
+
+	// Pass 2: validate all objects now that the relation graph is complete.
+	for _, objectState := range loaded {
+		if o, ok := objectState.RemoteState().(model.ObjectWithRelations); ok {
+			if err := m.validateRelations(o); err != nil {
+				errs.Append(errors.PrefixErrorf(err, "invalid %s", objectState.RemoteState().Desc()))
+			}
 		}
 	}
 
@@ -44,20 +77,7 @@ func (m *relationsMapper) AfterRemoteOperation(ctx context.Context, changes *mod
 	return nil
 }
 
-func (m *relationsMapper) linkAndValidateRelations(object model.Object, allObjects model.Objects) error {
-	errs := errors.NewMultiError()
-	if o, ok := object.(model.ObjectWithRelations); ok {
-		if err := m.linkRelations(o, allObjects); err != nil {
-			errs.Append(err)
-		}
-		if err := m.validateRelations(o); err != nil {
-			errs.Append(errors.PrefixErrorf(err, "invalid %s", object.Desc()))
-		}
-	}
-	return errs.ErrorOrNil()
-}
-
-// lintRelations finds the other side of the relation and create a corresponding relation on the other side.
+// linkRelations finds the other side of the relation and creates a corresponding relation on the other side.
 func (m *relationsMapper) linkRelations(object model.ObjectWithRelations, allObjects model.Objects) error {
 	errs := errors.NewMultiError()
 	relations := object.GetRelations()
diff --git a/internal/pkg/mapper/relations/link_test.go b/internal/pkg/mapper/relations/link_test.go
@@ -4,6 +4,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/keboola/keboola-sdk-go/v2/pkg/keboola"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -66,6 +67,99 @@ func TestRelationsMapperLinkRelations(t *testing.T) {
 	}, object2.Local.Relations)
 }
 
+// TestRelationsMapperVariablesSharedAcrossConsumers verifies that when a variables config
+// is loaded before its consumers in changes.Loaded(), the two-pass approach still produces
+// exactly one warning instead of crashing with "multiple parents defined by relations".
+// It also exercises the > 1 guard in VariablesValuesForRelation.NewOtherSideRelation by
+// including a values row loaded after both consumers so that, during Pass 1, the variables
+// config already holds two variablesFor relations when linkRelations(valuesRow) runs.
+func TestRelationsMapperVariablesSharedAcrossConsumers(t *testing.T) {
+	t.Parallel()
+	state, d := createStateWithMapper(t)
+	logger := d.DebugLogger()
+
+	branchID := keboola.BranchID(1)
+	consumerCompID := keboola.ComponentID("ex-generic-v2")
+
+	// Variables config (Y) — added to state and loaded FIRST to exercise the ordering
+	// that previously caused a fatal "multiple parents" crash in PathsGenerator.
+	varsKey := model.ConfigKey{BranchID: branchID, ComponentID: keboola.VariablesComponentID, ID: "vars"}
+	varsConfig := &model.ConfigState{
+		ConfigManifest: &model.ConfigManifest{ConfigKey: varsKey},
+		Remote:         &model.Config{ConfigKey: varsKey},
+	}
+	require.NoError(t, state.Set(varsConfig))
+
+	// Consumer 1 — variablesFrom and variablesValuesFrom relations pointing to varsKey.
+	// VariablesValuesFromRelation causes linkRelations(consumer1) to add a VariablesValuesFor
+	// relation to the values row, which in turn exercises the > 1 guard when
+	// linkRelations(valuesRow) runs later in Pass 1.
+	valuesRowID := keboola.RowID("val1")
+	consumer1Key := model.ConfigKey{BranchID: branchID, ComponentID: consumerCompID, ID: "consumer1"}
+	consumer1 := &model.ConfigState{
+		ConfigManifest: &model.ConfigManifest{ConfigKey: consumer1Key},
+		Remote: &model.Config{
+			ConfigKey: consumer1Key,
+			Relations: model.Relations{
+				&model.VariablesFromRelation{VariablesID: varsKey.ID},
+				&model.VariablesValuesFromRelation{VariablesValuesID: valuesRowID},
+			},
+		},
+	}
+	require.NoError(t, state.Set(consumer1))
+
+	// Consumer 2 — also variablesFrom relation pointing to the same varsKey.
+	consumer2Key := model.ConfigKey{BranchID: branchID, ComponentID: consumerCompID, ID: "consumer2"}
+	consumer2 := &model.ConfigState{
+		ConfigManifest: &model.ConfigManifest{ConfigKey: consumer2Key},
+		Remote: &model.Config{
+			ConfigKey: consumer2Key,
+			Relations: model.Relations{
+				&model.VariablesFromRelation{VariablesID: varsKey.ID},
+			},
+		},
+	}
+	require.NoError(t, state.Set(consumer2))
+
+	// Values row — loaded LAST so that when linkRelations(valuesRow) runs in Pass 1, the
+	// variables config already holds 2 variablesFor relations (added by consumer1 and
+	// consumer2). The > 1 guard in VariablesValuesForRelation.NewOtherSideRelation fires
+	// and returns (nil, nil, nil), preventing a duplicate "invalid config Y" error.
+	valuesRowKey := model.ConfigRowKey{BranchID: branchID, ComponentID: keboola.VariablesComponentID, ConfigID: varsKey.ID, ID: valuesRowID}
+	valuesRow := &model.ConfigRowState{
+		ConfigRowManifest: &model.ConfigRowManifest{ConfigRowKey: valuesRowKey},
+		Remote:            &model.ConfigRow{ConfigRowKey: valuesRowKey},
+	}
+	require.NoError(t, state.Set(valuesRow))
+
+	// Variables config is first in Loaded(), values row is last — both orderings that
+	// previously caused problems are exercised in a single pass.
+	changes := model.NewRemoteChanges()
+	changes.AddLoaded(varsConfig)
+	changes.AddLoaded(consumer1)
+	changes.AddLoaded(consumer2)
+	changes.AddLoaded(valuesRow)
+
+	// Must return nil — the duplicate is a warning, not a fatal error.
+	require.NoError(t, state.Mapper().AfterRemoteOperation(t.Context(), changes))
+
+	// Exactly one warning about the duplicate variablesFor relation.
+	allTxt := logger.AllMessagesTxt()
+	assert.Equal(t, 1, strings.Count(allTxt, `Only one relation "variablesFor" expected, but found 2`))
+	assert.Empty(t, logger.ErrorMessages())
+
+	// The > 1 guard in VariablesValuesForRelation.NewOtherSideRelation prevented
+	// linkRelations(valuesRow) from adding a VariablesValuesFromRelation to any consumer.
+	// consumer1 retains its original single VariablesValuesFromRelation (no duplicate was
+	// added from the values-row side), and consumer2 has none at all.
+	assert.Len(t, consumer1.Remote.Relations.GetByType(model.VariablesValuesFromRelType), 1)
+	assert.Empty(t, consumer2.Remote.Relations.GetByType(model.VariablesValuesFromRelType))
+
+	// AfterLocalOperation is structurally identical to AfterRemoteOperation (same two-pass
+	// logic). It is not re-tested here because the shared-variables scenario only arises
+	// from the remote API path, where object order is non-deterministic.
+}
+
 func TestRelationsMapperOtherSideMissing(t *testing.T) {
 	t.Parallel()
 	state, d := createStateWithMapper(t)
diff --git a/internal/pkg/model/variables.go b/internal/pkg/model/variables.go
@@ -178,6 +178,14 @@ func (t *VariablesValuesForRelation) NewOtherSideRelation(relationDefinedOn Obje
 		return nil, nil, errors.Errorf(`%s not found, referenced from %s, by relation "%s"`, variablesConfigKey.Desc(), relationDefinedOn.Desc(), t.Type())
 	}
 	variablesConfig := variablesConfigRaw.(*Config)
+	// When the variables config temporarily holds multiple variablesFor relations (shared
+	// across consumers), skip linking here — Pass 2 validation will detect and warn about
+	// the duplicates and remove them. The variablesValuesFor relation on the row is
+	// intentionally left in place; the ignore mapper excludes the parent variables config
+	// (and this row transitively) after Pass 2 removes all variablesFor entries.
+	if len(variablesConfig.Relations.GetByType(VariablesForRelType)) > 1 {
+		return nil, nil, nil
+	}
 	variablesForRaw, err := variablesConfig.Relations.GetOneByType(VariablesForRelType)
 	if err != nil {
 		return nil, nil, errors.PrefixErrorf(err, "invalid %s", variablesConfig.Desc())
diff --git a/test/cli/pull/variables-used-twice/expected-stderr b/test/cli/pull/variables-used-twice/expected-stderr
@@ -3,6 +3,3 @@ Warning:
   - Only one relation "variablesFor" expected, but found 2:
     - {"componentId":"ex-generic-v2","configId":"%s"}
     - {"componentId":"ex-generic-v2","configId":"%s"}
-- Missing relation "variablesFor" in config "branch:%s/component:keboola.variables/config:%s":
-  - Referenced from config row "branch:%s/component:keboola.variables/config:%s/row:%s".
-  - By relation "variablesValuesFor".
diff --git a/test/templates/api/projects/project-with-shared-variables/001-list-instances/expected-http-code b/test/templates/api/projects/project-with-shared-variables/001-list-instances/expected-http-code
@@ -0,0 +1 @@
+200
diff --git a/test/templates/api/projects/project-with-shared-variables/001-list-instances/expected-response.json b/test/templates/api/projects/project-with-shared-variables/001-list-instances/expected-response.json
@@ -0,0 +1,3 @@
+{
+  "instances": []
+}
diff --git a/test/templates/api/projects/project-with-shared-variables/001-list-instances/request.json b/test/templates/api/projects/project-with-shared-variables/001-list-instances/request.json
@@ -0,0 +1,9 @@
+{
+  "path": "/v1/project/%%TEST_BRANCH_MAIN_ID%%/instances",
+  "method": "GET",
+  "headers": {
+    "Content-Type": "application/json",
+    "X-StorageApi-Token": "%%TEST_KBC_STORAGE_API_TOKEN%%"
+  },
+  "body": {}
+}
diff --git a/test/templates/api/projects/project-with-shared-variables/expected-state.json b/test/templates/api/projects/project-with-shared-variables/expected-state.json
@@ -0,0 +1,64 @@
+{
+  "branches": [
+    {
+      "branch": {
+        "name": "Main",
+        "description": "",
+        "isDefault": true
+      },
+      "configs": [
+        {
+          "componentId": "keboola.variables",
+          "name": "Variables",
+          "description": "test fixture",
+          "configuration": {
+            "variables": [
+              {
+                "name": "foo",
+                "type": "string"
+              }
+            ]
+          },
+          "rows": [
+            {
+              "name": "Default values",
+              "description": "test fixture",
+              "isDisabled": false,
+              "configuration": {
+                "values": [
+                  {
+                    "name": "foo",
+                    "value": "bar"
+                  }
+                ]
+              }
+            }
+          ],
+          "isDisabled": false
+        },
+        {
+          "componentId": "ex-generic-v2",
+          "name": "With Variables 1",
+          "description": "test fixture",
+          "configuration": {
+            "variables_id": "%s",
+            "variables_values_id": "%s"
+          },
+          "rows": [],
+          "isDisabled": false
+        },
+        {
+          "componentId": "ex-generic-v2",
+          "name": "With Variables 2",
+          "description": "test fixture",
+          "configuration": {
+            "variables_id": "%s",
+            "variables_values_id": "%s"
+          },
+          "rows": [],
+          "isDisabled": false
+        }
+      ]
+    }
+  ]
+}
diff --git a/test/templates/api/projects/project-with-shared-variables/initial-state.json b/test/templates/api/projects/project-with-shared-variables/initial-state.json
