Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs #11418

Zaid-Ajaj · 2022-11-21T15:22:16Z

Description

This PR implements simplified invoke gen for both SDKs and PCL programs by allowing invokes to:

Accepts multi arguments as inputs (instead of a bag of properties)
Return a single value as output (instead of a bag of outputs)

Related to #7435

This implementation handles dotnet and nodejs. Python and Go can be implementation on a separate pass. Currently both python and go will fail generation (sdk/program) when encountering simplified invokes

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-11-22T17:19:28Z

Changelog

[uncommitted] (2022-12-16)

Features

[sdkgen/{dotnet,nodejs}] Initial implementation of simplified invokes for dotnet and nodejs.
#11418

Zaid-Ajaj · 2022-11-29T01:35:39Z

@iwahbe @Frassle @AaronFriel I've added you for a PR review if you have time 🙏 I also made a quick explanation of what the is and what it does, hope this helps 😄

simplified-invokes-pr.mp4

iwahbe

I left an initial set of feedback. It would be really helpful if you reverted the TS changes so that the only generated code that changes is the code we expect to see changed.

pkg/codegen/schema/bind.go

pkg/codegen/schema/schema.go

iwahbe · 2022-11-29T18:37:55Z

pkg/codegen/pcl/utilities.go

+// SortedFunctionParameters returns a list of properties of the input type from the schema
+// for an invoke function call which has multi argument inputs. We assume here
+// that the expression is an invoke which has it's args (2nd parameter) annotated
+// with the original schema type. The original schema type has properties sorted.
+// This is important because model.ObjectType has no guarantee of property order.
+func SortedFunctionParameters(expr *model.FunctionCallExpression) []*schema.Property {


This indicates a footgun to me. It looks like this function must be called and then nil checked every time an invoke is generated.

Could we just spread multi-argument invoke parameters into the Signature.Paremeters field? Then retrieving the sorted parameters would be as simple as express.Signature.Parameters[1:].

I changed the implementation to return an empty array instead of nil because if MultiArgumentInputs then the array of properties is must be non-empty which relies on invoke annotations.

Could we just spread multi-argument invoke parameters into the Signature.Paremeters field? Then retrieving the sorted parameters would be as simple as express.Signature.Parameters[1:].

That would change the shape of the invoke in PCL too and I am more inclined to leave as an object that corresponds with the schema inputs object.

pkg/codegen/nodejs/gen_program_expressions.go

pkg/codegen/dotnet/gen_program_expressions.go

pkg/codegen/schema/schema.go

pkg/codegen/testing/test/program_driver.go

pkg/codegen/testing/test/testdata/external-resource-schema/nodejs/argFunction.ts

pkg/codegen/testing/test/testdata/output-funcs-tfbridge20/nodejs/getAmiIds.ts

pkg/codegen/testing/test/testdata/simplified-invokes/schema.json

pkg/codegen/schema/schema.go

11511: [typescript/sdk-gen] Generate JS doc comments for output-versioned invokes r=Zaid-Ajaj a=Zaid-Ajaj - Generate JS doc comments for output-versioned invokes - Use explicit `any` type within apply lambda This is part of #11418 but irrelevant to that (big) PR so this will help make the changes smaller and more clear ## 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>

11564: [sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs r=Zaid-Ajaj a=Zaid-Ajaj  # Description Needed for #11418 where we have invoke calls that return a simple type (such as `number`) but the runtime and engine _require_ the outputs to be a struct (as per the RPC definition) so here we are adding variants of `invoke` that unwraps the returned object and gets the first value from that object. For example, a provider with a schema `outputs: { type: "number" }` will have an invoke implementation that returns `{ __result: 42.0 }`, in these case (non-object return types) we use `InvokeSingle` instead of `Invoke` > It doesn't matter what the name of the key is, we always get the first value by first key. ## 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>

iwahbe

The changes look good to me.

Can you remove whatever you did that caused whitespace changes to all generated TS functions?

pkg/codegen/testing/test/program_driver.go

11564: [sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs r=Zaid-Ajaj a=Zaid-Ajaj  # Description Needed for #11418 where we have invoke calls that return a simple type (such as `number`) but the runtime and engine _require_ the outputs to be a struct (as per the RPC definition) so here we are adding variants of `invoke` that unwraps the returned object and gets the first value from that object. For example, a provider with a schema `outputs: { type: "number" }` will have an invoke implementation that returns `{ __result: 42.0 }`, in these case (non-object return types) we use `InvokeSingle` instead of `Invoke` > It doesn't matter what the name of the key is, we always get the first value by first key. ## 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  11575: Simplify `findFunctionSchema` r=iwahbe a=iwahbe Instead of indirecting through .NET function names, just use the schema token to lookup the function. Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com> Co-authored-by: Ian Wahbe <ian@wahbe.com>

pkg/codegen/testing/test/testdata/external-resource-schema/nodejs/argFunction.ts

11564: [sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs r=Zaid-Ajaj a=Zaid-Ajaj  # Description Needed for #11418 where we have invoke calls that return a simple type (such as `number`) but the runtime and engine _require_ the outputs to be a struct (as per the RPC definition) so here we are adding variants of `invoke` that unwraps the returned object and gets the first value from that object. For example, a provider with a schema `outputs: { type: "number" }` will have an invoke implementation that returns `{ __result: 42.0 }`, in these case (non-object return types) we use `InvokeSingle` instead of `Invoke` > It doesn't matter what the name of the key is, we always get the first value by first key. ## 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>

11564: [sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs r=Zaid-Ajaj a=Zaid-Ajaj  # Description Needed for #11418 where we have invoke calls that return a simple type (such as `number`) but the runtime and engine _require_ the outputs to be a struct (as per the RPC definition) so here we are adding variants of `invoke` that unwraps the returned object and gets the first value from that object. For example, a provider with a schema `outputs: { type: "number" }` will have an invoke implementation that returns `{ __result: 42.0 }`, in these case (non-object return types) we use `InvokeSingle` instead of `Invoke` > It doesn't matter what the name of the key is, we always get the first value by first key. ## 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  11581: Fix PCL binding of options.range = number r=Frassle a=Frassle  # Description  Fixes the binding issue with `options { range = number }`. ## Checklist  - [x] 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  Co-authored-by: Zaid Ajaj <zaid.naom@gmail.com> Co-authored-by: Fraser Waters <fraser@pulumi.com>

11564: [sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs r=Zaid-Ajaj a=Zaid-Ajaj  # Description Needed for #11418 where we have invoke calls that return a simple type (such as `number`) but the runtime and engine _require_ the outputs to be a struct (as per the RPC definition) so here we are adding variants of `invoke` that unwraps the returned object and gets the first value from that object. For example, a provider with a schema `outputs: { type: "number" }` will have an invoke implementation that returns `{ __result: 42.0 }`, in these case (non-object return types) we use `InvokeSingle` instead of `Invoke` > It doesn't matter what the name of the key is, we always get the first value by first key. ## 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>

Zaid-Ajaj · 2022-12-08T11:09:13Z

@iwahbe @Frassle I updated the PR such that it no longer creates diffs in unaffected TS files.

pkg/codegen/testing/test/testdata/output-funcs/nodejs/funcWithEmptyOutputs.ts

11596: [sdk/gen] Avoid generating Result types for functions with empty outputs r=Zaid-Ajaj a=Zaid-Ajaj  # Description  Part of #11418 where we shouldn't generate result types for function invokes that have no properties. This is a separate PR to showcase the exact changes required to improve the situation and reduce the diff from #11418 ## 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>

iwahbe

I have a couple of suggestions for code quality. My last area of concern is serialization, but I think that should be relatively quick to fix. I'm hoping there is a way to do it without manual re-implementation.

pkg/codegen/dotnet/gen.go

pkg/codegen/nodejs/gen.go

iwahbe · 2022-12-14T16:29:27Z

pkg/codegen/schema/schema.go

+	return len(*objectData) == 0, nil
+}
+
+func UnmarshalFunctionSpec(funcSpec *FunctionSpec, data map[string]RawMessage) error {


Why do we need to custom unmarshal the entire FunctionSpec? This seems really error prone.

Also, does this need to be public?

Because I need to control how two fields are unmarshalled: Outputs and ReturnType, the former being derived from the latter. Same goes for marshalling. However, it does not need to be public. I will change that

Does this need to be done at the FunctionSpec level. I imagine it would be much easier and less error prone to do at the bind level.

P.S. The current implementation directly calls json.Unmarshal, which won't work for yaml.

Does this need to be done at the FunctionSpec level. I imagine it would be much easier and less error prone to do at the bind level.

I tried removing the unmarshalling logic and instead only have custom marshalling for ReturnTypeSpec, however that doesn't work with YAML because if I put the attribute yaml:"outputs" on the ReturnTypeSpec, then it fails saying that there are duplicate outputs definitions because the Outputs fields is still there. It can be simplified when we remove Outputs entirely

P.S. The current implementation directly calls json.Unmarshal, which won't work for yaml.

It works because it uses map[string]RawMessage as an intermediate data structure for both JSON and YAML. See unit tests I've added in schema_test.go for marshalling and unmarshalling FunctionSpec

pkg/codegen/schema/schema.go

iwahbe

I'm satisfied. LGTM!

Zaid-Ajaj · 2022-12-16T13:12:31Z

bors merge

11418: Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs r=Zaid-Ajaj a=Zaid-Ajaj  # Description This PR implements simplified invoke gen for both SDKs and PCL programs by allowing invokes to: - Accepts multi arguments as inputs (instead of a bag of properties) - Return a single value as output (instead of a bag of outputs) Related to #7435 This implementation handles dotnet and nodejs. Python and Go can be implementation on a separate pass. Currently both python and go will fail generation (sdk/program) when encountering simplified invokes ## Checklist  - [x] 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-12-16T15:22:27Z

Timed out.

Zaid-Ajaj · 2022-12-16T15:36:19Z

bors retry

bors · 2022-12-16T16:04:29Z

Build succeeded:

bors-ok

11701: Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs" r=iwahbe a=AaronFriel Reverts #11418. Fixes #11699. The addition of the `ReturnType` field on `schema.FunctionSpec` broke the API contract between Pulumi's codegen package and tfbridge and other tools. From my comment on #11699. > I was able to bisect pu/pu to a commit that had the issue, but couldn't reproduce the behavior with a synthetic schema. PR #11418 should be reverted because it introduces the breaking change, and before that PR is reintroduced, the breaking change to the schema should be addressed. > > Notes on root causing: > > Adding an unserialized `ReturnType` field on the `schema.FunctionSpec` type was a breaking change in the API contract with the tfgen bridge and any other tools that generate a PackageSpec directly (without marshaling/unmarshaling) before binding it and calling `GeneratePackage`. > > In [cd6f658](cd6f658) we can see that a regression test fails to catch this case, the generated code is correct and matches the "before" case! That's because the regression tests in Pulumi load schemas from JSON, so the `ReturnType` field is populated. That occurs in the implementation of `unmarshalFunctionSpec` at line 1656: > > https://github.com/pulumi/pulumi/blob/78c9bf4ee4def5130bfd5b29346ece7c61cec08a/pkg/codegen/schema/schema.go#L1649-L1658 > > This function in the bridge is unaware of the new field, and so it only populates `Outputs`. > > https://github.com/pulumi/pulumi-terraform-bridge/blob/36b10921bfd25a5cabf0a342d0e449278a256a8b/pkg/tfgen/generate_schema.go#L607-L638 Co-authored-by: Aaron Friel <mayreply@aaronfriel.com>

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from a522da4 to 5ab3f46 Compare November 23, 2022 21:06

Zaid-Ajaj marked this pull request as ready for review November 23, 2022 21:06

Zaid-Ajaj force-pushed the simplified-invoke-gen branch 2 times, most recently from 46f092b to 02d53b5 Compare November 24, 2022 09:55

Zaid-Ajaj mentioned this pull request Nov 27, 2022

Provider support for dotnet #9496

Closed

3 tasks

Zaid-Ajaj requested review from AaronFriel, Frassle and iwahbe November 29, 2022 01:33

iwahbe requested changes Nov 29, 2022

View reviewed changes

Zaid-Ajaj mentioned this pull request Dec 2, 2022

[typescript/sdk-gen] Generate JS doc comments for output-versioned invokes #11511

Merged

3 tasks

iwahbe reviewed Dec 3, 2022

View reviewed changes

pkg/codegen/schema/schema.go Outdated Show resolved Hide resolved

Frassle reviewed Dec 3, 2022

View reviewed changes

pkg/codegen/schema/schema.go Show resolved Hide resolved

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from 59731be to f1e6008 Compare December 5, 2022 17:24

Zaid-Ajaj requested review from iwahbe and Frassle December 5, 2022 17:25

Zaid-Ajaj mentioned this pull request Dec 6, 2022

[sdk/dotnet/nodejs] Add InvokeSingle variants to dotnet and nodejs SDKs #11564

Merged

3 tasks

iwahbe reviewed Dec 7, 2022

View reviewed changes

pkg/codegen/testing/test/program_driver.go Show resolved Hide resolved

Frassle reviewed Dec 7, 2022

View reviewed changes

pkg/codegen/testing/test/testdata/external-resource-schema/nodejs/argFunction.ts Outdated Show resolved Hide resolved

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from d0893a1 to 378db6c Compare December 8, 2022 11:07

Zaid-Ajaj requested a review from iwahbe December 8, 2022 11:09

iwahbe reviewed Dec 8, 2022

View reviewed changes

pkg/codegen/testing/test/testdata/output-funcs/nodejs/funcWithEmptyOutputs.ts Outdated Show resolved Hide resolved

Zaid-Ajaj mentioned this pull request Dec 8, 2022

[sdk/gen] Avoid generating Result types for functions with empty outputs #11596

Merged

3 tasks

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from c8d8e45 to b1d1df9 Compare December 12, 2022 11:58

Zaid-Ajaj requested a review from iwahbe December 12, 2022 11:59

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from a78d49d to ac4b693 Compare December 12, 2022 14:36

iwahbe requested changes Dec 14, 2022

View reviewed changes

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from 578f429 to eec2d91 Compare December 16, 2022 00:58

Zaid-Ajaj requested a review from iwahbe December 16, 2022 01:03

Initial implementation of simplified invokes for dotnet and nodejs

a6f26d2

Zaid-Ajaj force-pushed the simplified-invoke-gen branch from 5b40f45 to a6f26d2 Compare December 16, 2022 12:39

iwahbe approved these changes Dec 16, 2022

View reviewed changes

bors bot merged commit 68bf791 into master Dec 16, 2022

bors bot deleted the simplified-invoke-gen branch December 16, 2022 16:04

This was referenced Dec 21, 2022

Nodejs codegen regression impacting Azure provider #11699

Closed

Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs" #11701

Merged

iwahbe restored the simplified-invoke-gen branch December 21, 2022 19:14

iwahbe mentioned this pull request Jan 9, 2023

Revert "Revert "Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs"" #11753

Merged

mikhailshilkov mentioned this pull request Mar 20, 2024

Python codegen doesn't support non-object function return types #15738

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs #11418

Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs #11418

Zaid-Ajaj commented Nov 21, 2022

pulumi-bot commented Nov 22, 2022 •

edited

Zaid-Ajaj commented Nov 29, 2022

iwahbe left a comment

iwahbe Nov 29, 2022

Zaid-Ajaj Dec 2, 2022

iwahbe left a comment

Zaid-Ajaj commented Dec 8, 2022

iwahbe left a comment

iwahbe Dec 14, 2022

Zaid-Ajaj Dec 14, 2022

iwahbe Dec 16, 2022

Zaid-Ajaj Dec 16, 2022

iwahbe left a comment

Zaid-Ajaj commented Dec 16, 2022

bors bot commented Dec 16, 2022

Zaid-Ajaj commented Dec 16, 2022

bors bot commented Dec 16, 2022

Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs #11418

Simplified invokes: SDK-gen and program-gen implementation for dotnet and nodejs #11418

Conversation

Zaid-Ajaj commented Nov 21, 2022

Description

Checklist

pulumi-bot commented Nov 22, 2022 • edited

Changelog

[uncommitted] (2022-12-16)

Features

Zaid-Ajaj commented Nov 29, 2022

iwahbe left a comment

Choose a reason for hiding this comment

iwahbe Nov 29, 2022

Choose a reason for hiding this comment

Zaid-Ajaj Dec 2, 2022

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Zaid-Ajaj commented Dec 8, 2022

iwahbe left a comment

Choose a reason for hiding this comment

iwahbe Dec 14, 2022

Choose a reason for hiding this comment

Zaid-Ajaj Dec 14, 2022

Choose a reason for hiding this comment

iwahbe Dec 16, 2022

Choose a reason for hiding this comment

Zaid-Ajaj Dec 16, 2022

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Zaid-Ajaj commented Dec 16, 2022

bors bot commented Dec 16, 2022

Zaid-Ajaj commented Dec 16, 2022

bors bot commented Dec 16, 2022

pulumi-bot commented Nov 22, 2022 •

edited