-
Notifications
You must be signed in to change notification settings - Fork 1.9k
support method option and add UT #1881
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
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 aSocketTimeoutExceptionis expected due to the method-specific options. The use ofAtomicReferencefor 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 ofCountDownLatchto 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
getThreadIdentifieris fine as it is used in the new methods.368-372: The
threadToMethodOptionsfield is correctly initialized in the constructor of theOptionsclass.
| 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
getThreadIdentifieris fine as it's used in the new methods.369-373: The
threadToMethodOptionsfield 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
getThreadIdentifieris fine as it is used in the new methods.370-374: The
threadToMethodOptionsfield 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] [](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [io.github.openfeign:feign-bom](https://togithub.com/openfeign/feign) | `13.0` -> `13.1` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](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://togithub.com/OpenFeign/feign/releases/tag/13.1): OpenFeign 13.1 [Compare Source](https://togithub.com/openfeign/feign/compare/13.0...13.1) ##### What's Changed - Support Conditional Parameter Expansion by [`@​gromspys](https://togithub.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2216](https://togithub.com/OpenFeign/feign/pull/2216) - Add typed response by [`@​gromspys](https://togithub.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2206](https://togithub.com/OpenFeign/feign/pull/2206) - Allow to ignore methods on provided interface by [`@​gromspys](https://togithub.com/gromspys)` in [https://github.com/OpenFeign/feign/pull/2218](https://togithub.com/OpenFeign/feign/pull/2218) - support method option and add UT by [`@​TaoJing96](https://togithub.com/TaoJing96)` in [https://github.com/OpenFeign/feign/pull/1881](https://togithub.com/OpenFeign/feign/pull/1881) - Do not decode URL encoding while setting up RequestTemplate by [`@​Breina](https://togithub.com/Breina)` in [https://github.com/OpenFeign/feign/pull/2228](https://togithub.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://togithub.com/OpenFeign/feign/pull/1881)l/1881 * `@​Breina` made their first contributi[https://github.com/OpenFeign/feign/pull/2228](https://togithub.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 <velo@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 <velo@users.noreply.github.com>
Background:
Change:
Usage:
socketTimeoutWithMethodOptionsTestin OptionsTest.javaSummary by CodeRabbit
New Features:
Optionsclass, providing more flexibility in handling different methods.getThreadIdentifier()for retrieving the current thread's identifier.Tests:
Refactor:
findOptionsmethod in theRequestTemplateclass to handle method-specific options, improving the overall request handling process.