-
Notifications
You must be signed in to change notification settings - Fork 332
http-client-java, bug fix, array encoding support closed enum #9413
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?
http-client-java, bug fix, array encoding support closed enum #9413
Conversation
|
No changes needing a change description found. |
|
You can try these changes here
|
|
could you please show the generated code diff by adding the fix? |
I cannot, because without this fix, emitter crashes, and there is no code generated. |
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 array encoding (comma-separated string representation) of closed enum types in the Java HTTP client generator. The change introduces a new ConvertFromJsonTypeTrait interface and updates the deserialization logic to properly handle enum values when deserializing from array-encoded strings.
Changes:
- Introduced
ConvertFromJsonTypeTraitinterface for converting JSON wire values back to client types - Implemented the new trait on
PrimitiveType,ClassType, andEnumTypeto support bidirectional conversion - Fixed array encoding deserialization logic in
StreamSerializationModelTemplateto use the new conversion trait instead of relying ondefaultValueExpression - Added unit tests for both
ConvertToJsonTypeTraitandConvertFromJsonTypeTraitconversion methods
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ConvertFromJsonTypeTrait.java | New interface defining conversion from JSON wire type to client type |
| ConvertToJsonTypeTrait.java | Updated documentation to clarify it converts to JSON wire type |
| PrimitiveType.java | Implements ConvertFromJsonTypeTrait and updates convertToJsonType to properly handle client-to-wire conversion |
| ClassType.java | Implements ConvertFromJsonTypeTrait with explicit handling for various special types like DATE_TIME, DURATION, URL |
| EnumType.java | Implements ConvertFromJsonTypeTrait to support enum deserialization from JSON values |
| StreamSerializationModelTemplate.java | Fixes array encoding deserialization to use ConvertFromJsonTypeTrait instead of defaultValueExpression, with proper error handling |
| ConvertToJsonTypeTraitTests.java | New test file verifying type conversion to JSON representation |
| ConvertFromJsonTypeTraitTests.java | New test file verifying type conversion from JSON representation |
...osoft/typespec/http/client/generator/core/model/clientmodel/ConvertToJsonTypeTraitTests.java
Show resolved
Hide resolved
...oft/typespec/http/client/generator/core/model/clientmodel/ConvertFromJsonTypeTraitTests.java
Show resolved
Hide resolved
...main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ClassType.java
Outdated
Show resolved
Hide resolved
…/src/main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ClassType.java Co-authored-by: Copilot <[email protected]>
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
...main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ClassType.java
Show resolved
Hide resolved
.../java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/PrimitiveType.java
Show resolved
Hide resolved
| // TODO (weidxu): it may be better to refactor it to type initialization, similar as | ||
| // defaultValueExpressionConverter | ||
| if (this == ClassType.INTEGER_AS_STRING) { | ||
| return variableName + " == null ? null : Integer.parseInt(" + variableName + ")"; | ||
| } else if (this == ClassType.LONG_AS_STRING) { | ||
| return variableName + " == null ? null : Long.parseInt(" + variableName + ")"; | ||
| } else if (this == ClassType.DATE_TIME) { | ||
| return variableName + " == null ? null : OffsetDateTime.parse(" + variableName + ")"; | ||
| } else if (this == ClassType.DATE_TIME_RFC_1123) { | ||
| return variableName + " == null ? null : new DateTimeRfc1123(" + variableName + ")"; | ||
| } else if (this == ClassType.DURATION) { | ||
| return variableName + " == null ? null : Duration.parse(" + variableName + ")"; | ||
| } else if (this == ClassType.URL) { | ||
| return variableName + " == null ? null : new URL(" + variableName + ")"; | ||
| } else { | ||
| return convertToClientType(variableName); | ||
| } |
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.
Wonder can we put this into convertToClientType implementation?
And what's the difference between wireType and jsonType?
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.
Wire type can be e.g. DateTimeRfc1123. JSON type would be JSON string/numeric/boolean etc. (object or array not counted)
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 "wire type" in codegen may not be what you think "on wire".
It is more an "internal type" (compared to client type as "customer facing type").
For models, since codegen now own the serialization/de- (no Jackson needed), this "internal type" may not be useful at all. E.g. we should be able to directly convert a JSON string to OffsetDateTime, according to RFC1123/7231 protocol (instead of the usual RFC3339).
However, for proxy method, the core / clientcore still takes this "internal type" for request parameter / body / response body.
test not yet published https://github.com/microsoft/typespec/blob/main/packages/http-specs/specs/encode/array/main.tsp#L44-L62