core: Fix fallback variable dedup colliding by name instead of UUID - #139
Merged
Conversation
resolveOneReference/collectFallbackVars (orchestrator.go) and addEmbeddedFallbackVariable/deriveVariableName (embedded_references.go) both deduped fallback Terraform variables by their derived name, never by the UUID being resolved. Two resources referencing different UUIDs of the same not-yet-exported type, or two DaVinci nodes sharing a title but different UUIDs, silently collapsed onto one variable — the first UUID won as the default and every other resource was rewritten to reference the wrong ID with no error or warning. Both paths now route generated names through a shared fallbackVariableAllocator that dedupes on the UUID and disambiguates colliding names with a UUID-derived suffix. Closes #130, Closes #138 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Regression Test Results
✅ No regressions detectedAll export configurations produced compatible output. Generated by regression workflow • View run |
…, not the target UUID Follow-up to c0d54b7. The initial fix disambiguated colliding fallback variable names using a suffix derived from the target UUID being resolved. That UUID is per-environment API data, so it leaked environment-specific values into the checked-in variable *name* itself — exporting the same configuration from two environments could produce two different variable names for the same logical reference site, breaking the "one module, per-environment tfvars" model. Both fallback-variable code paths now disambiguate using a stable, environment-portable identifier for the *referencing* side instead: - orchestrator.go passes the referencing resource's own Terraform label (rd.Label). - embedded_references.go threads the accumulated wildcard-traversal key (e.g. a DaVinci node's own data.id) through walkAndProcessPath and processRawHCLValue. The allocator's dedup key is unchanged (still the target UUID, so repeat references to the same UUID still collapse onto one variable); only the disambiguation *hint* moved off the UUID. Added TestResolveEmbeddedReferences_DisambiguatedName_StableAcrossEnvironments to lock in that the same flow structure exported against different target UUIDs produces identical variable names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Regression Test Results
✅ No regressions detectedAll export configurations produced compatible output. Generated by regression workflow • View run |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two related bugs where fallback Terraform variables (emitted when a referenced resource type isn't yet exported) were deduplicated by their derived name instead of the UUID being resolved, causing two distinct referenced instances to silently collapse onto one variable.
Reference fallback-variable name collides across resources referencing different instances of the same not-yet-exported type #138 (
internal/core/orchestrator.go):resolveOneReference's fallback path names the variable fromReferencesType + ReferenceFieldonly — no UUID awareness.collectFallbackVarsdeduped on that same static name. Any two resources of the same type referencing different instances of a not-yet-exported target type (e.g. twopingone_populations with differenttheme.idvalues) collapsed onto one.tfvarsentry; the second resource's real theme ID was silently discarded.Embedded reference fallback-variable dedup collides on name, not UUID — silent misattribution when node titles repeat with different UUIDs #130 (
internal/core/embedded_references.go): sibling bug in the embedded-reference (JSON-blob UUID) path.addEmbeddedFallbackVariablededuped on the derived human-readable name (deriveVariableName), so two DaVinci flow nodes sharing a title (e.g. both "Continue") but pointing at different UUIDs collapsed onto one fallback variable.Both paths route generated names through a shared
fallbackVariableAllocator(internal/core/fallback_variable_allocator.go) that dedupes on the target UUID and disambiguates colliding base names using a stable, environment-portable identifier from the referencing side — not the target UUID itself:orchestrator.gouses the referencing resource's own Terraform label.embedded_references.gouses the accumulated wildcard-traversal key (e.g. a DaVinci node's owndata.id).This distinction matters: the target UUID is per-environment API data. Baking it into the variable name (rather than just its default value) would make the same logical reference site produce a different variable name every time the same configuration is exported from a different environment, breaking the "one checked-in module, per-environment tfvars" model. The chosen identifiers are stable across environment exports of the same configuration.
Documented the naming standard in
contributing/ARCHITECTURE.mdandcontributing/DEVELOPER_HANDBOOK.mdso future fallback-variable-producing code routes through the shared allocator with a referencing-side hint, instead of deduping by name directly or disambiguating with the target UUID.Test plan
TestDisambiguateFallbackVariableNames_DistinctUUIDsSameBaseName/TestDisambiguateFallbackVariableNames_SameUUIDStaysDeduped(unit-level, orchestrator.go) plusTestExportOrchestrator_Export_DistinctReferencedUUIDs_ProduceDistinctFallbackVars(full pipeline integration) cover the previously-missing different-UUID-same-name case for Reference fallback-variable name collides across resources referencing different instances of the same not-yet-exported type #138.TestResolveEmbeddedReferences_DistinctUUIDsSameTitle_NoCollisioncovers the previously-missing different-UUID-same-name case for Embedded reference fallback-variable dedup collides on name, not UUID — silent misattribution when node titles repeat with different UUIDs #130 (two nodes titled "Continue", different UUIDs → two distinct fallback variables, each pointing at its own UUID).TestResolveEmbeddedReferences_DisambiguatedName_StableAcrossEnvironmentslocks in environment-portability: exporting the same flow structure (same node ids/titles) with different target UUIDs — simulating the same configuration exported from two PingOne environments — produces identical variable names; only the defaults differ.go build ./...— cleango vet ./...— cleango test ./internal/... -v -count=1— all packages passgo run ./tools/validate-definitions definitions/— 8/8 passedmake regression-localagainst a live PingOne environment (re-run after the redesign) — no differences vsmainCloses #130, Closes #138