Skip to content

Set, Map, and Object TF framework types throw errors when used #700

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

Closed
mschuchard opened this issue Mar 24, 2023 · 8 comments
Closed

Set, Map, and Object TF framework types throw errors when used #700

mschuchard opened this issue Mar 24, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@mschuchard
Copy link

mschuchard commented Mar 24, 2023

Module version

1.2.0

Relevant provider source code

ElementType: types.MapType,
ElementType: types.SetType,
ElementType: types.ObjectType,

Expected Behavior

According to the documentation, these are the associated framework types, so they should function as expected.

Actual Behavior

types.MapType (type) is not an expression

Additional Information

I cynically worked around the errors by importing the basetypes package, and then:
ElementType: basetypes.MapType{},

However, I am unsure if this is a true fix for the issue, and would not feel confident proposing a PR with that as the fix for this issue. This is especially because this workaround then causes a segfault analogous to #699.

@mschuchard mschuchard added the bug Something isn't working label Mar 24, 2023
@bflad
Copy link
Contributor

bflad commented Mar 24, 2023

Hi @mschuchard 👋 Thank you for raising this and sorry you ran into trouble here.

Terraform and the framework type system both delineate between "primitive" and "collection" types. Primitive types do not require anything additional to be functional, while collection types must denote the underlying attribute or element types.

More specifically with the framework type system implementation, originally everything was implemented within a single types package, however it had extensibility issues with the primitive types since those were not implemented as their own Go structure types. This was mostly fixed with the implementation of the types/basetypes package, which introduced primitive types as their own types, however to maintain backwards compatibility with existing provider code the types package implementation kept support for the primitive types without the {}. Using types/basetypes always requires the {} (and for collection types, the AttributeTypes or ElementType field) since those are all Go structure types.

You can see the current quirks in the types package Go documentation where primitive types are variables and collection types are type aliases:

For most providers developers, working with the types package may be slightly more readable, where the types/basetypes package is generally only needed for creating custom types. Ideally, there wouldn't be two separate packages for the framework type system, but that particular detail was not rectified before the v1.0.0 release of the framework due to time constraints. The types and types/basetypes packages are now protected by major version compatibility promises, so we cannot introduce any breaking changes in the name of consistency until the next major version. I think our options to prevent this situation are quite limited. 😖

Ideally, it should not be possible for provider developers to incorrectly create types in the first place. This could be achieved, for example, by breaking the types API in the next major version by replacing the variable and type aliases with functions (e.g. func BoolType() basetypes.BoolType, func MapType(elementType attr.Type) basetypes.MapType). Another less likely option could be removing the second types implementation altogether and requiring everyone to implement basetypes types with {} (and any "required" fields) correctly. A third option would be to change those variables to type aliases, although that feels like it would introduce the most "burden" without any real benefit other than {} consistency.

As for what to do now given the current situation, I think there might be some hesitation with pointing provider developers directly at the types/basetypes implementations since it could be more confusing to change our recommendations and code examples between minor versions without knowing how we would eventually prefer the framework type system to be implemented. We would not want developers to feel compelled to update their code now to later need to update it differently.

Over in #695 we are exploring the option of expanding all the attribute/block/type documentation into separate, more manageable pages. With those extra pages, we could further get into more code examples and potentially provide a "Troubleshooting" section with the Go compiler error you mention. Do you think that might be reasonable from a documentation standpoint?

@mschuchard
Copy link
Author

mschuchard commented Mar 24, 2023

Thanks for the information. It is a bit unclear to me if this is pertaining to this issue, #699, or both in combination.

while collection types must denote the underlying attribute or element types

Great! Where in the documentation can I find information about that? I was taking shots in the dark around how to specify the collection type fully in the schema, and could not arrive at any positive results. I also did try to workaround this with a dummy NestedAttribute, but that always resulted in the nested attribute being null seemingly because it was computed. This meant I could not req.Config.Get with any useful results. Should I be using e,g, ElementType: basetypes.MapType{ElemType basetypes.StringType{}}? All I need is the capability to specify something like a TF type list(map) or set(object) as an attribute and not block, but that may currently be impossible.

I do also feel like I understand your concern over encouraging the use of basetypes because of the potential consequences there moving forward, but from my perspective I would just like a path forward through any reasonable means.

Over in #695 we are exploring the option of expanding all the attribute/block/type documentation into separate, more manageable pages.

