Implement array encoding for model properties#3659
Implement array encoding for model properties#3659JialinHuang803 merged 16 commits intoAzure:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive support for array encoding decorators (@encode(ArrayEncoding.*)) for TypeSpec model properties, extending functionality that previously existed only for query and header parameters. The implementation adds support for four delimiter formats: comma-delimited (CSV), space-delimited (SSV), pipe-delimited, and newline-delimited for string array properties.
Key Changes:
- Added parsing helpers for CSV, SSV, pipe, and newline-delimited formats to convert encoded strings back to arrays
- Added a build helper for newline-delimited format to convert arrays to encoded strings
- Refactored collection format handling logic to support model properties in addition to parameters
- Introduced a new diagnostic warning for unsupported array encodings (non-string array types)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/typespec-ts/static/static-helpers/serialization/parse-csv-collection.ts |
New helper to parse comma-delimited strings into string arrays |
packages/typespec-ts/static/static-helpers/serialization/parse-ssv-collection.ts |
New helper to parse space-delimited strings into string arrays |
packages/typespec-ts/static/static-helpers/serialization/parse-pipe-collection.ts |
New helper to parse pipe-delimited strings into string arrays |
packages/typespec-ts/static/static-helpers/serialization/parse-newline-collection.ts |
New helper to parse newline-delimited strings into string arrays |
packages/typespec-ts/static/static-helpers/serialization/build-newline-collection.ts |
New helper to build newline-delimited strings from arrays |
packages/typespec-ts/src/utils/operationUtil.ts |
Refactored collection format handling to support properties; added enum for known collection formats and helper functions |
packages/typespec-ts/src/modular/static-helpers-metadata.ts |
Registered new parse and build helpers for collection formats |
packages/typespec-ts/src/modular/helpers/operationHelpers.ts |
Updated serialization and deserialization logic to handle array encoding for model properties |
packages/typespec-ts/src/lib.ts |
Added new diagnostic warning for unsupported array encoding types |
packages/typespec-ts/test/modularUnit/scenarios/models/serialization/modelPropertyArrayEncoding.md |
New test scenarios demonstrating array encoding for model properties |
packages/typespec-ts/test/modularUnit/scenarios/models/deserialization/propertyType.md |
Fixed typo in property name from "propStringUnionOptioanl" to "propStringUnionOptional" |
packages/typespec-ts/test/modularUnit/scenarios/models/propertyFlatten/nameCollision.md |
Formatting updates adding trailing commas for consistency |
| property: SdkModelPropertyType | ||
| ): string | undefined { | ||
| if (property.encode && property.type.kind === "array") { | ||
| // Only arrays of string type can have collectionFormat encoding |
There was a problem hiding this comment.
where do we get the agreement on only supporting string?
There was a problem hiding this comment.
I found the TypeSpec issue mentioned maybe only string[] is supported. I think we can support only string[] for now and consider adding other primitive types if requested by other service teams.
| default: paramMessage`Model name conflict detected: "${"modelName"}" exists in multiple namespaces: ${"namespaces"}. Please use @clientName to rename them.` | ||
| } | ||
| }, | ||
| "un-supported-array-encoding": { |
| return `${nullOrUndefinedPrefix}Object.fromEntries(Object.entries(${restValue}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}p]))`; | ||
| } else if (type.valueType) { | ||
| return `Object.fromEntries(Object.entries(${prefix}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}${deserializeResponseValue(context, type.valueType, "p", getEncodeForType(type.valueType))}]))`; | ||
| return `${nullOrUndefinedPrefix}Object.fromEntries(Object.entries(${restValue}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}${deserializeResponseValue(context, type.valueType, "p", true, getEncodeForType(type.valueType))}]))`; |
There was a problem hiding this comment.
why the requred value is fixed as true here? not devired from other values like prop.optional?
There was a problem hiding this comment.
This is to keep the behavior consistent with before. In addition, whether the value p should have optional prefix is decided by elementNullOrUndefinedPrefix.
| : ""; | ||
| return `${optionalPrefixForString}${parseHelper}(${restValue})`; | ||
| } else { | ||
| throw `No supported collection format parse helper found for ${format}.`; |
There was a problem hiding this comment.
throw exeptions? does that mean the emitter will crash?
There was a problem hiding this comment.
The exception is added here because the original getCollectionFormat method throws an exception if getCollectionFormatHelper doesn't return a helper:
const collectionInfo = getCollectionFormatHelper(param.kind, format ?? "");
if (!collectionInfo) {
throw "Has collection format info but without helper function detected";
}
I remove the logic from that method here. It's fine if we don't want to throw exceptions here.
There was a problem hiding this comment.
I remember we have some earlier discussion that emitters should not throw error (crash) on purpose instead we should reportDiagnostic #3064
There was a problem hiding this comment.
Removed the exception.
There was a problem hiding this comment.
Strange, the code diff doesn't show as removed. Also, is possible to report diagnostic warning?
| requiredCsvColors: parseCsvCollection(item["requiredCsvColors"]), | ||
| requiredPipeColors: parsePipeCollection(item["requiredPipeColors"]), | ||
| optionalSsvColors: | ||
| item["optionalSsvColors"] == null |
There was a problem hiding this comment.
ususally we don't recommand to use "==" check, instead use "===" or more explicit expression.
There was a problem hiding this comment.
Ok, I will update it to item["optionalSsvColors"] === null || item["optionalSsvColors"] === undefined.
| @@ -0,0 +1,3 @@ | |||
| export function parseCsvCollection(value: string): string[] { | |||
| return value ? value.split(",") : []; | |||
There was a problem hiding this comment.
no need for the ? check since this is required?
There was a problem hiding this comment.
The value ? here is to ensure [] is returned if the input value is "". If return value.split(",") directly, an array containing an empty string [""] will be returned which is not expected.
| ): string | undefined { | ||
| if (property.encode && property.type.kind === "array") { | ||
| // Only arrays of string type can have collectionFormat encoding | ||
| if (property.type.valueType.kind !== "string") { |
There was a problem hiding this comment.
i wonder if the string-like type is supported like ("a" | "b")[]?
There was a problem hiding this comment.
Great question. I tested it, and it's not supported since it's an array of enum type...
| case "commaDelimited": | ||
| return KnownCollectionFormat.Csv; | ||
| case "newlineDelimited": | ||
| return KnownCollectionFormat.Newline; |
There was a problem hiding this comment.
No KnownCollectionFormat.Multi for model property?
There was a problem hiding this comment.
No. ArrayEncoding only includes "pipeDelimited" | "spaceDelimited" | "commaDelimited" | "newlineDelimited".
| : ""; | ||
| return `${optionalPrefixForString}${parseHelper}(${restValue})`; | ||
| } else { | ||
| throw `No supported collection format parse helper found for ${format}.`; |
There was a problem hiding this comment.
I remember we have some earlier discussion that emitters should not throw error (crash) on purpose instead we should reportDiagnostic #3064
qiaozha
left a comment
There was a problem hiding this comment.
just a minor comment left, feel free to merge if it's resolved.
| : ""; | ||
| return `${optionalPrefixForString}${parseHelper}(${restValue})`; | ||
| } else { | ||
| throw `No supported collection format parse helper found for ${format}.`; |
There was a problem hiding this comment.
Strange, the code diff doesn't show as removed. Also, is possible to report diagnostic warning?
Resolve: #3604
Overview
This PR implements support for array encoding decorators @encode(ArrayEncoding.*) within TypeSpec model properties. Previously, array encoding was only supported for query and header parameters, but now it extends to
model propertiesas well, enabling proper serialization and deserialization of array fields with different delimited formats. Currently only array ofstringtype is supported for encoding.What's included
Support 4 delimiters for encoding model properties of string array type, including
',',' ','|', and'\n'.un-supported-array-encoding) that triggers when an array of an unsupported type is marked for encoding.For example:
Serialization
string[] -> string, an empty array will be serialized as
"".Deserialization
string -> string[], an empty string "" will be deserialized as an empty array.
Impact on existing generated code
The generated code related to the collection format query and header parameter got changed if the parameter is optional, but its behavior should remain unchanged.
For example,
TypeSpec:
Before:
After: