-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
support method option and add UT #1881
Conversation
You spoke a lot of spring up there, I'm not sure if I got it all. Lemme see if I got it correctly, what you want is to be able to change timeouts for individual methods dynamically. So, you want to call |
Yes, i want to change timeouts for individual methods dynamically and i don't need add param Options for each method.
In case 1, i want to call |
Could you describe me a bit more of what you are trying to accomplish? |
OK, let me show you the code.
However, i want to add request timeout while calling |
Why it looks so fragile and a memory leakage? Could you give more infos?
|
WalkthroughThe changes introduced enhance the Changes
Poem
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- core/src/main/java/feign/Request.java (4 hunks)
- core/src/main/java/feign/SynchronousMethodHandler.java (1 hunks)
- core/src/main/java/feign/Util.java (1 hunks)
- core/src/test/java/feign/OptionsTest.java (2 hunks)
Files skipped from review due to trivial changes (2)
- core/src/main/java/feign/SynchronousMethodHandler.java
- core/src/main/java/feign/Util.java
Additional comments: 5
core/src/test/java/feign/OptionsTest.java (3)
18-23: The newly added import statements are necessary for the new test methods. They are correctly placed and used.
94-116: The
socketTimeoutWithMethodOptionsTest()
method is correctly implemented. It tests the scenario where aSocketTimeoutException
is expected due to the method-specific options. The use ofAtomicReference
for exception handling in a separate thread is a good practice.118-135: The
normalResponseWithMethodOptionsTest()
method is correctly implemented. It tests the scenario where no exceptions are expected due to the method-specific options. The use ofCountDownLatch
to ensure the API call is completed before the test ends is a good practice.core/src/main/java/feign/Request.java (2)
16-17: The import of
getThreadIdentifier
is fine as it is used in the new methods.368-372: The
threadToMethodOptions
field is correctly initialized in the constructor of theOptions
class.
private final long readTimeout; | ||
private final TimeUnit readTimeoutUnit; | ||
private final boolean followRedirects; | ||
private final Map<String, Map<String, Options>> threadToMethodOptions; | ||
|
||
/** | ||
* Get an Options by methodName | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @return method Options | ||
*/ | ||
public Options getMethodOptions(String methodName) { | ||
Map<String, Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>()); | ||
return methodOptions.getOrDefault(methodName, this); | ||
} | ||
|
||
/** | ||
* Set methodOptions by methodKey and options | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @param options it's the Options for this method. | ||
*/ | ||
public void setMethodOptions(String methodName, Options options) { | ||
String threadIdentifier = getThreadIdentifier(); | ||
Map<String, Request.Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>()); | ||
threadToMethodOptions.put(threadIdentifier, methodOptions); | ||
methodOptions.put(methodName, options); | ||
} |
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 new methods getMethodOptions
and setMethodOptions
are correctly implemented. However, there is a potential issue with thread safety. The getMethodOptions
method retrieves a Map
from threadToMethodOptions
and then directly returns an Options
object from it. If another thread modifies this Map
in the meantime (via setMethodOptions
), it could lead to inconsistent states. Consider returning a deep copy of the Options
object instead of the object itself to avoid this issue.
public Options getMethodOptions(String methodName) {
Map<String, Options> methodOptions =
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>());
Options originalOptions = methodOptions.getOrDefault(methodName, this);
// Return a deep copy of the originalOptions
return originalOptions.copy();
}
You would need to implement a copy
method in the Options
class that creates a deep copy of the Options
object.
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- core/src/main/java/feign/Request.java (4 hunks)
Additional comments: 2
core/src/main/java/feign/Request.java (2)
16-17: The import of
getThreadIdentifier
is fine as it's used in the new methods.369-373: The
threadToMethodOptions
field is correctly initialized in the constructor.
private final long readTimeout; | ||
private final TimeUnit readTimeoutUnit; | ||
private final boolean followRedirects; | ||
private final Map<String, Map<String, Options>> threadToMethodOptions; | ||
|
||
/** | ||
* Get an Options by methodName | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @return method Options | ||
*/ | ||
public Options getMethodOptions(String methodName) { | ||
Map<String, Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>()); | ||
return methodOptions.getOrDefault(methodName, this); | ||
} | ||
|
||
/** | ||
* Set methodOptions by methodKey and options | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @param options it's the Options for this method. | ||
*/ | ||
@Experimental | ||
public void setMethodOptions(String methodName, Options options) { | ||
String threadIdentifier = getThreadIdentifier(); | ||
Map<String, Request.Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>()); | ||
threadToMethodOptions.put(threadIdentifier, methodOptions); | ||
methodOptions.put(methodName, options); | ||
} |
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 new field threadToMethodOptions
and the methods getMethodOptions
and setMethodOptions
are introduced correctly. However, there's a potential issue with thread safety. The setMethodOptions
method retrieves a Map
from the ConcurrentHashMap
and then modifies it. This operation is not atomic and can lead to data races if multiple threads call setMethodOptions
concurrently. Consider synchronizing the method or using a thread-safe Map
implementation.
- public void setMethodOptions(String methodName, Options options) {
- String threadIdentifier = getThreadIdentifier();
- Map<String, Request.Options> methodOptions =
- threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>());
- threadToMethodOptions.put(threadIdentifier, methodOptions);
- methodOptions.put(methodName, options);
- }
+ public synchronized void setMethodOptions(String methodName, Options options) {
+ String threadIdentifier = getThreadIdentifier();
+ Map<String, Request.Options> methodOptions =
+ threadToMethodOptions.computeIfAbsent(threadIdentifier, k -> new ConcurrentHashMap<>());
+ methodOptions.put(methodName, options);
+ }
Commitable suggestion (Beta)
private final long readTimeout; | |
private final TimeUnit readTimeoutUnit; | |
private final boolean followRedirects; | |
private final Map<String, Map<String, Options>> threadToMethodOptions; | |
/** | |
* Get an Options by methodName | |
* | |
* @param methodName it's your FeignInterface method name. | |
* @return method Options | |
*/ | |
public Options getMethodOptions(String methodName) { | |
Map<String, Options> methodOptions = | |
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>()); | |
return methodOptions.getOrDefault(methodName, this); | |
} | |
/** | |
* Set methodOptions by methodKey and options | |
* | |
* @param methodName it's your FeignInterface method name. | |
* @param options it's the Options for this method. | |
*/ | |
@Experimental | |
public void setMethodOptions(String methodName, Options options) { | |
String threadIdentifier = getThreadIdentifier(); | |
Map<String, Request.Options> methodOptions = | |
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>()); | |
threadToMethodOptions.put(threadIdentifier, methodOptions); | |
methodOptions.put(methodName, options); | |
} | |
private final long readTimeout; | |
private final TimeUnit readTimeoutUnit; | |
private final boolean followRedirects; | |
private final Map<String, Map<String, Options>> threadToMethodOptions; | |
/** | |
* Get an Options by methodName | |
* | |
* @param methodName it's your FeignInterface method name. | |
* @return method Options | |
*/ | |
public Options getMethodOptions(String methodName) { | |
Map<String, Options> methodOptions = | |
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>()); | |
return methodOptions.getOrDefault(methodName, this); | |
} | |
/** | |
* Set methodOptions by methodKey and options | |
* | |
* @param methodName it's your FeignInterface method name. | |
* @param options it's the Options for this method. | |
*/ | |
@Experimental | |
public synchronized void setMethodOptions(String methodName, Options options) { | |
String threadIdentifier = getThreadIdentifier(); | |
Map<String, Request.Options> methodOptions = | |
threadToMethodOptions.computeIfAbsent(threadIdentifier, k -> new ConcurrentHashMap<>()); | |
methodOptions.put(methodName, options); | |
} |
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- core/src/main/java/feign/Request.java (4 hunks)
Additional comments: 2
core/src/main/java/feign/Request.java (2)
16-17: The import of
getThreadIdentifier
is fine as it is used in the new methods.370-374: The
threadToMethodOptions
field is correctly initialized in the constructor.
private final long readTimeout; | ||
private final TimeUnit readTimeoutUnit; | ||
private final boolean followRedirects; | ||
private final Map<String, Map<String, Options>> threadToMethodOptions; | ||
|
||
/** | ||
* Get an Options by methodName | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @return method Options | ||
*/ | ||
@Experimental | ||
public Options getMethodOptions(String methodName) { | ||
Map<String, Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>()); | ||
return methodOptions.getOrDefault(methodName, this); | ||
} | ||
|
||
/** | ||
* Set methodOptions by methodKey and options | ||
* | ||
* @param methodName it's your FeignInterface method name. | ||
* @param options it's the Options for this method. | ||
*/ | ||
@Experimental | ||
public void setMethodOptions(String methodName, Options options) { | ||
String threadIdentifier = getThreadIdentifier(); | ||
Map<String, Request.Options> methodOptions = | ||
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>()); | ||
threadToMethodOptions.put(threadIdentifier, methodOptions); | ||
methodOptions.put(methodName, options); | ||
} |
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 new methods getMethodOptions
and setMethodOptions
are correctly implemented. However, there is a potential issue with thread safety. The getMethodOptions
method retrieves a Map
from threadToMethodOptions
and then directly returns an Options
object from it. If another thread modifies this Map
in the meantime (via setMethodOptions
), it could lead to inconsistent states. Consider returning a deep copy of the Options
object instead of the object itself to avoid this issue.
public Options getMethodOptions(String methodName) {
Map<String, Options> methodOptions =
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>());
Options originalOptions = methodOptions.getOrDefault(methodName, this);
// Return a deep copy of the originalOptions
return new Options(originalOptions.connectTimeout, originalOptions.connectTimeoutUnit,
originalOptions.readTimeout, originalOptions.readTimeoutUnit,
originalOptions.followRedirects);
}
15205: deps(maven): Update dependency io.github.openfeign:feign-bom to v13.1 (main) r=github-actions[bot] a=renovate[bot] [![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [io.github.openfeign:feign-bom](https://github.com/openfeign/feign) | `13.0` -> `13.1` | [![age](https://developer.mend.io/api/mc/badges/age/maven/io.github.openfeign:feign-bom/13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/io.github.openfeign:feign-bom/13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/io.github.openfeign:feign-bom/13.0/13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/io.github.openfeign:feign-bom/13.0/13.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>openfeign/feign (io.github.openfeign:feign-bom)</summary> ### [`v13.1`](https://github.com/OpenFeign/feign/releases/tag/13.1): OpenFeign 13.1 [Compare Source](https://github.com/openfeign/feign/compare/13.0...13.1) ##### What's Changed - Support Conditional Parameter Expansion by [`@​gromspys](https://github.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2216](https://github.com/OpenFeign/feign/pull/2216) - Add typed response by [`@​gromspys](https://github.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2206](https://github.com/OpenFeign/feign/pull/2206) - Allow to ignore methods on provided interface by [`@​gromspys](https://github.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2218](https://github.com/OpenFeign/feign/pull/2218) - support method option and add UT by [`@​TaoJing96](https://github.com/TaoJing96)` in [https://github.com/OpenFeign/feign/pull/1881](https://github.com/OpenFeign/feign/pull/1881) - Do not decode URL encoding while setting up RequestTemplate by [`@​Breina](https://github.com/Breina)` in [https://github.com/OpenFeign/feign/pull/2228](https://github.com/OpenFeign/feign/pull/2228) *** <details> <summary>List of PRs that updated libraries versions</summary> <br /> * build(deps): bump jakarta.xml.ws:jakarta.xml.ws-api from 4.0.0 to 4.0.1 by `@​dependabot` in OpenFeign/feign#2211 * build(deps-dev): bump org.glassfish.jersey.core:jersey-client from 2.40 to 2.41 by `@​dependabot` in OpenFeign/feign#2210 * build(deps-dev): bump org.glassfish.jersey.inject:jersey-hk2 from 2.40 to 2.41 by `@​dependabot` in OpenFeign/feign#2209 * build(deps): bump jakarta.xml.soap:jakarta.xml.soap-api from 3.0.0 to 3.0.1 by `@​dependabot` in OpenFeign/feign#2208 * build(deps): bump com.sun.xml.bind:jaxb-impl from 2.3.8 to 2.3.9 by `@​dependabot` in OpenFeign/feign#2207 * build(deps): bump io.sundr:sundr-maven-plugin from 0.101.0 to 0.101.1 by `@​dependabot` in OpenFeign/feign#2213 * build(deps): bump maven-surefire-plugin.version from 3.1.2 to 3.2.1 by `@​dependabot` in OpenFeign/feign#2212 * build(deps): bump io.sundr:sundr-maven-plugin from 0.101.1 to 0.101.2 by `@​dependabot` in OpenFeign/feign#2215 * build(deps): bump kotlin.version from 1.9.10 to 1.9.20 by `@​dependabot` in OpenFeign/feign#2219 * build(deps): bump io.sundr:sundr-maven-plugin from 0.101.2 to 0.101.3 by `@​dependabot` in OpenFeign/feign#2220 * build(deps): bump org.mockito:mockito-core from 5.6.0 to 5.7.0 by `@​dependabot` in OpenFeign/feign#2221 * build(deps): bump org.moditect:moditect-maven-plugin from 1.0.0.Final to 1.1.0 by `@​dependabot` in OpenFeign/feign#2224 * build(deps): bump io.dropwizard.metrics:metrics-core from 4.2.21 to 4.2.22 by `@​dependabot` in OpenFeign/feign#2222 * build(deps): bump org.junit:junit-bom from 5.10.0 to 5.10.1 by `@​dependabot` in OpenFeign/feign#2223 * build(deps): bump org.apache.maven.plugins:maven-javadoc-plugin from 3.6.0 to 3.6.2 by `@​dependabot` in OpenFeign/feign#2226 * build(deps): bump maven-surefire-plugin.version from 3.2.1 to 3.2.2 by `@​dependabot` in OpenFeign/feign#2225 </details> ##### New Contributors * `@​TaoJing96` made their first contributi[https://github.com/OpenFeign/feign/pull/1881](https://github.com/OpenFeign/feign/pull/1881)l/1881 * `@​Breina` made their first contributi[https://github.com/OpenFeign/feign/pull/2228](https://github.com/OpenFeign/feign/pull/2228)l/2228 **Full Changelog**: OpenFeign/feign@13.0...13.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "every weekday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/camunda/zeebe). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzcuNDYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* support method option and add UT * format code style * add Experimental annotation * Added @experimental to new method --------- Co-authored-by: Marvin Froeder <[email protected]>
* support method option and add UT * format code style * add Experimental annotation * Added @experimental to new method --------- Co-authored-by: Marvin Froeder <[email protected]>
Background:
Change:
Usage:
socketTimeoutWithMethodOptionsTest
in OptionsTest.javaSummary by CodeRabbit
New Features:
Options
class, providing more flexibility in handling different methods.getThreadIdentifier()
for retrieving the current thread's identifier.Tests:
Refactor:
findOptions
method in theRequestTemplate
class to handle method-specific options, improving the overall request handling process.