-
Notifications
You must be signed in to change notification settings - Fork 80
Implement array encoding for model properties #3659
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
Changes from 14 commits
271493e
f49d53b
eca307d
4e7d9ee
0e64fa8
c03d5e1
e2ae2e8
1c6edd3
780f0fc
4c21083
37640c2
4c74720
64d8b09
549110e
2a36630
9b102d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,10 @@ import { | |
| getCollectionFormatHelper, | ||
| hasCollectionFormatInfo, | ||
| isBinaryPayload, | ||
| ServiceOperation | ||
| ServiceOperation, | ||
| getCollectionFormatParseHelper, | ||
| getCollectionFormatFromArrayEncoding, | ||
| KnownCollectionFormat | ||
| } from "../../utils/operationUtil.js"; | ||
| import { | ||
| getPropertyWithOverrides, | ||
|
|
@@ -245,6 +248,7 @@ export function getDeserializePrivateFunction( | |
| context, | ||
| deserializedType, | ||
| deserializedRoot, | ||
| true, | ||
| isBinaryPayload(context, response.type!.__raw!, contentTypes!) | ||
| ? "binary" | ||
| : getEncodeForType(deserializedType) | ||
|
|
@@ -856,6 +860,7 @@ function buildBodyParameter( | |
| isBinaryPayload(context, bodyParameter.__raw!, bodyParameter.contentTypes) | ||
| ? "binary" | ||
| : getEncodeForType(bodyParameter.type), | ||
| undefined, | ||
| true | ||
| ); | ||
| return `\nbody: ${serializedBody.startsWith(nullOrUndefinedPrefix) ? "" : nullOrUndefinedPrefix}${serializedBody},`; | ||
|
|
@@ -884,7 +889,7 @@ export function getParameterMap( | |
| } | ||
|
|
||
| if (hasCollectionFormatInfo(param.kind, (param as any).collectionFormat)) { | ||
| return getCollectionFormat(context, param, optionalParamName); | ||
| return getCollectionFormatForParam(context, param, optionalParamName); | ||
| } | ||
|
|
||
| // if the parameter or property is optional, we don't need to handle the default value | ||
|
|
@@ -909,39 +914,22 @@ export function getParameterMap( | |
| return `"${param.name}": undefined`; | ||
| } | ||
|
|
||
| function getCollectionFormat( | ||
| function getCollectionFormatForParam( | ||
| context: SdkContext, | ||
| param: SdkHttpParameter, | ||
| optionalParamName: string = "options" | ||
| ) { | ||
| const serializedName = getPropertySerializedName(param); | ||
| const format = (param as any).collectionFormat; | ||
| const collectionInfo = getCollectionFormatHelper(param.kind, format ?? ""); | ||
| if (!collectionInfo) { | ||
| throw "Has collection format info but without helper function detected"; | ||
| } | ||
| const isMulti = format.toLowerCase() === "multi"; | ||
| const additionalParam = isMulti ? `, "${serializedName}"` : ""; | ||
| if (!param.optional) { | ||
| return `"${serializedName}": ${collectionInfo}(${serializeRequestValue( | ||
| context, | ||
| param.type, | ||
| param.name, | ||
| true, | ||
| getEncodeForType(param.type), | ||
| true | ||
| )}${additionalParam})`; | ||
| } | ||
| return `"${serializedName}": ${optionalParamName}?.${ | ||
| param.name | ||
| } !== undefined ? ${collectionInfo}(${serializeRequestValue( | ||
| return `"${serializedName}": ${serializeRequestValue( | ||
| context, | ||
| param.type, | ||
| `${optionalParamName}?.${param.name}`, | ||
| false, | ||
| getEncodeForType(param.type), | ||
| param.optional ? `${optionalParamName}?.${param.name}` : param.name, | ||
| !param.optional, | ||
| format, | ||
| serializedName, | ||
| true | ||
| )}${additionalParam}): undefined`; | ||
| )}`; | ||
| } | ||
|
|
||
| function isContentType(param: SdkHttpParameter): boolean { | ||
|
|
@@ -992,6 +980,7 @@ function getRequired(context: SdkContext, param: SdkHttpParameter) { | |
| clientValue, | ||
| true, | ||
| getEncodeForType(param.type), | ||
| serializedName, | ||
| true | ||
| )}`; | ||
| } | ||
|
|
@@ -1033,6 +1022,7 @@ function getOptional( | |
| paramName, | ||
| false, | ||
| getEncodeForType(param.type), | ||
| serializedName, | ||
| true | ||
| )}`; | ||
| } | ||
|
|
@@ -1172,6 +1162,41 @@ function getNullableCheck(name: string, type: SdkType) { | |
| return `${name} === null ? null :`; | ||
| } | ||
|
|
||
| /** | ||
| * Determines the appropriate encoding format for a model property, especially for arrays with collection format encoding. | ||
| * For example, returns "csv" for comma-delimited arrays or the property's type encoding for regular properties. | ||
| */ | ||
| function getEncodeForModelProperty( | ||
| context: SdkContext, | ||
| property: SdkModelPropertyType | ||
| ): string | undefined { | ||
| if (property.encode && property.type.kind === "array") { | ||
| // Only arrays of string type can have collectionFormat encoding | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where do we get the agreement on only supporting string?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found the TypeSpec issue mentioned maybe only |
||
| if (property.type.valueType.kind !== "string") { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i wonder if the string-like type is supported like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great question. I tested it, and it's not supported since it's an array of enum type... |
||
| reportDiagnostic(context.program, { | ||
| code: "un-supported-array-encoding", | ||
| format: { | ||
| arrayName: property.name, | ||
| arrayType: property.type.valueType.kind | ||
| }, | ||
| target: NoTarget | ||
| }); | ||
| return getEncodeForType(property.type); | ||
| } | ||
|
|
||
| const collectionFormat = getCollectionFormatFromArrayEncoding( | ||
| property.encode | ||
| ); | ||
| if ( | ||
| collectionFormat && | ||
| hasCollectionFormatInfo(property.kind, collectionFormat) | ||
| ) { | ||
| return collectionFormat; | ||
| } | ||
| } | ||
| return getEncodeForType(property.type); | ||
| } | ||
|
|
||
| function getSerializationExpressionForFlatten( | ||
| context: SdkContext, | ||
| property: SdkModelPropertyType, | ||
|
|
@@ -1245,7 +1270,8 @@ export function getSerializationExpression( | |
| property.type, | ||
| propertyFullName, | ||
| !property.optional, | ||
| getEncodeForType(property.type), | ||
| getEncodeForModelProperty(context, property), | ||
| getPropertySerializedName(property), | ||
| propertyPath === "" ? true : false | ||
| ); | ||
| } | ||
|
|
@@ -1373,27 +1399,25 @@ export function getResponseMapping( | |
| context, | ||
| property.type, | ||
| `${propertyPath}${dot}["${serializedName}"]`, | ||
| getEncodeForType(property.type) | ||
| ); | ||
| props.push( | ||
| `${propertyName}: ${deserializeValue === `${propertyPath}${dot}["${serializedName}"]` ? "" : nullOrUndefinedPrefix}${deserializeValue}` | ||
| !property.optional, | ||
| getEncodeForModelProperty(context, property) | ||
| ); | ||
| props.push(`${propertyName}: ${deserializeValue}`); | ||
| } | ||
| } | ||
| return props; | ||
| } | ||
|
|
||
| /** | ||
| * This function helps converting strings into JS complex types recursively. | ||
| * We need to drill down into Array elements to make sure that the element type is | ||
| * deserialized correctly | ||
| * Converts JavaScript values to their serialized wire format for HTTP requests. | ||
| */ | ||
| export function serializeRequestValue( | ||
| context: SdkContext, | ||
| type: SdkType, | ||
| clientValue: string, | ||
| required: boolean, | ||
| format?: string, | ||
| serializedName?: string, | ||
| isTopLevel: boolean = false | ||
| ): string { | ||
| const getSdkType = useSdkTypes(); | ||
|
|
@@ -1414,8 +1438,8 @@ export function serializeRequestValue( | |
| return `${nullOrUndefinedPrefix}${clientValue}.toISOString()`; | ||
| } | ||
| case "array": { | ||
| const prefix = nullOrUndefinedPrefix + clientValue; | ||
| if (type.valueType) { | ||
| const prefix = nullOrUndefinedPrefix + clientValue; | ||
| const elementNullOrUndefinedPrefix = | ||
| isTypeNullable(type.valueType) || getOptionalForType(type.valueType) | ||
| ? "!p ? p : " | ||
|
|
@@ -1435,7 +1459,17 @@ export function serializeRequestValue( | |
| ) { | ||
| return `${prefix}.map((p: any) => { return ${elementNullOrUndefinedPrefix}p})`; | ||
| } else { | ||
| return `${prefix}.map((p: any) => { return ${elementNullOrUndefinedPrefix}${serializeRequestValue(context, type.valueType, "p", true, getEncodeForType(type.valueType))}})`; | ||
| const serializedValue = `${clientValue}.map((p: any) => { return ${elementNullOrUndefinedPrefix}${serializeRequestValue(context, type.valueType, "p", true, getEncodeForType(type.valueType))}})`; | ||
| if (format) { | ||
| const formatHelper = getCollectionFormatHelper(format); | ||
| if (formatHelper) { | ||
| if (format?.toLowerCase() === KnownCollectionFormat.Multi) { | ||
| return `${nullOrUndefinedPrefix}${formatHelper}(${serializedValue}, "${serializedName}")`; | ||
| } | ||
| return `${nullOrUndefinedPrefix}${formatHelper}(${serializedValue})`; | ||
| } | ||
| } | ||
| return `${nullOrUndefinedPrefix}${serializedValue}`; | ||
| } | ||
| } | ||
| return clientValue; | ||
|
|
@@ -1507,14 +1541,15 @@ export function deserializeResponseValue( | |
| context: SdkContext, | ||
| type: SdkType, | ||
| restValue: string, | ||
| required: boolean, | ||
| format?: string | ||
| ): string { | ||
| const dependencies = useDependencies(); | ||
| const stringToUint8ArrayReference = resolveReference( | ||
| dependencies.stringToUint8Array | ||
| ); | ||
| const nullOrUndefinedPrefix = | ||
| isTypeNullable(type) || getOptionalForType(type) | ||
| isTypeNullable(type) || getOptionalForType(type) || !required | ||
|
qiaozha marked this conversation as resolved.
|
||
| ? `!${restValue}? ${restValue}: ` | ||
| : ""; | ||
| switch (type.kind) { | ||
|
|
@@ -1547,13 +1582,25 @@ export function deserializeResponseValue( | |
| ) { | ||
| return `${prefix}.map((p: any) => { return ${elementNullOrUndefinedPrefix}p})`; | ||
| } else if (type.valueType) { | ||
| return `${prefix}.map((p: any) => { return ${elementNullOrUndefinedPrefix}${deserializeResponseValue(context, type.valueType, "p", getEncodeForType(type.valueType))}})`; | ||
| if (format) { | ||
| const parseHelper = getCollectionFormatParseHelper(format); | ||
| if (parseHelper) { | ||
| // We shouldn't check for an empty string here since an empty string should be parsed as an empty array | ||
| const optionalPrefixForString = | ||
| isTypeNullable(type) || getOptionalForType(type) || !required | ||
| ? `${restValue} === null || ${restValue} === undefined ? ${restValue}: ` | ||
| : ""; | ||
| return `${optionalPrefixForString}${parseHelper}(${restValue})`; | ||
| } else { | ||
| throw `No supported collection format parse helper found for ${format}.`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. throw exeptions? does that mean the emitter will crash?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception is added here because the original getCollectionFormat method throws an exception if I remove the logic from that method here. It's fine if we don't want to throw exceptions here.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember we have some earlier discussion that emitters should not throw error (crash) on purpose instead we should reportDiagnostic #3064
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the exception.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange, the code diff doesn't show as removed. Also, is possible to report diagnostic warning? |
||
| } | ||
| } | ||
| return `${prefix}.map((p: any) => { return ${elementNullOrUndefinedPrefix}${deserializeResponseValue(context, type.valueType, "p", true, getEncodeForType(type.valueType))}})`; | ||
| } else { | ||
| return restValue; | ||
| } | ||
| } | ||
| case "dict": { | ||
| const prefix = nullOrUndefinedPrefix + restValue; | ||
| let elementNullOrUndefinedPrefix = ""; | ||
| if ( | ||
| type.valueType && | ||
|
|
@@ -1572,21 +1619,21 @@ export function deserializeResponseValue( | |
| ) | ||
| : undefined; | ||
| if (deserializeFunctionName) { | ||
| return `Object.fromEntries(Object.entries(${prefix}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}${deserializeFunctionName}(p)]))`; | ||
| return `${nullOrUndefinedPrefix}Object.fromEntries(Object.entries(${restValue}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}${deserializeFunctionName}(p)]))`; | ||
| } else if ( | ||
| type.valueType && | ||
| isAzureCoreErrorType(context.program, type.valueType.__raw) | ||
| ) { | ||
| return `Object.fromEntries(Object.entries(${prefix}).map(([k, p]: [string, any]) => [k, ${elementNullOrUndefinedPrefix}p]))`; | ||
| 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))}]))`; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to keep the behavior consistent with before. In addition, whether the value |
||
| } else { | ||
| return restValue; | ||
| } | ||
| } | ||
| case "bytes": | ||
| if (format !== "binary" && format !== "bytes") { | ||
| return `typeof ${restValue} === 'string' | ||
| return `${nullOrUndefinedPrefix}typeof ${restValue} === 'string' | ||
| ? ${stringToUint8ArrayReference}(${restValue}, "${format ?? "base64"}") | ||
| : ${restValue}`; | ||
| } | ||
|
|
@@ -1616,6 +1663,7 @@ export function deserializeResponseValue( | |
| context, | ||
| type.type, | ||
| restValue, | ||
| false, | ||
| getEncodeForType(type.type) | ||
| ); | ||
| default: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have an UT for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UT added.