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

[core v2] incorrect way to determine whether a response body is a stream or not #13235

Closed
jeremymeng opened this issue Jan 15, 2021 · 1 comment
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Milestone

Comments

@jeremymeng
Copy link
Contributor

jeremymeng commented Jan 15, 2021

Currently, when sending a request, if any of the operation response body mapper has Stream type, we consider the operation a streaming one, and set request.streamResponseBody to true.

export function isStreamOperation(operationSpec: OperationSpec): boolean {
let result = false;
for (const statusCode in operationSpec.responses) {
const operationResponse: OperationResponse = operationSpec.responses[statusCode];
if (
operationResponse.bodyMapper &&
operationResponse.bodyMapper.type.name === MapperType.Stream
) {
result = true;
break;
}
}
return result;
}

And we use the property request.streamResponseBody to determine the shape of the response after we receive the raw response from http layer.

  • if the value is true, the body is stored in readableStreamBody (NodeJS.ReadableStream) for Node, or blobBody (Promise) for browser.
  • if the value is false, the body is stored in bodyAsText (string)

The problem is that some responses of a streaming operation can have non-stream body. One example is the default mapper, which is often used to map error cases, can have text body type.

  responses: {
    200: {
      bodyMapper: {
        serializedName: "parsedResponse",
        type: {
          name: "Stream"
        }
      },
      headersMapper: Mappers.BlobDownloadHeaders
    },
    ...
    default: {
      bodyMapper: Mappers.StorageError,
      headersMapper: Mappers.BlobDownloadHeaders
    }
  },

The fix in PR #13192 uses the response status code to check the response type in the operation spec. However, in core v2, core-https is where we return the response and need to determine the shape of the response, while core-client is where we deal with operation specs. With this separation of responsibilities, we may need a different fix since we don't have access to operation specs in core-https.

@jeremymeng jeremymeng added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Jan 15, 2021
@jeremymeng jeremymeng added this to the [2021] February milestone Jan 15, 2021
@jeremymeng jeremymeng self-assigned this Jan 15, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this issue Mar 9, 2021
Microsoft.DataProtection swagger changes (Azure#13235)

* manual changes for swagger

* changes for long-running operations

* changes for validate for backup and restore

* removing 204 from operationResults

* changes for a monitoring

* adding format

* removing date-time from duration

* changing to readonly

* changes for listRPs

* fixes swagger

* reverting readonly for scheduleTimes

* fixes for checkgates

* prettier fixes

* cahnges for friendlyname checks

* fixing gates

* removing tracking-via

* model validation fixes

Co-authored-by: Mayank Aggarwal <mayaggar@microsoft.com>
@jeremymeng
Copy link
Contributor Author

Fixed in #13192 too

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

No branches or pull requests

2 participants