-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Multi encoder #3485
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
base: 14.x
Are you sure you want to change the base?
Multi encoder #3485
Changes from all commits
cf28371
7a5d815
46d604f
6bda942
68815c8
e2ce674
0181b69
9d573b4
0fd7247
2ea2dec
b6474fa
7446c1f
d9da80c
d7ea97b
0056795
f10e817
a52a378
5b81580
935954a
8896c30
a527c35
7510344
6b0a283
5f749db
2a8209a
14e1842
cbb53a6
9e2aad2
7965183
710dab7
9c51c3b
33bf63b
2f548d5
cd9b364
a3403f0
0629332
ed41e7a
e9b0f35
855c1f6
b5ab18f
26a3c4a
9009483
5370c8e
e461dc4
661d121
df2c15b
f130cce
10c9265
a696a4a
4621dd7
7451e9d
c39b706
2db791b
c5c19f2
4d7b063
0a767ad
0386c8d
fb503f7
4ed95e6
db95df3
3a93590
6e91211
ef33b17
5b11c91
36761fc
ffbf4c3
d00312c
ea970f6
7af1863
d00ad86
095acdb
b484275
6710ffc
5922082
f6e3743
5440c15
c830176
b3e884d
a6f05c4
da348f5
49b7d91
ee828e7
9106c0b
983a244
6bdb435
af582ce
60984fb
85c98a9
f1f07b0
2b715d0
9134d7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -355,6 +355,105 @@ VertxFeign.builder() | |
|
|
||
| --- | ||
|
|
||
| ### 14. `Encoder.encode()` now returns `boolean` (https://github.com/OpenFeign/feign/pull/3485) | ||
|
|
||
| `Encoder.encode()` now returns `boolean` instead of `void`. Return `true` when the encoder | ||
| handles the object, `false` otherwise. | ||
|
|
||
| **Before:** | ||
|
|
||
| ```java | ||
| public class MyEncoder implements Encoder { | ||
| @Override | ||
| public void encode(Object object, Type bodyType, RequestTemplate template) { | ||
| if(bodyType != String.class) throw new EncodeException("Encoding not supported"); | ||
| template.body(Request.Body.of(serialize(object))); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **After:** | ||
|
|
||
| ```java | ||
| public class MyEncoder implements Encoder { | ||
| @Override | ||
| public boolean encode(Object object, Type bodyType, RequestTemplate template) { | ||
| template.body(Request.Body.of(serialize(object))); | ||
| return true; // or return false if the encoder does not handle this type. Do NOT throw EncodeException unless a problem actually happens during encoding. | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Built-in encoders (`DefaultEncoder`, `FormEncoder`, `MeteredEncoder`, `GraphqlEncoder`, etc.) already return `boolean` | ||
| from `encode()`. If your encoder returns `false`, the `MultiEncoder` (see section 19) will try the next encoder. If | ||
| no encoder returns `true`, an `EncodeException` is thrown. | ||
|
|
||
| --- | ||
|
|
||
| ### 15. `Encoder` moved from `core` to `api` module | ||
|
|
||
| `feign.codec.Encoder` has been relocated from the `feign-core` module to the new `feign-api` | ||
| module. The package name (`feign.codec`) is unchanged. If you have a direct dependency on | ||
| `feign-core` without `feign-api`, you need to add `feign-api` to your classpath. | ||
|
|
||
| --- | ||
|
|
||
| ### 16. `Encoder.Default` removed | ||
|
|
||
| The deprecated inner class `Encoder.Default` (which extended `DefaultEncoder`) has been removed. | ||
|
|
||
| **Before:** | ||
|
|
||
| ```java | ||
| new Encoder.Default() | ||
| ``` | ||
|
|
||
| **After:** | ||
|
|
||
| ```java | ||
| new feign.core.codec.DefaultEncoder() | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 17. Composing multiple encoders with `MultiEncoder.of()` (https://github.com/OpenFeign/feign/pull/3485) | ||
|
|
||
| Use `MultiEncoder.of(...)` to compose multiple encoders into a single `MultiEncoder`, which | ||
| delegates to the first encoder whose `encode()` returns `true`. | ||
|
|
||
| **Before:** | ||
|
|
||
| ```java | ||
| Feign.builder() | ||
| .encoder(new JacksonEncoder()) | ||
| .target(MyApi.class, "https://api.example.com"); | ||
| ``` | ||
|
|
||
| **After (multiple encoders):** | ||
|
|
||
| ```java | ||
| Feign.builder() | ||
| .encoder(MultiEncoder.of( | ||
| new FormEncoder(), | ||
| new JacksonEncoder(), | ||
| new JAXBEncoder(factory) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example is probably misleading as |
||
| )) | ||
| .target(MyApi.class, "https://api.example.com"); | ||
| ``` | ||
|
|
||
| **After (single encoder is unchanged):** | ||
|
|
||
| ```java | ||
| Feign.builder() | ||
| .encoder(new JacksonEncoder()) | ||
| .target(MyApi.class, "https://api.example.com"); | ||
| ``` | ||
|
|
||
| The `MultiEncoder.of()` factory returns a `MultiEncoder`, which tries | ||
| each encoder's `encode()` and uses the first one that returns `true`. | ||
|
|
||
| --- | ||
|
|
||
| ## Implementing a Custom Streaming Body | ||
|
|
||
| If you want to stream a body (e.g., from a file or `InputStream`), implement `Request.Body` directly. Because | ||
|
|
@@ -420,6 +519,26 @@ public class FileBody implements Request.Body { | |
|
|
||
| --- | ||
|
|
||
| ## Selecting Encoder Based on a Predicate | ||
|
|
||
| TODO: Once we have specialized EncoderPredicate factory methods (i.e. for xml content types), update this example to use those factory methods | ||
|
|
||
| The new `PredicatingEncoder` and `EncoderPredicate` classes can be used in conjunction with `MultiEncoder` to fine tune which Encoder | ||
| handles different types of encode requests. | ||
|
|
||
| ```java | ||
| Feign.builder() | ||
| .encoder(MultiEncoder.of( | ||
| new DefaultEncoder(), | ||
| new PredicatingEncoder((obj, type, templ) -> templ.headers().get("Content-Type").contains("application/xml"), new JAXBEncoder(factory), // handle xml requests | ||
| new JacksonEncoder() // handle everything else | ||
| )) | ||
| .target(MyApi.class, "https://api.example.com"); | ||
|
|
||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Spring Cloud OpenFeign Compatibility | ||
|
|
||
| `RequestTemplate#body(byte[], Charset)` is kept `@Deprecated` for backward compatibility with | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not critical; the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, certainly possible - quick question, though: Right now, I do not have a predicate for json or xml (in fact, these two methods theoretically should be removed from this PR). I think that adding XmlContentTypeEncoderPredicate and JsonContentTypeEncoderPredicate would be fine - but should that maybe be in a separate PR? I was trying to keep this PR focused on raw capability... Also, for another PR ( https://github.com/OpenFeign/feign/pull/3494/changes#diff-3d5ee4752b168285974eb09fc4782f489edeadba936b5c71dc59ff6a043d779d ), I have introduced a dedicated content-type header parser that handles charset sub-elements, etc...). It may be better to use that, then compare the actual extracted content-type to the regex.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that having a dedicated With that in mind, I'd put the responsibility for parsing the I just noticed that Feign.builder()
.encoder(Encoder.of(
new PredicatingEncoder(EncoderPredicate.forContentType(".*json.*"), new GsonEncoder()),
new DefaultEncoder()
)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| /* | ||
| * Copyright © 2012 The Feign Authors (feign@commonhaus.dev) | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package feign.codec; | ||
|
|
||
| import feign.RequestTemplate; | ||
| import java.lang.reflect.Type; | ||
|
|
||
| /** A predicate that determines whether a given object can be encoded by an encoder. */ | ||
| @FunctionalInterface | ||
| public interface EncoderPredicate { | ||
|
|
||
| /** | ||
| * Tests whether the given object can be encoded by an encoder. | ||
| * | ||
| * @param object the object to be encoded | ||
| * @param bodyType the type of the object to be encoded | ||
| * @param template the request template that will be used to encode the object | ||
| * @return {@code true} if the object can be encoded, {@code false} otherwise | ||
| */ | ||
| boolean test(Object object, Type bodyType, RequestTemplate template); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| * Copyright © 2012 The Feign Authors (feign@commonhaus.dev) | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package feign; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
| import static org.junit.jupiter.params.provider.Arguments.arguments; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| import feign.codec.EncodeException; | ||
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
| import java.util.stream.Stream; | ||
| import org.junit.jupiter.api.Nested; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.FieldSource; | ||
|
|
||
| class RequestTemplateFactoryResolverTest { | ||
| @Nested | ||
| class BuildFormEncodedTemplateFromArgsTest { | ||
| @Test | ||
| void shouldThrowEncodeException() { | ||
| var methodMetadata = new MethodMetadata(); | ||
| var variables = Map.<String, Object>of("data", "Hello, World!"); | ||
| var factory = | ||
| new RequestTemplateFactoryResolver.BuildFormEncodedTemplateFromArgs( | ||
| methodMetadata, mock(), mock(), mock()); | ||
|
|
||
| methodMetadata.formParams().add("data"); | ||
|
|
||
| assertThrows( | ||
| EncodeException.class, | ||
| () -> factory.resolve(new Object[0], new RequestTemplate(), variables)); | ||
| } | ||
| } | ||
|
|
||
| @Nested | ||
| class BuildEncodedTemplateFromArgsTest { | ||
| private static final Supplier<Stream<Arguments>> shouldThrowEncodeException = | ||
| () -> { | ||
| var methodMetadata1 = new MethodMetadata(); | ||
| methodMetadata1.alwaysEncodeBody(true); | ||
|
|
||
| var methodMetadata2 = new MethodMetadata(); | ||
| methodMetadata2.bodyIndex(0); | ||
|
|
||
| return Stream.of( | ||
| arguments(methodMetadata1, new Object[0]), | ||
| arguments(methodMetadata2, new Object[] {"Hello, World!"})); | ||
| }; | ||
|
|
||
| @ParameterizedTest | ||
| @FieldSource | ||
| void shouldThrowEncodeException(MethodMetadata methodMetadata, Object[] argv) { | ||
| var factory = | ||
| new RequestTemplateFactoryResolver.BuildEncodedTemplateFromArgs( | ||
| methodMetadata, mock(), mock(), mock()); | ||
| var mutable = new RequestTemplate(); | ||
|
|
||
| mutable.methodMetadata(methodMetadata); | ||
|
|
||
| assertThrows(EncodeException.class, () -> factory.resolve(argv, mutable, Map.of())); | ||
| } | ||
| } | ||
| } |
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.
Probably worth adding an example with multiple encoders for clarity