[Core v2] Return null when the response is empty and nullable (Azure#14366)

deyaaeldeen · web-flow · commit 9e8890a94154 · 2021-03-22T02:50:56.000Z
Fixes Azure#12009. # Scenario When the service does not return a body ([Example in our test server](https://github.com/Azure/autorest.testserver/blob/4b994be5fd68b3ac009af30399ad6b817630dbc8/legacy/routes/dictionary.js#L24)). # Expectation Response will be `null` if the mapper says it is `nullable`. # Current State Response is an empty object or array ([Example](https://github.com/Azure/autorest.typescript/blob/c8e68b845ccc3780d8c4be3787eb2c5e74272697/test/integration/bodyArray.spec.ts#L21)). # Fix Implements the expectation by doing case analysis and adds test cases.
diff --git a/sdk/core/core-client/src/utils.ts b/sdk/core/core-client/src/utils.ts
@@ -37,6 +37,61 @@ export function isValidUuid(uuid: string): boolean {
   return validUuidRegex.test(uuid);
 }
 
+/**
+ * Representation of parsed response headers and body coupled with information
+ * about how to map them:
+ * - whether the response body should be wrapped (typically if its type is primitive).
+ * - whether the response is nullable so it can be null if the combination of
+ *   the headers and the body is empty.
+ */
+interface ResponseObjectWithMetadata {
+  /** whether the mapper allows nullable body */
+  hasNullableType: boolean;
+  /** whether the response's body should be wrapped */
+  shouldWrapBody: boolean;
+  /** parsed headers of the response */
+  headers:
+    | {
+        [key: string]: unknown;
+      }
+    | undefined;
+  /** parsed body of the response */
+  body: any;
+}
+
+/**
+ * Maps the response as follows:
+ * - wraps the response body if needed (typically if its type is primitive).
+ * - returns null if the combination of the headers and the body is empty.
+ * - otherwise, returns the combination of the headers and the body.
+ *
+ * @param responseObject - a representation of the parsed response
+ * @returns the response that will be returned to the user which can be null and/or wrapped
+ *
+ * @internal
+ */
+function handleNullableResponseAndWrappableBody(
+  responseObject: ResponseObjectWithMetadata
+): unknown | null {
+  const combinedHeadersAndBody = {
+    ...responseObject.headers,
+    ...responseObject.body
+  };
+  if (
+    responseObject.hasNullableType &&
+    Object.getOwnPropertyNames(combinedHeadersAndBody).length === 0
+  ) {
+    return responseObject.shouldWrapBody ? { body: null } : null;
+  } else {
+    return responseObject.shouldWrapBody
+      ? {
+          ...responseObject.headers,
+          body: responseObject.body
+        }
+      : combinedHeadersAndBody;
+  }
+}
+
 /**
  * Take a `FullOperationResponse` and turn it into a flat
  * response object to hand back to the consumer.
@@ -50,8 +105,19 @@ export function flattenResponse(
   responseSpec: OperationResponseMap | undefined
 ): unknown {
   const parsedHeaders = fullResponse.parsedHeaders;
+
+  /**
+   * If body is not asked for, we return the response headers only. If the response
+   * has a body anyway, that body must be ignored.
+   */
+  if (fullResponse.request.method === "HEAD") {
+    return parsedHeaders;
+  }
+
   const bodyMapper = responseSpec && responseSpec.bodyMapper;
+  const isNullable = Boolean(bodyMapper?.nullable);
 
+  /** If the body is asked for, we look at the mapper to handle it */
   if (bodyMapper) {
     const typeName = bodyMapper.type.name;
     if (typeName === "Stream") {
@@ -82,30 +148,28 @@ export function flattenResponse(
           arrayResponse[key] = parsedHeaders[key];
         }
       }
-      return arrayResponse;
+      return isNullable &&
+        !fullResponse.parsedBody &&
+        !parsedHeaders &&
+        Object.getOwnPropertyNames(modelProperties).length === 0
+        ? null
+        : arrayResponse;
     }
 
     if (typeName === "Composite" || typeName === "Dictionary") {
-      return {
-        ...parsedHeaders,
-        ...fullResponse.parsedBody
-      };
+      return handleNullableResponseAndWrappableBody({
+        body: fullResponse.parsedBody,
+        headers: parsedHeaders,
+        hasNullableType: isNullable,
+        shouldWrapBody: false
+      });
     }
   }
 
