Skip to content

Commit

Permalink
Infer Map[string] when the Map's element type is unspecified (#2316)
Browse files Browse the repository at this point in the history 
SDKv2 providers allow specifying a map type without a backing element
type:

```go
schema.Schema{
	Type:     schema.TypeMap,
	Required: true,
}
```

Previously, the bridge interpreted this as `map[any]`. This does not
agree with TF's interpretation: `map[string]`. This PR changes the
bridge to correctly interpret `map[???]` as `map[string]`.

Fixes pulumi/pulumi-nomad#389
  • Loading branch information
@iwahbe
iwahbe authored Aug 15, 2024
1 parent 0f1e694 commit 662459e
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 30 deletions.
61 changes: 61 additions & 0 deletions pkg/tests/internal/tfcheck/tf_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,67 @@ resource "test_resource" "test" {
t.Logf(driver.GetState(t))
}

// TestTfMapMissingElem shows that maps missing Elem types are equivalent to specifying:
//
// Elem: &schema.Schema{Type: schema.TypeString}
//
// Previously, the bridge treated a missing map element type as `schema.TypeAny` instead
// of `schema.TypeString`, which caused provider panics. For example:
//
// - https://github.com/pulumi/pulumi-nomad/issues/389
func TestTfMapMissingElem(t *testing.T) {
prov := schema.Provider{
ResourcesMap: map[string]*schema.Resource{
"test_resource": {
Schema: map[string]*schema.Schema{
"m": {
Type: schema.TypeMap,
Required: true,
},
},
CreateContext: func(ctx context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics {
var errs diag.Diagnostics
if _, ok := d.Get("m").(map[string]any)["string"].(string); !ok {
errs = append(errs, diag.Errorf(`expected m["string"] to be a string`)...)
}
if _, ok := d.Get("m").(map[string]any)["number"].(string); !ok {
errs = append(errs, diag.Errorf(`expected m["number"] to be a string`)...)
}

d.SetId("test")
return errs
},
},
},
}

driver := NewTfDriver(t, t.TempDir(), "test", &prov)

driver.Write(t, `
resource "test_resource" "test" {
m = {
"string" = "123"
"number" = 123
}
}
`,
)

plan := driver.Plan(t)
t.Logf(driver.Show(t, plan.PlanFile))
driver.Apply(t, plan)

t.Logf(driver.GetState(t))

newPlan := driver.Plan(t)

t.Logf(driver.Show(t, plan.PlanFile))

driver.Apply(t, newPlan)

t.Logf(driver.GetState(t))
}

func TestTfUnknownObjects(t *testing.T) {
prov := schema.Provider{
ResourcesMap: map[string]*schema.Resource{
Expand Down
9 changes: 5 additions & 4 deletions pkg/tfgen/generate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ type moduleMember interface {
type typeKind int

const (
kindInvalid = iota
kindInvalid typeKind = iota
kindBool
kindInt
kindFloat
Expand All @@ -330,9 +330,6 @@ const (
kindObject
)

// Avoid an unused warning from varcheck.
var _ = kindInvalid

// propertyType represents a non-resource, non-datasource type. Property types may be simple
type propertyType struct {
name string
Expand Down Expand Up @@ -431,6 +428,10 @@ func (g *Generator) makePropertyType(typePath paths.TypePath,
switch sch.Type() {
case shim.TypeMap:
t.kind = kindMap
// TF treats maps without a specified type as map[string]string, so we do the same.
if element == nil || element.kind == kindInvalid {
element = &propertyType{kind: kindString}
}
case shim.TypeList:
t.kind = kindList
case shim.TypeSet:
Expand Down
28 changes: 14 additions & 14 deletions pkg/tfgen/generate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func Test_makePropertyType(t *testing.T) {
t.Run("String", func(t *testing.T) {
p, err := g.makePropertyType(path, "obj", strType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindString), p.kind)
assert.Equal(t, kindString, p.kind)
})

//TODO: change this test to assert typeKind when implementing
Expand All @@ -183,8 +183,8 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", strListType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindList), p.kind)
assert.Equal(t, typeKind(kindString), p.element.kind)
assert.Equal(t, kindList, p.kind)
assert.Equal(t, kindString, p.element.kind)
})

t.Run("MapString", func(t *testing.T) {
Expand All @@ -194,8 +194,8 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", strMapType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindMap), p.kind)
assert.Equal(t, typeKind(kindString), p.element.kind)
assert.Equal(t, kindMap, p.kind)
assert.Equal(t, kindString, p.element.kind)
})

t.Run("MapUnknown", func(t *testing.T) {
Expand All @@ -204,8 +204,8 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", unkMapType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindMap), p.kind)
assert.Nil(t, p.element)
assert.Equal(t, kindMap, p.kind)
assert.Equal(t, kindString, p.element.kind)
})

t.Run("SingleNestedBlock", func(t *testing.T) {
Expand All @@ -215,7 +215,7 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", objType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindObject), p.kind)
assert.Equal(t, kindObject, p.kind)
assert.Equal(t, "config.prop", p.properties[0].parentPath.String())
})

Expand All @@ -226,8 +226,8 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", objType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindList), p.kind)
assert.Equal(t, typeKind(kindObject), p.element.kind)
assert.Equal(t, kindList, p.kind)
assert.Equal(t, kindObject, p.element.kind)
assert.Equal(t, "config.prop.$", p.element.properties[0].parentPath.String())
})

Expand All @@ -239,7 +239,7 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", objType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindObject), p.kind)
assert.Equal(t, kindObject, p.kind)
assert.Equal(t, "config.prop", p.properties[0].parentPath.String())
})

Expand All @@ -250,8 +250,8 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", objType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindSet), p.kind)
assert.Equal(t, typeKind(kindObject), p.element.kind)
assert.Equal(t, kindSet, p.kind)
assert.Equal(t, kindObject, p.element.kind)
assert.Equal(t, "config.prop.$", p.element.properties[0].parentPath.String())
})

Expand All @@ -263,7 +263,7 @@ func Test_makePropertyType(t *testing.T) {
}).Shim()
p, err := g.makePropertyType(path, "obj", objType, nil, false, entityDocs{})
require.NoError(t, err)
assert.Equal(t, typeKind(kindObject), p.kind)
assert.Equal(t, kindObject, p.kind)
assert.Equal(t, "config.prop", p.properties[0].parentPath.String())
})
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/tfgen/test_data/minimuxed-schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n"
},
Expand Down Expand Up @@ -79,7 +79,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down Expand Up @@ -110,7 +110,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down
6 changes: 3 additions & 3 deletions pkg/tfgen/test_data/minirandom-schema-csharp.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n"
},
Expand Down Expand Up @@ -75,7 +75,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down Expand Up @@ -111,7 +111,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down
6 changes: 3 additions & 3 deletions pkg/tfgen/test_data/regress-minirandom-schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n"
},
Expand Down Expand Up @@ -65,7 +65,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down Expand Up @@ -96,7 +96,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down
6 changes: 3 additions & 3 deletions pkg/tfgen/test_data/test-propagate-language-options.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n"
},
Expand Down Expand Up @@ -82,7 +82,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down Expand Up @@ -113,7 +113,7 @@
"keepers": {
"type": "object",
"additionalProperties": {
"$ref": "pulumi.json#/Any"
"type": "string"
},
"description": "Arbitrary map of values that, when changed, will trigger recreation of resource. See the main provider documentation for\nmore information.\n",
"willReplaceOnChanges": true
Expand Down

0 comments on commit 662459e

Please sign in to comment.