Require provider-defined function parameter naming #964

bendbennett · 2024-03-20T09:50:14Z

Terraform core requires that all parameters that are used in the context of a provider-defined function are named. The plugin framework has previously either allowed provider developers to either explicitly supply names for provider-defined function parameters, or to generate default parameter names (e.g., param1, param2, varparam) when explicit names are absent.

This PR removes the generation of default parameter names by the plugin framework and therefore requires that explicit names are supplied for all provider-defined function parameters. This change was made in order to try and reduce any potential ambiguity for practitioners arising from the Terraform CLI outputting default parameter names. For example, prior to the changes in this PR, an error arising from, for example, a missing parameter value, would give rise to an error of the following form:

│ Error: Not enough function arguments
│ 
│   on resource.tf line 10, in resource "example_resource" "example":
│   10:   configurable_attribute = provider::example::string_len("some-values")
│     ├────────────────
│     │ while calling provider::example::string_len(param1, param2)
│ 
│ Function "provider::example::string_len" expects 2 argument(s). Missing value for "param2".

Breaking Change

Any provider-defined functions that do not have explicitly defined parameter names will now generate a runtime error of the following form:

│ Error: Failed to load plugin schemas
│ 
│ Error while loading schemas for plugin components: Failed to obtain provider schema: Could not load the schema for provider registry.terraform.io/bendbennett/playground: failed to
│ retrieve schema from provider "registry.terraform.io/bendbennett/playground": Invalid Function Definition: When validating the function definition, an implementation issue was
│ found. This is always an issue with the provider and should be reported to the provider developers.
│ 
│ Function "string_len" - Parameter at position 0 does not have a name..

Errors of this type can be fixed by explicitly naming all provider-defined function parameters. For example:

func (f ExampleFunction) Definition(ctx context.Context, req function.DefinitionRequest, resp *function.DefinitionResponse) {
	resp.Definition = function.Definition{
		Parameters: []function.Parameter{
			function.StringParameter{
				Name: "input", // Name explicitly supplied
			},
		},
		Return: function.StringReturn{},
	}
}

References:

… context in diagnostics

bflad

A few small things, otherwise looks good to me 👍

.changes/unreleased/BREAKING CHANGES-20240320-095152.yaml

bflad · 2024-03-20T13:04:10Z

function/definition.go

