-
Notifications
You must be signed in to change notification settings - Fork 191
chore: Define conversion function for generating API request body from Resource Model in create (non-nested) #3239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
4c78c75
f0527b2
39dde8b
ac09871
3f17644
efbde62
5533fe9
f773342
b590a98
5dcab18
2a5577f
d6c212b
065089b
bf5e83b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,67 +4,133 @@ import ( | |
"encoding/json" | ||
"fmt" | ||
"reflect" | ||
"strings" | ||
|
||
"github.com/hashicorp/terraform-plugin-framework/attr" | ||
"github.com/hashicorp/terraform-plugin-framework/types" | ||
"github.com/huandu/xstrings" | ||
) | ||
|
||
const ( | ||
tagKey = "autogeneration" | ||
tagValOmitJSON = "omitjson" | ||
tagValCreateOnly = "createonly" | ||
) | ||
|
||
// Marshal gets a Terraform model and marshals it into JSON (e.g. for an Atlas request). | ||
func Marshal(model any, isCreate bool) ([]byte, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would leave an additional comment clarifying when attributes are sent in payload, and how use of tags changes that behaviour There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done here: 065089b |
||
valModel := reflect.ValueOf(model) | ||
if valModel.Kind() != reflect.Ptr { | ||
panic("model must be pointer") | ||
} | ||
valModel = valModel.Elem() | ||
if valModel.Kind() != reflect.Struct { | ||
panic("model must be pointer to struct") | ||
} | ||
objJSON, err := marshalAttrs(valModel, isCreate) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return json.Marshal(objJSON) | ||
} | ||
|
||
// Unmarshal gets a JSON (e.g. from an Atlas response) and unmarshals it into a Terraform model. | ||
// It supports the following Terraform model types: String, Bool, Int64, Float64. | ||
func Unmarshal(raw []byte, dest any) error { | ||
var src map[string]any | ||
if err := json.Unmarshal(raw, &src); err != nil { | ||
func Unmarshal(raw []byte, model any) error { | ||
var objJSON map[string]any | ||
if err := json.Unmarshal(raw, &objJSON); err != nil { | ||
return err | ||
} | ||
return mapFields(src, dest) | ||
return unmarshalAttrs(objJSON, model) | ||
} | ||
|
||
func marshalAttrs(valModel reflect.Value, isCreate bool) (map[string]any, error) { | ||
objJSON := make(map[string]any) | ||
for i := 0; i < valModel.NumField(); i++ { | ||
attrTypeModel := valModel.Type().Field(i) | ||
tag := attrTypeModel.Tag.Get(tagKey) | ||
if strings.Contains(tag, tagValOmitJSON) { | ||
continue // skip fields with tag `omitjson` | ||
} | ||
if !isCreate && strings.Contains(tag, tagValCreateOnly) { | ||
continue // skip fields with tag `createonly` if not in create | ||
} | ||
attrNameModel := attrTypeModel.Name | ||
attrValModel := valModel.Field(i) | ||
if err := marshalAttr(attrNameModel, attrValModel, objJSON); err != nil { | ||
return nil, err | ||
} | ||
} | ||
return objJSON, nil | ||
} | ||
|
||
func marshalAttr(attrNameModel string, attrValModel reflect.Value, objJSON map[string]any) error { | ||
attrNameJSON := xstrings.ToSnakeCase(attrNameModel) | ||
obj, ok := attrValModel.Interface().(attr.Value) | ||
if !ok { | ||
panic("marshal expects only Terraform types in the model") | ||
} | ||
if obj.IsNull() || obj.IsUnknown() { | ||
return nil // skip nil or unknown values | ||
} | ||
switch v := attrValModel.Interface().(type) { | ||
case types.String: | ||
objJSON[attrNameJSON] = v.ValueString() | ||
case types.Int64: | ||
objJSON[attrNameJSON] = v.ValueInt64() | ||
case types.Float64: | ||
objJSON[attrNameJSON] = v.ValueFloat64() | ||
default: | ||
return fmt.Errorf("marshal not supported yet for type %T for field %s", v, attrNameJSON) | ||
} | ||
return nil | ||
} | ||
|
||
func mapFields(src map[string]any, dest any) error { | ||
valDest := reflect.ValueOf(dest) | ||
if valDest.Kind() != reflect.Ptr { | ||
panic("dest must be pointer") | ||
func unmarshalAttrs(objJSON map[string]any, model any) error { | ||
valModel := reflect.ValueOf(model) | ||
if valModel.Kind() != reflect.Ptr { | ||
panic("model must be pointer") | ||
} | ||
valDest = valDest.Elem() | ||
if valDest.Kind() != reflect.Struct { | ||
panic("dest must be pointer to struct") | ||
valModel = valModel.Elem() | ||
if valModel.Kind() != reflect.Struct { | ||
panic("model must be pointer to struct") | ||
} | ||
for nameAttrSrc, valueAttrSrc := range src { | ||
if err := mapField(nameAttrSrc, valueAttrSrc, valDest); err != nil { | ||
for attrNameJSON, attrObjJSON := range objJSON { | ||
if err := unmarshalAttr(attrNameJSON, attrObjJSON, valModel); err != nil { | ||
return err | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
func mapField(nameAttrSrc string, valueAttrSrc any, valDest reflect.Value) error { | ||
nameDest := xstrings.ToPascalCase(nameAttrSrc) | ||
fieldDest := valDest.FieldByName(nameDest) | ||
if !fieldDest.CanSet() { | ||
func unmarshalAttr(attrNameJSON string, attrObjJSON any, valModel reflect.Value) error { | ||
attrNameModel := xstrings.ToPascalCase(attrNameJSON) | ||
fieldModel := valModel.FieldByName(attrNameModel) | ||
if !fieldModel.CanSet() { | ||
return nil // skip fields that cannot be set, are invalid or not found | ||
} | ||
switch v := valueAttrSrc.(type) { | ||
switch v := attrObjJSON.(type) { | ||
case string: | ||
return assignField(nameDest, fieldDest, types.StringValue(v)) | ||
return setAttrModel(attrNameModel, fieldModel, types.StringValue(v)) | ||
case bool: | ||
return assignField(nameDest, fieldDest, types.BoolValue(v)) | ||
return setAttrModel(attrNameModel, fieldModel, types.BoolValue(v)) | ||
case float64: // number: try int or float | ||
if assignField(nameDest, fieldDest, types.Float64Value(v)) == nil { | ||
if setAttrModel(attrNameModel, fieldModel, types.Float64Value(v)) == nil { | ||
return nil | ||
} | ||
return assignField(nameDest, fieldDest, types.Int64Value(int64(v))) | ||
return setAttrModel(attrNameModel, fieldModel, types.Int64Value(int64(v))) | ||
case nil: | ||
return nil // skip nil values, no need to set anything | ||
default: | ||
return fmt.Errorf("not supported yet type %T for field %s", v, nameAttrSrc) | ||
return fmt.Errorf("unmarshal not supported yet for type %T for field %s", v, attrNameJSON) | ||
} | ||
} | ||
|
||
func assignField(nameDest string, fieldDest reflect.Value, valueDest attr.Value) error { | ||
valObj := reflect.ValueOf(valueDest) | ||
if !fieldDest.Type().AssignableTo(valObj.Type()) { | ||
return fmt.Errorf("can't assign value to model field %s", nameDest) | ||
func setAttrModel(name string, field reflect.Value, val attr.Value) error { | ||
obj := reflect.ValueOf(val) | ||
if !field.Type().AssignableTo(obj.Type()) { | ||
return fmt.Errorf("unmarshal can't assign value to model field %s", name) | ||
} | ||
fieldDest.Set(valObj) | ||
field.Set(obj) | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,50 +3,175 @@ package autogeneration_test | |
import ( | ||
"testing" | ||
|
||
"github.com/hashicorp/terraform-plugin-framework/attr" | ||
"github.com/hashicorp/terraform-plugin-framework/types" | ||
"github.com/mongodb/terraform-provider-mongodbatlas/internal/common/autogeneration" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestMarshalBasic(t *testing.T) { | ||
model := struct { | ||
AttrFloat types.Float64 `tfsdk:"attribute_float"` | ||
AttrString types.String `tfsdk:"attribute_string"` | ||
// values with tag `omitjson` are not marshaled, and they don't need to be Terraform types | ||
AttrOmit types.String `tfsdk:"attribute_omit" autogeneration:"omitjson"` | ||
AttrOmitNoTerraform string `autogeneration:"omitjson"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any particular reason why this is not types.String? Would also define it with a known string value to ensure it is never sent even if it has a value in the config There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean AttrOmitNoTerraform? It's to prove that in this case you can use "invalid" types that are not Terraform types in the model, and that's ok because it's not sent in JSON, like explained in the comment above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aah makes sense, I see |
||
AttrUnkown types.String `tfsdk:"attribute_unknown"` | ||
AttrNull types.String `tfsdk:"attribute_null"` | ||
AttrInt types.Int64 `tfsdk:"attribute_int"` | ||
}{ | ||
AttrFloat: types.Float64Value(1.234), | ||
AttrString: types.StringValue("hello"), | ||
AttrOmit: types.StringValue("omit"), | ||
AttrOmitNoTerraform: "omit", | ||
AttrUnkown: types.StringUnknown(), // unknown values are not marshaled | ||
AttrNull: types.StringNull(), // null values are not marshaled | ||
AttrInt: types.Int64Value(1), | ||
} | ||
const expectedJSON = `{ "attr_string": "hello", "attr_int": 1, "attr_float": 1.234 }` | ||
raw, err := autogeneration.Marshal(&model, false) | ||
require.NoError(t, err) | ||
assert.JSONEq(t, expectedJSON, string(raw)) | ||
} | ||
|
||
func TestMarshalCreateOnly(t *testing.T) { | ||
const ( | ||
expectedCreate = `{ "attr": "val1", "attr_create_only": "val2" }` | ||
expectedNoCreate = `{ "attr": "val1" }` | ||
) | ||
model := struct { | ||
Attr types.String `tfsdk:"attr"` | ||
AttrCreateOnly types.String `tfsdk:"attr_create_only" autogeneration:"createonly"` | ||
AttrOmit types.String `tfsdk:"attr_omit" autogeneration:"omitjson"` | ||
}{ | ||
Attr: types.StringValue("val1"), | ||
AttrCreateOnly: types.StringValue("val2"), | ||
AttrOmit: types.StringValue("omit"), | ||
} | ||
noCreate, errNoCreate := autogeneration.Marshal(&model, false) | ||
require.NoError(t, errNoCreate) | ||
assert.JSONEq(t, expectedNoCreate, string(noCreate)) | ||
|
||
create, errCreate := autogeneration.Marshal(&model, true) | ||
require.NoError(t, errCreate) | ||
assert.JSONEq(t, expectedCreate, string(create)) | ||
} | ||
|
||
func TestMarshalUnsupported(t *testing.T) { | ||
testCases := map[string]any{ | ||
"Object not supported yet, only no-nested types": &struct { | ||
Attr types.Object | ||
}{ | ||
Attr: types.ObjectValueMust(map[string]attr.Type{ | ||
"key": types.StringType, | ||
}, map[string]attr.Value{ | ||
"key": types.StringValue("value"), | ||
}), | ||
}, | ||
"List not supported yet, only no-nested types": &struct { | ||
Attr types.List | ||
}{ | ||
Attr: types.ListValueMust(types.StringType, []attr.Value{ | ||
types.StringValue("value"), | ||
}), | ||
}, | ||
"Map not supported yet, only no-nested types": &struct { | ||
Attr types.Map | ||
}{ | ||
Attr: types.MapValueMust(types.StringType, map[string]attr.Value{ | ||
"key": types.StringValue("value"), | ||
}), | ||
}, | ||
"Set not supported yet, only no-nested types": &struct { | ||
Attr types.Set | ||
}{ | ||
Attr: types.SetValueMust(types.StringType, []attr.Value{ | ||
types.StringValue("value"), | ||
}), | ||
}, | ||
"Int32 not supported yet as it's not being used in any model": &struct { | ||
Attr types.Int32 | ||
}{ | ||
Attr: types.Int32Value(1), | ||
}, | ||
"Float32 not supported yet as it's not being used in any model": &struct { | ||
Attr types.Float32 | ||
}{ | ||
Attr: types.Float32Value(1.0), | ||
}, | ||
} | ||
for name, model := range testCases { | ||
t.Run(name, func(t *testing.T) { | ||
raw, err := autogeneration.Marshal(model, false) | ||
require.Error(t, err) | ||
assert.Nil(t, raw) | ||
}) | ||
} | ||
} | ||
|
||
func TestMarshalPanic(t *testing.T) { | ||
str := "string" | ||
testCases := map[string]any{ | ||
"no Terraform types": &struct { | ||
Attr string | ||
}{ | ||
Attr: "a", | ||
}, | ||
"no pointer": struct { | ||
Attr types.String | ||
}{ | ||
Attr: types.StringValue("a"), | ||
}, | ||
"no struct": &str, | ||
} | ||
for name, model := range testCases { | ||
t.Run(name, func(t *testing.T) { | ||
assert.Panics(t, func() { | ||
_, _ = autogeneration.Marshal(model, false) | ||
}) | ||
}) | ||
} | ||
} | ||
|
||
func TestUnmarshalBasic(t *testing.T) { | ||
var model struct { | ||
AttributeFloat types.Float64 `tfsdk:"attribute_float"` | ||
AttributeFloatWithInt types.Float64 `tfsdk:"attribute_float_with_int"` | ||
AttributeString types.String `tfsdk:"attribute_string"` | ||
AttributeNotInJSON types.String `tfsdk:"attribute_not_in_json"` | ||
AttributeInt types.Int64 `tfsdk:"attribute_int"` | ||
AttributeIntWithFloat types.Int64 `tfsdk:"attribute_int_with_float"` | ||
AttributeTrue types.Bool `tfsdk:"attribute_true"` | ||
AttributeFalse types.Bool `tfsdk:"attribute_false"` | ||
AttrFloat types.Float64 `tfsdk:"attribute_float"` | ||
AttrFloatWithInt types.Float64 `tfsdk:"attribute_float_with_int"` | ||
AttrString types.String `tfsdk:"attribute_string"` | ||
AttrNotInJSON types.String `tfsdk:"attribute_not_in_json"` | ||
AttrInt types.Int64 `tfsdk:"attribute_int"` | ||
AttrIntWithFloat types.Int64 `tfsdk:"attribute_int_with_float"` | ||
AttrTrue types.Bool `tfsdk:"attribute_true"` | ||
AttrFalse types.Bool `tfsdk:"attribute_false"` | ||
} | ||
const ( | ||
epsilon = 10e-15 // float tolerance | ||
// attribute_not_in_model is ignored because it is not in the model, no error is thrown. | ||
// attribute_null is ignored because it is null, no error is thrown even if it is not in the model. | ||
tfResponseJSON = ` | ||
{ | ||
"attribute_string": "value_string", | ||
"attribute_true": true, | ||
"attribute_false": false, | ||
"attribute_int": 123, | ||
"attribute_int_with_float": 10.6, | ||
"attribute_float": 456.1, | ||
"attribute_float_with_int": 13, | ||
"attribute_not_in_model": "val", | ||
"attribute_null": null | ||
"attr_string": "value_string", | ||
"attr_true": true, | ||
"attr_false": false, | ||
"attr_int": 123, | ||
"attr_int_with_float": 10.6, | ||
"attr_float": 456.1, | ||
"attr_float_with_int": 13, | ||
"attr_not_in_model": "val", | ||
"attr_null": null | ||
} | ||
` | ||
) | ||
require.NoError(t, autogeneration.Unmarshal([]byte(tfResponseJSON), &model)) | ||
assert.Equal(t, "value_string", model.AttributeString.ValueString()) | ||
assert.True(t, model.AttributeTrue.ValueBool()) | ||
assert.False(t, model.AttributeFalse.ValueBool()) | ||
assert.Equal(t, int64(123), model.AttributeInt.ValueInt64()) | ||
assert.Equal(t, int64(10), model.AttributeIntWithFloat.ValueInt64()) // response floats stored in model ints have their decimals stripped. | ||
assert.InEpsilon(t, float64(456.1), model.AttributeFloat.ValueFloat64(), epsilon) | ||
assert.InEpsilon(t, float64(13), model.AttributeFloatWithInt.ValueFloat64(), epsilon) | ||
assert.True(t, model.AttributeNotInJSON.IsNull()) // attributes not in JSON response are not changed, so null is kept. | ||
assert.Equal(t, "value_string", model.AttrString.ValueString()) | ||
assert.True(t, model.AttrTrue.ValueBool()) | ||
assert.False(t, model.AttrFalse.ValueBool()) | ||
assert.Equal(t, int64(123), model.AttrInt.ValueInt64()) | ||
assert.Equal(t, int64(10), model.AttrIntWithFloat.ValueInt64()) // response floats stored in model ints have their decimals stripped. | ||
assert.InEpsilon(t, float64(456.1), model.AttrFloat.ValueFloat64(), epsilon) | ||
assert.InEpsilon(t, float64(13), model.AttrFloatWithInt.ValueFloat64(), epsilon) | ||
assert.True(t, model.AttrNotInJSON.IsNull()) // attributes not in JSON response are not changed, so null is kept. | ||
} | ||
|
||
func TestUnmarshalErrors(t *testing.T) { | ||
|
@@ -132,11 +257,11 @@ func TestUnmarshalUnsupportedResponse(t *testing.T) { | |
}{ | ||
"JSON objects not support yet": { | ||
responseJSON: `{"attr": {"key": "value"}}`, | ||
errorStr: "not supported yet type map[string]interface {} for field attr", | ||
errorStr: "unmarshal not supported yet for type map[string]interface {} for field attr", | ||
}, | ||
"JSON arrays not supported yet": { | ||
responseJSON: `{"attr": [{"key": "value"}]}`, | ||
errorStr: "not supported yet type []interface {} for field attr", | ||
errorStr: "unmarshal not supported yet for type []interface {} for field attr", | ||
}, | ||
} | ||
for name, tc := range testCases { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Giving some more thought to the naming, would
omitjsonupdate
overcreateonly
be more consistent?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, changed here: d6c212b