Deserialization should not rely on classes with constructors having large parameter counts

[dixler]

Hello!

• Vote on this issue by adding a 👍 reaction
• If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Deserialization should not rely on classes with constructors having large parameter counts.

Code generation for Java SDK classes that need to be deserialized from protobuf currently relies on constructors with many parameters (one per desired property) internally. This breaks down when it hits hard limits on Java parameter counts.

Earlier when we worked around the problem of not being able to compile providers with large parameter counts, we introduced a special pathway in the SDK code generator for the special case of "jumbo" constructors. This does not completely solve the problem however. While you can construct these Jumbo objects, tryConvertObjectInner errors when facing them.

Desired complete fix would:

• fix the deserializer in the Java SDK to not require one parameter per property in any method or constructor of the generated classes, so as to not hit the Java limit
• fix the codegen to align with the deserializer expectations
• remove branching on parameter count from codegen code
• have no breaking changes for the users (it seems the problem can be solved internally)
• fix or avoid "Converter::tryConvertObjectInner errors when calling jumbo constructors" found earlier

Affected area/feature

[pawelprazak]

I'm curious, what was the error?

[dixler]

https://github.com/pulumi/pulumi-jvm/runs/6028205171?check_suite_focus=true

[pawelprazak]

Hmm

Caused by: java.lang.IllegalArgumentException: Expected type 'io.pulumi.awsnative.s3.outputs.BucketWebsiteConfiguration' (annotated with 'io.pulumi.core.annotations.CustomType') to provide a constructor annotated with 'io.pulumi.core.annotations.CustomType$Constructor', and the parameter names in the annotation matching the parameters being deserialized. Constructor 'private io.pulumi.awsnative.s3.outputs.BucketWebsiteConfiguration()' expects parameter names of: '', but does not expect: 'indexDocument'. Unable to deserialize.

looks like unexpected parameter 'indexDocument' on parameter-less constructor

[pawelprazak]

I've added jumbo custom type support to the SDK, it is already quite big (because of codegened test)
so it feel like I should do the codegen changes in a separate PR, WDYT?

[pawelprazak]

I assume we don't want to break current providers, so I plan to remove the constructor-based code, after we release both the new codegen and regenerate all of the (affected) providers.

As a result, I plan to do as follows, in 4 stages:

• add new mechanism to SDK (Support custom jumbo type #720, Custom jumbo type with less annotations #730)
• release the SDK
• update codegen to use the new mechanism (Custom jumpo type sunset #744)
• remove the old mechanism from codegen
• wait for providers release with the new codegen
• remove the old mechanism from SDK

There might be some additional cleanup needed in the codegen, after removal of the old mechanism, I'm not super familiar with this part of the codebase, but I anticipate this eventuality.

[t0yv0]

Interesting.

1. We are still in a position that we could roll out a breaking change if it makes things a lot easier. The workaround for users is relatively simple - they need to update both SDK and providers.

2. Providers are migrating to https://github.com/pulumi/pulumi-azuread/tree/master/sdk/java etc so we need to keep merging changes to main and logging in CHANGELOG_PENDING so that if there are changes to codegen that are going to result in breaking provider changes they're reflected in the changelog. They will be released eventually.

3. after removal of the old mechanism, I'm not super familiar with this part of the codebase - it should be simple to do (a few hours) so I would love to see it included.

[t0yv0]

Let's create a follow-up issue (s) for cleanup issues, and consider this done.