Skip to content

Commit f03ca33

Browse files
austinvallebfladbendbennett
authored
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 <[email protected]> * refactor the logic, tests and docs for defaulting * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Benjamin Bennett <[email protected]> --------- Co-authored-by: Brian Flad <[email protected]> Co-authored-by: Benjamin Bennett <[email protected]>
1 parent 87c1b41 commit f03ca33

32 files changed

+624
-201
lines changed

Diff for: ‎.changes/unreleased/BUG FIXES-20240228-151959.yaml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
kind: BUG FIXES
2+
body: 'function: Added implementation validation to `function.Definition` to ensure
3+
all parameter names (including the variadic parameter) are unique.'
4+
time: 2024-02-28T15:19:59.843244-05:00
5+
custom:
6+
Issue: "926"

Diff for: ‎.changes/unreleased/BUG FIXES-20240228-152155.yaml

+6
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
kind: BUG FIXES
2+
body: 'function: Updated the default parameter name to include the position of the
3+
parameter (i.e. `param1`, `param2`, etc.). Variadic parameters will default to `varparam`.'
4+
time: 2024-02-28T15:21:55.182389-05:00
5+
custom:
6+
Issue: "926"

Diff for: ‎function/bool_parameter.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,11 @@ type BoolParameter struct {
6060

6161
// Name is a short usage name for the parameter, such as "data". This name
6262
// is used in documentation, such as generating a function signature,
63-
// however its usage may be extended in the future. If no name is provided,
64-
// this will default to "param".
63+
// however its usage may be extended in the future.
64+
//
65+
// If no name is provided, this will default to "param" with a suffix of the
66+
// position the parameter is in the function definition. ("param1", "param2", etc.)
67+
// If the parameter is variadic, the default name will be "varparam".
6568
//
6669
// This must be a valid Terraform identifier, such as starting with an
6770
// alphabetical character and followed by alphanumeric or underscore
@@ -91,11 +94,7 @@ func (p BoolParameter) GetMarkdownDescription() string {
9194

9295
// GetName returns the parameter name.
9396
func (p BoolParameter) GetName() string {
94-
if p.Name != "" {
95-
return p.Name
96-
}
97-
98-
return DefaultParameterName
97+
return p.Name
9998
}
10099

101100
// GetType returns the parameter data type.

Diff for: ‎function/bool_parameter_test.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,7 @@ func TestBoolParameterGetName(t *testing.T) {
182182
}{
183183
"unset": {
184184
parameter: function.BoolParameter{},
185-
expected: function.DefaultParameterName,
186-
},
187-
"Name-empty": {
188-
parameter: function.BoolParameter{
189-
Name: "",
190-
},
191-
expected: function.DefaultParameterName,
185+
expected: "",
192186
},
193187
"Name-nonempty": {
194188
parameter: function.BoolParameter{

Diff for: ‎function/definition.go

+42
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,48 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics
108108
)
109109
}
110110

111+
paramNames := make(map[string]int, len(d.Parameters))
112+
for pos, param := range d.Parameters {
113+
name := param.GetName()
114+
// If name is not set, default the param name based on position: "param1", "param2", etc.
115+
if name == "" {
116+
name = fmt.Sprintf("%s%d", DefaultParameterNamePrefix, pos+1)
117+
}
118+
119+
conflictPos, exists := paramNames[name]
120+
if exists {
121+
diags.AddError(
122+
"Invalid Function Definition",
123+
"When validating the function definition, an implementation issue was found. "+
124+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
125+
"Parameter names must be unique. "+
126+
fmt.Sprintf("Parameters at position %d and %d have the same name %q", conflictPos, pos, name),
127+
)
128+
continue
129+
}
130+
131+
paramNames[name] = pos
132+
}
133+
134+
if d.VariadicParameter != nil {
135+
name := d.VariadicParameter.GetName()
136+
// If name is not set, default the variadic param name
137+
if name == "" {
138+
name = DefaultVariadicParameterName
139+
}
140+
141+
conflictPos, exists := paramNames[name]
142+
if exists {
143+
diags.AddError(
144+
"Invalid Function Definition",
145+
"When validating the function definition, an implementation issue was found. "+
146+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
147+
"Parameter names must be unique. "+
148+
fmt.Sprintf("Parameter at position %d and the variadic parameter have the same name %q", conflictPos, name),
149+
)
150+
}
151+
}
152+
111153
return diags
112154
}
113155

Diff for: ‎function/definition_test.go

+172-1
Original file line numberDiff line numberDiff line change
@@ -152,11 +152,35 @@ func TestDefinitionValidateImplementation(t *testing.T) {
152152
definition function.Definition
153153
expected diag.Diagnostics
154154
}{
155-
"valid": {
155+
"valid-no-params": {
156156
definition: function.Definition{
157157
Return: function.StringReturn{},
158158
},
159159
},
160+
"valid-only-variadic": {
161+
definition: function.Definition{
162+
VariadicParameter: function.StringParameter{},
163+
Return: function.StringReturn{},
164+
},
165+
},
166+
"valid-param-name-defaults": {
167+
definition: function.Definition{
168+
Parameters: []function.Parameter{
169+
function.StringParameter{},
170+
function.StringParameter{},
171+
},
172+
Return: function.StringReturn{},
173+
},
174+
},
175+
"valid-param-names-defaults-with-variadic": {
176+
definition: function.Definition{
177+
Parameters: []function.Parameter{
178+
function.StringParameter{},
179+
},
180+
VariadicParameter: function.NumberParameter{},
181+
Return: function.StringReturn{},
182+
},
183+
},
160184
"result-missing": {
161185
definition: function.Definition{},
162186
expected: diag.Diagnostics{
@@ -168,6 +192,153 @@ func TestDefinitionValidateImplementation(t *testing.T) {
168192
),
169193
},
170194
},
195+
"conflicting-param-names": {
196+
definition: function.Definition{
197+
Parameters: []function.Parameter{
198+
function.StringParameter{
199+
Name: "string-param",
200+
},
201+
function.Float64Parameter{
202+
Name: "float-param",
203+
},
204+
function.Int64Parameter{
205+
Name: "param-dup",
206+
},
207+
function.NumberParameter{
208+
Name: "number-param",
209+
},
210+
function.BoolParameter{
211+
Name: "param-dup",
212+
},
213+
},
214+
Return: function.StringReturn{},
215+
},
216+
expected: diag.Diagnostics{
217+
diag.NewErrorDiagnostic(
218+
"Invalid Function Definition",
219+
"When validating the function definition, an implementation issue was found. "+
220+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
221+
"Parameter names must be unique. "+
222+
"Parameters at position 2 and 4 have the same name \"param-dup\"",
223+
),
224+
},
225+
},
226+
"conflicting-param-name-with-default": {
227+
definition: function.Definition{
228+
Parameters: []function.Parameter{
229+
function.StringParameter{
230+
Name: "param2",
231+
},
232+
function.Float64Parameter{}, // defaults to param2
233+
},
234+
Return: function.StringReturn{},
235+
},
236+
expected: diag.Diagnostics{
237+
diag.NewErrorDiagnostic(
238+
"Invalid Function Definition",
239+
"When validating the function definition, an implementation issue was found. "+
240+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
241+
"Parameter names must be unique. "+
242+
"Parameters at position 0 and 1 have the same name \"param2\"",
243+
),
244+
},
245+
},
246+
"conflicting-param-names-variadic": {
247+
definition: function.Definition{
248+
Parameters: []function.Parameter{
249+
function.Float64Parameter{
250+
Name: "float-param",
251+
},
252+
function.Int64Parameter{
253+
Name: "param-dup",
254+
},
255+
function.NumberParameter{
256+
Name: "number-param",
257+
},
258+
},
259+
VariadicParameter: function.BoolParameter{
260+
Name: "param-dup",
261+
},
262+
Return: function.StringReturn{},
263+
},
264+
expected: diag.Diagnostics{
265+
diag.NewErrorDiagnostic(
266+
"Invalid Function Definition",
267+
"When validating the function definition, an implementation issue was found. "+
268+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
269+
"Parameter names must be unique. "+
270+
"Parameter at position 1 and the variadic parameter have the same name \"param-dup\"",
271+
),
272+
},
273+
},
274+
"conflicting-param-names-variadic-multiple": {
275+
definition: function.Definition{
276+
Parameters: []function.Parameter{
277+
function.StringParameter{
278+
Name: "param-dup",
279+
},
280+
function.Float64Parameter{
281+
Name: "float-param",
282+
},
283+
function.Int64Parameter{
284+
Name: "param-dup",
285+
},
286+
function.NumberParameter{
287+
Name: "number-param",
288+
},
289+
function.BoolParameter{
290+
Name: "param-dup",
291+
},
292+
},
293+
VariadicParameter: function.BoolParameter{
294+
Name: "param-dup",
295+
},
296+
Return: function.StringReturn{},
297+
},
298+
expected: diag.Diagnostics{
299+
diag.NewErrorDiagnostic(
300+
"Invalid Function Definition",
301+
"When validating the function definition, an implementation issue was found. "+
302+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
303+
"Parameter names must be unique. "+
304+
"Parameters at position 0 and 2 have the same name \"param-dup\"",
305+
),
306+
diag.NewErrorDiagnostic(
307+
"Invalid Function Definition",
308+
"When validating the function definition, an implementation issue was found. "+
309+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
310+
"Parameter names must be unique. "+
311+
"Parameters at position 0 and 4 have the same name \"param-dup\"",
312+
),
313+
diag.NewErrorDiagnostic(
314+
"Invalid Function Definition",
315+
"When validating the function definition, an implementation issue was found. "+
316+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
317+
"Parameter names must be unique. "+
318+
"Parameter at position 0 and the variadic parameter have the same name \"param-dup\"",
319+
),
320+
},
321+
},
322+
"conflicting-param-name-with-variadic-default": {
323+
definition: function.Definition{
324+
Parameters: []function.Parameter{
325+
function.Float64Parameter{
326+
Name: function.DefaultVariadicParameterName,
327+
},
328+
},
329+
VariadicParameter: function.BoolParameter{}, // defaults to varparam
330+
Return: function.StringReturn{},
331+
},
332+
expected: diag.Diagnostics{
333+
diag.NewErrorDiagnostic(
334+
"Invalid Function Definition",
335+
"When validating the function definition, an implementation issue was found. "+
336+
"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
337+
"Parameter names must be unique. "+
338+
"Parameter at position 0 and the variadic parameter have the same name \"varparam\"",
339+
),
340+
},
341+
},
171342
}
172343

173344
for name, testCase := range testCases {

Diff for: ‎function/float64_parameter.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,11 @@ type Float64Parameter struct {
5757

5858
// Name is a short usage name for the parameter, such as "data". This name
5959
// is used in documentation, such as generating a function signature,
60-
// however its usage may be extended in the future. If no name is provided,
61-
// this will default to "param".
60+
// however its usage may be extended in the future.
61+
//
62+
// If no name is provided, this will default to "param" with a suffix of the
63+
// position the parameter is in the function definition. ("param1", "param2", etc.)
64+
// If the parameter is variadic, the default name will be "varparam".
6265
//
6366
// This must be a valid Terraform identifier, such as starting with an
6467
// alphabetical character and followed by alphanumeric or underscore
@@ -88,11 +91,7 @@ func (p Float64Parameter) GetMarkdownDescription() string {
8891

8992
// GetName returns the parameter name.
9093
func (p Float64Parameter) GetName() string {
91-
if p.Name != "" {
92-
return p.Name
93-
}
94-
95-
return DefaultParameterName
94+
return p.Name
9695
}
9796

9897
// GetType returns the parameter data type.

Diff for: ‎function/float64_parameter_test.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,7 @@ func TestFloat64ParameterGetName(t *testing.T) {
182182
}{
183183
"unset": {
184184
parameter: function.Float64Parameter{},
185-
expected: function.DefaultParameterName,
186-
},
187-
"Name-empty": {
188-
parameter: function.Float64Parameter{
189-
Name: "",
190-
},
191-
expected: function.DefaultParameterName,
185+
expected: "",
192186
},
193187
"Name-nonempty": {
194188
parameter: function.Float64Parameter{

Diff for: ‎function/int64_parameter.go

+6-7
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,11 @@ type Int64Parameter struct {
5656

5757
// Name is a short usage name for the parameter, such as "data". This name
5858
// is used in documentation, such as generating a function signature,
59-
// however its usage may be extended in the future. If no name is provided,
60-
// this will default to "param".
59+
// however its usage may be extended in the future.
60+
//
61+
// If no name is provided, this will default to "param" with a suffix of the
62+
// position the parameter is in the function definition. ("param1", "param2", etc.)
63+
// If the parameter is variadic, the default name will be "varparam".
6164
//
6265
// This must be a valid Terraform identifier, such as starting with an
6366
// alphabetical character and followed by alphanumeric or underscore
@@ -87,11 +90,7 @@ func (p Int64Parameter) GetMarkdownDescription() string {
8790

8891
// GetName returns the parameter name.
8992
func (p Int64Parameter) GetName() string {
90-
if p.Name != "" {
91-
return p.Name
92-
}
93-
94-
return DefaultParameterName
93+
return p.Name
9594
}
9695

9796
// GetType returns the parameter data type.

Diff for: ‎function/int64_parameter_test.go

+1-7
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,7 @@ func TestInt64ParameterGetName(t *testing.T) {
182182
}{
183183
"unset": {
184184
parameter: function.Int64Parameter{},
185-
expected: function.DefaultParameterName,
186-
},
187-
"Name-empty": {
188-
parameter: function.Int64Parameter{
189-
Name: "",
190-
},
191-
expected: function.DefaultParameterName,
185+
expected: "",
192186
},
193187
"Name-nonempty": {
194188
parameter: function.Int64Parameter{

0 commit comments

Comments
 (0)