-
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 Conditional Parameter Expansion #2216
Conversation
WalkthroughThe changes introduce support for Changes
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- core/src/main/java/feign/template/Expressions.java (2} hunks)
- spring4/src/test/java/feign/spring/SpringContractTest.java (3} hunks)
Additional comments: 5
spring4/src/test/java/feign/spring/SpringContractTest.java (3)
57-59: The new mock client setup includes additional endpoints for testing optional parameters. This is in line with the PR's goal of introducing support for conditional parameter expansion in Feign, specifically handling
Optional
variables.121-147: These new test methods are designed to validate the handling of
Optional
parameters in different scenarios: when theOptional
is present, not present, empty, and nullable. Each test method makes a request to thecheckWithOptional
method of theResource
interface with differentOptional
parameters and verifies the expected HTTP request sent by the client. This is a good practice as it ensures that the new functionality works as expected in different scenarios.287-289: The
checkWithOptional
method has been added to theResource
interface to handle the/health/optional
endpoint. It takes anOptional<String>
parameter, which is annotated with@RequestParam
. This is consistent with the PR's goal of adding support forOptional
parameters in Feign.core/src/main/java/feign/template/Expressions.java (2)
20-20: The
java.util.Optional
import has been added to handleOptional
variables in the URI template expressions.158-168: The code has been updated to handle
Optional
variables. If the variable is present, it is expanded recursively. If the variable is empty andnameRequired
isfalse
,null
is returned. IfnameRequired
istrue
, the encoded name is appended to the expanded string.+ } else if (Optional.class.isAssignableFrom(variable.getClass())) { + Optional<?> optional = (Optional) variable; + if (optional.isPresent()) { + expanded.append(this.expand(optional.get(), encode)); + } else { + if (!this.nameRequired) { + return null; + } + expanded.append(this.encode(this.getName())) + .append("="); + } + } else {While the changes are correct, it's worth noting that returning
null
from a method that is expected to return aString
can lead toNullPointerExceptions
in the calling code if not handled properly. Consider returning an empty string instead ofnull
to avoid potential issues.Committable suggestion (Beta)
} else if (Optional.class.isAssignableFrom(variable.getClass())) { Optional<?> optional = (Optional) variable; if (optional.isPresent()) { expanded.append(this.expand(optional.get(), encode)); } else { if (!this.nameRequired) { return ""; } expanded.append(this.encode(this.getName())) .append("="); } } else {
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>
Issue: #521
Summary by CodeRabbit
Optional
variables in our core functionality. This allows for more flexible handling of variables, improving the software's adaptability to different data scenarios.Optional
variables more intuitively, reducing potential confusion and making the software easier to use.