Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not panic when the type of a property in resource outputs doesn't match #643

Closed
Tracked by #598
t0yv0 opened this issue Jun 1, 2022 · 5 comments 路 Fixed by #813
Closed
Tracked by #598

Do not panic when the type of a property in resource outputs doesn't match #643

t0yv0 opened this issue Jun 1, 2022 · 5 comments 路 Fixed by #813
Assignees
Labels
area/sdks kind/enhancement Improvements or new features language/java resolution/fixed This issue was fixed
Milestone

Comments

@t0yv0
Copy link
Member

t0yv0 commented Jun 1, 2022

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

The motivation behind it can be found in:
pulumi/pulumi#7329

There is also a PR implementing it for C#.

pulumi/pulumi#8286

The fix proceed with a null / default value and a warning in place of an error. This makes some programs succeed.

Consider implementing the same here.

Affected area/feature

@t0yv0 t0yv0 added kind/enhancement Improvements or new features needs-triage Needs attention from the triage team labels Jun 1, 2022
@pawelprazak
Copy link
Contributor

Ideally we'd have as complete as possible test coverage for this change, since this requires refactoring the Converter/Deserializer, and I'm always worried about changing code there ;)

@mikhailshilkov mikhailshilkov added language/java and removed needs-triage Needs attention from the triage team labels Jun 3, 2022
@mikhailshilkov
Copy link
Member

This sounds important to me, should we add to the epic?

@t0yv0 t0yv0 mentioned this issue Jun 3, 2022
40 tasks
@t0yv0 t0yv0 self-assigned this Jun 7, 2022
@t0yv0 t0yv0 added this to the 0.74 milestone Jun 7, 2022
@mikhailshilkov mikhailshilkov modified the milestones: 0.74, 0.75 Jul 1, 2022
@t0yv0 t0yv0 assigned pawelprazak and unassigned t0yv0 Jul 12, 2022
@t0yv0 t0yv0 modified the milestones: 0.75, 0.76 Jul 21, 2022
@pawelprazak
Copy link
Contributor

@Frassle had a very interesting idea pulumi/pulumi#7329 (comment)
I'm just not sure how feasible it would be to implement.

@t0yv0
Copy link
Member Author

t0yv0 commented Aug 8, 2022

The straightforward approach would be to proceed with Output.ofNullable(null) in case of error. It would work if unused, and it would throw NPE if used. If we want to improve the NPE and throw something more meaningful, we could explore the path of:

Output.of(CompletableFuture.ofException(new ProviderTypeMismatchException(...)))

This would throw a better exception. Without special care, this would still make the program fail even if unused though because we have the machinery to note exceptions in every Output. That would have to be special cased, either ignoring exceptions of ProviderTypeMismatchException or having a flag to disable the machinery in this call of Output.of.

@mikhailshilkov
Copy link
Member

I think .NET does something like the former option?

@mikhailshilkov mikhailshilkov modified the milestones: 0.76, 0.77 Aug 15, 2022
pawelprazak added a commit that referenced this issue Aug 19, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
pawelprazak added a commit that referenced this issue Aug 19, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
pawelprazak added a commit that referenced this issue Aug 19, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
pawelprazak added a commit that referenced this issue Aug 23, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
pawelprazak added a commit that referenced this issue Aug 23, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
pawelprazak added a commit that referenced this issue Aug 23, 2022
- do not panic when the type of property in resource outputs doesn't match the one from the wire

Fixes #643
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sdks kind/enhancement Improvements or new features language/java resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants