From 75ef61c7833962038d8832a420a366721cfc2a02 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 21 Dec 2021 15:08:48 -0500 Subject: [PATCH] check for nested module index changes Changing only the index on a nested module will cause all nested moves to create cycles, since their full addresses will match both the From and To addresses. When building the dependency graph, check if the parent is only changing the index of the containing module, and prevent the backwards edge for the move. --- internal/addrs/move_endpoint_module.go | 40 +++++----------- internal/addrs/move_endpoint_module_test.go | 21 ++++++++- internal/refactoring/move_execute.go | 30 ++++++++++-- internal/refactoring/move_validate_test.go | 52 +++++++++++++++++++++ 4 files changed, 109 insertions(+), 34 deletions(-) diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 7ff17621b..fdc8a5c25 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -705,10 +705,10 @@ func (r AbsResourceInstance) MoveDestination(fromMatch, toMatch *MoveEndpointInM } } -// IsModuleMoveReIndex takes the from and to endpoints from a move statement, -// and returns true if the only changes are to module indexes, and all -// non-absolute paths remain the same. -func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { +// IsModuleReIndex takes the From and To endpoints from a single move +// statement, and returns true if the only changes are to module indexes, and +// all non-absolute paths remain the same. +func (from *MoveEndpointInModule) IsModuleReIndex(to *MoveEndpointInModule) bool { // The statements must originate from the same module. if !from.module.Equal(to.module) { panic("cannot compare move expressions from different modules") @@ -718,37 +718,21 @@ func IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { case AbsModuleCall: switch t := to.relSubject.(type) { case ModuleInstance: - if len(t) != 1 { - // An AbsModuleCall only ever has one segment, so the - // ModuleInstance length must match. - return false - } - - return f.Call.Name == t[0].Name + // Generate a synthetic module to represent the full address of + // the module call. We're not actually comparing indexes, so the + // instance doesn't matter. + callAddr := f.Instance(NoKey).Module() + return callAddr.Equal(t.Module()) } case ModuleInstance: switch t := to.relSubject.(type) { case AbsModuleCall: - if len(f) != 1 { - return false - } - - return f[0].Name == t.Call.Name + callAddr := t.Instance(NoKey).Module() + return callAddr.Equal(f.Module()) case ModuleInstance: - // We must have the same number of segments, and the names must all - // match in order for this to solely be an index change operation. - if len(f) != len(t) { - return false - } - - for i := range f { - if f[i].Name != t[i].Name { - return false - } - } - return true + return t.Module().Equal(f.Module()) } } diff --git a/internal/addrs/move_endpoint_module_test.go b/internal/addrs/move_endpoint_module_test.go index 1e2758239..c1643d44c 100644 --- a/internal/addrs/move_endpoint_module_test.go +++ b/internal/addrs/move_endpoint_module_test.go @@ -1686,6 +1686,25 @@ func TestIsModuleMoveReIndex(t *testing.T) { }, expect: false, }, + + { + from: AbsModuleCall{ + Module: mustParseModuleInstanceStr(`module.bar[0]`), + Call: ModuleCall{Name: "baz"}, + }, + to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + expect: true, + }, + + { + from: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), + to: AbsModuleCall{ + Module: mustParseModuleInstanceStr(`module.bar[0]`), + Call: ModuleCall{Name: "baz"}, + }, + expect: true, + }, + { from: mustParseModuleInstanceStr(`module.baz`), to: mustParseModuleInstanceStr(`module.bar.module.baz[0]`), @@ -1709,7 +1728,7 @@ func TestIsModuleMoveReIndex(t *testing.T) { relSubject: test.to, } - if got := IsModuleMoveReIndex(from, to); got != test.expect { + if got := from.IsModuleReIndex(to); got != test.expect { t.Errorf("expected %t, got %t", test.expect, got) } }, diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 7f5fbae23..9d5b25520 100644 --- a/internal/refactoring/move_execute.go +++ b/internal/refactoring/move_execute.go @@ -238,11 +238,31 @@ func statementDependsOn(a, b *MoveStatement) bool { // // Since we are only interested in checking if A depends on B, we only need // to check the 4 possibilities above which result in B being executed - // first. - return a.From.NestedWithin(b.To) || - a.To.NestedWithin(b.To) || - b.From.NestedWithin(a.From) || - b.To.NestedWithin(a.From) + // first. If we're there's no dependency at all we can return immediately. + if !(a.From.NestedWithin(b.To) || a.To.NestedWithin(b.To) || + b.From.NestedWithin(a.From) || b.To.NestedWithin(a.From)) { + return false + } + + // If a nested move has a dependency, we need to rule out the possibility + // that this is a move inside a module only changing indexes. If an + // ancestor module is only changing the index of a nested module, any + // nested move statements are going to match both the From and To address + // when the base name is not changing, causing a cycle in the order of + // operations. + + // if A is not declared in an ancestor module, then we can't be nested + // within a module index change. + if len(a.To.Module()) >= len(b.To.Module()) { + return true + } + // We only want the nested move statement to depend on the outer module + // move, so we only test this in the reverse direction. + if a.From.IsModuleReIndex(a.To) { + return false + } + + return true } // MoveResults describes the outcome of an ApplyMoves call. diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 53bbbe6c2..60122511f 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -404,6 +404,58 @@ Each resource can have moved from only one source resource.`, }, WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`, }, + "crossing nested statements": { + // overlapping nested moves will result in a cycle. + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `module.nonexist.test.single`, + `module.count[0].test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.nonexist`, + `module.count[0]`, + ), + }, + WantError: `Cyclic dependency in move statements: The following chained move statements form a cycle, and so there is no final location to move objects to: + - test:1,1: module.nonexist → module.count[0] + - test:1,1: module.nonexist.test.single → module.count[0].test.count[0] + +A chain of move statements must end with an address that doesn't appear in any other statements, and which typically also refers to an object still declared in the configuration.`, + }, + "fully contained nested statements": { + // we have to avoid a cycle because the nested moves appear in both + // the from and to address of the parent when only the module index + // is changing. + Statements: []MoveStatement{ + makeTestMoveStmt(t, `count`, + `test.count`, + `test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.count`, + `module.count[0]`, + ), + }, + }, + "double fully contained nested statements": { + // we have to avoid a cycle because the nested moves appear in both + // the from and to address of the parent when only the module index + // is changing. + Statements: []MoveStatement{ + makeTestMoveStmt(t, `count`, + `module.count`, + `module.count[0]`, + ), + makeTestMoveStmt(t, `count.count`, + `test.count`, + `test.count[0]`, + ), + makeTestMoveStmt(t, ``, + `module.count`, + `module.count[0]`, + ), + }, + }, } for name, test := range tests {