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

Don't flatten outputs to top level for InputList and InputMap #449

Merged
merged 7 commits into from
Jan 31, 2025

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Jan 29, 2025

Fixes #22

InputList and InputMap are types used to make it easier to work with lists/maps where both the overall structure or individual elements can be Inputs.

The current implementation just flattens everything to a top-level input. That is InputList<T> is just Input<ImmutableArray<T>>. So if you start with a known empty list then add an unknown input element to it you end up with an unknown list structure. This is not ideal for previews (see #22) and doesn't align with the other SDKs where if you add an unknown element to a list the overall list shape is still known and sent to the engine for preview correctly.

This PR fixes the above by keeping track of non-flattened data inside InputMap/List. i.e. the internal shape of InputList<T> is now Input<ImmutableArray<Input<T>>>. This allows us to take a known list and add an unknown element to it and leave the overall shape of the list still known.

We do this without changing the public surface of InputList/Map, so they still inherit from Input<ImmutableArray<T>>. We add another field for the nested structure and ensure that we just always update both fields (the one in InputList/Map and the one in the base Input class) together. In the property serializer we then use reflection to pull out the fully nested value (e.g. Input<ImmutableArray<Input<T>>>) and then recursively serialize that, rather than hitting the standard IInput case and getting the flattened value (e.g. Input<ImmutableArray<T>>).

@Frassle Frassle changed the title Fraser/input list map Don't flatten outputs to top level for InputList and InputMap Jan 29, 2025
@Frassle Frassle changed the title Don't flatten outputs to top level for InputList and InputMap Don't flatten outputs to top level for InputList, InputMap, etc Jan 29, 2025
@Frassle Frassle changed the title Don't flatten outputs to top level for InputList, InputMap, etc Don't flatten outputs to top level for InputList and InputMap Jan 29, 2025
@Frassle Frassle marked this pull request as ready for review January 29, 2025 15:38
@Frassle Frassle requested a review from a team as a code owner January 29, 2025 15:38
Copy link
Contributor

@lunaris lunaris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love a good hack. Some comments but happy either way. And thanks for the excellent PR description!

@@ -46,18 +46,30 @@ namespace Pulumi
/// </summary>
public sealed class InputList<T> : Input<ImmutableArray<T>>, IEnumerable, IAsyncEnumerable<Input<T>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reckon it's worth extending the doc comment to include some of the awesome context you put in the PR description? Or perhaps using that as a comment for the Value property/_inputValue instance variable if you think it's better kept private/internal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Value for sure, not sure it can be sensibly added to the user level description.

var (first, second) = t;
return first.AddRange(second);
});
return list;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing Output.Concat does an Apply internally, so this use of Apply doesn't change "knownness" or anything like that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, if we just Concat'd the whole thing we'd end up lifting all the sub-element secret/known bits to the top level.

}

Input<ImmutableDictionary<string, Input<V>>> _inputValue;
Input<ImmutableDictionary<string, Input<V>>> Value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as for list around doc comments/use of Apply

@Frassle Frassle force-pushed the fraser/inputListMap branch from 05e25ff to e82f9b7 Compare January 31, 2025 16:10
@Frassle Frassle enabled auto-merge January 31, 2025 16:22
@Frassle Frassle added this pull request to the merge queue Jan 31, 2025
Merged via the queue into main with commit f7020ec Jan 31, 2025
19 checks passed
@Frassle Frassle deleted the fraser/inputListMap branch January 31, 2025 17:08
@pulumi-bot
Copy link

This PR has been shipped in release v3.73.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sdk/dotnet] Input<T> eagerly converts values to Output<T> leading to undesirable preview diffs
3 participants