Fix body optionality for contentType and request body setting #45528
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
samvaity
requested review from
vcolin7,
JonathanGiles,
srnagar and
alzimmermsft
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR enhances how request bodies and headers are handled by:
- Adding ISO_INSTANT formatting support for
OffsetDateTimein query and header parameters - Only setting the
Content-Typeheader when a non-null body is present and choosing serialization based on JSON vs. other types - Updating generated client templates and updating/removing legacy tests to reflect the new body‐optional behavior
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/clientcore/core/src/main/java/io/clientcore/core/utils/GeneratedCodeUtils.java | Added OffsetDateTime query‐parameter formatting support |
| sdk/clientcore/annotation-processor/src/main/java/io/clientcore/annotation/processor/utils/RequestBodyHandler.java | Refactored configureRequestBody to honor body optionality and JSON |
| sdk/clientcore/annotation-processor/src/main/java/io/clientcore/annotation/processor/templating/JavaParserTemplateProcessor.java | Updated template logic for body header handling and skip content-type |
| sdk/clientcore/annotation-processor/pom.xml | Lowered Jacoco coverage thresholds |
| sdk/clientcore/annotation-processor/src/test/java/io/clientcore/annotation/processor/utils/RequestBodyHandlerTest.java | Removed outdated RequestBodyHandler tests |
| sdk/clientcore/annotation-processor-test/src/test/java/io/clientcore/annotation/processor/test/serializers/XmlSerializableTests.java | Tweaked XML tests to propagate new header logic |
| sdk/clientcore/annotation-processor-test/src/test/java/io/clientcore/annotation/processor/test/TestInterfaceServiceClientGenerationTests.java | Adjusted JSON body expectations (now quoted) |
| sdk/clientcore/annotation-processor-test/src/main/java/io/clientcore/annotation/processor/test/implementation/TestInterfaceClientServiceImpl.java | Regenerated client impls to only set Content-Type when body exists |
| sdk/clientcore/annotation-processor-test/src/main/java/io/clientcore/annotation/processor/test/implementation/SpecialReturnBodiesService.java | Added new rfc3339 and omit operations to service interface |
| sdk/clientcore/annotation-processor-test/src/main/java/io/clientcore/annotation/processor/test/SpecialReturnBodiesServiceImpl.java | Implemented new rfc3339 and omit behaviors with ISO_INSTANT |
| sdk/clientcore/annotation-processor-test/src/main/java/io/clientcore/annotation/processor/test/SimpleXmlSerializableServiceImpl.java | Moved XML header setup inside null check |
Comments suppressed due to low confidence (3)
sdk/clientcore/annotation-processor/src/main/java/io/clientcore/annotation/processor/utils/RequestBodyHandler.java:51
- [nitpick] The variable name
contentTypeParamParamOptis verbose and contains a repeated 'Param'. Consider renaming it tocontentTypeParamOptfor clarity.
Optional<HttpRequestContext.MethodParameter> contentTypeParamParamOpt
sdk/clientcore/annotation-processor/src/test/java/io/clientcore/annotation/processor/utils/RequestBodyHandlerTest.java:1
- Removing the
RequestBodyHandlerTestleaves new request-body logic untested. Add replacement tests to cover cases whereContent-Typeis only set when the body is non-null.
Entire test file removed
sdk/clientcore/annotation-processor/src/main/java/io/clientcore/annotation/processor/utils/RequestBodyHandler.java:247
- Switching to always JSON‐serialize non-null bodies introduces quoting for string payloads (including empty strings). This is a behavioral change—ensure it’s documented as a breaking change in release notes.
if (bodyContentType != null && bodyContentType.trim().equalsIgnoreCase(ContentType.APPLICATION_JSON)) {
...src/main/java/io/clientcore/annotation/processor/templating/JavaParserTemplateProcessor.java
Show resolved
Hide resolved
alzimmermsft
approved these changes
May 30, 2025
srnagar
approved these changes
May 30, 2025
| @@ -457,6 +441,10 @@ private void addHeadersToRequest(BlockStmt body, HttpRequestContext method) { | |||
| // Start building the header addition for the HttpRequest. | |||
| StringBuilder addHeader = new StringBuilder(); | |||
|
|
|||
| String constantName = LOWERCASE_HEADER_TO_HTTPHEADENAME_CONSTANT.get(headerKey.toLowerCase(Locale.ROOT)); | |||
There was a problem hiding this comment.
nit: typo in the name - LOWERCASE_HEADER_TO_HTTPHEADERNAME_CONSTANT
| @@ -457,6 +441,10 @@ private void addHeadersToRequest(BlockStmt body, HttpRequestContext method) { | |||
| // Start building the header addition for the HttpRequest. | |||
| StringBuilder addHeader = new StringBuilder(); | |||
|
|
|||
| String constantName = LOWERCASE_HEADER_TO_HTTPHEADENAME_CONSTANT.get(headerKey.toLowerCase(Locale.ROOT)); | |||
| if ("CONTENT_TYPE".equals(constantName)) { | |||
| continue; | |||
There was a problem hiding this comment.
nit: Add a comment to specify why we are skipping content type header handling here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description