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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve schema-provider response mismatch issue #150

Closed
lbialy opened this issue Sep 21, 2023 · 3 comments
Closed

Resolve schema-provider response mismatch issue #150

lbialy opened this issue Sep 21, 2023 · 3 comments
Assignees
Labels
area/core The SDK's core code impact/first-48 This bug is likely to be hit during a user's first 48 hours of product evaluation kind/bug Some behavior is incorrect or out of spec P0 Bugs severe enough to interrupt existing work
Milestone

Comments

@lbialy
Copy link
Collaborator

lbialy commented Sep 21, 2023

Opened due to #148

We have to solve a problem where mismatch between what schema deems required and what provider treats as optional (nullable) crashes the whole besom user program.

Proposals:

  • deserialize to OutputData.Known(None) with isSecret respectively
  • OutputData.Broken with explode-on-touch semantics (quite involved due to separation of Output and OutputData, needs an actual idea on how the semantics would work because OutputData.Broken should infect the wrapping Output with error on any form of evaluation of that Output
  • rewrite to Scala 3 nullable types and just lie to user and cause NPEs (lol nope)

Rationale and ideas:
One thing that we could do is to just deserialize that to an empty OutputData.Known but I strongly oppose that.

In Scala, we're used to the facts that if something doesn't deserialize, it fails immediately (without throwing) and that Option is propagating None everywhere via combinators. However, if we consistently propagate this null through OutputData.Known with None, the user won't be aware of the issue (because we preserve Option semantics and short-circuit). The None will propagate to another place expecting a value. This null will eventually reach the Pulumi engine, causing issues.

The optimal approach is to ignore the situation if the user doesn't interact with it, but notify them immediately if they do, offering a workaround. One such workaround is "don't touch this field." Another potential solution is to provide a recover or change semantics to orElse on Output, allowing the user to substitute their value. This has low usability I fear.

The key takeaway is that every such case indicates that the Terraform provider might be misleading about the schema. There are edge cases where a required field isn't truly required and is optional.

In my opinion, we should embed logic that logs this to the user's console, even if user doesn't touch that field, allowing them to open a full issue with a description of the event and a generated jsonpatch rule to add to the patch library. If the user touches that field - we have to stop execution with an error - there is nothing sane we can do without completely subverting the typesystem. The codegen should check during generation whether the patch indicates that the field has a different real semantics. This would allow us to actively type-fix schemas instead of pretending everything is fine and "tolerating" nulls in non-nullable fields.

The generated documentation for the field says it's required and non-nullable, but you might get a null - laughable.

https://docs.github.com/en/issues/tracking-your-work-with-issues/creating-an-issue#creating-an-issue-from-a-url-query

Originally posted by @lbialy in #148 (comment)

@lbialy lbialy self-assigned this Sep 21, 2023
@lbialy lbialy added kind/bug Some behavior is incorrect or out of spec impact/first-48 This bug is likely to be hit during a user's first 48 hours of product evaluation P0 Bugs severe enough to interrupt existing work labels Sep 21, 2023
@pawelprazak pawelprazak added the area/core The SDK's core code label Sep 21, 2023
@pawelprazak
Copy link
Collaborator

pawelprazak commented Sep 21, 2023

OutputData.Broken with explode-on-touch semantics

👍

In my opinion, we should embed logic that logs this to the user's console

👍

I'd defer more elegant escape hatches til later - we'll see if users actually hit this issue often.

IMO "explode-on-touch" is best solution, because this is not a simple case of schema mismatch but a provider breaking their own contract.

I'd defer the "patch-lib" til later, sound very interesting, but also sounds like issue to be solved upstream.

@lbialy
Copy link
Collaborator Author

lbialy commented Sep 21, 2023

Completely agreed, first step is to actually revert the semantics I have introduced as a bugfix lately. Original implementation of hydration logic did not crash the whole program, it failed all of the promises that backed up the Outputs in CustomResource family of records as failed with a DecodingError so the whole resource exploded on touch. It's a bit complicated to allow for a per-field failure resolution. I think the shape of Decoder and especially it's throwing nature will have to change (it was throwing internally because OutputData doesn't have an error channel but given the changes described here it will).

lbialy added a commit that referenced this issue Sep 24, 2023
lbialy added a commit that referenced this issue Sep 25, 2023
#163)

* Minimal change that brings the behaviour desired in #150
* added tests to confirm #150 is solved
@lbialy
Copy link
Collaborator Author

lbialy commented Sep 25, 2023

In the end we didn't go with OutputData.Broken and we just fail Promises individually. Closed by #163.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core The SDK's core code impact/first-48 This bug is likely to be hit during a user's first 48 hours of product evaluation kind/bug Some behavior is incorrect or out of spec P0 Bugs severe enough to interrupt existing work
Projects
None yet
Development

No branches or pull requests

2 participants