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

Support optional client-level parameter derived from x-ms-parameterized-host #1635

Closed
MaryGao opened this issue Oct 25, 2022 · 1 comment · Fixed by #1643
Closed

Support optional client-level parameter derived from x-ms-parameterized-host #1635

MaryGao opened this issue Oct 25, 2022 · 1 comment · Fixed by #1643
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. DPG/RLC v2.0 GA DPG Epic: Recreate API Contracts and SDKs https://github.com/Azure/cadl-azure/issues/1948 LowLevelClient

Comments

@MaryGao
Copy link
Member

MaryGao commented Oct 25, 2022

Background

In recent RLC release we notice the requirement to handle optional client-level parameters in codegen.

In our current implementation we put these parameters in the signature of client factory. This could work fine for required parameter without default value. However if there exists default value we couldn't leverage it defined in swagger.

Current implementation

If we set the x-ms-parameterized-host as

  "x-ms-parameterized-host": {
    "hostTemplate": "{Endpoint}/atlas/{serviceVersion}",
    "useSchemePrefix": false,
    "parameters": [
      {
        "$ref": "#/parameters/Endpoint"
      },
      {
        "$ref": "#/parameters/serviceVersion"
      }
    ]
  },

and generated client constructor factory function look like this

export default function createClient(
  Endpoint: string,
  credentials: TokenCredential,
  serviceVersion: string,
  options: ClientOptions = {}
): PurviewCatalogClient {
  const baseUrl =
    options.baseUrl ?? `${Endpoint}/atlas/{serviceVersion}/catalog/api`;
    // ...
}

Proposal

Option 1(preferred) - Put necessary parameter into XXXClientOption interface

export interface AzureDevCenterClientOption extends ClientOptions {
   devCenterDnsSuffix: string;
}

export default function createClient(
  tenantId: string,
  devCenter: string,
  credentials: TokenCredential,
  options: AzureDevCenterClientOption = {}
): AzureDevCenterClient { 
  // Handle the default value here
}

Option 2 - Put the default value in method signature

export default function createClient(
  tenantId: string,
  devCenter: string = "asia",  credentials: TokenCredential,
  devCenterDnsSuffix: string = "devcenter.azure.com",  options: ClientOptions = {}
): AzureDevCenterClient {
  const baseUrl = options.baseUrl ?? `https://${tenantId}-${devCenter}.${devCenterDnsSuffix}`

We have two concerns

  1. Position: Usually we put the default value at the last position of the method. But if the parameter from required -> optional, it could involve the position change, finally it could introduce breaking change.
  2. Another concern is for callee to use the method. Like the below definition if the user wants to use the default value, with devCenter and devCenterDnsSuffix but inputs the configured options, he needs to call this way:
const client = createClient("tenantId", undefined, credentials, undefined, { customized options });

PS: To support apiVersion parameter we have dependency in autorest bug Azure/autorest#4656.

/cc @qiaozha @joheredi

@MaryGao MaryGao changed the title Support apiVersion in x-ms-parameterized-host to upgrade autorest Upgrade autorest to support apiVersion parameter in x-ms-parameterized-host Oct 25, 2022
@MaryGao MaryGao changed the title Upgrade autorest to support apiVersion parameter in x-ms-parameterized-host Support optional client-level parameter derived from x-ms-parameterized-host Oct 25, 2022
@MaryGao MaryGao added LowLevelClient Client This issue points to a problem in the data-plane of the library. labels Oct 26, 2022
@MaryGao MaryGao self-assigned this Oct 27, 2022
@MaryGao MaryGao added Epic: Recreate API Contracts and SDKs https://github.com/Azure/cadl-azure/issues/1948 DPG DPG/RLC v2.0b2 labels Oct 27, 2022
@MaryGao
Copy link
Member Author

MaryGao commented Oct 28, 2022

Move to GA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. DPG/RLC v2.0 GA DPG Epic: Recreate API Contracts and SDKs https://github.com/Azure/cadl-azure/issues/1948 LowLevelClient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants