-
Notifications
You must be signed in to change notification settings - Fork 75
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
Modular: Encapsulate serializers into functions #2613
Conversation
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.
Thanks for taking for many issues together, but I don't think #2506 is being resolved by this PR as we didn't remove the compatibilityMode
...s/typespec-test/test/hierarchy_generic/generated/typespec-ts/review/hierarchy-generic.api.md
Outdated
Show resolved
Hide resolved
@@ -217,7 +221,8 @@ export function getDeserializePrivateFunction( | |||
allParents.some((p) => p.type === "dict")) || | |||
response.isBinaryPayload | |||
) { | |||
statements.push(`return result.body`); | |||
// TODO: Fix this any cast when implementing handling dict. | |||
statements.push(`return result.body as any`); |
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.
Is this temporary?
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.
This will be addressed in follow up work tracked by: #2619
packages/typespec-ts/test/modularIntegration/generated/azure/core/basic/src/models/options.ts
Outdated
Show resolved
Hide resolved
...espec-ts/test/modularIntegration/generated/azure/core/lro/rpc/generated/src/models/models.ts
Show resolved
Hide resolved
@@ -68,7 +68,7 @@ export async function _responseBodyOctetStreamDeserialize( | |||
throw createRestError(result); | |||
} | |||
|
|||
return result.body; | |||
return result.body as any; |
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.
In this case, I think the result.body type is Uint8Array right?
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.
This will be addressed in follow up work tracked by: #2619
...dularIntegration/generated/type/model/inheritance/nested-discriminator/src/api/operations.ts
Show resolved
Hide resolved
}); | ||
|
||
// Fix deserialization | ||
it.skip("should get a dictionary of datetime", async () => { |
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.
we don't support deserialization for dictionary now?
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.
I'll revisit this, but this test is giving us the date as string. Probably an issue specific to Record of datetime. I'll see if the fix is scoped I'll fit it in this PR, otherwise I'll address in a follow up
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.
This will be addressed in follow up work tracked by: #2619
acc[key] = item[key] as any; | ||
} else if (serializer) { | ||
const value = item[key]; | ||
if (value !== undefined) { |
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.
I think we had a discussion about undefined properties that essentially boiled down to the decision to ensure that serialization of a model type doesn't change the output of Object.keys
(except for renames). Do we want record types to have the same behavior?
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.
Yeah I think so, I thought this would keep all existing keys. Am I missing something?
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.
In this case, the else if block won't add a field for undefined values. See this suggestion.
) { | ||
const propertyFullName = getPropertyFullName(modularType, propertyPath); | ||
if (modularType.optional || modularType.type.nullable) { | ||
return `!${propertyFullName} ? ${propertyFullName} :`; |
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.
Should this be an explicit check for null/undefined? To account for serializing falsy values?
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.
I think we'll have to do an analysis of all the places where we deal with properties this way to make sure we are null checking consistently and correctly accross the board. Maybe come up with helpers that make it easier for us. Tracking with #2619
): string | undefined { | ||
if ( | ||
(type.tcgcType as SdkModelType).usage !== undefined && | ||
((type.tcgcType as SdkModelType).usage & UsageFlags.Input) !== |
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.
I think TCGC and the compiler have distinct UsageFlags
enums. They might have some kind of regression test to make sure the values for Input
and Output
stay in sync, or it might extend from the compiler enum directly. But it might be a good idea to consume the type from TCGC here just to be safe.
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.
Yeah I think we should revisit all the places where we use these flags and make sure we are consistent. Tracking follow up work with: #2619
} | ||
|
||
if (!type.name) { | ||
throw new Error(`NYI Serialization of anonymous types`); |
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.
TCGC generates unique names for anonymous models, so it shouldn't be very expensive to use those if/when we want to handle 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.
Agreed we discuss this, added an item to this issue for tracking: #2619
}); | ||
|
||
// This is only handling the compatibility mode, will need to update when we handle additionalProperties property. | ||
const additionalPropertiesSpread = hasAdditionalProperties(type.tcgcType) |
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.
I think when we discussed model serialization a few months ago, we landed on passing all extra properties through. Would this also work for that?
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.
I think I'm handling this with spreading the "unknown" properties. I also added an item to revisit how we approach this, tracking with #2619
interface RequestModelMappingResult { | ||
propertiesStr: string[]; | ||
directAssignment?: boolean; | ||
} | ||
export function getRequestModelMapping( |
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.
When we were discussing serializing model types, I think we landed on preserving extra properties, such that Object.keys
is the same between the input and output. There's an exception with renamed properties, where the new name should replace the old name in Object.keys if and only if the new name exists. This sample should have that behavior:
interface Foo {
fooProp?: FooProp;
bazProp?: BazProp;
}
interface RestFoo {
barProp?: RestFooProp;
bazProp?: RestBazProp;
}
type FooProp = unknown;
type RestFooProp = unknown;
type BazProp = unknown;
type RestBazProp = unknown;
declare function serializeFooProp(obj: FooProp): RestFooProp;
declare function serializeBazProp(obj: BazProp): RestBazProp;
function serializeFoo(obj: Foo): RestFoo {
return Object.entries(obj).reduce<RestFoo>((acc, [key, value]) => {
switch (key) {
case "fooProp":
acc["barProp"] = serializeFooProp(value);
break;
case "bazProp":
acc["bazProp"] = serializeBazProp(value);
break;
default:
acc[key] = value;
}
return acc;
}, {});
}
There are some breaking change risks in the emitted code that arise whenever we spread objects, use Object.assign
, or return an object to the user that they then use in a similar manner. I think we should be intentional about what behavior we stabilize here.
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.
Thanks bringing this up. I think we need to look a bit closer into this. Added a tracking item for this #2619
...-ts/test/modularIntegration/generated/azure/client-generator-core/usage/src/models/models.ts
Show resolved
Hide resolved
.../modularIntegration/generated/type/property/additional-properties/src/utils/serializeUtil.ts
Show resolved
Hide resolved
...es/typespec-test/test/openai_generic/generated/typespec-ts/src/api/chat/completions/index.ts
Outdated
Show resolved
Hide resolved
frequency_penalty: body["frequencyPenalty"], | ||
logit_bias: !body.logitBias | ||
? body.logitBias | ||
: (serializeRecord(body.logitBias as any) as any), |
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.
not sure why we need cast any here?
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.
We need to revisit these, tracking with: #2619
model: result.body["model"], | ||
choices: result.body["choices"].map((p) => ({ | ||
index: p["index"], | ||
message: { |
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.
we are not generate serializer for ChatMessage? I remember this is problematic. as there's recursive reference.
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.
Or maybe union,
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 you remember the repro of the issues you mention? This is being generated like this because we get an anonymous model, some of the properties here do call a serializer thoguh, which should resolve recursive issues I think
a373e9e
to
3c749a3
Compare
...es/typespec-test/test/NetworkAnalytics.Management/generated/typespec-ts/src/models/models.ts
Show resolved
Hide resolved
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.
LGTM, just a minor comment.
@@ -0,0 +1,32 @@ | |||
// vitest.config.ts |
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.
what is this?
import { | ||
getPropertySerializationPrefix, | ||
getRequestModelMapping | ||
} from "../helpers/operationHelpers.js"; |
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.
Is the derializers in preview scope?
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.
It is something we'll have to iterate during preview. As far as my exploration went, iterating on this won't impact the API
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.
LGTM, just a few questions
if (value !== undefined) { | ||
acc[key] = serializer(value); | ||
} |
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.
if (value !== undefined) { | |
acc[key] = serializer(value); | |
} | |
acc[key] = value !== undefined ? serializer(value) : value; |
acc[key] = item[key] as any; | ||
} else if (serializer) { | ||
const value = item[key]; | ||
if (value !== undefined) { |
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.
In this case, the else if block won't add a field for undefined values. See this suggestion.
@@ -25,7 +25,7 @@ import { ModularCodeModel, Type } from "./modularCodeModel.js"; | |||
*/ | |||
export function buildSerializeUtils(model: ModularCodeModel) { | |||
const serializeUtilFiles = []; | |||
for (const serializeType of ["serialize", "deserialize"]) { | |||
for (const serializeType of ["deserialize"]) { |
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.
How is serialization being handled?
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.
It is now being handled in buildSerializeFunction
This PR encapsulates inline serializers into functions to be able to handle nested scenarios and cyclic models. As well as filling in some gaps such as serializing additional properties, optional and nullable properties.
Fixes:
#1971
#2220
#2219
#2216
#2506