support java.time.Instant serialization with gson #5298
Conversation
dosubot
bot
added
the
size:S
WalkthroughThe pull request introduces several updates, features, and fixes for Apollo version 2.4.0. Key changes include enhancements to server configuration links, a REST template client connection pool configuration, and the ability for administrators to perform global searches. Notable fixes address issues like duplicate comments and missing items in published namespaces. New features involve limits on namespaces per app ID, status access key observation, and a caching function for ConfigService. Additionally, dynamic configuration support for Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java:43
- The new serialization and deserialization logic for Instant is not covered by tests. Please add unit tests to verify the correct behavior of the custom Gson TypeAdapter for Instant.
// Custom Gson TypeAdapter for Instant
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java (1)
52-55: LGTM! GsonBuilder configuration is correct.The type adapters are properly registered while maintaining the existing date format configuration.
The PR objectives mention that unit tests are missing. Would you like me to help generate unit tests for the new serializer/deserializer implementation? This should include test cases for:
- Successful serialization/deserialization
- Handling of null values
- Error cases with malformed input
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGES.md(1 hunks)apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java(2 hunks)
🔇 Additional comments (3)
CHANGES.md (1)
28-28: LGTM! Change is properly documented.
The changelog entry accurately describes the feature addition and includes the correct PR link.
apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java (2)
22-24: LGTM! Required imports are properly added.
The new imports are correctly added for Gson serialization components and java.time.Instant.
43-55: Verify existing Instant usage in the codebase.
Let's ensure this change doesn't affect existing Instant serialization/deserialization.
✅ Verification successful
Let me gather more information about how Instant is actually used in these files.
Let me check the actual date/time field usage in the entities and DTOs to ensure we understand the full impact.
Based on the gathered information, I can now provide the final response:
The Instant serialization changes are safe to implement
The codebase analysis shows:
- Current Instant usage is minimal and purely for internal date conversions in
RelativeDateFormatand test code - The main date/time handling in entities and DTOs uses
java.util.Date - Only one other Gson configuration exists in
ConfigChangeContentBuilder, which uses a different date format forDateobjects - No existing JSON serialization/deserialization of
Instanttype is found
The new Instant type adapter implementation is an addition rather than a modification of existing behavior, making it safe to implement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing Instant usage in the codebase
echo "Searching for Instant usage in Java files..."
rg "java\.time\.Instant" --type java
echo -e "\nSearching for potential JSON serialization of Instant..."
ast-grep --pattern 'class $_ {
$$$
private Instant $_;
$$$
}'
Length of output: 713
Script:
#!/bin/bash
# Check the actual usage of Instant in the identified files
echo "Checking RelativeDateFormat.java..."
rg -A 5 "Instant" apollo-portal/src/main/java/com/ctrip/framework/apollo/portal/util/RelativeDateFormat.java
echo -e "\nChecking AccessKeyRepositoryTest.java..."
rg -A 5 "Instant" apollo-biz/src/test/java/com/ctrip/framework/apollo/biz/repository/AccessKeyRepositoryTest.java
# Look for any JSON-related classes that might be using date/time fields
echo -e "\nChecking for potential JSON response objects with date/time fields..."
rg -l "class.*(\{|implements)" --type java | xargs rg -l "Date|LocalDateTime|ZonedDateTime|Calendar"
Length of output: 8288
Script:
#!/bin/bash
# Check BaseEntity and BaseDTO as they might be parent classes for other entities
echo "Checking BaseEntity.java..."
rg -A 5 "Date" apollo-common/src/main/java/com/ctrip/framework/apollo/common/entity/BaseEntity.java
echo -e "\nChecking BaseDTO.java..."
rg -A 5 "Date" apollo-common/src/main/java/com/ctrip/framework/apollo/common/dto/BaseDTO.java
# Check if there are any existing Gson configurations
echo -e "\nChecking for other Gson configurations..."
rg "GsonBuilder|registerTypeAdapter" --type java
Length of output: 3787
...ain/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
apollo-common/src/main/java/com/ctrip/framework/apollo/common/controller/HttpMessageConverterConfiguration.java:46
- The new behavior for Instant serialization and deserialization is not covered by tests. Please add unit tests to verify this functionality.
JsonSerializer<Instant> instantJsonSerializer = (src, typeOfSrc, context) ->
What's the purpose of this PR
support java.time.Instant serialization with gson
Which issue(s) this PR fixes:
Fixes #5257
Brief changelog
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
java.time.Instantserialization with Gson.Bug Fixes
Refactor
RefreshAdminServerAddressTasktime interval.Documentation