Skip to content

Fix: Restore full error.details handling for binary wrap-non-model-return deserializers#3852

Merged
jeremymeng merged 10 commits intoAzure:mainfrom
joheredi:fix/binary-wrap-error-details
Mar 24, 2026
Merged

Fix: Restore full error.details handling for binary wrap-non-model-return deserializers#3852
jeremymeng merged 10 commits intoAzure:mainfrom
joheredi:fix/binary-wrap-error-details

Conversation

@joheredi
Copy link
Copy Markdown
Member

@joheredi joheredi commented Mar 18, 2026

Problem

Commit 49500efaa introduced the wrap-non-model-return feature which defaults to true for all Azure-flavored clients. The binary wrap path had two regressions:

  1. Deserializer skipped error.details extractionif (!(shouldWrap && isBinary)) completely bypassed getExceptionThrowStatement(), the only function that generates per-status-code error.details, exception headers, @clientOption headers, and restErrorCodeHeader assignment.
  2. Operation function bypassed getBinaryResponse/parsedBody/parsedHeaders — an early return skipped all normal response processing.

Root Cause

The binary wrap feature introduced an early-return block in getOperationFunction that bypassed the entire normal flow (storage-compat setup, binary response helper, header deserialization, response assembly). The deserializer also took StreamableMethod directly and called getBinaryStream internally, which consumed the HttpResponse inside the deserializer scope — making it unavailable for _downloadDeserializeHeaders() in the operation function.

Fix (Approach: Split helper)

Created a new getBinaryStreamResponse static helper that only handles platform-specific stream extraction (Node: asNodeStream(), Browser: asBrowserStream()), then let the generated deserializer handle error checking using the same getExceptionThrowStatement code path as all other operations.

The key architectural change: moved getBinaryStreamResponse from inside the deserializer to the operation function level, so the HttpResponse result is in scope for all downstream consumers:

  • _downloadDeserialize(result) — status check + error enrichment via getExceptionThrowStatement
  • _downloadDeserializeHeaders(result) — header parsing
  • addStorageCompatResponse(...) — storage-compat assembly

The deserializer parameter uses an intersection type (PathUncheckedResponse & XXXResponse) so it can access both error-handling properties and stream properties without type casts.

Changes

New files:

  • static/static-helpers/serialization/get-binary-stream-response.ts — Node variant
  • static/static-helpers/serialization/get-binary-stream-response-browser.mts — Browser variant

Modified files:

  • src/modular/static-helpers-metadata.ts — Registered getBinaryStreamResponse helper
  • src/modular/helpers/operationHelpers.ts:
    • Removed if (!(shouldWrap && isBinary)) guard that skipped the status check in the deserializer
    • Deserializer parameter uses intersection type PathUncheckedResponse & XXXResponse for binary wrap
    • Removed early-return block in operation function — binary-wrap now flows through the normal path
    • Operation function calls getBinaryStreamResponse for binary-wrap, getBinaryResponse for non-wrapped binary
    • Fixed bodyType to use wrapped response type name for StorageCompatResponseInfo generics
  • test/modularUnit/scenarios/operations/wrapNonModelReturn.md — Updated binary scenario + added error enrichment scenarios
  • test/modularUnit/scenarios/operations/storageCompat/storageCompatResponse.md — Added void+headers+error enrichment scenario

Generated code before vs after

Before (no error handling):

export async function _getLogsDeserialize(result: StreamableMethod): Promise<GetLogsResponse> {
  return getBinaryResponseBody(result, ["200"]);
}

After (full error handling):

export async function _getLogsDeserialize(
  result: PathUncheckedResponse & GetLogsResponse,
): Promise<GetLogsResponse> {
  const expectedStatuses = ["200"];
  if (!expectedStatuses.includes(result.status)) {
    const error = createRestError(result);
    error.details = apiErrorDeserializer(result.body);
    error.details = { ...(error.details as any), ..._getLogsDeserializeExceptionHeaders(result) };
    throw error;
  }

  return { blobBody: result.blobBody, readableStreamBody: result.readableStreamBody };
}
export async function getLogs(
  context: Client,
  options: GetLogsOptionalParams = { requestOptions: {} },
): Promise<GetLogsResponse> {
  const streamableMethod = _getLogsSend(context, options);
  const result = await getBinaryStreamResponse(streamableMethod);
  return _getLogsDeserialize(result);
}

Validation

  • pnpm build — success
  • npm run unit-test — 585 passing, 2 pending
  • pnpm format — clean
  • npm run lint — clean

joheredi and others added 3 commits March 18, 2026 01:26
…turn deserializers