@@ -89,41 +89,46 @@ func (d Definition) Parameter(ctx context.Context, position int) (Parameter, dia
 // implementation of the definition to prevent unexpected errors or panics. This
 // logic runs during the GetProviderSchema RPC, or via provider-defined unit
 // testing, and should never include false positives.
-func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics {
+func (d Definition) ValidateImplementation(ctx context.Context, funcName string) diag.Diagnostics {


Since this is an exported function, should we switch this to request/response types while we are making a breaking change to the signature and to allow for future-proofing?

Good call. Have added request-response pattern for ValidateImplementation along with DefinitionValidateRequest and DefinitionValidateResponse.

bflad · 2024-03-20T13:07:26Z

function/parameter.go

-const (
-	// DefaultParameterNamePrefix is the prefix used to default the name of parameters which do not declare
-	// a name. Use this to prevent Terraform errors for missing names. This prefix is used with the parameter
-	// position in a function definition to create a unique name (param1, param2, etc.)
-	DefaultParameterNamePrefix = "param"
-
-	// DefaultVariadicParameterName is the default name given to a variadic parameter that does not declare
-	// a name. Use this to prevent Terraform errors for missing names.
-	DefaultVariadicParameterName = "varparam"
-)


For breaking change completeness, we'll need to also call out these constant removals.

Have added a changelog entry for this.

austinvalle

Looks awesome, great work Ben!

One note: we'll also want to clean up the documentation to reflect these changes, from a quick glance, some pages I found that should be updated:

https://developer.hashicorp.com/terraform/plugin/framework/functions/implementation#reading-argument-data (example code doesn't have names)
https://developer.hashicorp.com/terraform/plugin/framework/functions/documentation#parameters (mentions old defaulting pattern of param1, etc.)
- I wonder if we should give an example of an error message when you have a named parameter? Just to give the provider developer a visual of how it will be used (we mention it's used in docs/errors already)
https://developer.hashicorp.com/terraform/plugin/framework/functions/parameters/bool (Pretty much all of the type specific parameter docs have an example that doesn't have Name defined)

There may be more, but that's what I initially found 👍🏻

Co-authored-by: Brian Flad <bflad417@gmail.com>

… DefaultVariadicParameterName constants

… potential runtime errors

bendbennett · 2024-03-20T18:06:37Z

Looks awesome, great work Ben!

One note: we'll also want to clean up the documentation to reflect these changes, from a quick glance, some pages I found that should be updated:

https://developer.hashicorp.com/terraform/plugin/framework/functions/implementation#reading-argument-data (example code doesn't have names)

https://developer.hashicorp.com/terraform/plugin/framework/functions/documentation#parameters (mentions old defaulting pattern of param1, etc.)

I wonder if we should give an example of an error message when you have a named parameter? Just to give the provider developer a visual of how it will be used (we mention it's used in docs/errors already)

https://developer.hashicorp.com/terraform/plugin/framework/functions/parameters/bool (Pretty much all of the type specific parameter docs have an example that doesn't have Name defined)

There may be more, but that's what I initially found 👍🏻

I've done a search through the docs for all examples of parameter usage and added Name to each example.

I added an addition subsection for Parameter Naming to terraform/plugin/framework/functions/parameters#parameter-naming, which includes examples of runtime errors generated if unnamed parameters are present, and errors generated when there is an issue with a specific parameter.

austinvalle · 2024-03-20T18:42:17Z

Looks great ❤️ , I updated the dynamic docs PR to follow that pattern 👍🏻
c75a530

bflad

Looks good to me after the documentation updates, just some minor formatting nits 🚀

bflad · 2024-03-20T19:36:22Z

website/docs/plugin/framework/functions/documentation.mdx

-| `MarkdownDescription` | Documentation about the parameter and its expected values in Markdown format. |
+| Field Name            | Description                                                                                                                                           |
+|-----------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `Name`                | **Required**: Single word or abbreviation of parameter for function signature generation. If name is not provided, a runtime error will be generated. |


website/docs/plugin/framework/functions/implementation.mdx

website/docs/plugin/framework/functions/parameters/index.mdx

* initial implementation of dynamic type and value * fix doc string * add dynamic defaults * add plan modifier for dynamic * add dynamic attribute, validator, and plan modifiers * add provider and data source attributes and tests * update comment * add resource schema attribute test * add provider metaschema * add hook into attribute validation * add hook into plan modification for resources * add hook into dynamic default logic * add hooks for dynamic semantic equality * add function param and return definitions * add function tests * modify ToTerraformValue to always return DPT * switch back * make reflection update * update dep * use safe equal * return attr.Type * add todos and docs based on findings * random doc cleanup * doc fix * fix for data path * switch to tuples and add reflection support * update docs * add names to variadic param names * update doc note * update docs * update docs * add doc note * add changelog * delete metaschema and fix broken tests * updated some docs * switch changelog to breaking change * update to tuple index * update comment wording * update doc * add new maintainer note * remove top part of comment * add logic to look at value type for dynamic schemas * add data value tests * add dynamic get at path tests * add dynamic get tests * add path exist tests * add get provider schema tests * add comment to weird obj fix * make changes to support reflection (get) * function: Switch the representation of variadic arguments to `types.Tuple` (#923) * switch to tuples and add reflection support * update docs * add names to variadic param names * update doc note * update docs * update docs * add doc note * add changelog * switch changelog to breaking change * update to tuple index * update comment wording * update doc * add new maintainer note * remove top part of comment * fixed the odd spacing in the comment :) * add dynamic interface * reflection test * add list type update * add list value updates * small refactoring * quick check * revert list type and value * add detailed comments about the lack of dynamic element type support * add some object tests for good measure * add tuple tests * add validation logic for list attribute only * add block and attribute validation for list * function: Replace usage of diagnostics with function errors during execution of provider-defined functions (#925) * Replacing function.RunResponse diagnostics with error * Adding custom FunctionError * Adding custom FunctionErrors * Removing unneeded equateErrors gocmp option * Switching to using convenience functions for adding errors to FunctionErrors * Add copyright headers * Refactor to use Error() method on function errors when converting to tfprotov5/6.FunctionError * Adding documentation and testing for fwerrors types * Formatting errors during conversion to tfprotov<5|6>.FunctionError * Removing argument error and argument warning diagnostics * Renaming field name for FunctionErrors from Error to Errors * Modifying documentation to reflect that executing the Run() method of a provider-defined function returns FunctionErrors * Remove references to AddArgumentError and AddArgumentWarning from diagnostics documentation * Removing fwerror package and moving FunctionError to function package * Refactoring to replace FunctionErrors slice with single FunctionError * Bumping terraform-plugin-go to v0.22.0 * Removing unneeded DiagnosticWithFunctionArgument interface and implementation * Altering function signature of ConcatFuncErrors * Removing HasError method * Updating docs * Updates following code review * Adding changelog entries * Fix naming * Update website/docs/plugin/framework/functions/errors.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Formatting * Updates following code review --------- Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update changelog * diag: remove incorrect code (#935) * update plugin-go dependency, fix errors and switch equal method * function: Add validation for parameter name conflicts and update defaulting logic (#936) * add validation and refactor defaulting logic * add changelogs * test fix * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * refactor the logic, tests and docs for defaulting * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com> --------- Co-authored-by: Brian Flad <bflad417@gmail.com> Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com> * resource/schema: Ensure invalid attribute default value errors are raised (#933) Reference: #590 Reference: #930 Previously the logic handling attribute `Default` values would silently ignore any type errors, which would lead to confusing planning data behaviors. This updates the logic to raise those error properly and adds covering unit testing. These error messages are using the underlying `tftypes` type system errors which is currently a pragmatic compromise throughout various parts of the framework logic that bridges between both type systems to save additional type assertion logic and potential bugs relating to those conversions. In the future if the internal `tftypes` handling and exported fields are replaced with the framework type system types, this logic would instead return error messaging based on the framework type system errors. This also will enhance the schema validation logic to check any `Default` response value and compare its type to the schema, which will raise framework type system errors during the `GetProviderSchema` RPC, or during schema unit testing if provider developers have implemented that additional testing. * Update provider functions testing docs to help users avoid nil pointer error (#940) * Fix capitalisation * Update example test to mitigate nil pointer error See #928 * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> --------- Co-authored-by: Austin Valle <austinvalle@gmail.com> Co-authored-by: Brian Flad <bflad417@gmail.com> * Reinstate go toolchain and add changelog for Go version bump to 1.21 (#937) * Reinstate go toolchain and add changelog for Go version bump to 1.21 * Adding changelog * Updating changelog * Squashed commit of the following: commit e7415b7 Author: Benjamin Bennett <ben.bennett@hashicorp.com> Date: Fri Mar 1 16:16:39 2024 +0000 Reinstate go toolchain and add changelog for Go version bump to 1.21 (#937) * Reinstate go toolchain and add changelog for Go version bump to 1.21 * Adding changelog * Updating changelog commit 1597a95 Author: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Fri Mar 1 16:12:22 2024 +0000 Update provider functions testing docs to help users avoid nil pointer error (#940) * Fix capitalisation * Update example test to mitigate nil pointer error See #928 * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> * Update website/docs/plugin/framework/functions/testing.mdx Co-authored-by: Austin Valle <austinvalle@gmail.com> --------- Co-authored-by: Austin Valle <austinvalle@gmail.com> Co-authored-by: Brian Flad <bflad417@gmail.com> commit bd22b58 Author: Brian Flad <bflad417@gmail.com> Date: Fri Mar 1 07:20:55 2024 -0500 resource/schema: Ensure invalid attribute default value errors are raised (#933) Reference: #590 Reference: #930 Previously the logic handling attribute `Default` values would silently ignore any type errors, which would lead to confusing planning data behaviors. This updates the logic to raise those error properly and adds covering unit testing. These error messages are using the underlying `tftypes` type system errors which is currently a pragmatic compromise throughout various parts of the framework logic that bridges between both type systems to save additional type assertion logic and potential bugs relating to those conversions. In the future if the internal `tftypes` handling and exported fields are replaced with the framework type system types, this logic would instead return error messaging based on the framework type system errors. This also will enhance the schema validation logic to check any `Default` response value and compare its type to the schema, which will raise framework type system errors during the `GetProviderSchema` RPC, or during schema unit testing if provider developers have implemented that additional testing. commit f03ca33 Author: Austin Valle <austinvalle@gmail.com> Date: Thu Feb 29 14:23:29 2024 -0500 function: Add validation for parameter name conflicts and update defaulting logic (#936) * add validation and refactor defaulting logic * add changelogs * test fix * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * refactor the logic, tests and docs for defaulting * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com> --------- Co-authored-by: Brian Flad <bflad417@gmail.com> Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com> * fix dynamic param * incorrect merge conflict fix :) * update comments from feedback * refactor validation logic * license headers * implement remaining resource and datasource validation * implement provider schema validation * update error msg and add object validator * add validation to object attributes * update existing attributes to use new bool return * add validation to function parameters and definitions * refactor to only have one exported function * add tuple tests for completeness * create new fwtype package * add parameter name to validate implementation definition * various PR fixes * add more docs * fix docs from default param change * update comment * add more to reflection case + update comment * comment wording * remove todos and add package docs * add changelogs * Fix the use-case where dynamic value type is known, but the value itself is still null/unknown * check for dynamic underlying value for null/unknown detection * removed the unneccessary interface and removed half-implemented reflection support * doc formatting * update static default naming * add happy path tests for datasource + provider validate implementations * update definition validation messaging with variadic * add doc explicitly saying that dynamic types aren't supported * add docs mentioning required dynamic type to functions * prevent nil panics in `types.Dynamic` implementation w/ tests * proposal for `NewDynamicValue` * add recommendations in doc string * update block msg * update all doc strings and error messages to make recommendations * move the function validate interfaces to internal package * add maintainer note about parameter name * prevent attribute paths from stepping into dynamic types + new tests * add helper methods for checking underlying value unknown/null * add tests and comments about edge case with empty values * move maintainer note * Add dynamic type support documentation and considerations * first pass at dynamic documentation * Apply suggestions from code review Co-authored-by: Brian Flad <bflad417@gmail.com> * update literal representation in attribute page * drop callouts and link to collection type page * Update website/docs/plugin/framework/handling-data/attributes/dynamic.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * Apply suggestions from code review Co-authored-by: Brian Flad <bflad417@gmail.com> * Update website/docs/plugin/framework/functions/parameters/dynamic.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * Update website/docs/plugin/framework/handling-data/dynamic-data.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * include parameter name in example * adjust the path documentation to reflect new changes * rework the first paragraph of the data page * add callout for null underlying element values * add underlying value null/unknown information * Update website/docs/plugin/framework/handling-data/types/dynamic.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * Update website/docs/plugin/framework/handling-data/dynamic-data.mdx Co-authored-by: Brian Flad <bflad417@gmail.com> * add float64 and number notes --------- Co-authored-by: Brian Flad <bflad417@gmail.com> --------- Co-authored-by: Benjamin Bennett <ben.bennett@hashicorp.com> Co-authored-by: hc-github-team-tf-provider-devex <github-team-tf-provider-devex@hashicorp.com> Co-authored-by: John Behm <jxsl13@googlemail.com> Co-authored-by: Brian Flad <bflad417@gmail.com> Co-authored-by: Sarah French <15078782+SarahFrench@users.noreply.github.com>

austinvalle

I merged #931, so I saved you the headache of the merge conflict (note 5c1f575 + 9b2199b). I had already fixed the documentation for website/docs/plugin/framework/functions/parameters/dynamic.mdx in my PR.

Did one last review of your PR to make sure it was all good after merge and looks clean now from my perspective! 🚀

Co-authored-by: Brian Flad <bflad417@gmail.com>

bendbennett · 2024-03-21T06:56:05Z

I merged #931, so I saved you the headache of the merge conflict (note 5c1f575 + 9b2199b). I had already fixed the documentation for website/docs/plugin/framework/functions/parameters/dynamic.mdx in my PR.

Did one last review of your PR to make sure it was all good after merge and looks clean now from my perspective! 🚀

Thank you. Much appreciated!

github-actions · 2024-04-22T02:04:22Z

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

bendbennett added 3 commits March 20, 2024 05:53

Enforce provider-defined function parameter naming

a3d2acf

Passing function name to ValidateImplementation to provide additional…

c5433f5

… context in diagnostics

Add nil check for Error() method on function error

29c49ae

bendbennett added the breaking-change This PR introduces a breaking change or the resolution of this issue may require a breaking change. label Mar 20, 2024

bendbennett added this to the v1.7.0 milestone Mar 20, 2024

bendbennett requested a review from a team as a code owner March 20, 2024 09:50

Adding changelog entry

fb236ec

bendbennett mentioned this pull request Mar 20, 2024

Updating provider-defined functions to use named parameters hashicorp/terraform-provider-corner#227

Merged

bflad approved these changes Mar 20, 2024

View reviewed changes

austinvalle approved these changes Mar 20, 2024

View reviewed changes

bendbennett and others added 4 commits March 20, 2024 15:49

Update .changes/unreleased/BREAKING CHANGES-20240320-095152.yaml

5a9e5e8

Co-authored-by: Brian Flad <bflad417@gmail.com>

Switching to request-response pattern for function definition validation

11433b9

Adding change log entry for removal of DefaultParameterNamePrefix and…

e2ae3cd

… DefaultVariadicParameterName constants

Using parameter names throughout examples, and adding illustration of…

66e4fa1

… potential runtime errors

bflad approved these changes Mar 20, 2024

View reviewed changes

bflad mentioned this pull request Mar 20, 2024

all: Add dynamic type, attribute, and function support #931

Merged

austinvalle mentioned this pull request Mar 20, 2024

function: Refactor name validation to utilize new internal/fwfunction parameter validation interfaces #967

Closed

austinvalle and others added 4 commits March 20, 2024 17:40

Merge branch 'main' into bendbennett/pdf-enforce-param-naming

c91b157

delete duplicate tests and add name params

5c1f575

remove duplicate from merge conflict

9b2199b

austinvalle approved these changes Mar 20, 2024

View reviewed changes

Apply suggestions from code review

a2f04e2

Co-authored-by: Brian Flad <bflad417@gmail.com>

bendbennett merged commit a7607bb into main Mar 21, 2024
27 checks passed

bendbennett deleted the bendbennett/pdf-enforce-param-naming branch March 21, 2024 07:01

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require provider-defined function parameter naming #964

Require provider-defined function parameter naming #964

bendbennett commented Mar 20, 2024 •

edited

bflad left a comment

bflad Mar 20, 2024

bendbennett Mar 20, 2024

bflad Mar 20, 2024

bendbennett Mar 20, 2024

austinvalle left a comment

bendbennett commented Mar 20, 2024

austinvalle commented Mar 20, 2024

bflad left a comment

bflad Mar 20, 2024

austinvalle left a comment •

edited

bendbennett commented Mar 21, 2024

github-actions bot commented Apr 22, 2024

Require provider-defined function parameter naming #964

Require provider-defined function parameter naming #964

Conversation

bendbennett commented Mar 20, 2024 • edited

Breaking Change

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 20, 2024

Choose a reason for hiding this comment

bendbennett Mar 20, 2024

Choose a reason for hiding this comment

bflad Mar 20, 2024

Choose a reason for hiding this comment

bendbennett Mar 20, 2024

Choose a reason for hiding this comment

austinvalle left a comment

Choose a reason for hiding this comment

bendbennett commented Mar 20, 2024

austinvalle commented Mar 20, 2024

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 20, 2024

Choose a reason for hiding this comment

austinvalle left a comment • edited

Choose a reason for hiding this comment

bendbennett commented Mar 21, 2024

github-actions bot commented Apr 22, 2024

bendbennett commented Mar 20, 2024 •

edited

austinvalle left a comment •

edited