I am familiar with that one as I commented already about my struggles with the typing and my appreciation for documentation expansion. I am unsure about others' experiences thus far, but I am not finding examples to cross-reference in the major providers where code is attempting to achieve the same kind of results that I am working towards. I code in many languages for multiple bindings/integrations/APIs and the types package gives me "Rust vibes" so I feel like there is some familiarity there, but alternatively I may be mistaken about everything.

@bflad
Copy link
Contributor

bflad commented Mar 24, 2023

Thank you for the additional information. It's the end of my day, but I do want to provide you a quick answer here:

All I need is the capability to specify something like a TF type list(map) or set(object) as an attribute and not block

In your schema, I would anticipate something like the below for list(map(??)):

"list_of_map_attribute": schema.ListAttribute{
  ElementType: types.Map{
    ElemType: /* ... types.String, etc. */, // ElemType inside a Map is required (old naming convention)
  },
  // ... other attribute configuration ...
},

For the set(object) you have two options. You can literally use a set of objects, or I'm guessing preferably, use a set nested attribute. The latter allows you to fully define behaviors on the underlying attributes, rather than just specify type information.

"set_nested_attribute": schema.SetNestedAttribute{
  NestedObject: schema.NestedAttributeObject{
    // ... nested object configuration ...
  },
  // ... other attribute configuration ...
},

In your data models:

type ExampleResourceModel struct {
  ListOfMapAttribute types.List `tfsdk:"list_of_map_attribute"`
  SetNestedAttribute types.Set `tfsdk:"set_nested_attribute"`
}

type SetNestedAttributeModel struct {
  // ... underlying attributes ...
}

And converting out (assuming a list of map of strings):

var config ExampleResourceModel

resp.Diagnostics.Append(req.Config.Get(ctx, &config)...)

if resp.Diagnostics.HasError() {
  return
}

// generally good to check config.ListOfMapAttribute.IsNull() or IsUnknown() before doing the below
// similarly with config.SetNestedAttribute.IsNull()/IsUnknown()

var listOfMaps []types.Map

resp.Diagnostics.Append(config.ElementsAs(ctx, &listOfMaps))

if resp.Diagnostics.HasError() {
  return
}

for _, listMap := range listOfMaps {
  var mapOfStrings map[string]types.String

  resp.Diagnostics.Append(listMap.ElementsAs(ctx, &mapOfStrings)...)

  if resp.Diagnostics.HasError() {
    return
  }

  // hopefully you're good from here :)
}

var []setNestedAttribute SetNestedAttributeModel

resp.Diagnostics.Append(config.SetNestedAttribute.ElementsAs(ctx, &setNestedAttribute)...)

if resp.Diagnostics.HasError() {
  return
}

// hopefully you're good from here :)

Complex attributes are tricky to talk about and write out code-wise, since there are multiple potential ways you can write the Go code or use the type system, but hopefully this extra context/example code is helpful. I apologize if any of the code is buggy above, I was trying to hastily write this before heading out. If you have further questions about this over the weekend, you may have good luck raising a topic on HashiCorp Discuss. I'll try to provide a better response to the rest next week.

@mschuchard
Copy link
Author

mschuchard commented Mar 27, 2023

Ok yes wow: that first block of code is exactly what I needed. I had no idea from the types documentation and source code that I could do that; I had assumed I needed to use basetypes to interface like that.

For the second and third blocks of code: I actually do not want nested attributes, but now I am curious because I would have thought the SetNestedAttribute types.Set would be a []SetNestedAttributeModel instead of a types.Set?

// generally good to check config.ListOfMapAttribute.IsNull() or IsUnknown() before doing the below

Good point.

resp.Diagnostics.Append(listMap.ElementsAs(ctx, &mapOfStrings)...): This is probably more pedantic than anything else, but I have been wondering about this kind of code. This is probably a drawback to my knowledge of multiple languages because I become confused about the differences in behavior between them, but is that reference in the ElementsAs argument ensuring it stores the specific element of the map in the lambda from the listOfMaps enumerable even when it is the parameter/argument value to the Diagnostics.Append? In other words: later in that lambda scope, would I be dereferencing a nil pointer if attempted to access mapOfStrings? I would not think so, but I have been cautious and wary of trying that. I also assume it is overwritten on each iteration and not merged, but that is more curiosity than anything else.

Anyway: thanks so much for all of this, and expanded documentation for the types would definitely be great. You clearly know the package well, so I would hope you are documenting it. Also this is a super digression, but would the types package fix the issue with the Packer SDK being unable to upgrade to the new version of go-cty? Mart would probably know best since he gave me the info, but I am hopeful.

