Add support for array encoding with @encode(ArrayEncoding.*) decorator #9430
Conversation
microsoft-github-policy-service
bot
added
the
emitter:client:csharp
|
No changes needing a change description found. |
radhgupta
force-pushed
the
csv-encoding
branch
from
e7bb9b0 to
fbbfaf5
Compare
| IsDiscriminator = isDiscriminator; | ||
| IsHttpMetadata = isHttpMetadata; | ||
| SerializationOptions = serializationOptions; | ||
| Encode = encode; |
There was a problem hiding this comment.
I think we should follow the same pattern we use for other encoding formats. We can define a new ArrayKnownEncoding enum and add each of the possible values and then flow this through to a SerializationFormat. See https://github.com/microsoft/typespec/blob/main/packages/http-client-csharp/generator/Microsoft.TypeSpec.Generator/src/TypeFactory.cs#L340
There was a problem hiding this comment.
@JoshLove-msft the factory method only accepts an InputType. Should we add a new overload for the factory method for InputProperty ? ie.
public SerializationFormat GetSerializationFormat(InputProperty input) => input switch
{
InputModelProperty modelProperty => modelProperty.Encode switch
{
ArrayKnownEncoding.PipeDelimited => SerializationFormat.ArrayPipeDelimited,
},
_ => SerializationFormat.Default
};There was a problem hiding this comment.
I think the current approach is okay since these encodings are handled in PropertyProvider and we shouldn't need it outside of that scope. That said, we still need to update to use an ArrayKnownEncoding enum instead of a string.
...ient-csharp/generator/Microsoft.TypeSpec.Generator/src/Primitives/PropertyWireInformation.cs
Outdated
Show resolved
Hide resolved
radhgupta
force-pushed
the
csv-encoding
branch
from
83cc0d8 to
4be7888
Compare
radhgupta
requested review from
christothes,
jorgerangel-msft,
jsquire and
m-nash
as code owners
jorgerangel-msft
left a comment
jorgerangel-msft
left a comment
There was a problem hiding this comment.
It's good to see the spector tets, but could we also please add some unit tests for the serialization + deserialization code? It would be helpful to see the generated output of that.
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Outdated
Show resolved
Hide resolved
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Outdated
Show resolved
Hide resolved
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Outdated
Show resolved
Hide resolved
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Outdated
Show resolved
Hide resolved
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Show resolved
Hide resolved
...tor/Microsoft.TypeSpec.Generator.ClientModel/src/Providers/MrwSerializationTypeDefinition.cs
Outdated
Show resolved
Hide resolved
radhgupta
force-pushed
the
csv-encoding
branch
from
1ddb208 to
b2757da
Compare
jorgerangel-msft
left a comment
jorgerangel-msft
left a comment
There was a problem hiding this comment.
could we please add unit tests to see the generated code?
| useCustomDeserializationHook = true; | ||
| break; | ||
| [ | ||
| DeserializeValue(propertyType, jsonProperty.Value(), serializationFormat, out ValueExpression value), |
There was a problem hiding this comment.
nit: is there a way we can simplify the else condition here to avoid the duplication?
| // Check for encoded arrays and override the serialization statement | ||
| if (arrayEncoding.HasValue && IsArrayEncodingFormat(arrayEncoding.Value) && (propertyType.IsList || propertyType.IsArray)) | ||
| { | ||
| var elementType = GetCollectionElementType(propertyType); |
There was a problem hiding this comment.
we already know the type is a collection by this point, could we simply do propertyType.ElementType?
There was a problem hiding this comment.
Given that I added a check (propertyType.IsList || propertyType.IsArray) What I understand is for array we need to use .ElementType but for list we need to use .Argument[0].
Given the main.tsp I think mostly it would be an array, but I thought having a list check would be a good idea.
I am still tryin to learn the syntax here. Correct me if I am wrong.
There was a problem hiding this comment.
The ElementType property returns the element for these type of framework types: Array, ReadOnlyMemory, List, and Dictionaries. You can see the implementation for that here. If you are seeing issues with using this property for arrays, then that would be a bug
| return true; | ||
|
|
||
| // Support string enum arrays | ||
| if (elementType.IsEnum && !elementType.IsStruct && elementType.UnderlyingEnumType?.Equals(typeof(string)) == true) |
There was a problem hiding this comment.
question: do we need to also support string extensible enums? It looks like this check will skip that unless I'm missing something?
| } | ||
| else | ||
| { | ||
| throw new InvalidOperationException($"Encoded array serialization is only supported for string and string enum arrays. Element type: {elementType.Name}"); |
There was a problem hiding this comment.
Instead of throwing, can we please log a diagnostic? We could probably just return an empty statement if needed. In general, we want to avoid throwing and let their be compilation errors instead as the consumer could address those via custom code.
| { | ||
| // For string enum arrays, convert each enum to string | ||
| var x = new VariableExpression(typeof(object), "x"); | ||
| var selectExpression = propertyExpression.Invoke("Select", |
There was a problem hiding this comment.
could we possibly use the ToSerial extension method instead?
|
|
||
| if (isStringElement) | ||
| { | ||
| var splitResult = stringValueVar.Invoke("Split", delimiterChar); |
There was a problem hiding this comment.
(optional): we can add apis to StringSnippets if needed. I see a few places where we are using string apis that we haven't added snippets for. This can be a follow up clean item if you want 😃
| string propertyName, | ||
| MemberExpression propertyExpression) | ||
| MemberExpression propertyExpression, | ||
| SerializationFormat? arrayEncoding = null) |
There was a problem hiding this comment.
This seems a bit fragile to expect callers to pass null (or omit the format) if it is not an array. I think it would be more straightforward to just have this be the serializationFormat and always pass it, and just not use it if not needed.
| private static bool IsSupportedEncodedArrayElementType(CSharpType elementType) | ||
| { | ||
| // Support string arrays | ||
| if (elementType.IsFrameworkType && elementType.FrameworkType == typeof(string)) |
There was a problem hiding this comment.
nit: we try to use braces even for single line if statements.
| private MethodBodyStatement CreateEncodedArraySerializationStatement( | ||
| CSharpType propertyType, | ||
| ValueExpression propertyExpression, | ||
| SerializationFormat encoding) |
There was a problem hiding this comment.
nit: we call this parameter serializationFormat in other places in this class so it would be good to be consistent with that.
|
|
||
| var generatedCode = file.Content; | ||
|
|
||
| Assert.IsTrue(generatedCode.Contains("writer.WriteStringValue(item.ToSerialString())")); |
There was a problem hiding this comment.
what exactly is this testing? Also, any reason why we put this test in the ModelCustomizationTests suite?
3 participants
This pull request adds support for custom array encoding formats in the C# HTTP client generator, allowing arrays to be serialized and deserialized using delimiters such as comma, space, pipe, or newline. The changes span the model, serialization logic, type conversion, and test coverage to ensure correct handling of these encodings.
Array encoding support
encodeproperty toInputModelPropertyand related types to specify the desired array encoding (e.g.,commaDelimited,spaceDelimited, etc.). This is reflected in the type definitions, model constructors, and JSON serialization/deserialization logic.type-converter.ts) to propagate theencodeproperty from SDK model properties into the input model.Serialization and deserialization logic
MrwSerializationTypeDefinition.csto handle arrays with specified encodings. These methods join or split array elements using the appropriate delimiter and handle both string and primitive element types.Test coverage
EncodeArrayTests.csto verify correct serialization and deserialization for each supported encoding format (comma, space, pipe, newline).Implements: #9028