[import] Handle importing resource properties of type Union #10995

Zaid-Ajaj · 2022-10-11T16:39:40Z

Description

Fixes #10934

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2022-10-11T16:40:31Z

Changelog

[uncommitted] (2022-10-13)

Bug Fixes

[cli/import] Handle importing resource properties that are typed as a union
#10995

Frassle · 2022-10-11T16:44:13Z

Adding Ian and Justin to review as well, I don't fully get all the PCL codebase yet.

Frassle · 2022-10-11T16:50:27Z

pkg/codegen/importer/hcl2.go

+			selectElementType := func() (*schema.ObjectType, bool) {
+				discriminatorValue, ok := obj[resource.PropertyKey(discriminatorPropertyName)]
+				if !ok {
+					// discriminator property is not present, so we can't select a type


I think we need to fallback to the untyped map cases if we fail to work out the union case type correctly? @iwahbe or @justinvp might be able to confirm that, I'm not totally sure what the constraints and expectations of this method are.

iwahbe

I think it would be simpler to handle this in a more general case. After we set typ = codegen.UnwrapType(type), we could put

if union, ok := typ.(*schema.UnionType); ok {
  typ = reduceUnion(union, value)
}

Reduce union would look like

// reduceUnion returns the element type in union that bests fits value.
// If more than one type fits, reduceUnion will return the union.DefaultType if it is non-nil, otherwise it will return the first type that fits.
// If no type fits, nil is returned.
func reduceUnion(union *schema.UnionType, value resource.PropertyValue) schema.Type

UnionType.Discriminator is optional, so we will need to reduce based on the shape of the value, not just on the discriminator.

iwahbe · 2022-10-11T16:54:12Z

pkg/codegen/importer/hcl2.go

@@ -410,7 +410,8 @@ func generateValue(typ schema.Type, value resource.PropertyValue) (model.Express
 		obj := value.ObjectValue()
 		items := make([]model.ObjectConsItem, 0, len(obj))

-		if objectType, ok := typ.(*schema.ObjectType); ok {
+		switch objectType := typ.(type) {


It's idiomatic to shadow here. Regardless, we shouldn't call this objectType when it's not of type object.

Suggested change

switch objectType := typ.(type) {

switch typ := typ.(type) {

iwahbe · 2022-10-11T17:27:51Z

pkg/codegen/importer/hcl2.go

+					}
+				}
+			}
+


I think we need to do something for !ok. We can fall back to what we were doing before.

iwahbe · 2022-10-11T17:28:04Z

pkg/codegen/importer/hcl2.go

-		} else {
+		case *schema.UnionType:
+			discriminatorPropertyName := objectType.Discriminator
+			selectElementType := func() (*schema.ObjectType, bool) {


Why do we create a lambda here, only to invoke it on the next line? We should either separate out selectElementType into its own function or inline it.

Just a stylistic choice to make to more readable. I'll rewrite it.

iwahbe · 2022-10-11T17:42:34Z

pkg/codegen/importer/hcl2.go

+						// match it against the element type which should be an object
+						elementTypeObject, ok := codegen.UnwrapType(elementType).(*schema.ObjectType)
+						if ok {
+							elementTypeParts := strings.Split(elementTypeObject.Token, ":")


Can we replace this by casting to tokens.Type and then calling the appropriate methods?

Zaid-Ajaj · 2022-10-11T18:07:50Z

UnionType.Discriminator is optional, so we will need to reduce based on the shape of the value, not just on the discriminator.

@iwahbe you mean here for each property in the object value, go through each union element type and do a best-effect match to see one object fits? It doesn't seem deterministic imo, maybe in this case we fall back to the default case handling. What do you think?

iwahbe · 2022-10-11T18:26:17Z

UnionType.Discriminator is optional, so we will need to reduce based on the shape of the value, not just on the discriminator.

@iwahbe you mean here for each property in the object value, go through each union element type and do a best-effect match to see one object fits? It doesn't seem deterministic imo, maybe in this case we fall back to the default case handling. What do you think?

I mean for each Element type in the union, check if value fits. It can't be for for each property in the object value, because lots of unions have element types [string, SomeObject] for example. It will be deterministic, but might not always be able to fully narrow the type, here well make a best guess (which must be deterministic). That's ok since typ is advisory.

I think we should fall back to the default option iff no type fits. I'll amend my comments on the reduceUnion function to fit.

Zaid-Ajaj · 2022-10-12T11:48:44Z

@iwahbe I've addressed your comments, can you have another look? 🙏

iwahbe

I left a comment on union resolution. If you want, we can return nil for non-discriminated unions and open a new issue to add the correct checking.

We will definitely want a test here.

iwahbe · 2022-10-12T18:19:32Z

pkg/codegen/importer/hcl2.go

+	case value.IsObject():
+		// If the value is an object, use the discriminator to choose the element type.
+		if schemaUnion.Discriminator == "" {
+			// If there is no discriminator, return first type which is an object


I don't agree with this. If there is no discriminator, we need to return the first type it is legal to assign value to. This is a structural type check.

For example, given the value { foo: 1 } and Union[{ foo: string }, { bar: Union[none, string], foo: number } we should resolve to the second element, not the first.

An example of where this would be necessary is
https://github.com/pulumi/pulumi-aws-native/blob/edc3d04caddc8996061d1ce2a8b80fbe5236b313/provider/cmd/pulumi-resource-aws-native/schema.json#L13430-L13451.

I've implemented the structural type check to find the best fit + tests ✅

iwahbe

I left a last set of comments, but I'm out of corner cases. I appreciate the tests and working with me to add what is essentially a structural type checker to package importer.

LGTM!

iwahbe · 2022-10-13T18:47:03Z

pkg/codegen/importer/hcl2.go

+	case value.IsString():
+		// check to see if one of the element types in the union
+		// actually allows a string type and return it
+		for _, t := range schemaUnion.ElementTypes {
+			if t == schema.StringType {
+				return t
+			}
+		}
+
+	case value.IsBool():
+		// check to see if one of the element types in the union
+		// actually allows a bool type and return it
+		for _, t := range schemaUnion.ElementTypes {
+			if t == schema.BoolType {
+				return t
+			}
+		}
+
+	case value.IsNumber():
+		// check to see if one of the element types in the union
+		// actually allows a number type and return it
+		for _, t := range schemaUnion.ElementTypes {
+			if t == schema.NumberType {
+				return t
+			}
+		}
+	}


Unions come in two types: discriminated objects and structural. We can simplify this (and handle the missing case value.IsArray()) by invoking valueStructurallyTypedAs:

Suggested change

case value.IsString():

// check to see if one of the element types in the union

// actually allows a string type and return it

for _, t := range schemaUnion.ElementTypes {

if t == schema.StringType {

return t

}

}

case value.IsBool():

// check to see if one of the element types in the union

// actually allows a bool type and return it

for _, t := range schemaUnion.ElementTypes {

if t == schema.BoolType {

return t

}

}

case value.IsNumber():

// check to see if one of the element types in the union

// actually allows a number type and return it

for _, t := range schemaUnion.ElementTypes {

if t == schema.NumberType {

return t

}

}

}

default:

for _, t := range schemaUnion.ElementTypes {

if valueStructurallyTypedAs(value, t) {

return t

}

}

}

iwahbe · 2022-10-13T18:47:10Z

pkg/codegen/importer/hcl2.go

@@ -365,11 +366,222 @@ func generatePropertyValue(property *schema.Property, value resource.PropertyVal
 	return generateValue(property.Type, value)
 }

+// valueStructurallyTypedAs returns true if the given value is structurally typed as the given schema type.
+func valueStructurallyTypedAs(value resource.PropertyValue, schemaType schema.Type) bool {
+	switch {


We can hoist the union check here, since we need to repeat it for every value anyway:

Suggested change

switch {

if union, ok := schemaType.(*schema.Union); ok {

schemaType = reduceUnion(union, value)

}

switch

The reduction is mutually recursive since we want to handle nested discriminated unions.

This lets us remove the case *schema.UnionType checks in each branch of the case value.Is*().

iwahbe · 2022-10-13T18:51:57Z

pkg/codegen/importer/hcl2.go

+		switch arg := schemaType.(type) {
+		case *schema.ObjectType:
+			schemaProperties := make(map[string]schema.Type)
+			for _, schemaProperty := range arg.Properties {


This map is built into schema.ObjectType. The API is here.

iwahbe · 2022-10-13T18:57:13Z

pkg/codegen/importer/hcl2.go

+		case *schema.ObjectType:
+			schemaProperties := make(map[string]schema.Type)
+			for _, schemaProperty := range arg.Properties {
+				schemaProperties[schemaProperty.Name] = schemaProperty.Type


Suggested change

schemaProperties[schemaProperty.Name] = schemaProperty.Type

// If `value.ObjectValue()` is missing a property

if _, ok := value.ObjectValue()[schemaProperty.Name]; !ok

// And that property is not optional

if _, ok := schemaProperty.Type.(*schema.OptionalType); !ok {

// We are missing a required property, so the type does not match

return false

}

}

iwahbe · 2022-10-13T18:57:25Z

pkg/codegen/importer/hcl2.go

+	case value.IsObject():
+		switch arg := schemaType.(type) {
+		case *schema.ObjectType:
+			schemaProperties := make(map[string]schema.Type)


This map is built into schema.ObjectType. The API is here.

Zaid-Ajaj · 2022-10-13T23:38:55Z

bors merge

10995: [import] Handle importing resource properties of type Union r=Zaid-Ajaj a=Zaid-Ajaj  # Description  Fixes #10934 ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>

AaronFriel · 2022-10-14T00:23:00Z

bors cancel

bors · 2022-10-14T00:23:02Z

Canceled.

AaronFriel · 2022-10-14T00:23:05Z

bors retry

AaronFriel · 2022-10-14T00:23:15Z

bors merge

10995: [import] Handle importing resource properties of type Union r=AaronFriel a=Zaid-Ajaj  # Description  Fixes #10934 ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  11011: Add PCL as a target language for pulumi convert r=Zaid-Ajaj a=Zaid-Ajaj  # Description  Every time I need to debug `pulumi convert` I have to hack YAML program-gen and make it output the intermediate PCL program text to stdout and work from there. Here, I've added PCL as a proper target language for `pulumi convert` (without needing to parse the program text and generate `pcl.Program` from it) I tried a simpler approach: ```go projectGenerator = func(directory string, project workspace.Project, p *pcl.Program) error { programText := fmt.Sprintf("%v", p) outputFile := path.Join(directory, "main.pp") err := ioutil.WriteFile(outputFile, []byte(programText), 0600) if err != nil { return fmt.Errorf("could not write output program: %w", err) } return nil } ``` but the default formatter for `pcl.Program` writes out invalid PCL (including non-formatted type metadata) so I went with this approach instead ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>

AaronFriel · 2022-10-14T00:23:57Z

I canceled & requeued as one of the build jobs hanged: https://github.com/pulumi/pulumi/actions/runs/3246406652/jobs/5325125965

AaronFriel · 2022-10-14T01:08:15Z

bors cancel

bors · 2022-10-14T01:08:17Z

Canceled.

AaronFriel · 2022-10-14T01:08:20Z

bors merge

10995: [import] Handle importing resource properties of type Union r=AaronFriel a=Zaid-Ajaj  # Description  Fixes #10934 ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  11011: Add PCL as a target language for pulumi convert r=AaronFriel a=Zaid-Ajaj  # Description  Every time I need to debug `pulumi convert` I have to hack YAML program-gen and make it output the intermediate PCL program text to stdout and work from there. Here, I've added PCL as a proper target language for `pulumi convert` (without needing to parse the program text and generate `pcl.Program` from it) I tried a simpler approach: ```go projectGenerator = func(directory string, project workspace.Project, p *pcl.Program) error { programText := fmt.Sprintf("%v", p) outputFile := path.Join(directory, "main.pp") err := ioutil.WriteFile(outputFile, []byte(programText), 0600) if err != nil { return fmt.Errorf("could not write output program: %w", err) } return nil } ``` but the default formatter for `pcl.Program` writes out invalid PCL (including non-formatted type metadata) so I went with this approach instead ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>

AaronFriel · 2022-10-14T02:16:11Z

I would have liked to get this in, but it failed 3 times in CI.

https://github.com/pulumi/pulumi/actions/runs/3246806849/jobs/5326028122

AaronFriel · 2022-10-14T02:16:15Z

bors cancel

bors · 2022-10-14T02:16:16Z

Canceled.

Zaid-Ajaj · 2022-10-14T07:28:05Z

@AaronFriel can't seem to decipher why CI is failing or why it would fail in regard to this change, will just retry before digging further into it

Zaid-Ajaj · 2022-10-14T07:28:29Z

bors merge

10995: [import] Handle importing resource properties of type Union r=Zaid-Ajaj a=Zaid-Ajaj  # Description  Fixes #10934 ## Checklist  - [ ] I have added tests that prove my fix is effective or that my feature works  - [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change  - [ ] Yes, there are changes in this PR that warrants bumping the Pulumi Service API version  Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com>

bors · 2022-10-14T08:19:57Z

Build failed:

bors-ok

Zaid-Ajaj · 2022-10-14T12:30:38Z

bors retry

bors · 2022-10-14T13:14:59Z

Build succeeded:

bors-ok

Zaid-Ajaj requested a review from Frassle October 11, 2022 16:39

Frassle requested review from iwahbe and justinvp October 11, 2022 16:43

Frassle reviewed Oct 11, 2022

View reviewed changes

iwahbe reviewed Oct 11, 2022

View reviewed changes

iwahbe self-requested a review October 12, 2022 17:54

iwahbe reviewed Oct 12, 2022

View reviewed changes

Zaid-Ajaj requested a review from iwahbe October 13, 2022 12:13

iwahbe approved these changes Oct 13, 2022

View reviewed changes

[import] Handle importing resource properties of type Union

d1843cc

Zaid-Ajaj force-pushed the zaid/fix-import-unions-10934 branch from 11de0a1 to d1843cc Compare October 13, 2022 22:30

bors bot merged commit 69e3bf9 into master Oct 14, 2022

bors bot deleted the zaid/fix-import-unions-10934 branch October 14, 2022 13:15

-	switch {
+	if union, ok := schemaType.(*schema.Union); ok {
+		schemaType = reduceUnion(union, value)
+	}
+	switch

-				schemaProperties[schemaProperty.Name] = schemaProperty.Type
+			// If `value.ObjectValue()` is missing a property
+			if _, ok := value.ObjectValue()[schemaProperty.Name]; !ok
+				// And that property is not optional
+				if _, ok := schemaProperty.Type.(*schema.OptionalType); !ok {
+					// We are missing a required property, so the type does not match
+					return false
+				}
+			}

[import] Handle importing resource properties of type Union #10995

[import] Handle importing resource properties of type Union #10995

Conversation

Zaid-Ajaj commented Oct 11, 2022

Description

Checklist

pulumi-bot commented Oct 11, 2022 • edited Loading

Changelog

[uncommitted] (2022-10-13)

Bug Fixes

Frassle commented Oct 11, 2022

Choose a reason for hiding this comment

iwahbe left a comment • 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

Zaid-Ajaj commented Oct 11, 2022

iwahbe commented Oct 11, 2022

Zaid-Ajaj commented Oct 12, 2022

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

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

Zaid-Ajaj commented Oct 13, 2022

AaronFriel commented Oct 14, 2022

bors bot commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

bors bot commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

AaronFriel commented Oct 14, 2022

bors bot commented Oct 14, 2022

Zaid-Ajaj commented Oct 14, 2022

Zaid-Ajaj commented Oct 14, 2022

bors bot commented Oct 14, 2022

Zaid-Ajaj commented Oct 14, 2022

bors bot commented Oct 14, 2022

pulumi-bot commented Oct 11, 2022 •

edited

Loading

iwahbe left a comment •

edited

Loading