Skip to content
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

Refine tcgc logic for spread #572

Merged
merged 17 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 13 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
7 changes: 7 additions & 0 deletions .chronus/changes/refine_tcgc_logic-2024-3-2-16-27-30.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: fix
packages:
- "@azure-tools/typespec-client-generator-core"
---

Set spread model with none usage
7 changes: 7 additions & 0 deletions .chronus/changes/refine_tcgc_logic-2024-3-2-16-27-52.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@azure-tools/typespec-client-generator-core"
---

Workaround for arm provider method parameter
5 changes: 5 additions & 0 deletions packages/typespec-client-generator-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,17 @@
"pluralize": "^8.0.0"
},
"peerDependencies": {
"@azure-tools/typespec-autorest": "workspace:~",
"@azure-tools/typespec-azure-core": "workspace:~",
"@typespec/compiler": "workspace:~",
"@typespec/http": "workspace:~",
"@typespec/rest": "workspace:~",
"@typespec/versioning": "workspace:~"
},
"devDependencies": {
"@azure-tools/typespec-azure-core": "workspace:~",
"@azure-tools/typespec-azure-resource-manager": "workspace:~",
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved
"@azure-tools/typespec-autorest": "workspace:~",
"@types/node": "~18.11.19",
"@types/pluralize": "^0.0.33",
"@typespec/compiler": "workspace:~",
Expand All @@ -75,6 +79,7 @@
"@typespec/rest": "workspace:~",
"@typespec/tspd": "workspace:~",
"@typespec/versioning": "workspace:~",
"@typespec/openapi": "workspace:~",
iscai-msft marked this conversation as resolved.
Show resolved Hide resolved
"@vitest/coverage-v8": "^1.4.0",
"@vitest/ui": "^1.4.0",
"c8": "^9.1.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,7 @@ export function isAzureCoreModel(t: Type): boolean {
return (
t.kind === "Model" &&
t.namespace !== undefined &&
["Azure.Core", "Azure.Core.Foundations", "Azure.ResourceManager"].includes(
getNamespaceFullName(t.namespace)
)
["Azure.Core", "Azure.Core.Foundations"].includes(getNamespaceFullName(t.namespace))
);
}

Expand Down
6 changes: 3 additions & 3 deletions packages/typespec-client-generator-core/src/package.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ function getSdkBasicServiceMethod<
);
}
} else {
const methodParameter = diagnostics.pipe(getSdkMethodParameter(context, prop));
if (methodParameter.kind === "method") {
methodParameters.push(methodParameter);
// workaround for the provider parameter in arm, need to refine method design in tcgc later
if (!context.arm || prop.name !== "provider") {
methodParameters.push(diagnostics.pipe(getSdkMethodParameter(context, prop)));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
Diagnostic,
Enum,
Interface,
Model,
ModelProperty,
Expand Down Expand Up @@ -175,7 +176,7 @@ export function getLibraryName(
type.name +
type.templateMapper.args
.filter(
(arg): arg is Model =>
(arg): arg is Model | Enum =>
(arg.kind === "Model" || arg.kind === "Enum") && arg.name.length > 0
)
.map((arg) => pascalCase(arg.name))
Expand Down
21 changes: 9 additions & 12 deletions packages/typespec-client-generator-core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1121,7 +1121,7 @@ function checkAndGetClientType(

interface ModelUsageOptions {
seenModelNames?: Set<SdkType>;
recurseThroughProperties?: boolean;
propagation?: boolean;
}

function updateUsageOfModel(
Expand All @@ -1131,7 +1131,7 @@ function updateUsageOfModel(
options?: ModelUsageOptions
): void {
options = options ?? {};
options.recurseThroughProperties = options?.recurseThroughProperties ?? true;
options.propagation = options?.propagation ?? true;
if (!type || !["model", "enum", "array", "dict", "union", "enumvalue"].includes(type.kind))
return;
if (options?.seenModelNames === undefined) {
Expand Down Expand Up @@ -1162,10 +1162,7 @@ function updateUsageOfModel(
}

if (type.kind === "enum") return;
if (type.baseModel && (type.baseModel.usage & usage) === 0) {
// if it has a base model and the base model doesn't currently have that usage
type.baseModel.usage |= usage;
}
if (!options.propagation) return;
if (type.baseModel) {
updateUsageOfModel(context, usage, type.baseModel, options);
}
Expand All @@ -1174,13 +1171,11 @@ function updateUsageOfModel(
updateUsageOfModel(context, usage, discriminatedSubtype, options);
}
}
if (type.additionalProperties && options.recurseThroughProperties) {
if (type.additionalProperties) {
updateUsageOfModel(context, usage, type.additionalProperties, options);
}
if (options.recurseThroughProperties) {
for (const property of type.properties) {
updateUsageOfModel(context, usage, property.type, options);
}
for (const property of type.properties) {
updateUsageOfModel(context, usage, property.type, options);
}
}

Expand Down Expand Up @@ -1215,6 +1210,8 @@ function updateTypesFromOperation(
const bodies = diagnostics.pipe(checkAndGetClientType(context, httpBody.type, operation));
if (generateConvenient) {
bodies.forEach((body) => {
// spread body model should be none usage
if (body.kind === "model" && body.isGeneratedName) return;
updateUsageOfModel(context, UsageFlags.Input, body);
});
if (httpBody.contentTypes.includes("application/merge-patch+json")) {
Expand All @@ -1226,7 +1223,7 @@ function updateTypesFromOperation(
if (isMultipartFormData(context, httpBody.type, operation)) {
bodies.forEach((body) => {
updateUsageOfModel(context, UsageFlags.MultipartFormData, body, {
recurseThroughProperties: false,
propagation: false,
});
});
}
Expand Down
47 changes: 46 additions & 1 deletion packages/typespec-client-generator-core/test/package.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { AutorestTestLibrary } from "@azure-tools/typespec-autorest/testing";
import { AzureCoreTestLibrary } from "@azure-tools/typespec-azure-core/testing";
import { AzureResourceManagerTestLibrary } from "@azure-tools/typespec-azure-resource-manager/testing";
import { ApiKeyAuth, OAuth2Flow, Oauth2Auth } from "@typespec/http";
import { OpenAPITestLibrary } from "@typespec/openapi/testing";
import { deepStrictEqual, ok, strictEqual } from "assert";
import { beforeEach, describe, it } from "vitest";
import {
Expand Down Expand Up @@ -2757,7 +2760,7 @@ describe("typespec-client-generator-core: package", () => {
`);
const sdkPackage = runner.context.sdkPackage;
const method = getServiceMethodOfClient(sdkPackage);
strictEqual(sdkPackage.models.length, 1);
strictEqual(sdkPackage.models.length, 0);
strictEqual(method.name, "myOp");
strictEqual(method.kind, "basic");
strictEqual(method.parameters.length, 2);
Expand Down Expand Up @@ -3007,6 +3010,48 @@ describe("typespec-client-generator-core: package", () => {
strictEqual(op.bodyParam.name, "documentTranslateContent");
deepStrictEqual(op.bodyParam.correspondingMethodParams, [documentMethodParam]);
});
it("arm provider parameter spread", async () => {
const runnerWithArm = await createSdkTestRunner({
librariesToAdd: [
AzureCoreTestLibrary,
OpenAPITestLibrary,
AutorestTestLibrary,
AzureResourceManagerTestLibrary,
],
emitterName: "@azure-tools/typespec-java",
});
await runnerWithArm.compileWithBuiltInArmService(`
model Employee {}

model ProviderNamespace {
@path
@segment("providers")
@doc("The provider namespace for the resource.")
provider: "Microsoft.ThisWillBeReplaced";
}

op ArmReadOperation<Parameters, Response>(
...Parameters,
): Response;

@autoRoute
op get is ArmReadOperation<ProviderNamespace, Employee>;
`);
const sdkPackage = runnerWithArm.context.sdkPackage;
strictEqual(sdkPackage.models.length, 1);

const method = sdkPackage.clients[0].methods[0];
strictEqual(method.kind, "basic");
strictEqual(method.name, "get");
strictEqual(method.parameters.length, 1);
strictEqual(method.parameters[0].name, "accept");

const serviceOperation = method.operation;
const bodyParameter = serviceOperation.bodyParam;
strictEqual(bodyParameter, undefined);
strictEqual(serviceOperation.parameters.length, 1);
strictEqual(serviceOperation.parameters[0].name, "accept");
});
});
describe("versioning", () => {
it("define own api version param", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1284,8 +1284,7 @@ describe("typespec-client-generator-core: public-utils", () => {
`
);
const models = runner.context.sdkPackage.models;
strictEqual(models.length, 1);
ok(models.find((x) => x.name === "TestRequest" && x.isGeneratedName));
strictEqual(models.length, 0);
});

it("anonymous model for body parameter", async () => {
Expand All @@ -1295,8 +1294,7 @@ describe("typespec-client-generator-core: public-utils", () => {
`
);
const models = runner.context.sdkPackage.models;
strictEqual(models.length, 1);
ok(models.find((x) => x.name === "TestRequest" && x.isGeneratedName));
strictEqual(models.length, 0);
});

it("anonymous union in response header", async () => {
Expand Down
22 changes: 22 additions & 0 deletions packages/typespec-client-generator-core/test/test-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface SdkTestRunner extends BasicTestRunner {
context: SdkContext<CreateSdkTestRunnerOptions, SdkHttpOperation>;
compileWithBuiltInService(code: string): Promise<Record<string, Type>>;
compileWithBuiltInAzureCoreService(code: string): Promise<Record<string, Type>>;
compileWithBuiltInArmService(code: string): Promise<Record<string, Type>>;
compileWithCustomization(mainCode: string, clientCode: string): Promise<Record<string, Type>>;
compileAndDiagnoseWithCustomization(
mainCode: string,
Expand Down Expand Up @@ -155,6 +156,27 @@ export async function createSdkTestRunner(
return result;
};

// compile with arm service definition
sdkTestRunner.compileWithBuiltInArmService = async function compileWithBuiltInArmService(code) {
const result = await baseCompile(
`
@Azure.ResourceManager.armProviderNamespace
@server("http://localhost:3000", "endpoint")
@service()
namespace My.Service;
${code}`,
{
noEmit: true,
}
);
sdkTestRunner.context = createSdkContextTestHelper(
sdkTestRunner.program,
options,
options.emitterName
);
return result;
};

const mainAutoCode = [
...host.libraries
.filter((x) => x !== StandardTestLibrary)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1993,17 +1993,14 @@ describe("typespec-client-generator-core: types", () => {
interface StringExtensible extends GetAndSend<string | "b" | "c"> {}
`);
const sdkPackage = runner.context.sdkPackage;
strictEqual(sdkPackage.models.length, 2);
strictEqual(sdkPackage.models.length, 1);
strictEqual(sdkPackage.enums.length, 1);
const prop = sdkPackage.enums.find((x) => x.name === "GetResponseProp" && x.isGeneratedName);
ok(prop);
strictEqual(prop.isFixed, false);
strictEqual(prop.valueType.kind, "string");
const req = sdkPackage.models.find((x) => x.name === "SendRequest" && x.isGeneratedName);
const resp = sdkPackage.models.find((x) => x.name === "GetResponse" && x.isGeneratedName);
ok(req);
ok(resp);
strictEqual(req.properties[0].type, prop);
strictEqual(resp.properties[0].type, prop);
});

Expand Down
Loading
Loading