Add Optional BeanParam support for JAXRS2Contract #1935
Conversation
JKomoroski
force-pushed
the
issue/1264/jaxrs2_support_bean_param
branch
from
bde77f8 to
cd44762
Compare
|
Rebased on latest master. |
|
I'm not able to reproduce the build failures in circle CI on my local system. |
|
Did you run mvn clean install locally? It should fix code format for you
[image: --]
Marvin Froeder
[image: https://]about.me/velo
<https://about.me/velo?promo=email_sig&utm_source=email_sig&utm_medium=email_sig&utm_campaign=external_links>
…On Tue, Feb 14, 2023 at 8:34 AM Jared Komoroski ***@***.***> wrote:
I'm not able to reproduce the build failures in circle CI on my local
system.
—
Reply to this email directly, view it on GitHub
<#1935 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBLDVKERRG5YVRIKXJ5OLWXKEDHANCNFSM6AAAAAAU2UCOFQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Ran the build skipping the Example Wikipedia modules which were failing on my local environment, and committed the changes. I forgot about the plugin that does that in this project. |
|
Yes, please avoid unnecessary chances for easier code review
…On Tue, 14 Feb 2023, 11:05 Jared Komoroski, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In jaxrs/src/main/java/feign/jaxrs/JAXRSContract.java
<#1935 (comment)>:
> - final String name = param.value();
- checkState(emptyToNull(name) != null, "HeaderParam.value() was empty on parameter %s",
- paramIndex);
- final String header = addTemplatedParam(name);
- data.template().header(name, header);
- nameParam(data, name, paramIndex);
- });
- registerParameterAnnotation(FormParam.class, (param, data, paramIndex) -> {
- final String name = param.value();
- checkState(emptyToNull(name) != null, "FormParam.value() was empty on parameter %s",
- paramIndex);
- data.formParams().add(name);
- nameParam(data, name, paramIndex);
- });
- }
+ registerParameterAnnotation(PathParam.class, (param, data, paramIndex) -> {
@velo <https://github.com/velo> You want the extra set of curly braces?
—
Reply to this email directly, view it on GitHub
<#1935 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABBLDVBRZJXFEYMSZGO3Z3WXKV2JANCNFSM6AAAAAAU2UCOFQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| // parameter with unsupported jax-rs annotations should not be passed as body params. | ||
| // this will prevent interfaces from becoming unusable entirely due to single (unsupported) | ||
| // endpoints. | ||
| // https://github.com/OpenFeign/feign/issues/669 | ||
| super.registerParameterAnnotation(Suspended.class, (ann, data, i) -> data.ignoreParamater(i)); | ||
| super.registerParameterAnnotation(Context.class, (ann, data, i) -> data.ignoreParamater(i)); | ||
| super.registerParameterAnnotation(BeanParam.class, (ann, data, i) -> data.ignoreParamater(i)); |
There was a problem hiding this comment.
I think, before made sense to ignore any @BeanParam cause it was not implemented.
Now that you fixed that shortcoming, I think it's fine to have it enabled
There was a problem hiding this comment.
Let me know if you would like me to make this change or if you plan to make it
There was a problem hiding this comment.
I think is a nice to have, but your call
There was a problem hiding this comment.
Pushed this change. Jdk8 appears to be failing again in CI. Not able to duplicate on my local
JKomoroski
force-pushed
the
issue/1264/jaxrs2_support_bean_param
branch
from
28d56d5 to
9f5b484
Compare
|
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| @Path("/") |
There was a problem hiding this comment.
Now, the compile time error moved to this line.... probably some jaxrs 1 leaking
There was a problem hiding this comment.
I'll have to install jdk 8 again to check for these issues
|
I can't explain why moving the |
* Add Optional BeanParam support for JAXRS2Contract * Fix Code Style * Commit Build Code Format Changes * Roll Back Removal of braces * Make sure jaxrs1 dependencies not avaiable on jaxrs2 * Update JAXRS2ContractWithBeanParamSupportTest.java * Always Use Enable BeanParam Support * Roll Back Test Changes --------- Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
* Add Optional BeanParam support for JAXRS2Contract * Fix Code Style * Commit Build Code Format Changes * Roll Back Removal of braces * Make sure jaxrs1 dependencies not avaiable on jaxrs2 * Update JAXRS2ContractWithBeanParamSupportTest.java * Always Use Enable BeanParam Support * Roll Back Test Changes --------- Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
Implements a change to the JAXRS2Contract to optionally support
@BeanParamparameter aggregators. It only adds support for aggregated parameters already supported by the existing JAXRS2Contract (ie not@Contextor@Suspended).fixes #1264