feat: type check values based on pulumi schema #1800

corymhall · 2024-03-27T18:07:06Z

This adds an initial type checking implementation to pkg/tfbridge. The actually type checker is pretty generic and only has a dependency on pulumi/pulumi so it would be possible to pull this out into a separate library as a general type checker.

Feature Support

Along with the type checker it will also build an error message that contains the path to the property.

Todo

Add to pf/tfbridge (can be done in a later pr)
Add tests that hit the new type checker at the provider level
How do we roll this out?
- By default this will only warn users, but can override with an env var

re #1328

This adds an initial type checking implementation to `pkg/tfbridge`. The actually type checker is pretty generic and only has a dependency on `pulumi/pulumi` so it would be possible to pull this out into a separate library as a general type checker. **Feature Support** - [X] required properties - [X] property names - [X] `string` types - [X] `number` types - [X] `object` types - [X] `array` types - [X] `null` types - [X] `bool` types - [X] `output` types - [X] `secret` types - [ ] `asset` types - [ ] `archive` types - [ ] `resourceReference` types - [ ] String Enums - [ ] Number Enums - *It knows about enums, but it does not validate enum values since enums are not exhaustive* Along with the type checker it will also build an error message that contains the path to the property. re #1328

codecov · 2024-03-27T19:29:16Z

Codecov Report

Attention: Patch coverage is 81.49780% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 60.14%. Comparing base (d9452a5) to head (3e55e5a).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/tfbridge/provider.go	47.50%	19 Missing and 2 partials ⚠️
pkg/tfbridge/validate_input_types.go	88.77%	17 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1800      +/-   ##
==========================================
+ Coverage   59.99%   60.14%   +0.15%     
==========================================
  Files         327      328       +1     
  Lines       43976    44206     +230     
==========================================
+ Hits        26382    26587     +205     
- Misses      16102    16122      +20     
- Partials     1492     1497       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

iwahbe · 2024-04-01T14:55:30Z

pkg/tfbridge/provider.go