-  if (
-    bodyMapper ||
-    fullResponse.request.method === "HEAD" ||
-    isPrimitiveType(fullResponse.parsedBody)
-  ) {
-    return {
-      ...parsedHeaders,
-      body: fullResponse.parsedBody
-    };
-  }
-
-  return {
-    ...parsedHeaders,
-    ...fullResponse.parsedBody
-  };
+  return handleNullableResponseAndWrappableBody({
+    body: fullResponse.parsedBody,
+    headers: parsedHeaders,
+    hasNullableType: isNullable,
+    shouldWrapBody: isPrimitiveType(fullResponse.parsedBody)
+  });
 }
diff --git a/sdk/core/core-client/test/serviceClient.spec.ts b/sdk/core/core-client/test/serviceClient.spec.ts
@@ -31,6 +31,7 @@ import {
 import { deserializationPolicy } from "../src/deserializationPolicy";
 import { TokenCredential } from "@azure/core-auth";
 import { getCachedDefaultHttpClient } from "../src/httpClientCache";
+import { assertServiceClientResponse } from "./utils/serviceClient";
 
 describe("ServiceClient", function() {
   describe("Auth scopes", () => {
@@ -382,6 +383,198 @@ describe("ServiceClient", function() {
     assert.deepStrictEqual(res.slice(), [1, 2, 3]);
   });
 
+  it("should deserialize array response as null if it is empty and nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: true,
+            type: {
+              name: MapperTypeNames.Sequence,
+              element: {
+                type: {
+                  name: "Number"
+                }
+              }
+            }
+          }
+        }
+      },
+      null
+    );
+  });
+
+  it("should deserialize array response as empty array if it is empty and not nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: false,
+            type: {
+              name: MapperTypeNames.Sequence,
+              element: {
+                type: {
+                  name: "Number"
+                }
+              }
+            }
+          }
+        }
+      },
+      []
+    );
+  });
+
+  it("should deserialize dictionary response as null if it is empty and nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: true,
+            type: {
+              name: MapperTypeNames.Dictionary,
+              value: {
+                type: {
+                  name: "String"
+                }
+              }
+            }
+          }
+        }
+      },
+      null
+    );
+  });
+
+  it("should deserialize dictionary response as empty if it is empty and not nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: false,
+            type: {
+              name: MapperTypeNames.Dictionary,
+              value: {
+                type: {
+                  name: "String"
+                }
+              }
+            }
+          }
+        }
+      },
+      {}
+    );
+  });
+
+  it("should deserialize object response as null if it is empty and nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: true,
+            type: {
+              name: MapperTypeNames.Composite,
+              modelProperties: {
+                message: {
+                  type: {
+                    name: "String"
+                  }
+                }
+              }
+            }
+          }
+        }
+      },
+      null
+    );
+  });
+
+  it("should deserialize object response as empty if it is empty and not nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: false,
+            type: {
+              name: MapperTypeNames.Composite,
+              modelProperties: {
+                message: {
+                  type: {
+                    name: "String"
+                  }
+                }
+              }
+            }
+          }
+        }
+      },
+      {}
+    );
+  });
+
+  it("should deserialize primitive response as null if it is empty and nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: true,
+            type: {
+              name: MapperTypeNames.Boolean
+            }
+          }
+        }
+      },
+      {
+        body: null
+      }
+    );
+  });
+
+  it("should deserialize primitive response as undefined if it is empty and not nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: false,
+            type: {
+              name: MapperTypeNames.Boolean
+            }
+          }
+        }
+      },
+      {
+        body: undefined
+      }
+    );
+  });
+
+  it("should deserialize a head request with no body even if the mapper says the body is nullable", async function() {
+    await assertServiceClientResponse(
+      {
+        requestMethod: "HEAD",
+        responseBodyAsText: "",
+        responseMapper: {
+          bodyMapper: {
+            nullable: true,
+            type: {
+              name: MapperTypeNames.Boolean
+            }
+          }
+        }
+      },
+      undefined
+    );
+  });
+
   describe("getOperationArgumentValueFromParameter()", () => {
     it("should return undefined when the parameter path isn't found in the operation arguments or service client", () => {
       const operationArguments: OperationArguments = {};
diff --git a/sdk/core/core-client/test/utils/serviceClient.ts b/sdk/core/core-client/test/utils/serviceClient.ts