Also on an interesting note: someone on StackOverflow earlier today asked almost literally the same question as this issue's topic, but that person seemed to be using SDKv2. I informed the person I felt fairly certain an upgrade to the plugin framework was necessary because schema for list(map) in SDKv2 seems to result in blocks.

I will take another shot at this in a couple days, but I would expect this to move me forward. I also think this issue's root cause is a subset of the expanded documentation discussed in #695 because I was misusing the types package and not realizing it, and so I could close this in lieu of that issue.

@mschuchard
Copy link
Author

My apologies: I thought your reply above was functional for the current 1.2.0 version of this Go module. However, I understand now that your first example of:

"list_of_map_attribute": schema.ListAttribute{
  ElementType: types.Map{
    ElemType: /* ... types.String, etc. */, // ElemType inside a Map is required (old naming convention)
  },
  // ... other attribute configuration ...
},

is a proposal for a future version since it throws the exact same multiple errors as my attempt. I will eagerly await the implementation of that functionality in the types package.

@bflad
Copy link
Contributor

bflad commented Mar 29, 2023

@mschuchard can you please show all of your code and the errors you are seeing? I do see that I typo'd a few things in my quick writing last week (e.g. types.Map instead of types.MapType, and missing a third argument for ElementsAs). My apologies. I created and verified example code here: https://github.com/bflad/terraform-provider-framework/pull/1/files

resp.Diagnostics.Append(listMap.ElementsAs(ctx, &mapOfStrings)...): This is probably more pedantic than anything else, but I have been wondering about this kind of code.

I would recommend reading about variadic functions in the Go programming language here: https://www.digitalocean.com/community/tutorials/how-to-use-variadic-functions-in-go

And peeking at the Go package documentation for each call:

One way to break it down is:

diags := listMap.ElementsAs(ctx, &mapOfStrings)
// diags is a diag.Diagnostics (which itself is a type alias for []diag.Diagnostics)
resp.Diagnostics.Append(diags...)

Its a coding style to remove the intermediate variable (diags above) if its not going to be used elsewhere or interfere with further code. You are welcome to split those apart as you wish.

Also this is a super digression, but would the types package fix the issue with the Packer SDK being unable to upgrade to the new version of go-cty?

The terraform-plugin-framework Go module and its underlying types package is intended for Terraform Provider code. If there are other types of Go modules consuming this code, I think we would need to understand more context. The underlying terraform-plugin-go and terraform-plugin-framework type systems, while similar in some respects to cty, have no dependency on that type system or its Go module nor should affect upgrades of that Go module.

@mschuchard
Copy link
Author

mschuchard commented Mar 30, 2023

e.g. types.Map instead of types.MapType

That was it right there. I have egg on my face for just blindly following that snippet of the schema and not even attempting to modify it with something I should have known. After updating to:

ElementType: types.MapType{
  ElemType: types.StringType,
},

I was "off to the races", and only needed one more update to pass acceptance testing. Speaking of which...

https://github.com/bflad/terraform-provider-framework/pull/1/files

So I really appreciate this level of example, but sadly all I actually needed was var listOfTerraformMap []types.Map because I for some reason did not think I could declare a slice of types.Map. However, I am going to keep that somewhere locally to reference later for ideas on e.g. potential error situations. There are subtleties in that code I had not thought of, and I can always appreciate gleaning from someone else's experience. In my opinion it would also be a great add to examples publicly accessible somewhere.

https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/types/basetypes#ListValue.ElementsAs

That totally makes sense, and I appreciate the link to that because I could not find it. I think really the problem for me is the pointers/reference implementation aspect because it is my bane in C++/Go/Rust, and I think I only somewhat understand it even now thanks to Rust's memory safety aspects.

The terraform-plugin-framework Go module and its underlying types package is intended for Terraform Provider code

I think I was being kind of hopeful and facetious. In the Packer SDK currently we have something similar to the Model struct like (pasting from my Packer plugin):

type Config struct {
  InstallCmd []string `mapstructure:"install_cmd"`
  Keyword    string   `mapstructure:"keyword"`
  Local      bool     `mapstructure:"local"`

  ctx interpolate.Context
}

and the type conversion fails when generating HCL2 compatibility for reasons explained to me by Martin. I was wondering if swapping go-cty with the types package (note: this is due to an improvement in go-cty, and not an issue) would assist, but I knew it was such a long shot.

Anyway, thanks again for this truly.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants