diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 7ff17621b0d7..f6ec601ab69c 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -705,9 +705,9 @@ 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. +// IsModuleMoveReIndex 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 IsModuleMoveReIndex(from, to *MoveEndpointInModule) bool { // The statements must originate from the same module. if !from.module.Equal(to.module) { @@ -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 1e2758239fa3..a7368e9eff3e 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]`), diff --git a/internal/refactoring/move_execute.go b/internal/refactoring/move_execute.go index 7f5fbae23b36..d49b0d9c5ba1 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 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 + } + + // 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. + // We only want the nested move statement to depend on the outer module + // move, so we only test this in one direction. + if addrs.IsModuleMoveReIndex(a.To, a.From) { + 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 53bbbe6c2e1a..60122511fe51 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 {