tsp, dpg, keep enum type for client parameters, not change it to string type #4911

haolingdong-msft · 2024-10-30T10:19:13Z

Fix Azure/autorest.java#2974
related with https://gist.github.com/haolingdong-msft/df3f25a048baf83d8fabf5206aacc7db#8-clienttype-will-be-generated-after-adopting-tcgc

Existing code

We change enum type to string type in two classes:

ProxyParameterMapper:

typespec/packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/mapper/ProxyParameterMapper.java

Lines 69 to 71 in 08384aa

    
           if (settings.isDataPlaneClient()) { 
        
               clientType = SchemaUtil.removeModelFromParameter(parameterRequestLocation, clientType); 
        
           }

ServiceClientMapper:

typespec/packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/mapper/ServiceClientMapper.java

Lines 235 to 239 in 08384aa

    
           if (settings.isDataPlaneClient()) { 
        
               // mostly for Enum to String 
        
               serviceClientPropertyClientType = SchemaUtil.removeModelFromParameter(RequestParameterLocation.URI, 
        
                   serviceClientPropertyClientType); 
        
           }

Logic in this pr

Above classes calls SchemaUtil.removeModelFromParameter to remove enum type and change it to string type.
Currently I modify the logic in removeModelFromParameter() to not chang enum type to string type for dataplane path/uri parameters.

Would like to hear your thoughts on below: @weidongxu-microsoft @XiaofeiCao

The logics can also be put in each Mapper, currently I centralized the logics to SchemaUtil class.
We may also need to add a flag to control this because it will impact existing libraries. The flag can be named as path-parameter-enum-as-string.

Latest logics

After discussing with Weidong, I decided to put the logics into the mappers.

azure-sdk · 2024-10-30T10:21:54Z

No changes needing a change description found.

azure-sdk · 2024-10-30T10:32:46Z

You can try these changes here

🛝 Playground	🌐 Website	📚 Next docs

…eter

weidongxu-microsoft · 2024-11-04T01:54:07Z

...a/com/microsoft/typespec/http/client/generator/core/template/example/ProtocolTestWriter.java

-import com.microsoft.typespec.http.client.generator.core.model.clientmodel.ServiceClient;
-import com.microsoft.typespec.http.client.generator.core.model.clientmodel.ServiceClientProperty;
-import com.microsoft.typespec.http.client.generator.core.model.clientmodel.TestContext;
+import com.microsoft.typespec.http.client.generator.core.model.clientmodel.*;


nit, do no import *

weidongxu-microsoft · 2024-11-04T02:00:56Z

...or-core/src/main/java/com/microsoft/typespec/http/client/generator/core/util/SchemaUtil.java

-                returnType = ClassType.STRING;
+                boolean isPathOrURIParameter = RequestParameterLocation.PATH == parameterRequestLocation
+                    || RequestParameterLocation.URI == parameterRequestLocation;
+                boolean isEnumAsString = !(JavaSettings.getInstance().isDataPlaneClient() && isPathOrURIParameter);
+                if (isEnumAsString) { // do not change enum to string type for data-plane path/url parameter
+                    returnType = ClassType.STRING;
+                }


Wondering why isPathOrURIParameter matters here. Can we just say "DPG (from swagger) is string; anything from TypeSpec is enum" (the difference should be based on swagger / typespec)?

Trying to limit the scope to path/uri parameter only to reduce the impact, but using swagger/typespec to decide whether to make enum as string is simpler and more straightforward. I will try to use swagger/typespec to decide whether to make enum as string, and see the impact.

I think just try in ServiceClientMapper, call this method if swagger, don't call it if typespec (there should be a subclass for typespec).

If adding the condition to ServiceClientMapper, we will also need to add it to ProxyParameterMapper, otherwise generated proxy method will have issue, it will use string type instead of enum type. But if it is typespec, in ProxyParameterMapper, we also need to execute other logics except the enum part logic in removeModelFromParameter, e.g. below part will change body type to BinaryData

typespec/packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/util/SchemaUtil.java

Lines 224 to 225 in 27d1069

if (parameterRequestLocation == RequestParameterLocation.BODY) {

returnType = ClassType.BINARY_DATA;

Adding the checking logics to ProxyParameterMapper might make the checking a bit complex. That's why I added the logic in SchemaUtil currently.

Ideally, proxy method parameter should indeed be String. This is consistent with DPG protocol method / proxy method (that parameter be String and BinaryData).

The conversion seems should be done some other places (maybe protocol method?).

If there is real difficulty, I am fine keep the Enum on proxy method (if it is parameter from client).

Just don't condition on path/uri (I think swagger can have Enum type path/uri parameter from client too). Condition should be on swagger/typespec (probably also on parameter location -- I think we don't want to affect parameter from method).

