pulumi / pulumi-dotnet

.NET support for Pulumi
Apache License 2.0
28 stars 25 forks source link

[sdk] Initial implementation of a reflection-based PropertyValue deserializer #201

Closed Zaid-Ajaj closed 1 year ago

Zaid-Ajaj commented 1 year ago

This PR implements a deserializer from PropertyValue into objects via reflection, to be used in native-provider implementations that operate on PropertyValue instead of the lower-level Struct and Value from gRPC.

The implementation handles most commonly used primitive types and their containers: string, int, bool, double, enums, nullable types, arrays, immutable arrays, lists, dictionaries, immutable dictionaries, inputs, input list, input map and nested objects.

When deserializing an object, if a non-nullable primitive property is missing, an exception is thrown. However, this doesn't include arrays, dictionaries or lists: an empty container is constructed when the data is empty or missing. Also for strings, an empty string is created when deserializing from Null or the property is missing. This is to accommodate for the fact that gRPC treats null values as empty so I made the deserializer less strict about missing values.

Frassle commented 1 year ago

When deserializing an object, if a non-nullable primitive property is missing, an exception is thrown. However, this doesn't include arrays, dictionaries or lists: an empty container is constructed when the data is empty or missing. Also for strings, an empty string is created when deserializing from Null or the property is missing. This is to accommodate for the fact that gRPC treats null values as empty so I made the deserializer less strict about missing values.

Ooooh. I'm conflicted on this. I like strict systems, so I'd probably prefer the system to error on any missing required property. But then you've got the fun of working out if something is "required" or not. Easy for value types (is it Nullable), but we'd probably need an attribute for reference types.

But most of pulumi works on a very loose, protobuf style interface where a missing field is the same as the zero value for that field. So to work well in practice with that ecosystem we might need this system to treat missing as zero value as well.

Zaid-Ajaj commented 1 year ago

@Frassle I've refactored the serializer code into a PropertyValueSerializer class which exposes two instance methods as we discussed:

Removed all encoder/decoder APIs and the associated handling of attributes.

The only "extension point" right now is that users can use PropertyValue directly when serializing or deserializing when they want to handle the raw data directly (with the idea is the inferred schema later would be any when using PropertyValue directly)

Would appreciate a second look 🙏

Zaid-Ajaj commented 1 year ago

@Frassle refactored the API according to our discussion: