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

Add Payload contentNegotiation case for modular #2497

Closed
Tracked by #2450
v-jiaodi opened this issue May 7, 2024 · 2 comments · Fixed by #2508
Closed
Tracked by #2450

Add Payload contentNegotiation case for modular #2497

v-jiaodi opened this issue May 7, 2024 · 2 comments · Fixed by #2508
Assignees
Labels
HRLC p0 priority 0

Comments

@v-jiaodi
Copy link
Member

v-jiaodi commented May 7, 2024

image

@qiaozha
Copy link
Member

qiaozha commented Jun 3, 2024

Basically, the problem here is how to handle overload so that the constant type headers can accept custom values.

For this case, their typespec are defined as

namespace DifferentBody {
  model PngImage {
    @header contentType: "image/png";
    @body image: bytes;
  }

  model PngImageAsJson {
    @header contentType: "application/json";
    content: bytes;
  }

  @sharedRoute
  op getAvatarAsPng(@header accept: "image/png"): PngImage;

  @sharedRoute
  op getAvatarAsJson(@header accept: "application/json"): PngImageAsJson;
}

RLC generate the code as

export interface DifferentBodyGetAvatarAsPng {
  get(
    options: DifferentBodyGetAvatarAsJsonParameters,
  ): StreamableMethod<DifferentBodyGetAvatarAsJson200Response>;
  get(
    options: DifferentBodyGetAvatarAsPngParameters,
  ): StreamableMethod<DifferentBodyGetAvatarAsPng200Response>;
}

export interface Routes {
  /** Resource for '/content-negotiation/different-body' has methods for the following verbs: get */
  (path: "/content-negotiation/different-body"): DifferentBodyGetAvatarAsPng;
}

export type ContentNegotiationContext = Client & {
  path: Routes;
};

In RLC layer, there's no problem to provide a third undefined value here for accept header, we can just use as any cast.

    const result = await client
      .path("/content-negotiation/different-body")
      .get({ headers: { accept: "wrongAccept" } as any });
    assert.strictEqual(
      (result.body as any).message,
      "Unsupported Accept header"
    );

but in Modular, if we want to accept customer self defined header value,

export function _getAvatarAsPngSend(
  context: Client,
  options: DifferentBodyGetAvatarAsPngOptionalParams = { requestOptions: {} },
): StreamableMethod<DifferentBodyGetAvatarAsPng200Response> {
  return context
    .path("/content-negotiation/different-body")
    .get({
      ...operationOptionsToRequestParameters(options),
      headers: {
        accept:
          options.requestOptions?.headers?.["accept"] ?? "image/png" as any,
      },
    }) as StreamableMethod<DifferentBodyGetAvatarAsPng200Response>;
}

Since the accept header could be other values than "application/json" and "image/png", type inference system somehow thinks we are trying to convet DifferentBodyGetAvatarAsJson200Response as DifferentBodyGetAvatarAsPng200Response as the overload here, which is incompatible.

Proposal 1:
we could generate something like as unknown as StreamableMethod<DifferentBodyGetAvatarAsPng200Response>; to make it work.

Pros:

  1. it's simple to fix.

Cons:

  1. if we pass a "wrongType" here, we could also get the result of DifferentBodyGetAvatarAsPng200Response.

Proposal 2:
we can add an additional overload for accept
type any, as shown in this commit 1a59763

Pros:

  1. the user experience is good for both existing known types and other customer self-defined types.

Cons,

  1. feel like we need consider more complex cases like what if there's overload based on other query parameters/body types or body properties types?
  2. the user experience doesn't seem to be consistent if we have both overload operations and non-overload operation.

Proposal 3:
we don't design it as can accept other header value.

Cons:

  1. it would be strange customer can set the header but we just ignore it.

@qiaozha
Copy link
Member

qiaozha commented Jun 4, 2024

we should go for proposal 3 and maybe log a warrning if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HRLC p0 priority 0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants