perf(spec-parser): select first one if it only contains optional para…

…meters (#9954) * perf(spec-parser): select first one if it only contains optional parameters * fix: add missing period * perf: update message according to comments --------- Co-authored-by: turenlong <[email protected]>
OfficeDev · Sep 15, 2023 · 82d9638 · 82d9638
1 parent 7ba672e
commit 82d9638
Show file tree

Hide file tree

Showing 3 changed files with 127 additions and 24 deletions.
diff --git a/packages/fx-core/resource/package.nls.json b/packages/fx-core/resource/package.nls.json
@@ -399,7 +399,7 @@
   "core.common.UrlProtocolNotSupported": "Server url is not correct: protocol %s is not supported, you should use https protocol instead.",
   "core.common.RelativeServerUrlNotSupported": "Server url is not correct: relative server url is not supported.",
   "core.common.ResolveServerUrlFailed": "Unable to resolve the server URL: please make sure that the environment variable %s is defined.",
-  "core.common.OperationOnlyContainsOptionalParam": "Operation %s only contains optional parameters which are not supported",
+  "core.common.OperationOnlyContainsOptionalParam": "Operation %s contains multiple optional parameters. The first optional parameter is used for this command.",
   "core.common.ConvertSwaggerToOpenAPI": "The Swagger 2.0 file has been converted to OpenAPI 3.0.",
   "core.importAddin.label": "Import an Existing Outlook Add-ins",
   "core.importAddin.detail": "Upgrade an Add-ins project to the latest app manifest and project structure",

diff --git a/packages/fx-core/src/common/spec-parser/manifestUpdater.ts b/packages/fx-core/src/common/spec-parser/manifestUpdater.ts
@@ -52,21 +52,25 @@ export function generateParametersFromSchema(
   schema: OpenAPIV3.SchemaObject,
   name: string,
   isRequired = false
-): Parameter[] {
-  const parameters: Parameter[] = [];
+): [Parameter[], Parameter[]] {
+  const requiredParams: Parameter[] = [];
+  const optionalParams: Parameter[] = [];
 
   if (
     schema.type === "string" ||
     schema.type === "integer" ||
     schema.type === "boolean" ||
     schema.type === "number"
   ) {
+    const parameter = {
+      name: name,
+      title: updateFirstLetter(name),
+      description: schema.description ?? "",
+    };
     if (isRequired) {
-      parameters.push({
-        name: name,
-        title: updateFirstLetter(name),
-        description: schema.description ?? "",
-      });
+      requiredParams.push(parameter);
+    } else {
+      optionalParams.push(parameter);
     }
   } else if (schema.type === "object") {
     const { properties } = schema;
@@ -75,17 +79,18 @@ export function generateParametersFromSchema(
       if (schema.required && schema.required?.indexOf(property) >= 0) {
         isRequired = true;
       }
-      const result = generateParametersFromSchema(
+      const [requiredP, optionalP] = generateParametersFromSchema(
         properties[property] as OpenAPIV3.SchemaObject,
         property,
         isRequired
       );
 
-      parameters.push(...result);
+      requiredParams.push(...requiredP);
+      optionalParams.push(...optionalP);
     }
   }
 
-  return parameters;
+  return [requiredParams, optionalParams];
 }
 
 export async function generateCommands(
@@ -107,17 +112,21 @@ export async function generateCommands(
           if (method === ConstantString.PostMethod || method === ConstantString.GetMethod) {
             const operationItem = operations[method];
             if (operationItem) {
-              const parameters: Parameter[] = [];
+              const requiredParams: Parameter[] = [];
+              const optionalParams: Parameter[] = [];
               const paramObject = operationItem.parameters as OpenAPIV3.ParameterObject[];
 
               if (paramObject) {
                 paramObject.forEach((param: OpenAPIV3.ParameterObject) => {
+                  const parameter: Parameter = {
+                    name: param.name,
+                    title: updateFirstLetter(param.name),
+                    description: param.description ?? "",
+                  };
                   if (param.required) {
-                    parameters.push({
-                      name: param.name,
-                      title: updateFirstLetter(param.name),
-                      description: param.description ?? "",
-                    });
+                    requiredParams.push(parameter);
+                  } else {
+                    optionalParams.push(parameter);
                   }
                 });
               }
@@ -127,19 +136,28 @@ export async function generateCommands(
                 const requestJson = requestBody.content["application/json"];
                 if (Object.keys(requestJson).length !== 0) {
                   const schema = requestJson.schema as OpenAPIV3.SchemaObject;
-                  const result = generateParametersFromSchema(
+                  const [requiredP, optionalP] = generateParametersFromSchema(
                     schema,
                     "requestBody",
                     requestBody.required
                   );
-                  parameters.push(...result);
+                  requiredParams.push(...requiredP);
+                  optionalParams.push(...optionalP);
                 }
               }
 
               const operationId = operationItem.operationId!;
 
               const adaptiveCardPath = path.join(adaptiveCardFolder, operationId + ".json");
 
+              const parameters = [];
+
+              if (requiredParams.length != 0) {
+                parameters.push(...requiredParams);
+              } else {
+                parameters.push(optionalParams[0]);
+              }
+
               const command: IMessagingExtensionCommand = {
                 context: ["compose"],
                 type: "query",
@@ -152,7 +170,7 @@ export async function generateCommands(
               };
               commands.push(command);
 
-              if (parameters.length === 0) {
+              if (requiredParams.length === 0 && optionalParams.length > 1) {
                 warnings.push({
                   type: WarningType.OperationOnlyContainsOptionalParam,
                   content: format(ConstantString.OperationOnlyContainsOptionalParam, operationId),

diff --git a/packages/fx-core/tests/common/spec-parser/manifestUpdater.test.ts b/packages/fx-core/tests/common/spec-parser/manifestUpdater.test.ts
@@ -173,7 +173,13 @@ describe("manifestUpdater", () => {
               apiResponseRenderingTemplateFile: "adaptiveCards/createPet.json",
               context: ["compose"],
               id: "createPet",
-              parameters: [],
+              parameters: [
+                {
+                  description: "Name of the pet",
+                  name: "name",
+                  title: "Name",
+                },
+              ],
               title: "Create a pet",
               type: "query",
             },
@@ -459,7 +465,7 @@ describe("generateCommands", () => {
     expect(warnings).to.deep.equal([]);
   });
 
-  it("should throw error for each GET/POST operation in the spec if only contains optional parameters", async () => {
+  it("should show warning for each GET/POST operation in the spec if only contains optional parameters", async () => {
     const spec: any = {
       paths: {
         "/pets": {
@@ -502,15 +508,27 @@ describe("generateCommands", () => {
         apiResponseRenderingTemplateFile: "adaptiveCards/getPets.json",
         context: ["compose"],
         id: "getPets",
-        parameters: [],
+        parameters: [
+          {
+            description: "Maximum number of pets to return",
+            name: "limit",
+            title: "Limit",
+          },
+        ],
         title: "Get all pets",
         type: "query",
       },
       {
         apiResponseRenderingTemplateFile: "adaptiveCards/createPet.json",
         context: ["compose"],
         id: "createPet",
-        parameters: [],
+        parameters: [
+          {
+            description: "ID of the pet",
+            name: "id",
+            title: "Id",
+          },
+        ],
         title: "Create a pet",
         type: "query",
       },
@@ -529,6 +547,73 @@ describe("generateCommands", () => {
     ]);
   });
 
+  it("should not show warning for each GET/POST operation in the spec if only contains 1 optional parameters", async () => {
+    const spec: any = {
+      paths: {
+        "/pets": {
+          get: {
+            operationId: "getPets",
+            summary: "Get all pets",
+            parameters: [{ name: "id", description: "ID of the pet", required: false }],
+          },
+          post: {
+            operationId: "createPet",
+            summary: "Create a pet",
+            requestBody: {
+              content: {
+                "application/json": {
+                  schema: {
+                    type: "object",
+                    properties: {
+                      name: {
+                        type: "string",
+                        description: "Name of the pet",
+                      },
+                    },
+                  },
+                },
+              },
+            },
+          },
+        },
+      },
+    };
+    sinon.stub(fs, "pathExists").resolves(true);
+
+    const [result, warnings] = await generateCommands(spec, adaptiveCardFolder, manifestPath);
+    expect(result).to.deep.equal([
+      {
+        apiResponseRenderingTemplateFile: "adaptiveCards/getPets.json",
+        context: ["compose"],
+        id: "getPets",
+        parameters: [
+          {
+            description: "ID of the pet",
+            name: "id",
+            title: "Id",
+          },
+        ],
+        title: "Get all pets",
+        type: "query",
+      },
+      {
+        apiResponseRenderingTemplateFile: "adaptiveCards/createPet.json",
+        context: ["compose"],
+        id: "createPet",
+        parameters: [
+          {
+            description: "Name of the pet",
+            name: "name",
+            title: "Name",
+          },
+        ],
+        title: "Create a pet",
+        type: "query",
+      },
+    ]);
+    expect(warnings).to.deep.equal([]);
+  });
+
   it("should only generate commands for GET operation with required parameter", async () => {
     const spec: any = {
       paths: {