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

Implement support for ResponseWithTypedHeaders #42640

Open
samvaity opened this issue Oct 28, 2024 · 1 comment
Open

Implement support for ResponseWithTypedHeaders #42640

samvaity opened this issue Oct 28, 2024 · 1 comment
Assignees

Comments

@samvaity
Copy link
Member

samvaity commented Oct 28, 2024

ACR v2 generation that has a use case for ReponseWithTypedHeader concept.
https://github.com/srnagar/acr-core-v2

Design appraoch:

  1. Add support for custom classes:

    • Custom Response Class: ContainerRegistriesCreateManifestResponse holds HttpStatus and the custom header class ContainerRegistriesCreateManifestHeaders.
    • Custom Header Class: ContainerRegistriesCreateManifestHeaders provides strongly typed access to specific headers, which we extract from the HttpHeaders passed in the response.
  2. Add support for a typed header class in HttpRequestInformation with returnHeaderType property

    • allows specifying the class for typed headers, in this case, ContainerRegistriesCreateManifestHeaders.class, which helps define expected headers at compile time.

Pros:

  1. reducing deserialization overhead
  2. streamlines the API for header-only responses

Example update for ACR:

@HttpRequestInformation(
        method = HttpMethod.PUT,
        path = "/v2/{name}/manifests/{reference}",
        expectedStatusCodes = { 201 },
        returnHeaderType = ContainerRegistriesCreateManifestHeaders.class
)
@UnexpectedResponseExceptionDetail
Mono<ContainerRegistriesCreateManifestResponse> createManifest(
    @HostParam("url") String url,
    @PathParam("name") String name,
    @PathParam("reference") String reference,
    @HeaderParam("Content-Type") String contentType,
    @BodyParam("application/vnd.docker.distribution.manifest.v2+json") Flux<ByteBuffer> payload,
    @HeaderParam("Content-Length") long contentLength,
    @HeaderParam("Accept") String accept,
    Context context
);

@Generated
@ServiceMethod(returns = ReturnType.SINGLE)
public Mono<ContainerRegistriesCreateManifestResponse> createManifestWithResponse(
    String name, 
    String reference, 
    Flux<ByteBuffer> payload, 
    long contentLength, 
    String contentType) {

    return this.serviceClient.createManifestWithResponseAsync(name, reference, payload, contentLength, contentType)
        .map(response -> new ContainerRegistriesCreateManifestResponse(response.getStatusCode(), response.getHeaders()));
}

// Custom response class with respective header type
public final class ContainerRegistriesCreateManifestResponse extends Response<T> {

    private final int statusCode;
    private final ContainerRegistriesCreateManifestHeaders headers;

    public ContainerRegistriesCreateManifestResponse(int statusCode, HttpHeaders httpHeaders) {
        this.statusCode = statusCode;
        this.headers = new ContainerRegistriesCreateManifestHeaders(httpHeaders);
    }
}
@samvaity samvaity self-assigned this Oct 28, 2024
@alzimmermsft
Copy link
Member

Want to add in some historical context around this area from the azure-core libraries.

This concepts already exists in azure-core libraries but we changed code generation a few years ago to prefer direct usage of ResponseBase<HeaderType, BodyType> instead as the Response<T> extension type is owned by the downstream library. This forces the downstream library to add opens to com.azure.core as azure-core manages deserialization and creation of responses. So, this was transitioned to prefering ResponseBase<HeaderType, BodyType> as azure-core owns that type and doesn't need to use reflection to create an instance of it, though it still needs reflection access for the HeaderType and BodyType (but always helps to remove some usage of reflection).

I know one plan of clientcore is moving the deserialization logic into the library itself rather than clientcore, so this isn't as much a concern, just wanted to share historical context in this area.

One other minor reason we moved to ResponseBase<H, B> was to shrink the number of classes in libraries. Some had dozens of custom types.

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

No branches or pull requests

2 participants