command/show: fixing bugs in modulecalls (#20513)

* command/show: fixing bugs in modulecalls

jsonconfig and jsonplan both had subtle bugs with the logic for
marshaling module calls that only showed up when multiple modules were
referenced. This PR fixes those bugs and extends the existing tests to
include multiple modules.

* sort all the things, mostly for tests

command/jsonconfig/config.go

@@ -200,6 +200,7 @@ func marshalModule(c *configs.Config, schemas *terraform.Schemas, addr string) (
 		outputs[v.Name] = o
 	}
 	module.Outputs = outputs
+
 	module.ModuleCalls = marshalModuleCalls(c, schemas)
 
 	if len(c.Module.Variables) > 0 {
@@ -227,42 +228,43 @@ func marshalModule(c *configs.Config, schemas *terraform.Schemas, addr string) (
 
 func marshalModuleCalls(c *configs.Config, schemas *terraform.Schemas) map[string]moduleCall {
 	ret := make(map[string]moduleCall)
-	for _, mc := range c.Module.ModuleCalls {
-		retMC := moduleCall{
-			Source:            mc.SourceAddr,
-			VersionConstraint: mc.Version.Required.String(),
-		}
-		cExp := marshalExpression(mc.Count)
-		if !cExp.Empty() {
-			retMC.CountExpression = &cExp
-		} else {
-			fExp := marshalExpression(mc.ForEach)
-			if !fExp.Empty() {
-				retMC.ForEachExpression = &fExp
-			}
-		}
 
-		// get the called module's variables so we can build up the expressions
+	for name, mc := range c.Module.ModuleCalls {
-		childModule := c.Children[mc.Name]
+		mcConfig := c.Children[name]
-		schema := &configschema.Block{}
+		ret[name] = marshalModuleCall(mcConfig, mc, schemas)
-		schema.Attributes = make(map[string]*configschema.Attribute)
-		for _, variable := range childModule.Module.Variables {
-			schema.Attributes[variable.Name] = &configschema.Attribute{
-				Required: variable.Default == cty.NilVal,
-			}
-		}
-
-		retMC.Expressions = marshalExpressions(mc.Config, schema)
-
-		for name, cc := range c.Children {
-			childModule, _ := marshalModule(cc, schemas, name)
-			retMC.Module = childModule
-		}
-		ret[mc.Name] = retMC
 	}
 
 	return ret
+}
 
+func marshalModuleCall(c *configs.Config, mc *configs.ModuleCall, schemas *terraform.Schemas) moduleCall {
+	ret := moduleCall{
+		Source:            mc.SourceAddr,
+		VersionConstraint: mc.Version.Required.String(),
+	}
+	cExp := marshalExpression(mc.Count)
+	if !cExp.Empty() {
+		ret.CountExpression = &cExp
+	} else {
+		fExp := marshalExpression(mc.ForEach)
+		if !fExp.Empty() {
+			ret.ForEachExpression = &fExp
+		}
+	}
+
+	schema := &configschema.Block{}
+	schema.Attributes = make(map[string]*configschema.Attribute)
+	for _, variable := range c.Module.Variables {
+		schema.Attributes[variable.Name] = &configschema.Attribute{
+			Required: variable.Default == cty.NilVal,
+		}
+	}
+
+	ret.Expressions = marshalExpressions(mc.Config, schema)
+	module, _ := marshalModule(c, schemas, mc.Name)
+	ret.Module = module
+
+	return ret
 }
 
 func marshalResources(resources map[string]*configs.Resource, schemas *terraform.Schemas, moduleAddr string) ([]resource, error) {

command/jsonplan/values.go

@@ -99,9 +99,12 @@ func marshalPlannedValues(changes *plans.Changes, schemas *terraform.Schemas) (m
 			// root has no parents.
 			if containingModule != "" {
 				parent := resource.Addr.Module.Parent().String()
-				if !seenModules[parent] {
+				// we likely will see multiple resources in one module, so we
+				// only need to report the "parent" module for each child module
+				// once.
+				if !seenModules[containingModule] {
 					moduleMap[parent] = append(moduleMap[parent], resource.Addr.Module)
-					seenModules[parent] = true
+					seenModules[containingModule] = true
 				}
 			}
 		}
@@ -118,6 +121,10 @@ func marshalPlannedValues(changes *plans.Changes, schemas *terraform.Schemas) (m
 	if err != nil {
 		return ret, err
 	}
+	sort.Slice(childModules, func(i, j int) bool {
+		return childModules[i].Address < childModules[j].Address
+	})
+
 	ret.ChildModules = childModules
 
 	return ret, nil

command/test-fixtures/show-json/modules/bar/main.tf

@@ -0,0 +1,11 @@
+variable "test_var" {
+  default = "bar-var"
+}
+
+output "test" {
+  value = var.test_var
+}
+
+resource "test_instance" "test" {
+  ami = var.test_var
+}

command/test-fixtures/show-json/modules/foo/main.tf

@@ -1,5 +1,5 @@
 variable "test_var" {
-  default = "bar"
+  default = "foo-var"
 }
 resource "test_instance" "test" {
   ami   = var.test_var

command/test-fixtures/show-json/modules/main.tf

@@ -1,9 +1,13 @@
-module "module_test" {
+module "module_test_foo" {
   source   = "./foo"
   test_var = "baz"
 }
 
-output "test" {
+module "module_test_bar" {
-  value      = module.module_test.test
+  source = "./bar"
-  depends_on = [module.module_test]
+}
+
+output "test" {
+  value      = module.module_test_foo.test
+  depends_on = [module.module_test_foo]
 }

command/test-fixtures/show-json/modules/output.json

@@ -12,7 +12,23 @@
                 {
                     "resources": [
                         {
-                            "address": "module.module_test.test_instance.test[0]",
+                            "address": "module.module_test_bar.test_instance.test",
+                            "mode": "managed",
+                            "type": "test_instance",
+                            "name": "test",
+                            "provider_name": "test",
+                            "schema_version": 0,
+                            "values": {
+                                "ami": "bar-var"
+                            }
+                        }
+                    ],
+                    "address": "module.module_test_bar"
+                },
+                {
+                    "resources": [
+                        {
+                            "address": "module.module_test_foo.test_instance.test[0]",
                             "mode": "managed",
                             "type": "test_instance",
                             "name": "test",
@@ -24,7 +40,7 @@
                             }
                         },
                         {
-                            "address": "module.module_test.test_instance.test[1]",
+                            "address": "module.module_test_foo.test_instance.test[1]",
                             "mode": "managed",
                             "type": "test_instance",
                             "name": "test",
@@ -36,7 +52,7 @@
                             }
                         },
                         {
-                            "address": "module.module_test.test_instance.test[2]",
+                            "address": "module.module_test_foo.test_instance.test[2]",
                             "mode": "managed",
                             "type": "test_instance",
                             "name": "test",
@@ -48,15 +64,35 @@
                             }
                         }
                     ],
-                    "address": "module.module_test"
+                    "address": "module.module_test_foo"
                 }
             ]
         }
     },
     "resource_changes": [
         {
-            "address": "module.module_test.test_instance.test[0]",
+            "address": "module.module_test_bar.test_instance.test",
-            "module_address": "module.module_test",
+            "module_address": "module.module_test_bar",
+            "mode": "managed",
+            "type": "test_instance",
+            "name": "test",
+            "change": {
+                "actions": [
+                    "create"
+                ],
+                "before": null,
+                "after": {
+                    "ami": "bar-var"
+                },
+                "after_unknown": {
+                    "ami": false,
+                    "id": true
+                }
+            }
+        },
+        {
+            "address": "module.module_test_foo.test_instance.test[0]",
+            "module_address": "module.module_test_foo",
             "mode": "managed",
             "type": "test_instance",
             "name": "test",
@@ -76,8 +112,8 @@
             }
         },
         {
-            "address": "module.module_test.test_instance.test[1]",
+            "address": "module.module_test_foo.test_instance.test[1]",
-            "module_address": "module.module_test",
+            "module_address": "module.module_test_foo",
             "mode": "managed",
             "type": "test_instance",
             "name": "test",
@@ -97,8 +133,8 @@
             }
         },
         {
-            "address": "module.module_test.test_instance.test[2]",
+            "address": "module.module_test_foo.test_instance.test[2]",
-            "module_address": "module.module_test",
+            "module_address": "module.module_test_foo",
             "mode": "managed",
             "type": "test_instance",
             "name": "test",
@@ -134,16 +170,52 @@
                 "test": {
                     "expression": {
                         "references": [
-                            "module.module_test.test"
+                            "module.module_test_foo.test"
                         ]
                     },
                     "depends_on": [
-                        "module.module_test"
+                        "module.module_test_foo"
                     ]
                 }
             },
             "module_calls": {
-                "module_test": {
+                "module_test_bar": {
+                    "source": "./bar",
+                    "module": {
+                        "outputs": {
+                            "test": {
+                                "expression": {
+                                    "references": [
+                                        "var.test_var"
+                                    ]
+                                }
+                            }
+                        },
+                        "resources": [
+                            {
+                                "address": "test_instance.test",
+                                "mode": "managed",
+                                "type": "test_instance",
+                                "name": "test",
+                                "provider_config_key": "module_test_bar:test",
+                                "expressions": {
+                                    "ami": {
+                                        "references": [
+                                            "var.test_var"
+                                        ]
+                                    }
+                                },
+                                "schema_version": 0
+                            }
+                        ],
+                        "variables": {
+                            "test_var": {
+                                "default": "bar-var"
+                            }
+                        }
+                    }
+                },
+                "module_test_foo": {
                     "source": "./foo",
                     "expressions": {
                         "test_var": {
@@ -166,7 +238,7 @@
                                 "mode": "managed",
                                 "type": "test_instance",
                                 "name": "test",
-                                "provider_config_key": "module_test:test",
+                                "provider_config_key": "module_test_foo:test",
                                 "expressions": {
                                     "ami": {
                                         "references": [
@@ -182,7 +254,7 @@
                         ],
                         "variables": {
                             "test_var": {
-                                "default": "bar"
+                                "default": "foo-var"
                             }
                         }
                     }
@@ -190,8 +262,8 @@
             }
         },
         "provider_config": {
-            "module_test:test": {
+            "module_test_foo:test": {
-                "module_address": "module_test",
+                "module_address": "module_test_foo",
                 "name": "test"
             }
         }