Skip to content

Conversation

@radhgupta
Copy link
Member

@radhgupta radhgupta commented Jan 21, 2026

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

  • Introduced an optional encode property to InputModelProperty and 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.
  • Updated the type converter (type-converter.ts) to propagate the encode property from SDK model properties into the input model.

Serialization and deserialization logic

  • Added custom serialization and deserialization methods in MrwSerializationTypeDefinition.cs to 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

  • Added new tests in EncodeArrayTests.cs to verify correct serialization and deserialization for each supported encoding format (comma, space, pipe, newline).

Implements: #9028

@microsoft-github-policy-service microsoft-github-policy-service bot added the emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp label Jan 21, 2026
@github-actions
Copy link
Contributor

No changes needing a change description found.

IsDiscriminator = isDiscriminator;
IsHttpMetadata = isHttpMetadata;
SerializationOptions = serializationOptions;
Encode = encode;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
};

Copy link
Contributor

@JoshLove-msft JoshLove-msft Jan 29, 2026

Choose a reason for hiding this comment

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

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.

@radhgupta radhgupta marked this pull request as ready for review January 26, 2026 17:35
Copy link
Contributor

@jorgerangel-msft jorgerangel-msft left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jorgerangel-msft jorgerangel-msft left a comment

Choose a reason for hiding this comment

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

could we please add unit tests to see the generated code?

useCustomDeserializationHook = true;
break;
[
DeserializeValue(propertyType, jsonProperty.Value(), serializationFormat, out ValueExpression value),
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

we already know the type is a collection by this point, could we simply do propertyType.ElementType?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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}");
Copy link
Contributor

Choose a reason for hiding this comment

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

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

could we possibly use the ToSerial extension method instead?


if (isStringElement)
{
var splitResult = stringValueVar.Invoke("Split", delimiterChar);
Copy link
Contributor

Choose a reason for hiding this comment

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

(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)
Copy link
Contributor

@JoshLove-msft JoshLove-msft Jan 29, 2026

Choose a reason for hiding this comment

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

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))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we try to use braces even for single line if statements.

private MethodBodyStatement CreateEncodedArraySerializationStatement(
CSharpType propertyType,
ValueExpression propertyExpression,
SerializationFormat encoding)
Copy link
Contributor

Choose a reason for hiding this comment

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

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())"));
Copy link
Contributor

Choose a reason for hiding this comment

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

what exactly is this testing? Also, any reason why we put this test in the ModelCustomizationTests suite?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

emitter:client:csharp Issue for the C# client emitter: @typespec/http-client-csharp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants