-
Notifications
You must be signed in to change notification settings - Fork 324
http-client-java, support array encoded as CSV on property #9218
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, support array encoded as CSV on property #9218
Conversation
|
No changes needing a change description found. |
|
You can try these changes here
|
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.
This is not related to this PR/feature.
Removed due to failure in nightly build, because an unreleased commit there breaked this test into 4 tests. Hence test code here won't compile on dev.
Need to be added back in next typespec version bump. #9220
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 delimited array encoding (CSV and other formats) on properties in the http-client-java emitter. The implementation allows TypeSpec models to specify how arrays of strings should be serialized as delimited strings in JSON (comma, space, pipe, or newline-delimited) instead of as JSON arrays. The changes span the code generator, model templates, and include both Azure and clientcore test implementations.
Key Changes
- Introduces
ArrayEncodingenum to define delimiter strategies for array serialization - Extends
ClientModelPropertyto include array encoding metadata - Updates serialization/deserialization templates to handle delimited string encoding
- Adds validation to ensure only string element types are supported for array encoding
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/http-client-java/package.json | Updates TypeSpec compiler and various dev dependencies to newer versions |
| packages/http-client-java/package-lock.json | Lockfile updates corresponding to package.json changes |
| packages/http-client-java/generator/http-client-generator-test/src/test/java/encode/array/EncodeArrayTests.java | New test suite for array encoding with comma, space, pipe, and newline delimiters |
| packages/http-client-java/generator/http-client-generator-test/src/test/java/azure/clientgenerator/core/clientlocation/ClientLocationClientTests.java | Test file removed (appears to be replaced by generated tests) |
| packages/http-client-java/generator/http-client-generator-test/src/main/java/encode/array/models/*.java | Generated model classes implementing delimiter-based serialization for different array encoding types |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/template/StreamSerializationModelTemplate.java | Updates toJson/fromJson generation to handle array encoding with delimiter-based string serialization |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ClientModelProperty.java | Adds arrayEncoding field to track delimiter configuration |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ArrayEncoding.java | New enum defining delimiter types and their escape sequences |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/mapper/ModelPropertyMapper.java | Maps array encoding metadata from TypeSpec model to Java property |
| packages/http-client-java/generator/http-client-generator-core/src/main/java/com/microsoft/typespec/http/client/generator/core/extension/model/codemodel/Property.java | Adds arrayEncoding field to Property model |
| packages/http-client-java/generator/http-client-generator-clientcore-test/src/**/*.java | Clientcore test implementations mirroring the Azure test structure |
| packages/http-client-java/emitter/src/lib.ts | Adds diagnostic for unsupported non-string array encoding elements |
| packages/http-client-java/emitter/src/common/client.ts | Defines EncodedProperty interface for array encoding metadata |
| packages/http-client-java/emitter/src/code-model-builder.ts | Validates string element type requirement and maps array encoding to code model |
Files not reviewed (1)
- packages/http-client-java/package-lock.json: Language not supported
.../java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ArrayEncoding.java
Show resolved
Hide resolved
.../java/com/microsoft/typespec/http/client/generator/core/model/clientmodel/ArrayEncoding.java
Show resolved
Hide resolved
| methodBlock.ifBlock(propertyValueGetter + " != null", ifBlock -> { | ||
| String serializeExpression = propertyValueGetter | ||
| + ".stream().map(element -> element == null ? \"\" : element).collect(Collectors.joining(\"" | ||
| + property.getArrayEncoding().getDelimiter() + "\"))"; | ||
| methodBlock.line("jsonWriter.writeStringField(\"%s\", %s);", serializedName, | ||
| serializeExpression); | ||
| }); |
Copilot
AI
Dec 16, 2025
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.
When serializing array elements, null elements are converted to empty strings with "element == null ? "" : element". However, during deserialization with split(), there's no corresponding logic to convert empty strings back to null. This creates an asymmetry where null array elements become empty strings after a round-trip serialization/deserialization cycle. Consider either preserving null elements differently (e.g., using a special placeholder) or documenting that null elements will be converted to empty strings.
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.
This is valid concern. Had this in mind, particularly when the element type is utcDateTime, where empty string can only be mapped to null (no valid time on "").
However, for string, "" is valid.
...microsoft/typespec/http/client/generator/core/template/StreamSerializationModelTemplate.java
Outdated
Show resolved
Hide resolved
| jsonWriter.writeStringField("value", | ||
| this.value.stream().map(element -> element == null ? "" : element).collect(Collectors.joining(","))); |
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.
Both null and "" would map to same encoded string.
On the other direction, currently we map e.g. "," to ["", ""] for string element.
For other type that extend string, we may choose differently (or even break this behavior on string type too), e.g. "," on utcDateTime/uuid can only be [null, null]
Just hope service knows what it does on these...
...client-generator-clientcore-test/src/main/java/encode/array/CommaDelimitedArrayProperty.java
Outdated
Show resolved
Hide resolved
haolingdong-msft
left a comment
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.
Overall looks good to me. Add a nit. Thanks!
| isJsonMergePatch); | ||
| } else { | ||
| // wireType is String | ||
| // at present, only String element is supported |
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.
only String element is supported
nit: Do we want to add check here to mak sure the element type is string? Or could we at least mention we have the on emitter: https://github.com/microsoft/typespec/pull/9218/files#diff-a10993e19db8ea9bd765f4d6abeaff1624f8f9a3842b9203c207f07527ef3d2eR2869
Because I tried to find in the code below that do the check on supporting string element only, but seems did not find it. (happy to be corrected)
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.
js code would error when element is not String. https://github.com/microsoft/typespec/pull/9218/files#diff-3b4a90c1f1304a9d5376a1303d121b44f4d136282666142484ee61f445d540c6
| public JsonWriter toJson(JsonWriter jsonWriter) throws IOException { | ||
| jsonWriter.writeStartObject(); | ||
| jsonWriter.writeArrayField("value", this.value, (writer, element) -> writer.writeString(element)); | ||
| if (this.value != null) { |
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.
Generated code looks good to me.
bdc3cbd to
fd843b1
Compare
fix #9027
see https://github.com/microsoft/typespec/pull/9218/files#diff-e8b978e5080675f7527d4beb363a9dde604493dbff1db49116ea56c94b2339e1 on difference on generated code.
Currently only supports element as String. (the convert from a subtype to String need some refactor in existing code)
release PR is Azure/autorest.java#3242