Modular: Encapsulate serializers into functions #2613

joheredi · 2024-06-24T05:44:08Z

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

qiaozha

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

qiaozha · 2024-06-24T14:13:57Z

packages/typespec-ts/src/modular/helpers/operationHelpers.ts

@@ -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`);


Is this temporary?

This will be addressed in follow up work tracked by: #2619

packages/typespec-ts/src/modular/modularCodeModel.ts

packages/typespec-ts/src/contextManager.ts

packages/typespec-ts/src/modular/buildHelperSerializers.ts

packages/typespec-ts/test/modularIntegration/generated/azure/core/basic/src/models/options.ts

...espec-ts/test/modularIntegration/generated/azure/core/lro/rpc/generated/src/models/models.ts

qiaozha · 2024-06-24T14:49:13Z

...ges/typespec-ts/test/modularIntegration/generated/encode/bytes/src/api/responseBody/index.ts

@@ -68,7 +68,7 @@ export async function _responseBodyOctetStreamDeserialize(
    throw createRestError(result);
  }

-  return result.body;
+  return result.body as any;


In this case, I think the result.body type is Uint8Array right?

This will be addressed in follow up work tracked by: #2619

...dularIntegration/generated/type/model/inheritance/nested-discriminator/src/api/operations.ts

qiaozha · 2024-06-24T14:55:25Z

packages/typespec-ts/test/modularIntegration/dictionaryTypes.spec.ts

+  });
+
+  // Fix deserialization
+  it.skip("should get a dictionary of datetime", async () => {


we don't support deserialization for dictionary now?

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

This will be addressed in follow up work tracked by: #2619

dgetu · 2024-06-24T19:25:54Z

packages/typespec-ts/src/modular/buildHelperSerializers.ts

+        acc[key] = item[key] as any;
+      } else if (serializer) {
+        const value = item[key];
+        if (value !== undefined) {


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?

Yeah I think so, I thought this would keep all existing keys. Am I missing something?

In this case, the else if block won't add a field for undefined values. See this suggestion.

packages/typespec-ts/src/contextManager.ts

dgetu · 2024-06-24T19:56:57Z

packages/typespec-ts/src/modular/helpers/operationHelpers.ts

+) {
+  const propertyFullName = getPropertyFullName(modularType, propertyPath);
+  if (modularType.optional || modularType.type.nullable) {
+    return `!${propertyFullName} ? ${propertyFullName} :`;


Should this be an explicit check for null/undefined? To account for serializing falsy values?

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

dgetu · 2024-06-24T20:02:37Z

packages/typespec-ts/src/modular/serialization/buildSerializerFunction.ts

+): string | undefined {
+  if (
+    (type.tcgcType as SdkModelType).usage !== undefined &&
+    ((type.tcgcType as SdkModelType).usage & UsageFlags.Input) !==


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.

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

dgetu · 2024-06-24T20:09:24Z

packages/typespec-ts/src/modular/serialization/buildSerializerFunction.ts

+  }
+
+  if (!type.name) {
+    throw new Error(`NYI Serialization of anonymous types`);


TCGC generates unique names for anonymous models, so it shouldn't be very expensive to use those if/when we want to handle this.

Agreed we discuss this, added an item to this issue for tracking: #2619

dgetu · 2024-06-24T20:22:30Z

packages/typespec-ts/src/modular/serialization/buildSerializerFunction.ts

+    });
+
+    // This is only handling the compatibility mode, will need to update when we handle additionalProperties property.
+    const additionalPropertiesSpread = hasAdditionalProperties(type.tcgcType)


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?

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

dgetu · 2024-06-24T21:01:53Z

packages/typespec-ts/src/modular/helpers/operationHelpers.ts

+interface RequestModelMappingResult {
+  propertiesStr: string[];
+  directAssignment?: boolean;
+}
 export function getRequestModelMapping(


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.

Thanks bringing this up. I think we need to look a bit closer into this. Added a tracking item for this #2619

packages/typespec-ts/src/modular/modularCodeModel.ts

packages/typespec-ts/src/contextManager.ts

...-ts/test/modularIntegration/generated/azure/client-generator-core/usage/src/models/models.ts

.../modularIntegration/generated/type/property/additional-properties/src/utils/serializeUtil.ts

...es/typespec-test/test/openai_generic/generated/typespec-ts/src/api/chat/completions/index.ts

qiaozha · 2024-06-25T13:32:06Z

...es/typespec-test/test/openai_generic/generated/typespec-ts/src/api/chat/completions/index.ts

+        frequency_penalty: body["frequencyPenalty"],
+        logit_bias: !body.logitBias
+          ? body.logitBias
+          : (serializeRecord(body.logitBias as any) as any),


not sure why we need cast any here?

We need to revisit these, tracking with: #2619

qiaozha · 2024-06-25T13:33:23Z

...es/typespec-test/test/openai_generic/generated/typespec-ts/src/api/chat/completions/index.ts

+    model: result.body["model"],
+    choices: result.body["choices"].map((p) => ({
+      index: p["index"],
+      message: {


we are not generate serializer for ChatMessage? I remember this is problematic. as there's recursive reference.

Or maybe union,

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

packages/typespec-ts/test/modularUnit/serializeUtil.spec.ts

...es/typespec-test/test/NetworkAnalytics.Management/generated/typespec-ts/src/models/models.ts

qiaozha

LGTM, just a minor comment.

qiaozha · 2024-06-26T05:59:58Z

...ation/generated/azure/core/scalar/vitest.config.ts.timestamp-1719374191621-0a605de146966.mjs

@@ -0,0 +1,32 @@
+// vitest.config.ts


what is this?

MaryGao · 2024-06-26T09:09:02Z

packages/typespec-ts/src/modular/serialization/buildSerializerFunction.ts

+import {
+  getPropertySerializationPrefix,
+  getRequestModelMapping
+} from "../helpers/operationHelpers.js";


Is the derializers in preview scope?

It is something we'll have to iterate during preview. As far as my exploration went, iterating on this won't impact the API

dgetu

LGTM, just a few questions

dgetu · 2024-06-26T22:33:34Z

packages/typespec-ts/src/modular/buildHelperSerializers.ts

+        if (value !== undefined) {
+          acc[key] = serializer(value);
+        }


Suggested change

if (value !== undefined) {

acc[key] = serializer(value);

}

acc[key] = value !== undefined ? serializer(value) : value;

dgetu · 2024-06-26T22:35:10Z

packages/typespec-ts/src/modular/buildHelperSerializers.ts

+        acc[key] = item[key] as any;
+      } else if (serializer) {
+        const value = item[key];
+        if (value !== undefined) {


In this case, the else if block won't add a field for undefined values. See this suggestion.

dgetu · 2024-06-26T22:46:25Z

packages/typespec-ts/src/modular/buildSerializeUtils.ts

@@ -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"]) {


How is serialization being handled?

It is now being handled in buildSerializeFunction

joheredi requested review from sarangan12, qiaozha and MaryGao as code owners June 24, 2024 05:44

qiaozha reviewed Jun 24, 2024

View reviewed changes

joheredi requested a review from dgetu June 24, 2024 18:32

dgetu reviewed Jun 24, 2024

View reviewed changes

qiaozha reviewed Jun 25, 2024

View reviewed changes

.../modularIntegration/generated/type/property/additional-properties/src/utils/serializeUtil.ts Show resolved Hide resolved

qiaozha reviewed Jun 25, 2024

View reviewed changes

...es/typespec-test/test/openai_generic/generated/typespec-ts/src/api/chat/completions/index.ts Outdated Show resolved Hide resolved

qiaozha reviewed Jun 25, 2024

View reviewed changes

joheredi added 21 commits June 25, 2024 12:32

Track meta tree contexts

f89ebba

Generate and use model serializers

525f56f

Address issue when RLC doesn't have a corresponding model

faae113

Support record

d597805

support discriminated unions

0d52a63

Support discriminator and additional properties

9d38be7

Fix empty serializers and missing imports

2ed817e

fix key issue

d9a7711

fix nested additionalprops

f010a87

update tests

56d3b4b

Add dictionary tests

afc0fca

support optional

9c25bc5

Fix test

22e2722

Fix issue with direct assignment of serialized

80b766b

Fix duplicate models and unecessary serializers

44d7e2c

Re generate Smoke test

fcbd139

Update Unit Tests

34dd138

Fix tests

63c4d55

Re generate tests

44c1b74

regenerate smoke tests

9a3eea8

skip no-longer reelevant tests

3f3bcf4

joheredi added 2 commits June 25, 2024 12:33

Update generated files

ef68d29

update loadtesting

3c749a3

joheredi force-pushed the serialize-short-term branch from a373e9e to 3c749a3 Compare June 25, 2024 19:40

joheredi added 6 commits June 25, 2024 14:51

Address feedback

53f645f

skip exporting serializers

f789363

Handle polymorphic parents

e9b0f50

re-generate smoke

5d2fcd8

Merge remote-tracking branch 'upstream/main' into serialize-short-term

cf6d586

Update integration tests

103fcd8

This was referenced Jun 26, 2024

generate nullable from cadl-ranch #2227

Closed

Add modular coverage cases for optional property in cadl-ranch #2229

Closed

Emit serialize utils #2430

Closed

Emit serialize util #2544

Closed

Update smoke tests

8d37a98

This was referenced Jun 26, 2024

Emit smoke test clients #2545

Closed

[Serialization] Retrieve RLC type name from rlc-common #2590

Closed

Serialization Followup work #2619

Open

Update test

89e56a1

qiaozha reviewed Jun 26, 2024

View reviewed changes

...es/typespec-test/test/NetworkAnalytics.Management/generated/typespec-ts/src/models/models.ts Show resolved Hide resolved

qiaozha approved these changes Jun 26, 2024

View reviewed changes

joheredi added 2 commits June 26, 2024 00:42

Merge remote-tracking branch 'upstream/main' into serialize-short-term

9e7d1c8

Update tests

a98d09e

MaryGao reviewed Jun 26, 2024

View reviewed changes

Fix merge conflicts with new nullability from TCGC and regenerate assets

797bfde

dgetu approved these changes Jun 26, 2024

View reviewed changes

joheredi merged commit b579012 into Azure:main Jun 26, 2024
14 checks passed

This was referenced Jun 27, 2024

nullable property with all types #2219

Closed

type of dictionary with all kinds of value types #2216

Closed

optional property with all types #2220

Closed

Modular: Encapsulate serializers into functions #2613

Modular: Encapsulate serializers into functions #2613

Conversation

joheredi commented Jun 24, 2024

qiaozha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiaozha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgetu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment