perf(spec-parser): fix issue that OperationOnlyContainsOptionalParam …

…doesn't display (#11671) * perf(spec-parser): fix issue that OperationOnlyContainsOptionalParam doesn't display * perf: update test case --------- Co-authored-by: rentu <rentu@microsoft.com>
OfficeDev · May 21, 2024 · f82edb5 · f82edb5
1 parent cdf282c
commit f82edb5
Show file tree

Hide file tree

Showing 5 changed files with 147 additions and 53 deletions.
diff --git a/packages/fx-core/src/component/generator/copilotPlugin/helper.ts b/packages/fx-core/src/component/generator/copilotPlugin/helper.ts
@@ -617,36 +617,30 @@ function validateTeamsManifestLength(
       (o) => o.type === WarningType.OperationOnlyContainsOptionalParam
     );
 
-    const commands = teamsManifest.composeExtensions![0].commands;
     if (optionalParamsOnlyWarnings) {
       for (const optionalParamsOnlyWarning of optionalParamsOnlyWarnings) {
-        const command = commands.find(
-          (o: IMessagingExtensionCommand) => o.id === optionalParamsOnlyWarning.data
-        );
-
-        if (command && command.parameters) {
-          const parameterName = command.parameters[0]?.name;
-          resultWarnings.push(
+        resultWarnings.push(
+          getLocalizedString(
+            "core.copilotPlugin.scaffold.summary.warning.api.optionalParametersOnly",
+            optionalParamsOnlyWarning.data.commandId,
+            optionalParamsOnlyWarning.data.commandId
+          ) +
             getLocalizedString(
-              "core.copilotPlugin.scaffold.summary.warning.api.optionalParametersOnly",
-              optionalParamsOnlyWarning.data,
-              optionalParamsOnlyWarning.data
-            ) +
-              getLocalizedString(
-                "core.copilotPlugin.scaffold.summary.warning.api.optionalParametersOnly.mitigation",
-                parameterName,
-                optionalParamsOnlyWarning.data,
-                path.join(AppPackageFolderName, ManifestTemplateFileName),
-                path.join(
-                  AppPackageFolderName,
-                  teamsManifest.composeExtensions![0].apiSpecificationFile ?? ""
-                )
+              "core.copilotPlugin.scaffold.summary.warning.api.optionalParametersOnly.mitigation",
+              optionalParamsOnlyWarning.data.parameterName,
+              optionalParamsOnlyWarning.data.commandId,
+              path.join(AppPackageFolderName, ManifestTemplateFileName),
+              path.join(
+                AppPackageFolderName,
+                teamsManifest.composeExtensions![0].apiSpecificationFile ?? ""
               )
-          );
-        }
+            )
+        );
       }
     }
 
+    const commands = teamsManifest.composeExtensions![0].commands;
+
     for (const command of commands) {
       if (command.type === "query") {
         if (!command.apiResponseRenderingTemplateFile) {

diff --git a/packages/fx-core/src/core/FxCore.ts b/packages/fx-core/src/core/FxCore.ts
@@ -1411,7 +1411,10 @@ export class FxCore {
           manifestRes.value,
           path.relative(inputs.projectPath!, outputApiSpecPath)
         );
-        context.logProvider.info(warnSummary);
+
+        if (warnSummary) {
+          context.logProvider.info(warnSummary);
+        }
       }
     } catch (e) {
       let error: FxError;

diff --git a/packages/fx-core/tests/core/FxCore.test.ts b/packages/fx-core/tests/core/FxCore.test.ts
@@ -3732,15 +3732,93 @@ describe("copilotPlugin", async () => {
 
     const core = new FxCore(tools);
     sinon.stub(SpecParser.prototype, "generate").resolves({
-      warnings: [{ type: WarningType.OperationOnlyContainsOptionalParam, content: "fakeMessage" }],
+      warnings: [
+        {
+          type: WarningType.OperationOnlyContainsOptionalParam,
+          content: "fakeMessage",
+          data: { commandId: "fakeId", parameterName: "fakeName" },
+        },
+      ],
+      allSuccess: false,
+    });
+    sinon.stub(SpecParser.prototype, "list").resolves(listResult);
+    sinon.stub(manifestUtils, "_readAppManifest").resolves(ok(manifest));
+    sinon.stub(validationUtils, "validateInputs").resolves(undefined);
+    sinon.stub(tools.ui, "showMessage").resolves(ok("Add"));
+    const logSpy = sinon.spy(tools.logProvider, "info");
+    const result = await core.copilotPluginAddAPI(inputs);
+    assert.isTrue(result.isOk());
+    assert.isTrue(logSpy.calledOnce);
+  });
+
+  it("add API - unknown warning not show log", async () => {
+    const appName = await mockV3Project();
+    const inputs: Inputs = {
+      platform: Platform.VSCode,
+      [QuestionNames.Folder]: os.tmpdir(),
+      [QuestionNames.ApiSpecLocation]: "test.json",
+      [QuestionNames.ApiOperation]: ["GET /user/{userId}"],
+      [QuestionNames.ManifestPath]: path.join(os.tmpdir(), appName, "appPackage/manifest.json"),
+      projectPath: path.join(os.tmpdir(), appName),
+    };
+    const manifest = new TeamsAppManifest();
+    manifest.composeExtensions = [
+      {
+        composeExtensionType: "apiBased",
+        apiSpecificationFile: "apiSpecificationFiles/openapi.json",
+        commands: [
+          {
+            id: "getUserById",
+            title: "Get User By Id",
+          },
+          {
+            id: "notexist",
+            title: "Get User By Id",
+          },
+        ],
+      },
+    ];
+
+    const listResult: ListAPIResult = {
+      APIs: [
+        {
+          operationId: "getUserById",
+          server: "https://server",
+          api: "GET /user/{userId}",
+          isValid: true,
+          reason: [],
+        },
+        {
+          operationId: "getStoreOrder",
+          server: "https://server",
+          api: "GET /store/order",
+          isValid: true,
+          reason: [],
+        },
+      ],
+      validAPICount: 2,
+      allAPICount: 2,
+    };
+
+    const core = new FxCore(tools);
+    sinon.stub(SpecParser.prototype, "generate").resolves({
+      warnings: [
+        {
+          type: "unknown" as any,
+          content: "fakeMessage",
+          data: { commandId: "fakeId", parameterName: "fakeName" },
+        },
+      ],
       allSuccess: false,
     });
     sinon.stub(SpecParser.prototype, "list").resolves(listResult);
     sinon.stub(manifestUtils, "_readAppManifest").resolves(ok(manifest));
     sinon.stub(validationUtils, "validateInputs").resolves(undefined);
     sinon.stub(tools.ui, "showMessage").resolves(ok("Add"));
+    const logSpy = sinon.spy(tools.logProvider, "info");
     const result = await core.copilotPluginAddAPI(inputs);
     assert.isTrue(result.isOk());
+    assert.isTrue(logSpy.notCalled);
   });
 
   it("add API - readManifestFailed", async () => {

diff --git a/packages/spec-parser/src/manifestUpdater.ts b/packages/spec-parser/src/manifestUpdater.ts
@@ -408,15 +408,20 @@ export class ManifestUpdater {
                 ) {
                   command.parameters = command.parameters.filter((param) => param.isRequired);
                 } else if (command.parameters && command.parameters.length > 0) {
-                  command.parameters = [command.parameters[0]];
-                  warnings.push({
-                    type: WarningType.OperationOnlyContainsOptionalParam,
-                    content: Utils.format(
-                      ConstantString.OperationOnlyContainsOptionalParam,
-                      command.id
-                    ),
-                    data: command.id,
-                  });
+                  if (command.parameters.length > 1) {
+                    command.parameters = [command.parameters[0]];
+                    warnings.push({
+                      type: WarningType.OperationOnlyContainsOptionalParam,
+                      content: Utils.format(
+                        ConstantString.OperationOnlyContainsOptionalParam,
+                        command.id
+                      ),
+                      data: {
+                        commandId: command.id,
+                        parameterName: command.parameters[0].name,
+                      },
+                    });
+                  }
                 }
 
                 if (adaptiveCardFolder) {

diff --git a/packages/spec-parser/test/manifestUpdater.test.ts b/packages/spec-parser/test/manifestUpdater.test.ts
@@ -3492,6 +3492,12 @@ describe("manifestUpdater", () => {
                   default: 123,
                 },
               },
+              {
+                name: "limit",
+                title: "Limit",
+                inputType: "number",
+                description: "Maximum number of pets to return",
+              },
             ],
           },
         },
@@ -3552,7 +3558,10 @@ describe("manifestUpdater", () => {
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "getPets"),
-        data: "getPets",
+        data: {
+          commandId: "getPets",
+          parameterName: "id",
+        },
       },
     ]);
   });
@@ -4071,7 +4080,10 @@ describe("manifestUpdater", () => {
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "createPet"),
-        data: "createPet",
+        data: {
+          commandId: "createPet",
+          parameterName: "name",
+        },
       },
     ]);
   });
@@ -4589,12 +4601,18 @@ describe("generateCommands", () => {
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "getPets"),
-        data: "getPets",
+        data: {
+          commandId: "getPets",
+          parameterName: "limit",
+        },
       },
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "createPet"),
-        data: "createPet",
+        data: {
+          commandId: "createPet",
+          parameterName: "id",
+        },
       },
     ]);
   });
@@ -4655,16 +4673,10 @@ describe("generateCommands", () => {
         type: "query",
       },
     ]);
-    expect(warnings).to.deep.equal([
-      {
-        type: WarningType.OperationOnlyContainsOptionalParam,
-        content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "createPet"),
-        data: "createPet",
-      },
-    ]);
+    expect(warnings).to.deep.equal([]);
   });
 
-  it("should not show warning for each GET/POST operation in the spec if only contains 1 optional parameters", async () => {
+  it("should not show warning for each GET/POST operation in the spec if only contains 2 optional parameters", async () => {
     const spec: any = {
       paths: {
         "/pets": {
@@ -4686,6 +4698,10 @@ describe("generateCommands", () => {
                         type: "string",
                         description: "Name of the pet",
                       },
+                      id: {
+                        type: "string",
+                        description: "Id of the pet",
+                      },
                     },
                   },
                 },
@@ -4739,15 +4755,13 @@ describe("generateCommands", () => {
       },
     ]);
     expect(warnings).to.deep.equal([
-      {
-        type: WarningType.OperationOnlyContainsOptionalParam,
-        content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "getPets"),
-        data: "getPets",
-      },
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "createPet"),
-        data: "createPet",
+        data: {
+          commandId: "createPet",
+          parameterName: "name",
+        },
       },
     ]);
   });
@@ -4855,7 +4869,7 @@ describe("generateCommands", () => {
       {
         type: WarningType.OperationOnlyContainsOptionalParam,
         content: Utils.format(ConstantString.OperationOnlyContainsOptionalParam, "getPets"),
-        data: "getPets",
+        data: { commandId: "getPets", parameterName: "limit" },
       },
     ]);
   });