Standardize the layout of the http _response property in output types

[deyaaeldeen]

Existing consistencies

There are a few inconsistencies regarding how do we define the _response property in our output types. Those inconsistencies are discussed below:

Not every package supports the _response property

List of packages that do support the _response property:

• Form Recognizer
• Tables
• App Configuration
• Storage File Datalake
• Storage File Datashare
• Storage Queue

List of packages that do not:

• Identity
• Text Analytics
• Key Vault
• Event Hub
• Service Bus
• Search
• Cosmosdb
• Core (well, obviously!)

The shape of the _response property varies between packages

On one hand, App Configuration has the following type that is extended by output types:

export interface HttpResponseField<HeadersT> {
    _response: HttpResponse & {
        parsedHeaders: HeadersT;
        bodyAsText: string;
    };
}

On the other hand, all other packages mirror the generated response type in some sense and change the type that is being intersected with to be the corresponding one from the convenience layer alongside other convenience changes. For instance, Tables has the following generated response type (located at sdk/tables/azure-tables/src/generated/models/index.ts):

/**
 * Contains response data for the query operation.
 */
export type TableQueryOperationResponse = TableQueryHeaders &
  TableQueryResponse & {
    /**
     * The underlying HTTP response.
     */
    _response: coreHttp.HttpResponse & {
      /**
       * The response body as text (string format)
       */
      bodyAsText: string;

      /**
       * The response body as parsed JSON or XML
       */
      parsedBody: TableQueryResponse;
      /**
       * The parsed HTTP response headers.
       */
      parsedHeaders: TableQueryHeaders;
    };
  };

and in the convenience layer, it has the following corresponding response type (located at sdk/tables/azure-tables/src/models.ts):

export type ListTableItemsResponse = Array<TableResponseProperties> & {
  /**
   * This header contains the continuation token value.
   */
  nextTableName?: string;
  /**
   * The underlying HTTP response.
   */
  _response: HttpResponse & {
    /**
     * The response body as text (string format)
     */
    bodyAsText: string;

    /**
     * The response body as parsed JSON or XML
     */
    parsedBody: TableQueryResponse;
    /**
     * The parsed HTTP response headers.
     */
    parsedHeaders: TableQueryHeaders;
  };
};

Which looks similar to the App Configuration's one but has the extra property parsedBody that is typed using the generated type TableQueryResponse which is fully static in the sense that it does not have any any members.

What is the best shape for _response anyway?

From the JS SDK weekly team meeting on 8/26/2020, the proposed shape is to mirror the generated response types but if the parsedBody property exist, type it as any, so the Tables's ListTableItemsResponse would become:

export type ListTableItemsResponse = Array<TableResponseProperties> & {
  /**
   * This header contains the continuation token value.
   */
  nextTableName?: string;
  /**
   * The underlying HTTP response.
   */
  _response: HttpResponse & {
    /**
     * The response body as text (string format)
     */
    bodyAsText: string;

    /**
     * The response body as parsed JSON or XML
     */
    parsedBody: any;
    /**
     * The parsed HTTP response headers.
     */
    parsedHeaders: TableQueryHeaders;
  };
};

How to move forward?

I suggest that the _response property should be added to all output types in all packages according to the approach we agreed on. Furthermore, I suggest to change the typing of parsedBody property in packages that already support the _response property to be any and no longer expose the generated type if it is not needed elsewhere.

It is yet to be checked if there is any customer who relies on the type of parsedBody. I will reach out to the .Net SDK team asking for such scenarios if any.

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @wmengmsft, @MehaKaushik, @shurd, @anfeldma-ms

[nguerrera]

Related to discussion in #10602 (comment)

There was debate about whether it should be typed at all, but if it is typed, I agree that parsedBody: any is fine. Leaking all the generated goo into the typing for an escape hatch scenario would be too much. I think _response: HttpOperationResponse as I had it matches your proposal concisely and is also used in other places. (I borrowed it from somewhere.) I think it could be the standard that could be used everywhere should it be decided that it must be typed.

EDIT: Sorry, that also makes parsedHeaders a string -> any dictionary, which I think is also fine as an escape hatch typing, but is in fact different from your proposal.

cc @xirzec @bterlson

[willmtemple]

Widening to any has a tendency to virally cause other inferred types to become any as well. If there's any client user for an existing _response using parsedBody and allowing inference based on its stronger type, then that could cause any to suddenly start appearing in many places. I don't think it would be breaking, though, in the sense that the code would continue to function as normal but more type information than just this parsedBody could be lost in a consuming program.

I like unknown better here, since it will force appropriate casting, but we definitely can't widen to unknown without a break.

[xirzec]

Stepping around the "is this a breaking change?" issue, I think ultimately it's better to get out of the business of typing/guaranteeing intermediate request pipeline goop.

Right now parsedBody is just the unflattened contents of the response object itself and doesn't add anything useful (it is NOT the raw response from the server.) Parsed headers is equally useless, since our headers collection is simply a case insensitive dictionary of whatever we got back from the server. bodyAsText is questionably useful, but probably annoying for a developer to work with.

I would rather standardize around _request providing access to known useful things, such as the HTTP status code and all headers returned. For cases where folks want to debug our mappers or see the raw server payload, we should provide better logging/debuggability of the HTTP pipeline itself instead of pushing the work off onto whomever called a ServiceClient method wanting a specific shape.

While we're at it, we should revisit what we want to expose on RestError. Today it exposes the full response object, including parsedBody, parsedHeaders, etc. Whatever we decide for _request should probably match what is exposed by RestError.

[deyaaeldeen]

Closing this in favor of #12009. In particular, fixing #12009 means we will need to change the way we return the meta data in _response which most likely will be done by a callback to ask for that information on-demand.