The wrap-non-model-return feature (commit 49500ef) introduced early returns
in the binary wrap path that skipped status checking and error.details
extraction in generated deserialize functions.

This fix:
- Adds a new getBinaryStream static helper that separates stream extraction
  from error handling (Node + browser variants)
- Removes the status check skip for binary wrap - all operations now generate
  the full getExceptionThrowStatement with error.details, exception headers,
  and restErrorCodeHeader support
- The deserializer parameter is renamed to _streamableResult for binary wrap,
  with getBinaryStream resolving it into an HttpResponse named result so
  existing error handling code references work unchanged

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds two new unit test scenarios verifying that binary wrap deserializers
generate full error.details handling:
- Binary wrap + error model: verifies error.details = apiErrorDeserializer(result.body)
- Binary wrap + error model + exception headers: verifies error.details with
  exception header merging via _getBlobDeserializeExceptionHeaders(result)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verifies that when enable-storage-compat and include-headers-in-response are
both enabled on a void-body operation with response headers and an error model:
1. Deserializer generates error.details = storageErrorDeserializer(result.body)
2. Exception headers are merged into error.details
3. Operation function extracts parsedHeaders and passes them to
   addStorageCompatResponse

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread packages/typespec-ts/static/static-helpers/serialization/get-binary-stream.ts Outdated
@jeremymeng
Copy link
Copy Markdown
Member

I tried 8fcaf1f build for storage-blob generation. There are compilation errors:

[browser] C:/git/jssdk/sdk/storage/storage-blob/src/Clients.ts(1354,34): error TS2345: Argument of type 'HttpNodeStreamResponse | HttpBrowserStreamResponse' is not assignable to parameter of type 'StreamableMethod'.
  Type 'HttpNodeStreamResponse' is not assignable to type 'StreamableMethod'.
    Property 'then' is missing in type 'HttpNodeStreamResponse' but required in type 'PromiseLike<PathUncheckedResponse>'.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/Clients.ts(1460,38): error TS2345: Argument of type 'HttpNodeStreamResponse' is not assignable to parameter of type 'StreamableMethod'.
  Property 'then' is missing in type 'HttpNodeStreamResponse' but required in type 'PromiseLike<PathUncheckedResponse>'.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/Clients.ts(4143,31): error TS2345: Argument of type 'HttpNodeStreamResponse | HttpBrowserStreamResponse' is not assignable to parameter of type 'StreamableMethod'.
  Type 'HttpNodeStreamResponse' is not assignable to type 'StreamableMethod'.
    Property 'then' is missing in type 'HttpNodeStreamResponse' but required in type 'PromiseLike<PathUncheckedResponse>'.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/generated/api/blob/operations.ts(3813,42): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/generated/api/blob/operations.ts(4148,3): error TS2322: Type 'BlobDownloadResponse' is not assignable to type '{ requestId?: string | undefined; clientRequestId?: string | undefined; objectReplicationRules?: Record<string, string> | undefined; lastModified: Date; createdOn: Date; ... 39 more ...; contentCrc64?: Uint8Array<...> | undefined; } & Uint8Array<...> & StorageCompatResponseInfo<...>'.
  Type 'BlobDownloadResponse' is missing the following properties from type '{ requestId?: string | undefined; clientRequestId?: string | undefined; objectReplicationRules?: Record<string, string> | undefined; lastModified: Date; createdOn: Date; ... 39 more ...; contentCrc64?: Uint8Array<...> | undefined; }': lastModified, createdOn, contentLength, contentRange, and 12 more.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/generated/api/blockBlob/operations.ts(111,42): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.
[browser] C:/git/jssdk/sdk/storage/storage-blob/src/generated/api/blockBlob/operations.ts(356,3): error TS2322: Type 'BlockBlobQueryResponse' is not assignable to type '{ lastModified: Date; contentLength: number; contentRange: string; etag: string; contentMD5: Uint8Array<ArrayBufferLike>; contentEncoding: string; ... 25 more ...; contentType: "application/octet-stream"; } & Uint8Array<...> & StorageCompatResponseInfo<...>'.
  Type 'BlockBlobQueryResponse' is missing the following properties from type '{ lastModified: Date; contentLength: number; contentRange: string; etag: string; contentMD5: Uint8Array<ArrayBufferLike>; contentEncoding: string; ... 25 more ...; contentType: "application/octet-stream"; }': lastModified, contentLength, contentRange, etag, and 9 more.
  • in the unexpected response case, the result.body is not the expected string type
image
  • the type being returned from download() doesn't match what we created for storage (StorageCompatResponseInfo which has _response, parsedHeaders, and parsedBody). We lost the parsedHeaders, parsedBody probably isn't needed in this streaming case.
image

@jeremymeng
Copy link
Copy Markdown
Member

we can probably get away from casting as any for now, as storage convenience layer doesn't use the download() method

@jeremymeng
Copy link
Copy Markdown
Member

we can probably get away from casting as any for now, as storage convenience layer doesn't use the download() method

ah not really. Previously I was relying on _downloadDeserialize() to handle the error deserialization. With this build, I cannot because it calls getBinaryStream(_streamableResult) which calls await streamableMethod.asNodeStream() which makes another request already made by my previous await streamableMethod.asNodeStream() call in convenience layer to get the response so that I can deserialize the headers. So if download() is fixed to have parsedHeaders I can probably use it directly

@jeremymeng
Copy link
Copy Markdown
Member

So currently _downloadDeserialize() sends the request, gets the response, returns the streaming body shape { blobBody, readableStreamBody,} but it doesn't return the http response, so I have nothing to feed _downloadDeserializeHeaders()

@qiaozha qiaozha added the p0 priority 0 label Mar 19, 2026
@jeremymeng
Copy link
Copy Markdown
Member

I pushed a branch to show the issue and my manual fix. Not sure if it is easy to update the code gen to have similar fix, when the operation is returning stream body AND storage compat option is enabled.

my manual fix includes

  • move await getBinaryStream(streamableMethod) into _downloadDeserialize so that its result can be passed to _downloadDeserialize and _downloadDeserializeHeaders
  • update the parameter of _downloadDeserialize so that it takes the return value from getBinaryStream()
  • update return types of streaming methods to not have the incorrect Uint8Array return type

jeremymeng and others added 6 commits March 20, 2026 13:56
- move the `getBinaryStream()` call out of `_operationDeserialize()` and into the
operation method `operation()`, where its result is passed to both `_opDeserialize()` and
`_operationDeserializeHeaders()`

- `_operationDeserialize()` now takes the result of `getBinaryStream()`, process and
throw for error response if any; return the wrapped stream body.

- return type is fixed for wrapped operation.
@jeremymeng jeremymeng merged commit ac1a26b into Azure:main Mar 24, 2026
35 of 36 checks passed
JialinHuang803 added a commit to JialinHuang803/autorest.typescript that referenced this pull request Apr 9, 2026
…turn deserializers (Azure#3852)

* fix: restore full error.details handling for binary wrap-non-model-return deserializers

The wrap-non-model-return feature (commit 49500ef) introduced early returns
in the binary wrap path that skipped status checking and error.details
extraction in generated deserialize functions.

This fix:
- Adds a new getBinaryStream static helper that separates stream extraction
  from error handling (Node + browser variants)
- Removes the status check skip for binary wrap - all operations now generate
  the full getExceptionThrowStatement with error.details, exception headers,
  and restErrorCodeHeader support
- The deserializer parameter is renamed to _streamableResult for binary wrap,
  with getBinaryStream resolving it into an HttpResponse named result so
  existing error handling code references work unchanged

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test: add error enrichment scenarios for binary wrap-non-model-return

Adds two new unit test scenarios verifying that binary wrap deserializers
generate full error.details handling:
- Binary wrap + error model: verifies error.details = apiErrorDeserializer(result.body)
- Binary wrap + error model + exception headers: verifies error.details with
  exception header merging via _getBlobDeserializeExceptionHeaders(result)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* test: add storage-compat scenario for void+headers with error enrichment

Verifies that when enable-storage-compat and include-headers-in-response are
both enabled on a void-body operation with response headers and an error model:
1. Deserializer generates error.details = storageErrorDeserializer(result.body)
2. Exception headers are merged into error.details
3. Operation function extracts parsedHeaders and passes them to
   addStorageCompatResponse

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Update smoke tests

* Update for Storage Compat binary response

- move the `getBinaryStream()` call out of `_operationDeserialize()` and into the
operation method `operation()`, where its result is passed to both `_opDeserialize()` and
`_operationDeserializeHeaders()`

- `_operationDeserialize()` now takes the result of `getBinaryStream()`, process and
throw for error response if any; return the wrapped stream body.

- return type is fixed for wrapped operation.

* update smoke test

* attempt to fix missing import of OperationResponse

* react to recent fast-xml-parser change

* Remove unnecessary logic and rename the helper

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jialin Huang <jialinhuang@microsoft.com>
Co-authored-by: Jeremy Meng <jeremy.ymeng@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p0 priority 0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants