fix: two-pass relation linking to prevent order-dependent multiple-parents fatal error

When a keboola.variables config is referenced by more than one consumer
config, the mapper creates multiple variablesFor relations on it. In the
original single-pass AfterRemoteOperation (link+validate per object),
if the variables config is iterated before its consumers, validateRelations
runs when it has 0 variablesFor — finding nothing to clean up. Consumers
are processed later, leaving two variablesFor entries that cause
PathsGenerator to crash with "multiple parents defined by relations".

This ordering happened in the Templates service but not in CLI pull
(where the API returns consumers before the variables config).

Fix: split AfterRemoteOperation and AfterLocalOperation into two passes:
  Pass 1 — linkRelations for all objects (builds complete relation graph)
  Pass 2 — validateRelations for all objects (detects and removes duplicates)

This is order-independent: the variables config always has both variablesFor
entries present when validated, so cleanup always fires before PathsGenerator.

Also suppress the duplicate warning that would otherwise appear in Pass 1:
VariablesValuesForRelation.NewOtherSideRelation silently skips (nil, nil, nil)
when the parent variables config has multiple variablesFor — deferring to
Pass 2 to emit the canonical "only one relation variablesFor expected" warning.
The ignore mapper then excludes the orphaned config and its rows from the
local output, same as before.

Update expected-stderr for the variables-used-twice E2E test to reflect the
new single warning (the secondary "missing relation variablesFor" message was
an artifact of the old single-pass ordering and is no longer produced).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: Matovidlo

docs/relations-validation.md

@@ -29,58 +29,63 @@ Keboola's Storage API allows a variables config to be referenced by more than on
 
 ## 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 an error:
+`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 detect invalid state in the relation graph.
+This guard exists to protect the CLI sync engine: without it, path generation would be ambiguous and the local directory structure would be corrupted.
 
-## Config.ParentKey() and PathsGenerator
+## The ordering bug that broke Templates
 
-`Config.ParentKey()` (`internal/pkg/model/object.go`) calls `Relations.ParentKey()` to find the relation-defined parent. If that returns an error (multiple parents) or nil (no parent), it falls back to the structural parent — the branch.
+The relation mapper (`internal/pkg/mapper/relations/link.go`) previously processed objects in a single pass: link → validate per object. This is order-dependent:
 
-`PathsGenerator.doUpdate()` (`internal/pkg/state/local/paths.go`) calls `object.ParentKey()` on every loaded object to build local directory paths. With the branch-fallback in `Config.ParentKey()`, a variables config that has multiple `variablesFor` relations is placed at the branch root (e.g. `main/variables/`) rather than inside any specific parent folder. This is non-fatal: PathsGenerator can complete and remote state loading succeeds.
+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.
 
-## The ordering issue
+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 relation mapper (`internal/pkg/mapper/relations/link.go`) processes objects in `AfterRemoteOperation` and `AfterLocalOperation`. It links and validates each object in a single pass:
+## The fix: two-pass link and validate
 
-```
-for each object:
-    linkRelations(object)    // adds other-side relations to OTHER objects
-    validateRelations(object) // validates THIS object's relations
-```
+`AfterRemoteOperation` and `AfterLocalOperation` were refactored into two explicit passes:
 
-This is order-dependent. If the variables config Y is iterated **before** its consumers X and Z:
+**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.
 
-1. `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. Y ends up with two `variablesFor` relations in the loaded remote state.
-4. `PathsGenerator.doUpdate()` calls `Y.ParentKey()` — before the branch-fallback fix this was a fatal error.
+**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.
 
-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 triggered step 3-4.
+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).
 
-## The fix: branch fallback in Config.ParentKey()
+## Why the ignore mapper excludes orphaned variables configs
 
-`Config.ParentKey()` was changed to ignore the "multiple parents" error from `Relations.ParentKey()` and fall back to the branch (structural parent) instead of propagating the error. This makes PathsGenerator non-fatal for shared-variables configs regardless of iteration order.
+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.
 
-The `validateRelations` function still detects the duplicate `variablesFor` relations and logs a warning. In CLI `pull`, this warning fires correctly when consumers are iterated before the variables config (the API's typical return order). In the Templates service load path the warning may fire differently (or not at all for the duplicate, depending on iteration order), but Templates is only reading remote state — it never generates a local directory hierarchy — so the warning is not critical there.
+`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, leaving the variables config parentless for path generation purposes (it is placed at the branch root rather than inside a parent folder). The project can still be synced — it just does not represent the shared-variables scenario in the local directory hierarchy.
+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/model/object.go` | `Config.ParentKey()` — branch fallback when multiple relation parents |
+| `internal/pkg/mapper/relations/link.go` | Two-pass AfterRemoteOperation / AfterLocalOperation |
 | `internal/pkg/model/relation.go` | `Relations.ParentKey()`, `OneToXRelations()` |
-| `internal/pkg/mapper/relations/link.go` | Single-pass AfterRemoteOperation / AfterLocalOperation |
-| `internal/pkg/model/variables.go` | `VariablesForRelation`, `VariablesFromRelation` definitions |
+| `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 |

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,19 +77,6 @@ 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.
 func (m *relationsMapper) linkRelations(object model.ObjectWithRelations, allObjects model.Objects) error {
 	errs := errors.NewMultiError()

internal/pkg/model/object.go

@@ -595,17 +595,14 @@ func (r *ConfigRow) GetContent() *orderedmap.OrderedMap {
 }
 
 // ParentKey - config parent can be modified via Relations, for example variables config is embedded in another config.
-// If multiple relation-defined parents exist (e.g. a shared keboola.variables config referenced by more than one
-// consumer), the relations error is ignored and the config falls back to its structural parent (branch). The
-// duplicate-relations validation in AfterRemoteOperation logs a warning and removes the extra relations; until that
-// cleanup runs, PathsGenerator must not crash so that remote state loading can complete.
 func (c *Config) ParentKey() (Key, error) {
-	parentKey, err := c.Relations.ParentKey(c.Key())
-	if err == nil && parentKey != nil {
+	if parentKey, err := c.Relations.ParentKey(c.Key()); err != nil {
+		return nil, err
+	} else if parentKey != nil {
 		return parentKey, nil
 	}
 
-	// No relation-defined parent (or multiple parents — treat as no parent) -> parent is branch.
+	// No parent defined via "Relations" -> parent is branch
 	return c.ConfigKey.ParentKey()
 }
 

internal/pkg/model/variables.go

@@ -180,7 +180,10 @@ func (t *VariablesValuesForRelation) NewOtherSideRelation(relationDefinedOn Obje
 	variablesConfig := variablesConfigRaw.(*Config)
 	variablesForRaw, err := variablesConfig.Relations.GetOneByType(VariablesForRelType)
 	if err != nil {
-		return nil, nil, errors.PrefixErrorf(err, "invalid %s", variablesConfig.Desc())
+		// Multiple variablesFor relations exist on the variables config (shared across consumers).
+		// Pass 2 validation will detect and warn about the duplicates; skip linking here to avoid
+		// a redundant error before the cleanup runs.
+		return nil, nil, nil
 	}
 	if variablesForRaw == nil {
 		return nil, nil, errors.NewNestedError(

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

@@ -3,6 +3,4 @@ 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".
+