Use reshape to simplify deserialization #2180
Conversation
|
Very nice! I love that we're removing more lines than we're adding in this PR. I assume we could do a similar change for serializing input shapes from the customer? |
| }; | ||
| let deserializedResponse: unknown = result.body; | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].delta.session_state", | ||
| "sessionState" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].session_state", |
There was a problem hiding this comment.
@xirzec If we could support jsonpath, I think we could somehow use $..session_state to be more simpler ?
| })), | ||
| }; | ||
| let deserializedResponse: unknown = result.body; | ||
| return deserializedResponse as AnalyzeTextResult; |
There was a problem hiding this comment.
why not return result.body directly ?
There was a problem hiding this comment.
Yup I'm pushing that change next.
| let deserializedResponse: unknown = result.body; | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "value[].event.data_base64", | ||
| "dataBase64" | ||
| ); |
There was a problem hiding this comment.
I feel like this is incorrect ? because we still need to convert it into uint8Array ?
There was a problem hiding this comment.
Yes, still need to handle the conversion. I let it out temporarily b/c I wanted to double check if we want to keep the current approach or need to update
| let deserializedResponse: unknown = result.body; | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "created", | ||
| (value) => new Date(value as string) | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].message.function_call", | ||
| "functionCall" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].finish_reason", | ||
| "finishReason" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].content_filter_results", | ||
| "contentFilterResults" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "choices[].content_filter_results.self_harm", | ||
| "selfHarm" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "prompt_annotations", | ||
| "promptFilterResults" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "prompt_annotations[].prompt_index", | ||
| "promptIndex" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "prompt_annotations[].content_filter_results", | ||
| "contentFilterResults" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "usage.completion_tokens", | ||
| "completionTokens" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "usage.prompt_tokens", | ||
| "promptTokens" | ||
| ); | ||
| deserializedResponse = reshape( | ||
| deserializedResponse, | ||
| "usage.total_tokens", | ||
| "totalTokens" | ||
| ); | ||
| return deserializedResponse as ChatCompletions; |
There was a problem hiding this comment.
| id: result.body["id"], | ||
| created: new Date(result.body["created"]), | ||
| choices: result.body["choices"].map((p) => ({ | ||
| message: !p.message ? undefined : (p.message as any), |
There was a problem hiding this comment.
If the recursive properties don't need to be reshaped it should be fine, as we'll be returning the same as we got from the service. True, this doesn't handle recursive properties that need to be transformed.
| statements.push(`let deserializedResponse: unknown = result.body;`); | ||
|
|
||
| deserialize(responseProperties, statements, visitedTypes); | ||
| statements.push(`return deserializedResponse as ${resultType};`); |
There was a problem hiding this comment.
just a little curious, why we need as here ?
There was a problem hiding this comment.
Because the reshape function returns Unknown I was thinking to suggest adding some type magic to the function so that it could actually returns the type we need, however as I started trying it out it became too complex and likely not worth investing in it and maintaining for the internal added type-safety
There was a problem hiding this comment.
I'm fine changing reshape to just deal in any if that saves you effort :)
| } else if ( | ||
| (property.restApiName === "message" || | ||
| property.restApiName === "messages") && | ||
| (property.type.name === "ChatMessage" || | ||
| property.type.elementType?.name === "ChatMessage") | ||
| ) { | ||
| definition = `"${property.restApiName}": ${ | ||
| !property.optional | ||
| ? `${propertyFullName} as any` | ||
| : `!${propertyFullName} ? undefined : ${propertyFullName} as any` | ||
| }`; |
There was a problem hiding this comment.
This is the workaround for recursive reference in codegen.
There was a problem hiding this comment.
Yeah I wanted to try this out without the workaround, since the properties in message don't need to be reshaped I think we should be fine without it
| switch (type.type) { | ||
| case "datetime": | ||
| return "(value) => new Date(value as string)"; | ||
| default: |
There was a problem hiding this comment.
missing bytes ? and datetime serialization is another case
There was a problem hiding this comment.
yes, I'll add both of them
Integrate the new reshape function from core-util to simplify serialization and deserialization.
reshape is WIP in Azure/azure-sdk-for-js#28095