-
Notifications
You must be signed in to change notification settings - Fork 323
http-client-java, support advanced-versioning on @added optional parameter in TypeSpec, in non-DPG
#9087
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for advanced-versioning in non-DPG (Data Plane Generation) mode for the http-client-java emitter, specifically targeting management (ARM) scenarios. The feature generates method overloads for operations with versioned parameters, allowing REST APIs to add optional parameters across versions while maintaining backward compatibility.
Key changes:
- Updates
@azure-tools/typespec-azure-resource-managerdependency from 0.62.0 to 0.62.1 - Extends versioning support from DPG to management plane code generation
- Adds logic to generate multiple method signatures for operations with
@addedversioned parameters - Includes comprehensive test case with ARM resource operations demonstrating the feature
Reviewed changes
Copilot reviewed 36 out of 40 changed files in this pull request and generated 62 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/http-client-java/package.json | Updates dependency versions including ARM resource manager to 0.62.1 |
| packages/http-client-java/package-lock.json | Lock file updates matching package.json changes |
| packages/http-client-java/generator/http-client-generator-test/tsp/arm-versioned.tsp | New TypeSpec test specification demonstrating versioned ARM operations |
| packages/http-client-java/generator/http-client-generator-test/Generate.ps1 | Adds configuration to enable advanced-versioning option for arm-versioned test |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/mapper/ClientMethodMapper.java | Refactors versioning overload generation to support non-DPG scenarios with context parameters |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/template/ClientMethodTemplate.java | Improves logic for determining when optional parameters need local variable instantiation |
| packages/http-client-java/generator/http-client-generator-mgmt/src/main/java/com/microsoft/typespec/http/client/generator/mgmt/mapper/FluentClientMethodMapper.java | Adds versioning overload generation for LRO methods in management plane |
| packages/http-client-java/generator/http-client-generator-mgmt/src/main/java/com/microsoft/typespec/http/client/generator/mgmt/model/clientmodel/fluentmodel/ResourceOperation.java | Improves parameter selection by choosing client method with most parameters |
| Multiple generated Java files | Generated test code demonstrating the advanced-versioning feature output |
Files not reviewed (1)
- packages/http-client-java/package-lock.json: Language not supported
...in/java/com/microsoft/typespec/http/client/generator/core/template/ClientMethodTemplate.java
Show resolved
Hide resolved
...oft/typespec/http/client/generator/mgmt/model/clientmodel/fluentmodel/ResourceOperation.java
Outdated
Show resolved
Hide resolved
...in/java/com/microsoft/typespec/http/client/generator/core/template/ClientMethodTemplate.java
Outdated
Show resolved
Hide resolved
...rator/http-client-generator-test/src/main/java/tsptest/armversioned/ArmVersionedManager.java
Show resolved
Hide resolved
...generator-test/src/main/java/tsptest/armversioned/implementation/ArmVersionedClientImpl.java
Show resolved
Hide resolved
...r-test/src/main/java/tsptest/armversioned/implementation/TopLevelArmResourcesClientImpl.java
Show resolved
Hide resolved
...r-test/src/main/java/tsptest/armversioned/implementation/TopLevelArmResourcesClientImpl.java
Show resolved
Hide resolved
...r-test/src/main/java/tsptest/armversioned/implementation/TopLevelArmResourcesClientImpl.java
Show resolved
Hide resolved
...r-test/src/main/java/tsptest/armversioned/implementation/TopLevelArmResourcesClientImpl.java
Show resolved
Hide resolved
...r-test/src/main/java/tsptest/armversioned/implementation/TopLevelArmResourcesClientImpl.java
Show resolved
Hide resolved
…/src/main/java/com/microsoft/typespec/http/client/generator/core/template/ClientMethodTemplate.java Co-authored-by: Copilot <[email protected]>
…com/weidongxu-microsoft/typespec into http-client-java_versioning-non-dpg
|
Updated test case (now include Get and Delete) |
advanced-versioning in non-DPGadvanced-versioning on @added optional parameter in TypeSpec, in non-DPG
| $tspOptions += " --option ""@typespec/http-client-java.customization-class=../../customization/src/main/java/KeyVaultCustomization.java""" | ||
| } elseif ($tspFile -match "tsp[\\/]arm-versioned.tsp") { | ||
| # enable advanced versioning for resiliency test | ||
| $tspOptions += " --option ""@typespec/http-client-java.advanced-versioning=true""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for understanding purpose: is advanced-versioning default to false, so we have to explicitly set true here? Do we have plan to default it to true?
| * Mono<PagedResponseBase<H,T>>. | ||
| */ | ||
| PagingAsyncSinglePage(2, true, false, false), | ||
| SimulatedPagingSync(3, false, false, true), // unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double confirm: is this just code clean up unrelated to advanced versioning?
| // If the parameter isn't required and the client method only uses required parameters, optional | ||
| // parameters are omitted and will need to instantiated in the method. | ||
| boolean optionalOmitted = clientMethod.getOnlyRequiredParameters() && !parameter.isRequired(); | ||
| // If the parameter isn't required and the client method only uses required parameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the client method only uses required parameters: just trying to understasnd the meaning. Does it mean in the client method with only required parameters, we need to initialize the optional parameters value? But I thought in client method with full parameters, we need to initialize optional parameters if user does not provide as well.
Would be awesome if we could add an example (better in javadoc) to explain the case we want to handle in the logics from line 176-190?
|
|
||
| private static void createOverloadForVersioning(boolean isProtocolMethod, List<ClientMethod> methods, | ||
| ClientMethod baseMethod) { | ||
| protected void createOverloadForVersioning(List<ClientMethod> methods, ClientMethod baseMethod, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth to add some javadoc about this method.
| * @throws RuntimeException all other wrapped checked exceptions if the request fails to be sent. | ||
| */ | ||
| @ServiceMethod(returns = ReturnType.SINGLE) | ||
| void delete(String resourceGroupName, String topLevelArmResourcePropertiesName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does delete need overloaded functions similar as deleteWithResponse? e.g.
void delete(String resourceGroupName, String topLevelArmResourcePropertiesName, String parameter, Context context)
void delete(String resourceGroupName, String topLevelArmResourcePropertiesName, String parameter, String newParameter, Context context)
fix Azure/autorest.java#3225
This is already supported in DPG.
Here we support this for management. It should also apply to azure-v2, but we do not have test on it (as azure-v2 lib is not released).
This is enabled by
advanced-versioningemitter option.Test case
typespec/packages/http-client-java/generator/http-client-generator-test/tsp/arm-versioned.tsp
Lines 40 to 42 in 00fcc47
The difference on output be demonstrated here
2fe0045