-
Notifications
You must be signed in to change notification settings - Fork 447
chore: added a common serializer and its tests #878
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?
Conversation
* fix: use Object for types instead of Map * fix: use Object for anyType * fix: use Object for anyType
* fix: use Object for types instead of Map * fix: use Object for anyType * fix: use Object for anyType * fix: handle array of objects correctly * fix: add converter import statement
…8634d10afc0145e18ccb8d90598b642b67ad87
…1169e3becf53bb375f9c7413aec0c78fe78fcc
…518c9f2f9294597fd0471d5c647016ef4023ee
|
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 introduces a centralized serialization utility to standardize how different data types are converted to strings and added to HTTP requests. The change eliminates the need for type-specific handling in the twilio-oai-generator by providing a common Serializer
class.
- Added a
Serializer
class with overloadedtoString
methods for various data types - Introduced a
ParameterType
enum to distinguish between query, header, and URL-encoded parameters - Comprehensive test coverage for all supported data types and edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
src/main/java/com/twilio/converter/Serializer.java | Core serializer implementation with generic and date-specific toString methods |
src/test/java/com/twilio/converter/SerializerTest.java | Comprehensive test suite covering all data types and parameter types |
src/main/java/com/twilio/constant/EnumConstants.java | Added ParameterType enum for distinguishing parameter destinations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
private static <T> String convertToString(T value) { | ||
if (value instanceof Map) { | ||
return Converter.mapToJson((Map<String, ? extends Object>) value); |
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 cast to (Map<String, ? extends Object>)
is unsafe and may cause ClassCastException at runtime. Consider using a type-safe approach with instanceof checks or generic bounds to ensure the cast is valid.
return Converter.mapToJson((Map<String, ? extends Object>) value); | |
Map<?, ?> map = (Map<?, ?>) value; | |
boolean allStringKeys = true; | |
for (Object key : map.keySet()) { | |
if (!(key instanceof String)) { | |
allStringKeys = false; | |
break; | |
} | |
} | |
if (allStringKeys) { | |
@SuppressWarnings("unchecked") | |
Map<String, ? extends Object> stringKeyMap = (Map<String, ? extends Object>) map; | |
return Converter.mapToJson(stringKeyMap); | |
} else { | |
// Fallback: not a Map<String, ?>, use toString | |
return String.valueOf(value); | |
} |
Copilot uses AI. Check for mistakes.
@Test | ||
public void testToStringWithLocalDate() { | ||
Request request = buildRequest(); | ||
LocalDate date = LocalDate.of(2025, 07, 1); |
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 month value 07
should be written as 7
instead of using octal notation. While 07
equals 7
in this context, using leading zeros can be misleading and suggests octal representation.
Copilot uses AI. Check for mistakes.
LocalDate dateBefore = LocalDate.of(2025, 07, 5); | ||
LocalDate dateAfter = LocalDate.of(2025, 07, 1); |
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 month value 07
should be written as 7
instead of using octal notation. While 07
equals 7
in this context, using leading zeros can be misleading and suggests octal representation.
Copilot uses AI. Check for mistakes.
Request request = buildRequest(); | ||
LocalDate date = null; | ||
LocalDate dateBefore = null; | ||
LocalDate dateAfter = LocalDate.of(2025, 07, 1); |
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 month value 07
should be written as 7
instead of using octal notation. While 07
equals 7
in this context, using leading zeros can be misleading and suggests octal representation.
Copilot uses AI. Check for mistakes.
public void testToStringWithLocalDateRangeAfterNull() { | ||
Request request = buildRequest(); | ||
LocalDate date = null; | ||
LocalDate dateBefore = LocalDate.of(2025, 07, 5); |
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 month value 07
should be written as 7
instead of using octal notation. While 07
equals 7
in this context, using leading zeros can be misleading and suggests octal representation.
Copilot uses AI. Check for mistakes.
addParamToRequest(request, key, stringValue, parameterType); | ||
} | ||
|
||
private static <T> String convertToString(T value) { |
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 objectToJson() can be used here instead
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.
You mean using mapper to convert to string ?
We can not use it, because \n (new line) is treated as actual new line for toString method, whereas mapper keep /n as it as.
break; | ||
case URLENCODED: | ||
request.addPostParam(key, value); | ||
break; |
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.
How will body params be added?
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.
addressed json content type serialization.
6533f63
to
78a8ccd
Compare
|
Fixes
Added a centralized serialization so that we don't have to identify variable type in twilio-oai-generator.
Checklist
If you have questions, please file a support ticket, or create a GitHub Issue in this repository.