make sense, I'm trying to add the conversion logic to change enum type to string type and keep enum type in proxy method be string.

weidongxu-microsoft · 2024-11-04T02:05:09Z

...a/com/microsoft/typespec/http/client/generator/core/template/example/ProtocolTestWriter.java

-                            expr = String.format("Configuration.getGlobalConfiguration().get(\"%1$s\", %2$s)",
-                                serviceClientProperty.getName().toUpperCase(Locale.ROOT), ClassType.STRING
-                                    .defaultValueExpression(serviceClientProperty.getName().toLowerCase(Locale.ROOT)));
+                            if (serviceClientProperty.getType() instanceof EnumType) {
+                                expr = String.format(
+                                    "%1$s.fromString(Configuration.getGlobalConfiguration().get(\"%2$s\", %3$s))",
+                                    serviceClientProperty.getType(),
+                                    serviceClientProperty.getName().toUpperCase(Locale.ROOT),
+                                    ClassType.STRING.defaultValueExpression(
+                                        serviceClientProperty.getName().toLowerCase(Locale.ROOT)));
+
+                            } else {
+                                expr = String.format("Configuration.getGlobalConfiguration().get(\"%1$s\", %2$s)",
+                                    serviceClientProperty.getName().toUpperCase(Locale.ROOT),
+                                    ClassType.STRING.defaultValueExpression(
+                                        serviceClientProperty.getName().toLowerCase(Locale.ROOT)));
+                            }


Could there be problem of non-string enum here? @XiaofeiCao

Though I guess the probability that we have non-string enum here be pretty low. (but if it could happen, better to add a TODO for reminder)

Yeah, for EnumType's deserialization method, we should call ((EnumType) serviceClientProperty.getType()).getFromMethodName().

…eter in mappers

…s-enum # Conflicts: # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/ClientAClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/ClientBClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/FirstClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/RenamedOperationClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/ServiceClientClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/TwoOperationGroupClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/client/structure/service/subnamespace/SecondClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/added/AddedClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/added/implementation/AddedClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/added/implementation/InterfaceV2sImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/added/models/Versions.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/madeoptional/MadeOptionalClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/madeoptional/implementation/MadeOptionalClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/madeoptional/models/Versions.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/removed/RemovedClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/removed/implementation/RemovedClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/removed/models/Versions.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/renamedfrom/RenamedFromClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/renamedfrom/implementation/NewInterfacesImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/renamedfrom/implementation/RenamedFromClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/renamedfrom/models/Versions.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/returntypechangedfrom/ReturnTypeChangedFromClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/returntypechangedfrom/implementation/ReturnTypeChangedFromClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/typechangedfrom/TypeChangedFromClientBuilder.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/typechangedfrom/implementation/TypeChangedFromClientImpl.java # packages/http-client-java/generator/http-client-generator-test/src/main/java/versioning/typechangedfrom/models/Versions.java # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-added_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-madeoptional_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-removed_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-renamedfrom_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-returntypechangedfrom_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/main/resources/META-INF/versioning-typechangedfrom_apiview_properties.json # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/ClientOperationGroupTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/DefaultClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/MultiClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/OperationGroupClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/RenameOperationTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/service/generated/ClientAClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/service/generated/FirstClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/service/generated/RenamedOperationClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/service/generated/ServiceClientClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/client/structure/service/generated/TwoOperationGroupClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/type/scalar/ScalarTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/added/AddedClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/added/generated/AddedClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/madeoptional/MadeOptionalClienTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/madeoptional/generated/MadeOptionalClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/removed/RemovedClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/removed/generated/RemovedClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/renamedfrom/RenamedFromClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/renamedfrom/generated/RenamedFromClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/returntypechangedfrom/generated/ReturnTypeChangedFromClientTestBase.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/typechangedfrom/TypeChangedFromClientTests.java # packages/http-client-java/generator/http-client-generator-test/src/test/java/versioning/typechangedfrom/generated/TypeChangedFromClientTestBase.java

microsoft-github-policy-service bot added the emitter:client:java Issue for the Java client emitter: @typespec/http-client-java label Oct 30, 2024

haolingdong-msft changed the title ~~tsp, dpg, remove logics that model enum to string for client parameters~~ tsp, dpg, keep enum type for client parameters, not model it to string Oct 30, 2024

haolingdong-msft added 3 commits November 1, 2024 16:12

logic to not change enum to string type for data-plane path/url param…

27d1069

…eter

test template change

74b3f84

regen

ee68228

haolingdong-msft force-pushed the host-parameter-enum-as-enum branch from 2d26fa8 to ee68228 Compare November 1, 2024 08:13

haolingdong-msft changed the title ~~tsp, dpg, keep enum type for client parameters, not model it to string~~ tsp, dpg, keep enum type for path/uri parameters, not model it to string Nov 1, 2024

haolingdong-msft changed the title ~~tsp, dpg, keep enum type for path/uri parameters, not model it to string~~ tsp, dpg, keep enum type for path/uri parameters, not change it to string type Nov 1, 2024

format

c81d303

weidongxu-microsoft reviewed Nov 4, 2024

View reviewed changes

haolingdong-msft changed the title ~~tsp, dpg, keep enum type for path/uri parameters, not change it to string type~~ tsp, dpg, keep enum type for client parameters, not change it to string type Nov 12, 2024

haolingdong-msft added 7 commits November 14, 2024 16:45

Refine logics to add the checks on whether to remove model from param…

7ca033d

…eter in mappers

format

32035eb

regenerate

78f6a3f

update tests

0e5b7df

remove import *

7f98ae5

add todo for non-string enum

cd304bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsp, dpg, keep enum type for client parameters, not change it to string type #4911

tsp, dpg, keep enum type for client parameters, not change it to string type #4911

haolingdong-msft commented Oct 30, 2024 •

edited

Loading

azure-sdk commented Oct 30, 2024

azure-sdk commented Oct 30, 2024

weidongxu-microsoft Nov 4, 2024

weidongxu-microsoft Nov 4, 2024 •

edited

Loading

haolingdong-msft Nov 5, 2024

weidongxu-microsoft Nov 5, 2024

haolingdong-msft Nov 5, 2024 •

edited

Loading

weidongxu-microsoft Nov 5, 2024 •

edited

Loading

weidongxu-microsoft Nov 5, 2024 •

edited

Loading

haolingdong-msft Nov 5, 2024

weidongxu-microsoft Nov 4, 2024

XiaofeiCao Nov 4, 2024

	if (settings.isDataPlaneClient()) {
	clientType = SchemaUtil.removeModelFromParameter(parameterRequestLocation, clientType);
	}

	if (settings.isDataPlaneClient()) {
	// mostly for Enum to String
	serviceClientPropertyClientType = SchemaUtil.removeModelFromParameter(RequestParameterLocation.URI,
	serviceClientPropertyClientType);
	}

	if (parameterRequestLocation == RequestParameterLocation.BODY) {
	returnType = ClassType.BINARY_DATA;

tsp, dpg, keep enum type for client parameters, not change it to string type #4911

Are you sure you want to change the base?

tsp, dpg, keep enum type for client parameters, not change it to string type #4911

Conversation

haolingdong-msft commented Oct 30, 2024 • edited Loading

Existing code

Logic in this pr

Latest logics

azure-sdk commented Oct 30, 2024

azure-sdk commented Oct 30, 2024

weidongxu-microsoft Nov 4, 2024

Choose a reason for hiding this comment

weidongxu-microsoft Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

haolingdong-msft Nov 5, 2024

Choose a reason for hiding this comment

weidongxu-microsoft Nov 5, 2024

Choose a reason for hiding this comment

haolingdong-msft Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

weidongxu-microsoft Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

weidongxu-microsoft Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

haolingdong-msft Nov 5, 2024

Choose a reason for hiding this comment

weidongxu-microsoft Nov 4, 2024

Choose a reason for hiding this comment

XiaofeiCao Nov 4, 2024

Choose a reason for hiding this comment

haolingdong-msft commented Oct 30, 2024 •

edited

Loading

weidongxu-microsoft Nov 4, 2024 •

edited

Loading

haolingdong-msft Nov 5, 2024 •

edited

Loading

weidongxu-microsoft Nov 5, 2024 •

edited

Loading

weidongxu-microsoft Nov 5, 2024 •

edited

Loading