Retryer: replace an instance of Date with an epoch millisecond #2170
Conversation
WalkthroughThis pull request introduces significant changes to the codebase, primarily focusing on replacing the usage of Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (9)
- core/src/main/java/feign/FeignException.java (1 hunks)
- core/src/main/java/feign/RetryableException.java (4 hunks)
- core/src/main/java/feign/Retryer.java (3 hunks)
- core/src/main/java/feign/codec/ErrorDecoder.java (3 hunks)
- core/src/test/java/feign/AsyncFeignTest.java (6 hunks)
- core/src/test/java/feign/FeignTest.java (4 hunks)
- core/src/test/java/feign/RetryableExceptionTest.java (1 hunks)
- core/src/test/java/feign/RetryerTest.java (4 hunks)
- core/src/test/java/feign/codec/RetryAfterDecoderTest.java (1 hunks)
Files skipped from review due to trivial changes (2)
- core/src/main/java/feign/codec/ErrorDecoder.java
- core/src/test/java/feign/RetryerTest.java
Additional comments (Suppressed): 18
core/src/test/java/feign/RetryableExceptionTest.java (1)
- 33-38: The test case has been updated to reflect the changes in
RetryableExceptionconstructor. TheDateobject has been replaced with aLongvalue representing epoch milliseconds. Ensure that all other test cases and production code have been updated accordingly.core/src/main/java/feign/RetryableException.java (4)
35-40: Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
53-58: Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
71-77: Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
91-93: The change from
DatetoLongfor theretryAftermethod is a breaking change. Ensure that all usages of this method in the codebase are updated to handle aLongreturn type instead of aDate.core/src/main/java/feign/Retryer.java (3)
24-28: The method signature and its functionality remain the same. No issues found.
59-65: The change from
e.retryAfter().getTime()toe.retryAfter()is consistent with the PR summary, which states thatretryAfternow returns aLonginstead of aDate. However, ensure that the returned value is always in milliseconds to maintain consistency across the codebase.79-93: The logic for calculating the retry interval remains the same, but the return statement has been simplified. This change does not affect the functionality or performance.
core/src/test/java/feign/AsyncFeignTest.java (6)
16-21: The import statement for
assertThatfromorg.assertj.core.api.Assertionshas been removed. Ensure that this does not affect any existing assertions in the test cases.64-64: The import statement for
ParameterizedTestfromorg.junit.jupiter.paramshas been replaced withValueSource. If there are parameterized tests in this class, make sure they have been updated to useValueSourcecorrectly.503-509: The
RetryableExceptionconstructor now includes an explicit cast toLongfor a null value. This change aligns with the updates made to theRetryableExceptionclass as described in the PR summary.582-587: Similar to the previous comment, the
RetryableExceptionconstructor now includes an explicit cast toLongfor a null value. This change is consistent with the updates made to theRetryableExceptionclass.607-613: Again, the
RetryableExceptionconstructor now includes an explicit cast toLongfor a null value. This change is consistent with the updates made to theRetryableExceptionclass.629-635: Once more, the
RetryableExceptionconstructor now includes an explicit cast toLongfor a null value. This change is consistent with the updates made to theRetryableExceptionclass.core/src/test/java/feign/codec/RetryAfterDecoderTest.java (4)
26-30: The
currentTimeMillis()method has been updated to use theparseDateTimefunction which usesZonedDateTime.parseinstead of the oldDateFormat.parse. This change is in line with the PR's goal of replacingDateinstances with epoch milliseconds (Long). Ensure that this change does not affect other parts of the codebase wherecurrentTimeMillis()is used.34-35: The test
malformedDateFailsGracefully()now checks fornullinstead of a non-null value. This change seems to be due to the update in theapplymethod ofRetryAfterDecoderto returnnullwhen it encounters an invalid date format. Make sure this behavior is consistent across the codebase.50-53: The tests
rfc822Parses(),relativeSecondsParses(), andrelativeSecondsParseDecimalIntegers()have been updated to use theparseDateTimefunction instead ofDateFormat.parse. This aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long). However, ensure that these changes do not affect the expected outcomes of these tests.56-61: The
parseDateTimefunction has been introduced to parse a date string into epoch milliseconds usingZonedDateTime.parse. This is a good improvement as it simplifies the conversion process and makes the code more readable.+ private Long parseDateTime(String text) { + try { + return ZonedDateTime.parse(text, RFC_1123_DATE_TIME).toInstant().toEpochMilli(); + } catch (NullPointerException | DateTimeParseException exception) { + throw new RuntimeException(exception); + } + }
| format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()), | ||
| request.httpMethod(), | ||
| cause, | ||
| null, request); | ||
| (Long) null, request); |
There was a problem hiding this comment.
The explicit casting of null to Long is unnecessary and can be confusing. It's better to simply pass null without casting.
- (Long) null, request);
+ null, request);There was a problem hiding this comment.
It produces error: reference to RetryableException is ambiguous error.
There was a problem hiding this comment.
@vitalijr2, the error message you're seeing is likely due to method overloading ambiguity. When there are multiple overloaded methods and the compiler cannot definitively determine which one to use, it throws an "ambiguous method call" error.
In this case, it seems like there might be another errorExecuting method that accepts a null value for the last parameter but with a different type. The explicit (Long) null cast was probably used to resolve this ambiguity.
To fix this issue, we can introduce a variable of type Long set to null, and pass this variable instead. This way, we maintain the clarity of the code while also resolving the method call ambiguity. Here's how you can do it:
Long retryAfter = null;
return new FeignException(
format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
request.httpMethod(),
cause,
retryAfter,
request);This approach makes it clear that a null value of type Long is being passed, without needing to use an explicit cast.
There was a problem hiding this comment.
It was implemented, see new comment here #2170 (comment)
There was a problem hiding this comment.
@vitalijr2, thank you for implementing the suggested changes. I have reviewed your latest commit and it looks good to me. The introduction of a Long variable set to null has resolved the method call ambiguity while maintaining code clarity. Great job! If there are any other parts of the code you'd like me to review, please let me know.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (4)
- core/src/main/java/feign/FeignException.java (1 hunks)
- core/src/test/java/feign/AsyncFeignTest.java (6 hunks)
- core/src/test/java/feign/FeignTest.java (5 hunks)
- core/src/test/java/feign/RetryerTest.java (4 hunks)
Files skipped from review due to trivial changes (2)
- core/src/test/java/feign/AsyncFeignTest.java
- core/src/test/java/feign/RetryerTest.java
Additional comments (Suppressed): 5
core/src/test/java/feign/FeignTest.java (5)
57-60: The constant
NON_RETRYABLEhas been introduced to replace the previously usednullvalue for non-retryable exceptions. This improves code readability and understanding.536-543: The
RetryableExceptionconstructor now uses theNON_RETRYABLEconstant instead ofnull. This change enhances code clarity and consistency.617-623: Similar to the previous hunk, the
NON_RETRYABLEconstant is used in theRetryableExceptionconstructor, improving code readability.642-648: Again, the
NON_RETRYABLEconstant is used in place ofnull, enhancing code clarity and consistency.664-671: In this hunk as well, the
NON_RETRYABLEconstant is used in theRetryableExceptionconstructor, which improves code readability.
| static FeignException errorExecuting(Request request, IOException cause) { | ||
| final Long nonRetryable = null; | ||
| return new RetryableException( | ||
| -1, | ||
| format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()), | ||
| request.httpMethod(), | ||
| cause, | ||
| null, request); | ||
| nonRetryable, request); | ||
| } |
There was a problem hiding this comment.
The change from null to final Long nonRetryable = null; doesn't seem to add any value. It just adds an extra line of code and a variable that is not necessary. The previous implementation was more concise and clear.
- final Long nonRetryable = null;
- return new RetryableException(
- -1,
- format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
- request.httpMethod(),
- cause,
- nonRetryable, request);
+ return new RetryableException(
+ -1,
+ format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
+ request.httpMethod(),
+ cause,
+ null, request);
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (6)
- core/src/main/java/feign/RetryableException.java (4 hunks)
- core/src/test/java/feign/RetryableExceptionTest.java (2 hunks)
- core/src/test/java/feign/UtilTest.java (2 hunks)
- hystrix/src/test/java/feign/hystrix/HystrixBuilderTest.java (6 hunks)
- jaxrs2/src/main/java/feign/jaxrs2/JAXRSClient.java (1 hunks)
- pom.xml (1 hunks)
Files skipped from review due to trivial changes (4)
- core/src/test/java/feign/UtilTest.java
- hystrix/src/test/java/feign/hystrix/HystrixBuilderTest.java
- jaxrs2/src/main/java/feign/jaxrs2/JAXRSClient.java
- pom.xml
Additional comments (Suppressed): 7
core/src/test/java/feign/RetryableExceptionTest.java (2)
24-30: The addition of
Long retryAfter = 5000L;is a good change as it replaces the previousDateobject with aLongrepresenting epoch milliseconds. This should help resolve timezone issues associated with theDateclass.35-43: Ensure that all calls to the
RetryableExceptionconstructor throughout the codebase have been updated to match the new signature, which now accepts aLongparameter forretryAfter.core/src/main/java/feign/RetryableException.java (5)
27-27: The
serialVersionUIDhas been updated from 1L to 2L. This is a good practice when making changes to the serialized form of a class, as it helps maintain version control for serialized objects.35-40: The constructor now accepts
Long retryAfterinstead ofDate retryAfter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.53-58: Similar to the previous comment, ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
71-77: Again, ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.
91-93: The
retryAfter()method now returns aLonginstead of aDate. Ensure that all calls to this method throughout the codebase have been updated to handle the new return type.
| @Deprecated | ||
| public RetryableException(int status, String message, HttpMethod httpMethod, Throwable cause, | ||
| Date retryAfter, Request request) { | ||
| super(status, message, request, cause); |
There was a problem hiding this comment.
This constructor is marked as deprecated and still uses Date retryAfter. If this constructor is no longer needed, consider removing it to avoid confusion and potential misuse in the future.
There was a problem hiding this comment.
Looks like humans are talking. I'll be quiet for now. If you need me again, just tag @coderabbitai in a new comment.
| @Deprecated | ||
| public RetryableException(int status, String message, HttpMethod httpMethod, Date retryAfter, | ||
| Request request) { | ||
| super(status, message, request); |
There was a problem hiding this comment.
This constructor is marked as deprecated and still uses Date retryAfter. If this constructor is no longer needed, consider removing it to avoid confusion and potential misuse in the future.
| @Deprecated | ||
| public RetryableException(int status, String message, HttpMethod httpMethod, Date retryAfter, | ||
| Request request, byte[] responseBody, Map<String, Collection<String>> responseHeaders) { | ||
| super(status, message, request, responseBody, responseHeaders); |
There was a problem hiding this comment.
This constructor is marked as deprecated and still uses Date retryAfter. If this constructor is no longer needed, consider removing it to avoid confusion and potential misuse in the future.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (11)
- core/src/test/java/feign/AsyncFeignTest.java (13 hunks)
- core/src/test/java/feign/DefaultContractTest.java (4 hunks)
- core/src/test/java/feign/FeignUnderAsyncTest.java (7 hunks)
- core/src/test/java/feign/codec/DefaultEncoderTest.java (2 hunks)
- java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java (7 hunks)
- jaxb-jakarta/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java (2 hunks)
- jaxb/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java (3 hunks)
- json/src/test/java/feign/json/JsonDecoderTest.java (2 hunks)
- json/src/test/java/feign/json/JsonEncoderTest.java (2 hunks)
- okhttp/src/test/java/feign/okhttp/OkHttpClientAsyncTest.java (7 hunks)
- sax/src/test/java/feign/sax/examples/AWSSignatureVersion4.java (3 hunks)
Files skipped from review due to trivial changes (4)
- jaxb-jakarta/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java
- jaxb/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java
- json/src/test/java/feign/json/JsonEncoderTest.java
- sax/src/test/java/feign/sax/examples/AWSSignatureVersion4.java
Additional comments (Suppressed): 41
core/src/test/java/feign/codec/DefaultEncoderTest.java (2)
19-23: The import of
java.time.Clockis new and replaces the previousjava.util.Date. This change aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long). Ensure that all usages ofDatein this file have been replaced accordingly.54-54: The test case has been updated to use
Clock.systemUTC()instead ofnew Date(). This change is consistent with the PR's goal. However, ensure that theDefaultEncoder.encode()method can handleClockobjects correctly.- encoder.encode(new Date(), Date.class, new RequestTemplate()); + encoder.encode(Clock.systemUTC(), Clock.class, new RequestTemplate());json/src/test/java/feign/json/JsonDecoderTest.java (2)
29-33: The import of
java.util.Datehas been replaced withjava.time.Clock. This change aligns with the PR's goal of replacingDateinstances with a more reliable time mechanism.147-152: The test case previously checked for an exception when trying to decode into a
Dateclass. Now it checks for an exception when trying to decode into aClockclass. Ensure that this change is consistent with the updated behavior of theJsonDecoderclass.core/src/test/java/feign/DefaultContractTest.java (4)
20-28: The import statements have been updated to include
java.time.Clock,java.time.Instant, andjava.time.ZoneIdwhich are necessary for the changes made in this PR. The rest of the imports remain unchanged.315-322: The test method
customExpander()has been updated to useClockinstead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.586-600: The interface
CustomExpanderand classClockToMillishave been updated to useClockinstead ofDate. This is a good change as it aligns with the goal of the PR to replaceDateinstances with epoch milliseconds (Long). However, ensure that all implementations of theParam.Expanderinterface have been updated accordingly.879-905: A new class
TestClockextendingjava.time.Clockhas been added. This class seems to be used for testing purposes. It overrides thegetZone(),withZone(ZoneId zone), andinstant()methods from theClockclass. While this class seems to be correctly implemented, it's important to verify its usage across the test cases to ensure it behaves as expected.core/src/test/java/feign/FeignUnderAsyncTest.java (7)
36-48: The import of
java.util.Datehas been replaced with imports forjava.time.Clock,java.time.Instant, andjava.time.ZoneId. This aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long) across the codebase.203-210: The
expandmethod call now uses aTestClockinstance instead of aDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.215-222: The
expandListmethod call now uses a list ofTestClockinstances instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.227-234: The
expandListmethod call now uses a list ofTestClockinstances (with null) instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.859-872: The
@RequestLineannotations and corresponding method signatures have been updated to useClockinstead ofDate. Theexpanderattribute in the@Paramannotation has also been updated to useClockToMillisinstead ofDateToMillis. Ensure that all calls to these methods throughout the codebase have been updated to match the new signatures.897-907: The
DateToMillisclass has been replaced withClockToMillis. Theexpandmethod now casts the value toClockinstead ofDateand callsmillis()instead ofgetTime().1016-1044: A new
TestClockclass extendingjava.time.Clockhas been added. This class is used to replaceDateinstances in the test methods. Theinstantmethod returns anInstantof epoch milli from themillisfield.java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java (7)
24-36: The import of
java.util.Datehas been replaced with imports forjava.time.Clock,java.time.Instant, andjava.time.ZoneId. This aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long) across the codebase.225-233: The
expandmethod now takes aTestClockinstance instead of aDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.240-248: The
expandListmethod now takes a list ofTestClockinstances instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.254-262: The
expandListmethod now takes a list ofTestClockinstances (including null) instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.867-883: The
expand,expandList, andexpandArraymethods in theTestInterfaceAsyncinterface now takeClockparameters instead ofDate. Theexpanderattribute of the@Paramannotation has also been updated to use the newClockToMillisclass. Ensure that all implementations of these methods have been updated accordingly.908-918: The
DateToMillisclass has been replaced with theClockToMillisclass, which converts aClockinstance to milliseconds. This change is consistent with the PR's goal of replacingDateinstances with epoch milliseconds (Long).1065-1093: A new
TestClockclass has been added, extending theClockclass. This class is used in place ofDatein the test cases. Theinstantmethod returns anInstantobject representing the epoch millisecond value stored in theTestClockinstance.core/src/test/java/feign/AsyncFeignTest.java (13)
16-21: The import statement
import static org.assertj.core.api.Assertions.assertThat;has been removed. Ensure that this does not affect any existing assertions in the test cases.36-48: The import statement
import java.util.Date;has been replaced with imports forjava.time.Clock,java.time.Instant, andjava.time.ZoneId. This aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long).63-74: The import statement
import org.junit.jupiter.params.ParameterizedTest;has been removed. Ensure that this does not affect any existing parameterized tests in the test cases.227-235: The
expandmethod now takes aClockobject instead of aDateobject. TheTestClockclass is used to create aClockinstance with a specific time in milliseconds. This change aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long).241-250: Similar to the previous comment, the
expandListmethod now takes a list ofClockobjects instead ofDateobjects. TheTestClockclass is used to createClockinstances with specific times in milliseconds.256-264: Again, the
expandListmethod now takes a list ofClockobjects instead ofDateobjects. TheTestClockclass is used to createClockinstances with specific times in milliseconds. This change also handles null values in the list correctly.507-514: The
RetryableExceptionconstructor now includes aNON_RETRYABLEconstant (which isnull) as an argument. This change aligns with the PR's goal of deprecating the previous constructors that usedDate.586-592: Similar to the previous comment, the
RetryableExceptionconstructor now includes aNON_RETRYABLEconstant as an argument.611-617: Again, the
RetryableExceptionconstructor now includes aNON_RETRYABLEconstant as an argument.633-640: Once more, the
RetryableExceptionconstructor now includes aNON_RETRYABLEconstant as an argument.1024-1040: The
expand,expandList, andexpandArraymethods in theTestInterfaceAsyncinterface now takeClockobjects instead ofDateobjects. TheClockToMillisclass is used as a parameter expander to convertClockinstances to milliseconds. This change aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long).1065-1075: The
DateToMillisclass has been replaced with theClockToMillisclass, which convertsClockinstances to milliseconds. This change aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long).1252-1279: The
TestClockclass has been added. It extends theClockclass and overrides theinstantmethod to return anInstantobject with a specific time in milliseconds. This class is used in the test cases to createClockinstances with specific times.okhttp/src/test/java/feign/okhttp/OkHttpClientAsyncTest.java (6)
26-28: The import of
java.time.Clock,java.time.Instant, andjava.time.ZoneIdis a part of the changes to replaceDateinstances with epoch milliseconds (Long). This change is in line with the PR's goal of resolving timezone issues associated with theDateclass.223-231: The
TestInterfaceAsync'sexpandmethod now accepts aClockobject instead of aDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature. Also, verify that the server correctly handles the changed query parameter fromdatetoclock.238-246: Similar to the previous comment, the
expandListmethod now accepts a list ofClockobjects instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature. Also, verify that the server correctly handles the changed query parameters fromdatetoclock.252-260: Again, the
expandListmethod now accepts a list ofClockobjects (including nulls) instead ofDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature. Also, verify that the server correctly handles the changed query parameters fromdatetoclock.865-881: The
TestInterfaceAsyncinterface has been updated to useClockobjects instead ofDatein its methods. This change aligns with the PR's goal of replacingDateinstances with epoch milliseconds (Long). However, ensure that all implementations of this interface have been updated accordingly.906-916: The
ClockToMillisclass has been introduced as a replacement for theDateToMillisclass. It correctly converts aClockobject to milliseconds. This change is in line with the PR's goal of replacingDateinstances with epoch milliseconds (Long).
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- core/src/test/java/feign/FeignTest.java (12 hunks)
- hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java (8 hunks)
- java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java (8 hunks)
Additional comments (Suppressed): 26
java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java (8)
16-19: The import statement
import static org.assertj.core.api.Assertions.assertThat;has been removed. Ensure that this does not affect any existing assertions in the test cases.26-28: The new imports for
java.time.Clock,java.time.Instant, andjava.time.ZoneIdare introduced to replace the usage ofjava.util.Date. This is a good practice as thejava.timepackage provides a more comprehensive API for date and time operations.224-232: The method
api.expand()now accepts an instance ofTestClockinstead ofDate. Make sure that theexpand()method in theTestInterfaceAsyncinterface and its implementations have been updated accordingly.239-247: Similar to the previous comment, ensure that the
expandList()method in theTestInterfaceAsyncinterface and its implementations have been updated to accept a list ofClockinstances instead ofDate.253-261: Again, verify that the
expandList()method in theTestInterfaceAsyncinterface and its implementations have been updated to handle null values in the list ofClockinstances.866-882: The
@RequestLineannotations and method signatures have been updated to useClockinstead ofDate. This change aligns with the PR's goal of replacingDatewithClock. However, ensure that all calls to these methods throughout the codebase have been updated to match the new signatures.907-917: The
ClockToMillisclass correctly implements theParam.Expanderinterface and overrides theexpand()method to convert aClockinstance to milliseconds. This is a correct implementation of the expander pattern.1064-1092: The
TestClockclass extendsjava.time.Clockand overrides its methods to return custom values for testing purposes. This is a good practice for isolating the tests from the system clock.hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java (8)
16-38: The import statements have been updated to reflect the changes in the codebase. The usage of
Datehas been replaced withClock, and the necessary imports forClock,Instant, andZoneIdhave been added. The import statement forHttpClientBuilderhas been removed as it is no longer used.41-48: The import statements have been updated to reflect the changes in the codebase. The import statement for
JAXRSContracthas been removed as it is no longer used, and the import statement forCharsetsfrom Kotlin has also been removed.177-185: The method
api.expand()now accepts aTestClockobject instead of aDateobject. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.192-200: The method
api.expandList()now accepts a list ofTestClockobjects instead of a list ofDateobjects. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.206-214: The method
api.expandList()now accepts a list ofTestClockobjects instead of a list ofDateobjects. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.864-880: The methods
expand(),expandList(), andexpandArray()in theTestInterfaceAsyncinterface have been updated to acceptClockobjects instead ofDateobjects. Theexpanderattribute of the@Paramannotation has been updated to use theClockToMillisclass instead of theDateToMillisclass. Ensure that all implementations of these methods have been updated accordingly.905-915: The
DateToMillisclass has been replaced with theClockToMillisclass, which implements theParam.Expanderinterface. Theexpand()method now accepts aClockobject and returns its milliseconds value as a string.1067-1091: A new
TestClockclass has been added that extends theClockclass. This class is used for testing purposes to simulate a clock with a specific time in milliseconds. Theinstant()method returns anInstantobject representing the epoch millisecond value of theTestClock.core/src/test/java/feign/FeignTest.java (10)
60-60: The
NON_RETRYABLEconstant is introduced but it's not clear why it's set tonull. Please provide a comment or use a more descriptive name for the constant.216-219: Ensure that all calls to
api.expand()throughout the codebase have been updated to pass aTestClockobject instead of aDate.228-231: Ensure that all calls to
api.expandList()throughout the codebase have been updated to pass a list ofTestClockobjects instead ofDate.240-243: Ensure that all calls to
api.expandList()throughout the codebase have been updated to pass a list ofTestClockobjects instead ofDate. Also, verify how null values are handled in the new implementation.542-543: The
RetryableExceptionconstructor now accepts aLongparameter forretryAfter. Ensure that this change doesn't break any existing functionality whereretryAfterwas previously expected to benull.622-623: The
RetryableExceptionconstructor now accepts aLongparameter forretryAfter. Ensure that this change doesn't break any existing functionality whereretryAfterwas previously expected to benull.648-649: The
RetryableExceptionconstructor now accepts aLongparameter forretryAfter. Ensure that this change doesn't break any existing functionality whereretryAfterwas previously expected to benull.671-672: The
RetryableExceptionconstructor now accepts aLongparameter forretryAfter. Ensure that this change doesn't break any existing functionality whereretryAfterwas previously expected to benull.1178-1185: Ensure that all calls to these methods throughout the codebase have been updated to pass
Clockobjects instead ofDate.1226-1232: The
expand()method inClockToMillisnow expects aClockobject instead of aDate. Ensure that this change doesn't break any existing functionality where aDatewas previously expected.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (7)
- core/src/test/java/feign/AsyncFeignTest.java (13 hunks)
- core/src/test/java/feign/DefaultContractTest.java (4 hunks)
- core/src/test/java/feign/FeignTest.java (12 hunks)
- core/src/test/java/feign/FeignUnderAsyncTest.java (7 hunks)
- hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java (8 hunks)
- java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java (8 hunks)
- okhttp/src/test/java/feign/okhttp/OkHttpClientAsyncTest.java (7 hunks)
Files skipped from review due to trivial changes (1)
- java11/src/test/java/feign/http2client/test/Http2ClientAsyncTest.java
Additional comments (Suppressed): 41
core/src/test/java/feign/DefaultContractTest.java (4)
23-28: The import statements have been updated to include
java.time.Clock,java.time.Instant, andjava.time.ZoneIdwhich are necessary for the changes made in this PR. The rest of the imports remain unchanged.316-322: The test method
customExpander()has been updated to useClockinstead ofDate. TheparseAndValidateMetadata()method now takesClock.classas an argument, and the assertion checks that the expander class isClockToMillis.class. This change aligns with the PR's goal of replacingDatewithClock.586-600: The
CustomExpanderinterface and theClockToMillisclass have been updated to work withClockinstead ofDate. The@RequestLineannotation now includes aclockparameter, and theclock()method takes aClockobject as an argument. Theexpand()method inClockToMillisnow casts the value toClockand calls themillis()method. These changes are consistent with the PR's objective of replacingDatewithClock.879-905: A new
TestClockclass has been introduced. This class extendsClockand provides a custom implementation for testing purposes. It overrides thegetZone(),withZone(), andinstant()methods from theClockclass. Theinstant()method returns anInstantobject representing the current epoch millisecond, which is stored in themillisfield. This class will be useful for testing code that usesClockinstances.core/src/test/java/feign/FeignTest.java (8)
60-60: The
NON_RETRYABLEconstant is introduced but it's not clear why it's set tonull. Please ensure that this change doesn't introduce any unexpected behavior in the retry mechanism.216-219: Ensure that all calls to the
expandmethod throughout the codebase have been updated to pass aClockinstance instead of aDate.228-231: Ensure that all calls to the
expandListmethod throughout the codebase have been updated to pass a list ofClockinstances instead ofDate.240-243: Ensure that all calls to the
expandListmethod throughout the codebase have been updated to pass a list ofClockinstances instead ofDate, and that they handlenullvalues correctly.670-673: The
RetryableExceptionconstructor now accepts aLongparameter forretryAfter. Ensure that this change doesn't affect the retry mechanism and that theNON_RETRYABLEconstant (which isnull) is handled correctly in all scenarios whereRetryableExceptionis used.1178-1185: Ensure that all calls to these methods throughout the codebase have been updated to match the new signatures, which now accept
Clockinstances instead ofDate.1226-1232: This new
ClockToMillisclass replaces the oldDateToMillisclass. Ensure that this change doesn't introduce any unexpected behavior when expanding parameters.1400-1422: This new
TestClockclass provides a customClockimplementation for testing purposes. It seems to be well-implemented, but please ensure that its usage aligns with the intended test scenarios.hc5/src/test/java/feign/hc5/AsyncApacheHttp5ClientTest.java (7)
15-38: The import statements have been updated to reflect the changes in the codebase. The usage of
java.time.Clock,java.time.Instant, andjava.time.ZoneIdis new, which aligns with the PR's goal of replacingDatewithClock.41-48: The import statements are updated here as well. The removal of
feign.jaxrs.JAXRSContractandkotlin.text.Charsetsindicates that these classes are no longer used in this test file.177-185: The method
api.expand(new TestClock(1234l))now takes aTestClockinstance instead of aDate. Ensure all calls to this method throughout the codebase have been updated to match the new signature.192-200: Similar to the previous comment, the method
api.expandList(Arrays.asList(new TestClock(1234l), new TestClock(12345l)))now takes a list ofTestClockinstances instead ofDate. Verify that all calls to this method have been updated accordingly.864-880: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [206-880]
The interface
TestInterfaceAsynchas been updated to useClockinstead ofDate. This includes methods likeexpand,expandList, andexpandArray. Make sure all implementations of this interface have been updated to reflect these changes.
905-915: The inner class
ClockToMillisreplaces the previousDateToMillisand works withClockinstead ofDate. This change aligns with the PR's goal of replacingDatewithClock.1067-1091: A new
TestClockclass has been introduced, extendingjava.time.Clock. This class is used for testing purposes and provides a customClockimplementation. This is a good practice as it allows for more controlled testing.okhttp/src/test/java/feign/okhttp/OkHttpClientAsyncTest.java (7)
26-35: The import statements have been updated to include
java.time.Clock,java.time.Instant, andjava.time.ZoneIdwhich are necessary for the new changes. Thejava.util.Dateimport has been removed as it's no longer used in this file.223-231: The
expandmethod now accepts aTestClockinstance instead of aDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.238-246: Similar to the previous comment, the
expandListmethod now accepts a list ofTestClockinstances instead ofDate. Verify that all calls to this method have been updated accordingly.252-260: Again, the
expandListmethod now accepts a list ofTestClockinstances instead ofDate. One of the elements in the list isnull. This should be handled correctly by the method implementation.865-881: The
expand,expandList, andexpandArraymethods in theTestInterfaceAsyncinterface have been updated to acceptClockinstances instead ofDate. Also, theexpanderattribute of the@Paramannotation has been changed fromDateToMillistoClockToMillis. Make sure these changes are reflected wherever these methods are called or implemented.906-916: The
DateToMillisclass has been replaced withClockToMilliswhich implements theParam.Expanderinterface. Theexpandmethod now casts the value toClockinstead ofDateand calls themillismethod on it. This change aligns with the overall goal of the PR to replaceDatewithClock.1028-1056: A new
TestClockclass has been introduced which extends theClockclass. This class is used for testing purposes and overrides thegetZone,withZone, andinstantmethods of theClockclass. Theinstantmethod returns anInstantobject representing the epoch millisecond value stored in themillisfield. This is a good approach to mock theClockbehavior in tests.core/src/test/java/feign/AsyncFeignTest.java (8)
16-21: The import statement
import static org.assertj.core.api.Assertions.assertThat;has been removed. Ensure that this does not affect any existing assertions in the test cases.39-41: New imports related to Java's time API have been added, replacing the previous usage of
java.util.Date. This is a good improvement as it provides more flexibility and precision when dealing with dates and times.66-66: The import statement
import org.junit.jupiter.params.ParameterizedTest;has been replaced withimport org.junit.jupiter.params.provider.ValueSource;. Make sure that this change doesn't affect any parameterized tests in the codebase.230-234: The method call
api.expand(new Date(1234l));has been replaced withapi.expand(new TestClock(1234l) {});. This change aligns with the PR's goal of replacingDatewithClock. However, ensure that theTestClockclass correctly mimics the behavior ofDatefor the purposes of these tests.507-514: The
RetryableExceptionconstructor now includesNON_RETRYABLEas an argument instead ofnull. This change improves readability by making it clear that the exception is non-retryable.1027-1040: The
@RequestLineannotations and corresponding method signatures have been updated to useClockinstead ofDate, and theClockToMillisexpander is used instead ofDateToMillis. These changes are consistent with the PR's goal of replacingDatewithClock.1068-1074: The
DateToMillisclass has been replaced withClockToMillis, which converts aClockinstance to milliseconds. This change is consistent with the PR's goal of replacingDatewithClock.1255-1277: A new
TestClockclass has been introduced to replace the usage ofDatein tests. This class extendsClockand provides a custom implementation for testing purposes. Theinstant()method returns anInstantobject representing the epoch millisecond, which is consistent with the PR's goal of using epoch milliseconds.core/src/test/java/feign/FeignUnderAsyncTest.java (7)
36-48: The import of
java.util.Datehas been replaced withjava.time.Clockandjava.time.Instant,java.time.ZoneId. This change aligns with the PR's goal of replacingDateusage withClock.203-210: The
expandmethod now accepts aTestClockinstance instead of aDate. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.215-222: Similar to the previous comment, the
expandListmethod now accepts a list ofTestClockinstances. Verify that all calls to this method have been updated accordingly.227-234: Again, the
expandListmethod now accepts a list ofTestClockinstances, including null values. Ensure that all calls to this method have been updated to match the new signature.859-872: The
expand,expandList, andexpandArraymethods in theTestInterfacenow acceptClockinstances instead ofDate. Theexpanderclass has also been changed fromDateToMillistoClockToMillis. Make sure that all implementations and usages of these methods have been updated to reflect these changes.900-906: The
DateToMillisclass has been replaced withClockToMillis, which works withClockinstances. This change is consistent with the PR's goal of replacingDateusage withClock.1016-1042: A new
TestClockclass has been introduced, extending theClockclass. This customClockimplementation is used for testing purposes. It overrides thegetZone,withZone, andinstantmethods of theClockclass. Theinstantmethod returns anInstantof the epoch milli provided in the constructor, while thegetZoneandwithZonemethods throw anUnsupportedOperationException. This implementation seems to be suitable for the test scenarios in this class.
| import java.text.DateFormat; | ||
| import java.text.ParseException; | ||
| import java.text.SimpleDateFormat; | ||
| import java.time.ZonedDateTime; |
There was a problem hiding this comment.
It's this available on java8? Nice
* Retryer: replace an instance of Date with an epoch millisecond * Style issue: unnecessary explicit casting * Add another check to RetryableException's test * Update serialization ID. Resolve some deprecation issues of Integer. * Remove obsolete Date * Remove obsolete Date 2 * Resolve issue with overrided method of a mock class
* Retryer: replace an instance of Date with an epoch millisecond * Style issue: unnecessary explicit casting * Add another check to RetryableException's test * Update serialization ID. Resolve some deprecation issues of Integer. * Remove obsolete Date * Remove obsolete Date 2 * Resolve issue with overrided method of a mock class
2 participants
Resolves #2154
We do not use Date as date, every time we calculate epoch milliseconds. The Date class has issues with timezone.
Summary by CodeRabbit
DatewithClockin various classes and methods for better time handling.TestClockclass for more flexible testing.RetryableExceptionto useLonginstead ofDatefor retry-after value, providing more flexibility.Retryerusing exponential backoff.DateFormatwithDateTimeFormatterinRetryAfterDecoderfor modern date/time handling.