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

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

Open
Tracked by #2931
haolingdong-msft opened this issue Aug 28, 2024 · 6 comments
Assignees
Labels
design:needed A design request has been raised that needs a proposal feature New feature or request lib:tcgc Issues for @azure-tools/typespec-client-generator-core library

Comments

@haolingdong-msft
Copy link
Member

haolingdong-msft commented Aug 28, 2024

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.

  • For above example, if it is multipart body, we need to generate the correct type(not bytes provided by SdkMethodParameter) for the method signature and have additional handling in convenient method impl before calling protocol API;
  • accept and content-type headers will not be added to convenient method signature;
  • etc (There could be other cases where we need information from http parameter to generate convenient method, like json-merge-patch).

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?

@haolingdong-msft haolingdong-msft added the lib:tcgc Issues for @azure-tools/typespec-client-generator-core library label Aug 28, 2024
@haolingdong-msft haolingdong-msft changed the title [Discussion] Could TCGC provide an easier way to map from SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter? [Discussion][TCGC] Could TCGC provide an easier way to map from SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter? Aug 28, 2024
@iscai-msft
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

haolingdong-msft commented Aug 29, 2024

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
Copy link
Contributor

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
Copy link
Member

qiaozha commented Sep 6, 2024

@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

@tadelesh tadelesh self-assigned this Sep 23, 2024
@tadelesh tadelesh added design:needed A design request has been raised that needs a proposal feature New feature or request labels Oct 16, 2024
@haolingdong-msft haolingdong-msft changed the title [Discussion][TCGC] Could TCGC provide an easier way to map from SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter? [TCGC] Could TCGC provide an easier way to map from SdkMethodParameter to SdkPathParameter | SdkQueryParameter | SdkHeaderParameter | SdkBodyParameter? Nov 13, 2024
@qiaozha
Copy link
Member

qiaozha commented Dec 13, 2024

Have another issue in filtering cookie parameter in the method level Azure/autorest.typescript#2943 (comment) which could benefit from here if we could have the link from SdkMethodParameter to SdkHttpParameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:needed A design request has been raised that needs a proposal feature New feature or request lib:tcgc Issues for @azure-tools/typespec-client-generator-core library
Projects
None yet
Development

No branches or pull requests

5 participants
@tadelesh @iscai-msft @qiaozha @haolingdong-msft and others