Azure / Azure/typespec-azure

Refactor isBinaryPayload to operate on SdkType and drop __raw dependency

Open
#4,741 0 comments 0 reactions 1 assignee Claimed by @JialinHuang803 View on GitHub
emitter:typescript
Dominant language
TypeScript
Stars
27
Forks
90
Avg merge
1d 22h
Merged PRs (30d)
156

Description

## Background

PR Azure/autorest.typescript#3958 introduced an `encode` parameter to `isBinaryPayload` so that `encode === "bytes"` is treated as a binary signal. This is a pragmatic workaround for two related problems but does not fully resolve them.

## Problem 1: `__raw` can be `undefined`

`isBinaryPayload` accepts a raw TypeSpec `Type` and calls `isByteOrByteUnion(dpgContext, body)`, which dereferences `body.kind`.

In the Modular pipeline, callers pass `bodyParameter.__raw!` / `response.type!.__raw!`. Per TCGC's `SdkModelPropertyTypeBase`, `__raw` is **optional** (`__raw?: ModelProperty`). It can be `undefined` for synthesized body parameters such as **spread bodies** (e.g. `createWidget(...CreateWidget)` in `widget_dpg`).

Today this does not crash because the original logic short-circuits:

```typescript
if (type === KnownMediaType.Binary && isByteOrByteUnion(dpgContext, body)) { ... }
```

For JSON content types, `type !== KnownMediaType.Binary` short-circuits before `isByteOrByteUnion(ctx, undefined)` is evaluated. Any future change that breaks this short-circuit (e.g. changing `&&` to `||`) will crash with:

```
TypeError: Cannot read properties of undefined (reading 'kind')
at getEffectiveModelFromType (modelUtils.ts:289)
at getSchemaForType (modelUtils.ts:158)
at isByteOrByteUnion (modelUtils.ts:99)
at isBinaryPayload (operationUtil.ts:208)
at buildBodyParameter (operationHelpers.ts:1772)
```

## Problem 2: `isBinaryPayload` couples payload check with encoding

The function name suggests it checks the payload shape, but with Azure/autorest.typescript#3958 it also peeks at the `encode` metadata. This is a slight semantic coupling — preferably the function should be a pure check on the payload type itself.

## Proposed fix

Refactor `isBinaryPayload` (and the helpers it depends on) to operate on TCGC `SdkType` directly, instead of the raw TypeSpec `Type`. Concretely:

1. Add a helper such as `isSdkBytesType(sdkType: SdkType): boolean` that walks `bytes`, `nullable`, and `union` variants. This replaces `isByteOrByteUnion` for the Modular path.
2. Change the Modular call sites to pass `bodyParameter.type` / `response.type` (`SdkType`) instead of `bodyParameter.__raw` / `response.type.__raw`.
3. Drop the `encode` parameter from `isBinaryPayload` once the function operates on `SdkType` — the `SdkType` itself carries the `encode` metadata, so the function can check it internally as part of "is this payload bytes-on-the-wire".
4. Keep the existing raw-`Type` path for the RLC call site in `transform/transformResponses.ts` (which does not have an `SdkType`), or migrate it separately.

## Benefits

- Removes the latent `__raw === undefined` crash class.
- Restores `isBinaryPayload`'s semantics to "is this payload binary?" without leaking encode-detection into the caller.
- Aligns with the rest of the Modular emitter which already uses `SdkType`.

## Affected call sites

Current usages of `isBinaryPayload` to migrate:
- `packages/typespec-ts/src/modular/helpers/operationHelpers.ts` (3 call sites)
- `packages/typespec-ts/src/transform/transformResponses.ts` (1 call site, RLC path — may keep raw-Type form)

## Related

- PR Azure/autorest.typescript#3958 (workaround that introduced the `encode` parameter)
- Issue Azure/autorest.typescript#3913 (original bug fixed by Azure/autorest.typescript#3958)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.