Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
15 stars 41 forks source link

[TCGC] Could TCGC provide an easier way to map from `SdkMethodParameter` to `SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter`? #1441

Open haolingdong-msft opened 2 months ago

haolingdong-msft commented 2 months ago

Example:

TSP:

  @route("upload/{name}")
  @post
  uploadFile(
    @path name: string,
    @header contentType: "multipart/form-data",
    file_data: bytes,

    // this field should not be serialized to form-data
    @visibility("read") readOnly: string,

    constant: "constant",
  ): OkResponse;

Generated java sdk:


    @Generated
    @ServiceMethod(returns = ReturnType.SINGLE)
    Response<Void> uploadFileWithResponse(String name, BinaryData uploadFileRequest, RequestOptions requestOptions) {
        // Protocol API requires serialization of parts with content-disposition and data, as operation 'uploadFile' is
        // 'multipart/form-data'
        return this.serviceClient.uploadFileWithResponse(name, uploadFileRequest, requestOptions);
    }

    @Generated
    @ServiceMethod(returns = ReturnType.SINGLE)
    public void uploadFile(String name, FileDataFileDetails fileData) {
        // Generated convenience method for uploadFileWithResponse
        RequestOptions requestOptions = new RequestOptions();
        UploadFileRequest uploadFileRequestObj = new UploadFileRequest(fileData);
        BinaryData uploadFileRequest = new MultipartFormDataHelper(requestOptions)
            .serializeFileField("file_data", uploadFileRequestObj.getFileData().getContent(),
                uploadFileRequestObj.getFileData().getContentType(), uploadFileRequestObj.getFileData().getFilename())
            .serializeTextField("constant", uploadFileRequestObj.getConstant())
            .end()
            .getRequestBody();
        uploadFileWithResponse(name, uploadFileRequest, requestOptions).getValue();
    }

code-model:

image

TCGC response: SdkServiceMethod's file_data parameter

image

SdkHttpOperation's bodyParameter parameter

image

It is recommended to generate sdk's convenient method using SdkServiceMethod, but current SdkServiceMethod.SdkMethodParameter is lack of information related with body/header/query/path parameters, when you get a SdkMethodParameter, you don't know if it's body/header/query/path parameters. But the convenient method could depend on those information. e.g.

And for Java, you can see we use M4 code-model as intermediate output, ConvenientApi.parameters contain information like http.in. All those information are under SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter of SdkHttpOperation. So we need to map SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter to get more information to generate convenient method.

One thing I found inconvenient is current TCGC only contains mapping from SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter to SdkMethodParameter, I wonder if TCGC could provide a helper function or properties on SdkMethodParameter to let emitter map SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter?

@iscai-msft , @tadelesh , what do you think?

iscai-msft commented 2 months ago

this issue is raising the same concern that this previous issue raised. Currently the design is that the method signature is agnostic of the protocol and serialized information, but I think we can revisit this during our scrum talk and see what new thoughts are. We're in a weird spot right now, where I feel we want to move more towards what's directly in the typespec, but we are still modifying the method signature based off of HTTP-specific info. I think it would be good to revisit this as well during out discussions later today, to see if we can get more consistency here.

haolingdong-msft commented 2 months ago

Thanks @iscai-msft for the reply. Yeah, I know this issue and I totally understand the TCGC design is SdkServiceMethod is for generating method (in Java and .Net world, it is convenient method), and SdkHttpOperation is for generating serialization part of code. And I like the idea. But in some cases, we still need the information on SdkHttpOperation to generate convenient method signature and imp. So I wonder if TCGC could provide a helper function to let us get Http parameter from Sdk method parameter.

haolingdong-msft commented 2 months ago

Summarize on 8/29 discussion: We have three options regarding to this issue:

  1. Have bi-directional mapping between SdkMethodParameter and SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter, could be a proprety on SdkMethodParameter.
  2. Have a helper function to let emitter know the mapping from SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter, signature like getHttpOperationParameter(methodParam: SdkMethodParameter): SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter.
  3. Enrich SdkServiceMethod's functionality and add callbacks on SdkServiceMethod, e.g. on parameter ordering, missed inforation on multipart/json-merge-patch etc.

Personally I prefer option 2 together with option 3, we can first provide option 2, and do the enhancement/enrichment in option 3 together. /cc @iscai-msft @m-nash @srnagar @tadelesh

iscai-msft commented 2 months ago

I agree with your summary @haolingdong-msft thank you so much! To me, 1 and 2 are about the same, but it makes sense to expose it as a helper function since not all of us need it

qiaozha commented 2 months ago

@iscai-msft we also need the mapping between client.initialization.properties and path/query/header location information to build the correct RLC request in Modular. A small point, for server endpoint parameterized host parameters, we don't want them to be mapped to path parameters.

/cc @tadelesh