+	if p.pulumiSchema != nil {
+		var schema pschema.PackageSpec
+		if err := json.Unmarshal(p.pulumiSchema, &schema); err != nil {
+			return nil, err
+		}


I don't think we can afford to unmarshal the schema on each Check. Let's do this only on the first check, maybe with sync.OnceValue.

pkg/tfbridge/provider.go

pkg/tfbridge/validate_input_types.go

pkg/tfbridge/validate_input_types_test.go

Co-authored-by: Ian Wahbe <ian@wahbe.com>

pkg/tfbridge/provider.go

t0yv0 · 2024-04-16T16:50:10Z

Round of suggestions here: #1878 - happy to take some time to talk through that tomorrow.

…pe-checking

corymhall · 2024-04-17T14:54:17Z

Round of suggestions here: #1878 - happy to take some time to talk through that tomorrow.

@t0yv0 I've rebased against your PR and added in some of the TODOs. Let me know if you think.

I took a stab at the OneOf, but we can also just remove it and defer it to later.

t0yv0 · 2024-04-18T00:37:48Z

pkg/tfbridge/validate_input_types.go

+		// elementTypeSpec := typeSpec.AdditionalProperties
+		if propertyValue.IsObject() {
+			if typeSpec.AdditionalProperties == nil {
+				// Unknown item type so nothing more to check


The binder actually assumes this would be a stringly typed map[string]string. Could go either way here.

Are you saying that if Type: "object", AdditionalProperties: nil then the assumption is that the type is Type: "object", AdditionalProperties: { Type: "string" }?

Yes! Last I checked that seemed to be literally what the binder code was saying. It is not easy to tell though if that's intentional or "let's assume some type here". I guess I prefer your version here for now - no type specified so we won't check.

t0yv0 · 2024-04-18T00:38:37Z

pkg/tfbridge/validate_input_types.go

+			failures := []TypeFailure{}
+			for _, propertyKey := range objectValue.StableKeys() {
+				pb := append(propertyPath, string(propertyKey))
+				failure := v.validatePropertyValue(objectValue[propertyKey], *typeSpec.AdditionalProperties, pb)


This looks right.

t0yv0 · 2024-04-18T00:39:23Z

pkg/tfbridge/validate_input_types.go

+				return nil
+			}
+			// if there is a ref to another type, get that and then validate
+			if typeSpec.AdditionalProperties.Ref != "" {


You don't need this if clause at all, I think. What's going to happen is that v.validatePropertyValue on *typeSpec.AdditionalProperties is going to take full care of this.

There is one scenario where this is needed (example below). If we don't include this block then we end up comparing the objectStringProp type to the pkg:index/type:ObjectStringType.

input like this:

"prop": map[string]interface{}{ "objectStringProp": "foo", }

And a spec like this

map[string]pschema.ComplexTypeSpec{ "pkg:index/type:ObjectStringType": { ObjectTypeSpec: pschema.ObjectTypeSpec{ Type: "object", Properties: map[string]pschema.PropertySpec{ "objectStringProp": { TypeSpec: pschema.TypeSpec{ Type: "string", }, }, }, }, }, "pkg:index/type:ObjectDoubleNestedObjectType": { ObjectTypeSpec: pschema.ObjectTypeSpec{ Type: "object", Properties: map[string]pschema.PropertySpec{ "prop": { TypeSpec: pschema.TypeSpec{ Type: "object", // not using ref to test arbitrary object keys AdditionalProperties: &pschema.TypeSpec{ Type: "object", Ref: "#/types/pkg:index/type:ObjectStringType", }, }, }, }, }, }, }

I see it's under test, checking up on it, hm..

Removing this block makes TestValidateInputType_arrays/object_double_nested_object_type_success fail, I've instrumented that test case and I think it's actually not asserting the right behavior, with this block removed I think the validator correctly fails on that test case. Step by step:

=== RUN TestValidateInputType_arrays/object_double_nested_object_type_success # validating against array of objects validatePropertyValue {[{map[prop:{map[objectStringProp:{foo}]}]}]} object_double_nested_object_type_success4 { "type": "array", "items": { "$ref": "#/types/pkg:index/type:ObjectDoubleNestedObjectType" } } # validating first element against an object validatePropertyValue {map[prop:{map[objectStringProp:{foo}]}]} object_double_nested_object_type_success4[0] { "$ref": "#/types/pkg:index/type:ObjectDoubleNestedObjectType" } # down to prop attribute of the object, validating against a map of objects validatePropertyValue {map[objectStringProp:{foo}]} object_double_nested_object_type_success4[0].prop { "type": "object", "additionalProperties": { "type": "object", "$ref": "#/types/pkg:index/type:ObjectStringType" } } # down objectStringProp map key, validating "foo" string against an object type validatePropertyValue {foo} object_double_nested_object_type_success4[0].prop.objectStringProp { "type": "object", "$ref": "#/types/pkg:index/type:ObjectStringType" } # this should fail as it does validate_input_types_test.go:1552: 0 failures, got 1: [{expected object type, got string type object_double_nested_object_type_success4[0].prop.objectStringProp}] --- FAIL: TestValidateInputType_arrays (0.00s) --- FAIL: TestValidateInputType_arrays/object_double_nested_object_type_success4 (0.00s)

E.g. for the double nested object type, TypeScript would have to supply something like this:

{"prop": {"key1": {"objectStringProp": "A"}, "key2": {"objectStringProp": "B"}}}

We can always check by generating a TypeScript SDK for a schema that looks like that.

Ok I remove that block

t0yv0 · 2024-04-18T00:40:36Z

pkg/tfbridge/validate_input_types.go

+				// we have found the correct type
+				return nil
+			}
+			failures = append(failures, failure...)


Merging all these failures will be unreadable. Perhaps just ignore them all for now and emit one generic failure that "matched none of the union type members"

I took a stab at trying to return a better single error message, let me know what you think.

t0yv0 · 2024-04-18T00:41:32Z

pkg/tfbridge/validate_input_types.go

+
+		// default type
+		if typeSpec.Type != "" {
+			typeSpec.OneOf = append(typeSpec.OneOf, pschema.TypeSpec{Type: typeSpec.Type})


I always worry this mutation would leak outside the scope of this func, but looks like it doesn't perhaps because we have our own struct copy.

I don't quite understand what this is doing. Why is typeSpec.Type appended to OneOf?

I took that logic from https://github.com/pulumi/pulumi/blob/477a54b3de081a4927df3038c7b2266d599d5f3c/pkg/codegen/schema/bind.go#L849-L854

Not sure exactly why, but it looks like a "default" type can be stored in the Type field.

This is a strange encoding I don't quite fully understand how it's interpreted either, would need to dig in.

Looking for examples in the wild.

https://github.com/pulumi/pulumi-aws/blob/upstream-v5.46.0/provider/cmd/pulumi-resource-aws/schema.json#L126765

Looks like for all examples I could find, these are enums that can also be strings. So the encoding is specifying that .type is string ("default"), but then also providing string as one of the alternatives, and the enum type as the other alternative.

t0yv0 · 2024-04-18T00:42:15Z

pkg/tfbridge/validate_input_types.go

+		)
+	}
+
+	if typeSpec.OneOf != nil {


This looks right

pkg/tfbridge/validate_input_types.go

remove required property validation better oneof error messages

…pe-checking

iwahbe

This looks pretty good to me. Before we roll out a change like this, can you create a branch that has PULUMI_ERROR_TYPE_CHECKER default to on, then run all of our downstream tests. If our downstream tests are valid and the type checker is valid, then they should all pass.

iwahbe · 2024-04-19T14:55:53Z

pkg/tfbridge/validate_input_types.go

+	stableKeys := propertyMap.StableKeys()
+	failures := []TypeFailure{}
+
+	// TODO: handle required properties. Deferring for now


Let's make sure that all TODO's have an issue baked in:

Suggested change

// TODO: handle required properties. Deferring for now

// TODO[pulumi/pulumi-terraform-bridge#123]: handle required properties. Deferring for now

iwahbe · 2024-04-19T15:32:49Z

pkg/tfbridge/provider.go

+	logger := GetLogger(ctx)
+	// for now we are just going to log warnings if there are failures.
+	// over time we may want to turn these into actual errors
+	_, validateShouldError := os.LookupEnv("PULUMI_ERROR_TYPE_CHECKER")


PULUMI_ERROR_TYPE_CHECKER=false should not be equivalent to PULUMI_ERROR_TYPE_CHECKER=true.

Suggested change

_, validateShouldError := os.LookupEnv("PULUMI_ERROR_TYPE_CHECKER")

validateShouldError := "github.com/pulumi/pulumi/sdk/v3/go/common/util/cmdutil".IsTruthy(os.Getenv("PULUMI_ERROR_TYPE_CHECKER"))

pkg/tfbridge/validate_input_types_test.go

t0yv0 · 2024-04-19T17:36:52Z

pkg/tfbridge/validate_input_types.go

+			}
+			failures = append(failures, failure...)
+		}
+		// try to find the best failure message


Yeah IDK how this will play in practice I think it's just hard to get right. Looks like in practice mostly we use this unit feature for enums that can be string or one of the enum values. I think on the wire they'll all be strings then. I think it's really best leave it for a separate PR, or if we'r doing something here do something basic just fail in place, or not handle unions at all.

ok i'll remove the OneOf checking and leave a TODO.

remove oneof implementation remove additionalproperties.ref

t0yv0

This LGTM to me if we can pass a version of this that panics instead of warning and have all downstream tests pass with that (sorry for being so paranoid). Code looks good but we've historically had some trouble with this codebase behaving safely.

t0yv0 · 2024-04-19T19:06:35Z

Thanks @corymhall for all the work here.

corymhall · 2024-04-23T17:20:58Z

This LGTM to me if we can pass a version of this that panics instead of warning and have all downstream tests pass with that (sorry for being so paranoid). Code looks good but we've historically had some trouble with this codebase behaving safely.

Ran this branch through the downstream tests and nothing failed
https://github.com/pulumi/pulumi-terraform-bridge/tree/corymhall/better-type-checking-enabled-test

These tests were added in #1800 but seem to have been misplaced in the pf folder - the tests are for the SDKV2. The ~same tests are already present in the pkg folder under https://github.com/pulumi/pulumi-terraform-bridge/blob/18db7c57211af0eb7dfa26a70b145edc4d33a88f/pkg/tfbridge/tests/provider_test.go#L4

corymhall force-pushed the corymhall/better-type-checking branch from 3c5e839 to 0201d50 Compare March 27, 2024 18:17

forgot to handle the difference in "integer" & "boolean" types

06138b4

corymhall added 4 commits March 28, 2024 14:22

simplifying some things

677b637

linting...

c7e3ffc

more updates

a09ba3b

Merge branch 'master' into corymhall/better-type-checking

0cd1e17

t0yv0 self-requested a review March 29, 2024 19:07

mjeffryes assigned corymhall Mar 30, 2024

iwahbe reviewed Apr 1, 2024

View reviewed changes

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

t0yv0 reviewed Apr 8, 2024

View reviewed changes

pkg/tfbridge/validate_input_types_test.go Show resolved Hide resolved

corymhall and others added 10 commits April 8, 2024 10:24

Apply suggestions from code review

4e0fdf6

Co-authored-by: Ian Wahbe <ian@wahbe.com>

update based on comments

ff1d03f

removing logWarning

abe24e8

Merge branch 'master' into corymhall/better-type-checking

5fb5895

adding comments

9181a72

fixing linting error

7957457

adding isComputed to exclusions

fc14e02

refactoring some stuff

79bbdef

refactoring to PropertyPath

c91835e

fixing lint errors

2d40aff

t0yv0 reviewed Apr 16, 2024

View reviewed changes

pkg/tfbridge/provider.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into corymhall/better-ty…

c41e288

…pe-checking

fixing tests

af83021

corymhall requested review from t0yv0 and iwahbe April 17, 2024 15:29

t0yv0 reviewed Apr 18, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go

)

}

if typeSpec.OneOf != nil {

Copy link

Member

t0yv0 Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right

t0yv0 reviewed Apr 18, 2024

View reviewed changes

pkg/tfbridge/validate_input_types.go Outdated Show resolved Hide resolved

corymhall added 3 commits April 18, 2024 10:40

add pulumi schema to provider struct

bd62853

remove required property validation better oneof error messages

Merge remote-tracking branch 'origin/master' into corymhall/better-ty…

d9aff59

…pe-checking

run go mod tidy

58370dc

corymhall requested a review from t0yv0 April 19, 2024 13:42

iwahbe reviewed Apr 19, 2024

View reviewed changes

corymhall added 2 commits April 19, 2024 13:26

correctly read env var

8fa4cb3

fixing imports

abae1a5

t0yv0 reviewed Apr 19, 2024

View reviewed changes

add in todo comments

3e55e5a

remove oneof implementation remove additionalproperties.ref

t0yv0 self-requested a review April 19, 2024 19:03

t0yv0 approved these changes Apr 19, 2024

View reviewed changes

flostadler approved these changes Apr 22, 2024

View reviewed changes

corymhall merged commit 682a9ad into master Apr 23, 2024
11 checks passed

corymhall deleted the corymhall/better-type-checking branch April 23, 2024 17:21

VenelinMartinov mentioned this pull request Jul 9, 2024

Remove duplicated tests #2167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: type check values based on pulumi schema #1800

feat: type check values based on pulumi schema #1800

corymhall commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

iwahbe Apr 1, 2024

corymhall Apr 8, 2024

t0yv0 commented Apr 16, 2024

corymhall commented Apr 17, 2024

t0yv0 Apr 18, 2024

corymhall Apr 18, 2024

t0yv0 Apr 18, 2024

t0yv0 Apr 18, 2024

t0yv0 Apr 18, 2024

corymhall Apr 18, 2024

t0yv0 Apr 19, 2024

t0yv0 Apr 19, 2024 •

edited

Loading

t0yv0 Apr 19, 2024

t0yv0 Apr 19, 2024

corymhall Apr 19, 2024

t0yv0 Apr 18, 2024

corymhall Apr 18, 2024

t0yv0 Apr 18, 2024

flostadler Apr 19, 2024

corymhall Apr 19, 2024

t0yv0 Apr 19, 2024

t0yv0 Apr 18, 2024

iwahbe left a comment

iwahbe Apr 19, 2024

corymhall Apr 19, 2024

iwahbe Apr 19, 2024

corymhall Apr 19, 2024

t0yv0 Apr 19, 2024

corymhall Apr 19, 2024

t0yv0 left a comment

t0yv0 commented Apr 19, 2024

corymhall commented Apr 23, 2024

	// TODO: handle required properties. Deferring for now
	// TODO[pulumi/pulumi-terraform-bridge#123]: handle required properties. Deferring for now

	_, validateShouldError := os.LookupEnv("PULUMI_ERROR_TYPE_CHECKER")
	validateShouldError := "github.com/pulumi/pulumi/sdk/v3/go/common/util/cmdutil".IsTruthy(os.Getenv("PULUMI_ERROR_TYPE_CHECKER"))

feat: type check values based on pulumi schema #1800

feat: type check values based on pulumi schema #1800

Conversation

corymhall commented Mar 27, 2024 • edited Loading

codecov bot commented Mar 27, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Apr 16, 2024

corymhall commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

t0yv0 commented Apr 19, 2024

corymhall commented Apr 23, 2024

corymhall commented Mar 27, 2024 •

edited

Loading

codecov bot commented Mar 27, 2024 •

edited

Loading

t0yv0 Apr 19, 2024 •

edited

Loading