Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/typespec-ts/src/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,12 @@ const libDef = {
messages: {
default: paramMessage`Model name conflict detected: "${"modelName"}" exists in multiple namespaces: ${"namespaces"}. Please use @clientName to rename them.`
}
},
"un-supported-array-encoding": {
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

UT added.

severity: "warning",
messages: {
default: paramMessage`The array property "${"arrayName"}" of ${"arrayType"} type is not supported for encoding and will be ignored.`
}
}
},
emitter: {
Expand Down
135 changes: 92 additions & 43 deletions packages/typespec-ts/src/modular/helpers/operationHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ import {
getCollectionFormatHelper,
hasCollectionFormatInfo,
isBinaryPayload,
ServiceOperation
ServiceOperation,
getCollectionFormatParseHelper,
getCollectionFormatFromArrayEncoding
} from "../../utils/operationUtil.js";
import {
getPropertyWithOverrides,
Expand Down Expand Up @@ -245,6 +247,7 @@ export function getDeserializePrivateFunction(
context,
deserializedType,
deserializedRoot,
true,
isBinaryPayload(context, response.type!.__raw!, contentTypes!)
? "binary"
: getEncodeForType(deserializedType)
Expand Down Expand Up @@ -856,6 +859,7 @@ function buildBodyParameter(
isBinaryPayload(context, bodyParameter.__raw!, bodyParameter.contentTypes)
? "binary"
: getEncodeForType(bodyParameter.type),
undefined,
true
);
return `\nbody: ${serializedBody.startsWith(nullOrUndefinedPrefix) ? "" : nullOrUndefinedPrefix}${serializedBody},`;
Expand Down Expand Up @@ -884,7 +888,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
Expand All @@ -909,39 +913,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 {
Expand Down Expand Up @@ -992,6 +979,7 @@ function getRequired(context: SdkContext, param: SdkHttpParameter) {
clientValue,
true,
getEncodeForType(param.type),
serializedName,
true
)}`;
}
Expand Down Expand Up @@ -1033,6 +1021,7 @@ function getOptional(
paramName,
false,
getEncodeForType(param.type),
serializedName,
true
)}`;
}
Expand Down Expand Up @@ -1172,6 +1161,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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

where do we get the agreement on only supporting string?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

if (property.type.valueType.kind !== "string") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i wonder if the string-like type is supported like ("a" | "b")[]?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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,
Expand Down Expand Up @@ -1245,7 +1269,8 @@ export function getSerializationExpression(
property.type,
propertyFullName,
!property.optional,
getEncodeForType(property.type),
getEncodeForModelProperty(context, property),
getPropertySerializedName(property),
propertyPath === "" ? true : false
);
}
Expand Down Expand Up @@ -1373,27 +1398,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();
Expand All @@ -1414,8 +1437,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 : "
Expand All @@ -1435,7 +1458,19 @@ 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() === "multi") {
return `${nullOrUndefinedPrefix}${formatHelper}(${serializedValue}, "${serializedName}")`;
}
return `${nullOrUndefinedPrefix}${formatHelper}(${serializedValue})`;
} else {
throw `No supported collection format helper found for ${format}.`;
}
}
return `${nullOrUndefinedPrefix}${serializedValue}`;
}
}
return clientValue;
Expand Down Expand Up @@ -1507,14 +1542,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
Comment thread
qiaozha marked this conversation as resolved.
? `!${restValue}? ${restValue}: `
: "";
switch (type.kind) {
Expand Down Expand Up @@ -1547,13 +1583,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}.`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

throw exeptions? does that mean the emitter will crash?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed the exception.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 &&
Expand All @@ -1572,21 +1620,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))}]))`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why the requred value is fixed as true here? not devired from other values like prop.optional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The 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 p should have optional prefix is decided by elementNullOrUndefinedPrefix.

} else {
return restValue;
}
}
case "bytes":
if (format !== "binary" && format !== "bytes") {
return `typeof ${restValue} === 'string'
return `${nullOrUndefinedPrefix}typeof ${restValue} === 'string'
? ${stringToUint8ArrayReference}(${restValue}, "${format ?? "base64"}")
: ${restValue}`;
}
Expand Down Expand Up @@ -1616,6 +1664,7 @@ export function deserializeResponseValue(
context,
type.type,
restValue,
false,
getEncodeForType(type.type)
);
default:
Expand Down
25 changes: 25 additions & 0 deletions packages/typespec-ts/src/modular/static-helpers-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,31 @@ export const SerializationHelpers = {
name: "buildTsvCollection",
location: "serialization/build-tsv-collection.ts"
},
buildNewlineCollection: {
kind: "function",
name: "buildNewlineCollection",
location: "serialization/build-newline-collection.ts"
},
parseCsvCollection: {
kind: "function",
name: "parseCsvCollection",
location: "serialization/parse-csv-collection.ts"
},
parsePipeCollection: {
kind: "function",
name: "parsePipeCollection",
location: "serialization/parse-pipe-collection.ts"
},
parseSsvCollection: {
kind: "function",
name: "parseSsvCollection",
location: "serialization/parse-ssv-collection.ts"
},
parseNewlineCollection: {
kind: "function",
name: "parseNewlineCollection",
location: "serialization/parse-newline-collection.ts"
},
serializeRecord: {
kind: "function",
name: "serializeRecord",
Expand Down
Loading
Loading