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 client-level subscriptionId parameter #2348

Closed
Tracked by #2347
MaryGao opened this issue Mar 13, 2024 · 2 comments · Fixed by #2615
Closed
Tracked by #2347

Support client-level subscriptionId parameter #2348

MaryGao opened this issue Mar 13, 2024 · 2 comments · Fixed by #2615
Assignees
Labels
HRLC Mgmt This issue is related to a management-plane library. p0 priority 0

Comments

@MaryGao
Copy link
Member

MaryGao commented Mar 13, 2024

Background

This is found when we compared the gaps between HLC and mdoular. In Azure mgmt SDK, some services define subscriptionId as client level(see example) and others define as method level(see example). The x-ms-parameter-location could differentiate the location(def here).

In modular we need to support them both. Currently typespec can't express client-level parameter except ones in parameterized host. But TCGC is planning to support this(see issue).

Spec here

Proposal

Here is the proposal on how to support client-level subscriptionId.

  • RLC

In RLC we will keep REST-style so subscriptionId will be taken as method parameter because it is path-level parameter.

export interface Routes {
  /** Resource for '/providers/Microsoft.NetworkAnalytics/operations' has methods for the following verbs: get */
  (path: "/providers/Microsoft.NetworkAnalytics/operations"): OperationsList;
  /** Resource for '/subscriptions/\{subscriptionId\}/resourceGroups/\{resourceGroupName\}/providers/Microsoft.NetworkAnalytics/dataProductsCatalogs/default' has methods for the following verbs: get */
  (
    path: "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.NetworkAnalytics/dataProductsCatalogs/default",
    subscriptionId: string,
    resourceGroupName: string,
  ): DataProductsCatalogsGet;
// ....
}

Example code

const client = createClient(credential);
const result = client
      .path(
        "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.NetworkAnalytics/dataProductsCatalogs/default",
        "{subscriptionId}",
        "{resourceGroupName}"
      )
      .get();
  • API layer

Similar to RLC the subId would be in method level also. Generated code:

export async function get(
  context: Client,
  subscriptionId: string,
  resourceGroupName: string,
  options: DataProductsCatalogsGetOptionalParams = { requestOptions: {} },
): Promise<DataProductsCatalog>

Example code

const result = get(context, subscriptionId, resourceGroupName, options);
  • Classical layer

For client-level subscriptionId it will be in client constructor.

export class NetworkAnalyticsClient {
    constructor(credential: TokenCredential, subscriptionId: string, options?: NetworkAnalyticsClientOptions);
    readonly dataProducts: DataProductsCatalogsOperations;
    // ...
}

export interface DataProductsCatalogsOperations {
  get: (
    resourceGroupName: string,
    options?: DataProductsCatalogsGetOptionalParams
  ) => Promise<DataProductsCatalog>;
  // ...
}

Example code would be like:

const classicalClient = new NetworkAnalyticsClient(
      credential,
      "{subscriptionId}"
    );
const result = classicalClient.dataProductsCatalogs.get("resourceGroupName");

Short-term solution

Considering the design of client-level parameter in TCGC is not finalized, currently our generation would cause a lot of un-necessary breakings between HLC and modular. So I prefer we could enable the subscriptionId as client level param for ARM in compatibilityMode.

For long term we would trust what the customers defined in client.tsp.

@MaryGao MaryGao changed the title The param subscriptionId is not prompted to client level for cross all operations' one The param subscriptionId is not prompted to client level for cross all operations Mar 13, 2024
@MaryGao MaryGao self-assigned this Mar 13, 2024
@MaryGao MaryGao added Mgmt This issue is related to a management-plane library. HRLC priority-0 labels Mar 13, 2024
@qiaozha qiaozha added p0 priority 0 and removed priority-0 labels Apr 4, 2024
@MaryGao

This comment was marked as outdated.

@MaryGao MaryGao changed the title The param subscriptionId is not prompted to client level for cross all operations Revisit client level parameters for cross all operations May 21, 2024
@MaryGao MaryGao changed the title Revisit client level parameters for cross all operations Revisit client level parameters cross all operations May 21, 2024
@MaryGao MaryGao changed the title Revisit client level parameters cross all operations Support client-level subscriptionId parameter May 22, 2024
@MaryGao
Copy link
Member Author

MaryGao commented May 23, 2024

Breaking Change Categorization
Bug (Unwanted Breaking Changes):

  • Missing data from the spec in the new implementation.
  • Incorrect results produced by the new implementation.
  • Slight differences in the new implementation that do not improve user experience. If uncertain, categorize as unwanted.

Improvements (Acceptable Breaking Changes):

  • Fixes broken behavior in the new implementation.
  • Offers clear enhancements to user experience.
  • Eliminates or discourages bad patterns.
  • Provides additional functionality.
  • Aligns more closely with TypeSpec standards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HRLC Mgmt This issue is related to a management-plane library. p0 priority 0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants