Modifying Contract to support passing all parameters to encoders #1448 #1459

fabiocarvalho777 · 2021-07-13T15:45:19Z

This PR addresses issue #1448

@velo @kdavisk6 please just let me know if you have any requests for changes, or if this PR should be opened against a different branch and I will do it. Thank you very much.

fabiocarvalho777 · 2021-07-13T21:16:20Z

I noticed CircleCI build failed. However, the error is not related to my changes. Here is the error:

[ERROR] Failed to execute goal com.mycila:license-maven-plugin:3.0:check (default) on project feign-core: Some files do not have the expected license header -> [Help 1]

When building from my laptop I skip that license header check by adding -Dlicense.skip=true to my Maven command. I am surprised though the build in CircleCI is failing because of that.

What should be done to avoid this build error in CircleCI?

velo · 2021-07-13T23:51:13Z

Run license:format locally and you should manage to fix whatever needs fixing.

fabiocarvalho777 · 2021-07-14T02:23:06Z

@velo build passed.

velo · 2021-07-14T02:27:24Z

Sweet, I gave it a very initial pass. I see no tests for it, could you please include some?!

fabiocarvalho777 · 2021-07-14T19:39:08Z

@velo added tests. Builds passed. Thanks.

fabiocarvalho777 · 2021-07-16T12:35:44Z

@velo hello. Checking if you had a chance to review the PR. Thanks.

velo · 2021-07-19T00:26:52Z

@velo hello. Checking if you had a chance to review the PR. Thanks.

thanks Fabio, I downloaded a copy of your changes here, and I'm tinkering around, trying to avoid this boolean, in an attempt to create a more generalized solution. Hopefully I will have an answer over the weekend

kdavisk6

Meets the need and fits as a minimum viable release. I'm going to approve it, pending additional comments from @velo

fabiocarvalho777 · 2021-07-20T20:01:05Z

@velo hello. Checking if you had a chance to review the PR. Thanks.

thanks Fabio, I downloaded a copy of your changes here, and I'm tinkering around, trying to avoid this boolean, in an attempt to create a more generalized solution. Hopefully I will have an answer over the weekend

Thanks. Let me know if you have any question, or need any change.

velo · 2021-07-20T20:34:34Z

I still feel this should be a contract-only change.

Contract holds the magic on how to prepare payloads for encoders, so, it alone should be able to "translate" method arguments into an Object[] or Map<> or even a different object assembled method arguments. Just haven't figure HOW yet.

I just don't wanna introduce a "fabio" specific change now, to only figure that we should have done something else later.

fabiocarvalho777 · 2021-07-20T20:51:38Z

I still feel this should be a contract-only change.

Sorry, I thought this is the Contact change we have been discussing. Isn't it? As opposed to changing the Encoder API.

it alone should be able to "translate" method arguments into an Object[] or Map<> or even a different object assembled method arguments

But that is exactly what this solution does, it delivers all method arguments into a Object[]. And all of that preserving the current APIs (except for the addition of alwaysEncodeBody).

I just don't wanna introduce a "fabio" specific change now, to only figure that we should have done something else later.

I hear you, no one wants specific solutions, including myself, trust me, otherwise the project becomes unmaintainable and nobody wants their projects to rely on unmainatable libraries. I have maintained 3 different open source projects, so I know what you are talking about. I am just not sure that is the case here, as (please correct me if I am wrong) the PR meets what has been asked in the discussion in this thread, including the points in your last comment.

Anyways, please let me know if you have any other idea, or if there is anything else I can do to help, I would be glad to change anything and retest.

Thanks.

velo · 2021-07-20T21:41:57Z

Sorry, I thought this is the Contact change we have been discussing. Isn't it? As opposed to changing the Encoder API

My concern, is this boolean is behaving more like an "body no op contract", and then we need to check the boolean everywhere to ensure correct behavior.

Let's say we have a client like this:

@RequestLine("GET /{id}/something")
@Headers("Authorization: {login}")
public String readSomething(@Param("id") String id, @Param("login") String login, Object body);

Feign behave in very distinct ways with alwaysEncodeBody, and I'm sure this will be generating plenty of support, specially for those using third party encoders that will inherit this "for free"

I was not able to find a different solution for your problem over the weekend, and I won't have the time to get a different approach.

Let's compromise. Instead of having alwaysEncodeBody on BaseContract, that will spread around like wildfire, let's have a new Contract base class with this functionality and let's leave existing feign and third party contracts intact.

I don't exactly have a name suggestion for that class, maybe something like BodyNoOpContract, Body?Contract, AlwaysEncodeBodyBaseContract

If you happy to proceed with that, I would merge and release feign 11.5 today.

core/src/main/java/feign/MethodMetadata.java

fabiocarvalho777 · 2021-07-20T21:49:44Z

@velo Thank you very much. I will implement the changes you mentioned now.

fabiocarvalho777 · 2021-07-20T22:50:09Z

core/src/main/java/feign/AlwaysEncodeBodyContract.java

+ *
+ * @author fabiocarvalho777@gmail.com
+ */
+public abstract class AlwaysEncodeBodyContract extends DeclarativeContract {


Since the purpose of this class is to allow the body always to be defined by the encoder, there was no need to add alwaysEncodeBody field here.

Notice this class extends DeclarativeContract, instead of BaseContract. That is necessary to take advantage of the annotations processors, which is required for extensions of AlwaysEncodeBodyContract.

makes sense.

velo

Looking good

velo · 2021-07-20T22:50:44Z

core/src/main/java/feign/Contract.java

@@ -111,7 +84,9 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
      data.method(method);
      data.returnType(Types.resolve(targetType, targetType, method.getGenericReturnType()));
      data.configKey(Feign.configKey(targetType, method));
-      data.alwaysEncodeBody(this.alwaysEncodeBody);
+      if (AlwaysEncodeBodyContract.class.isAssignableFrom(this.getClass())) {


can't you override this method on the subclass?

na, I don't think it's possible.
ok, nvm

fabiocarvalho777 · 2021-07-20T22:54:28Z

core/src/main/java/feign/Contract.java

@@ -84,6 +84,9 @@ protected MethodMetadata parseAndValidateMetadata(Class<?> targetType, Method me
      data.method(method);
      data.returnType(Types.resolve(targetType, targetType, method.getGenericReturnType()));
      data.configKey(Feign.configKey(targetType, method));
+      if (AlwaysEncodeBodyContract.class.isAssignableFrom(this.getClass())) {


This setting is necessary here to avoid having to override this whole method (a copy plus this setting) under AlwaysEncodeBodyContract.

velo · 2021-07-21T03:35:40Z

Here you go

https://repo1.maven.org/maven2/io/github/openfeign/feign-core/11.5/

fabiocarvalho777 · 2021-07-21T19:07:37Z

@velo @kdavisk6 thank you very much, we really appreciate your time and work!!

velo · 2021-07-21T21:21:37Z

All good, sorry for holding this for so long.

FWIW, the way I would love to see this done (as in the future) is something like this:

@RequestLine("GET /{id}/something")
@Headers("Authorization: {login}")
@Body(expression = "java( new Object[] {id, login, name, age} )")
public String readSomething(@Param("id") String id, @Param("login") String login, String name, int age);

The main advantage of this Body#expression is the flexibility.

Prefer a Map instead, sure:

@Body("java( Map.of("id", id, "name": name, ... ) )")

Do you have your own object, not a problem

@Body("java( new InternalRecord( id, login, new UserIdentifiableInformation(name, age) ) )")

HOW you may ask?! No idea, I just have a dream!

#1459) * Modifying Contract to support passing all parameters to encoders * Formatting license * Adding unit tests * Adding AlwaysEncodeBodyContract abstract class (#1)

fabiocarvalho777 and others added 2 commits July 12, 2021 16:41

Modifying Contract to support passing all parameters to encoders

5787482

Merge branch 'master' into newcontract

9f6c6ee

fabiocarvalho777 mentioned this pull request Jul 13, 2021

Add support for encoders even when no or multiple body parameters are present #1448

Open

Formatting license

02b26b1

Adding unit tests

4f2c1ff

fabiocarvalho777 force-pushed the newcontract branch from cbdb8db to 4f2c1ff Compare July 14, 2021 19:35

kdavisk6 approved these changes Jul 19, 2021

View reviewed changes

kdavisk6 added the feedback provided Feedback has been provided to the author label Jul 19, 2021

velo reviewed Jul 20, 2021

View reviewed changes

core/src/main/java/feign/MethodMetadata.java Show resolved Hide resolved

Adding AlwaysEncodeBodyContract abstract class (#1)

ee2abe2

fabiocarvalho777 commented Jul 20, 2021

View reviewed changes

velo approved these changes Jul 20, 2021

View reviewed changes

fabiocarvalho777 commented Jul 20, 2021

View reviewed changes

Merge branch 'master' into newcontract

fe87687

velo merged commit aa96dda into OpenFeign:master Jul 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying Contract to support passing all parameters to encoders #1448 #1459

Modifying Contract to support passing all parameters to encoders #1448 #1459

fabiocarvalho777 commented Jul 13, 2021

fabiocarvalho777 commented Jul 13, 2021

velo commented Jul 13, 2021

fabiocarvalho777 commented Jul 14, 2021

velo commented Jul 14, 2021

fabiocarvalho777 commented Jul 14, 2021

fabiocarvalho777 commented Jul 16, 2021

velo commented Jul 19, 2021

kdavisk6 left a comment

fabiocarvalho777 commented Jul 20, 2021

velo commented Jul 20, 2021 •

edited

Loading

fabiocarvalho777 commented Jul 20, 2021

velo commented Jul 20, 2021

fabiocarvalho777 commented Jul 20, 2021 •

edited

Loading

fabiocarvalho777 Jul 20, 2021

fabiocarvalho777 Jul 20, 2021

velo Jul 20, 2021

velo left a comment

velo Jul 20, 2021

velo Jul 20, 2021

fabiocarvalho777 Jul 20, 2021

velo commented Jul 21, 2021

fabiocarvalho777 commented Jul 21, 2021

velo commented Jul 21, 2021

Modifying Contract to support passing all parameters to encoders #1448 #1459

Modifying Contract to support passing all parameters to encoders #1448 #1459

Conversation

fabiocarvalho777 commented Jul 13, 2021

fabiocarvalho777 commented Jul 13, 2021

velo commented Jul 13, 2021

fabiocarvalho777 commented Jul 14, 2021

velo commented Jul 14, 2021

fabiocarvalho777 commented Jul 14, 2021

fabiocarvalho777 commented Jul 16, 2021

velo commented Jul 19, 2021

kdavisk6 left a comment

Choose a reason for hiding this comment

fabiocarvalho777 commented Jul 20, 2021

velo commented Jul 20, 2021 • edited Loading

fabiocarvalho777 commented Jul 20, 2021

velo commented Jul 20, 2021

fabiocarvalho777 commented Jul 20, 2021 • edited Loading

fabiocarvalho777 Jul 20, 2021

Choose a reason for hiding this comment

fabiocarvalho777 Jul 20, 2021

Choose a reason for hiding this comment

velo Jul 20, 2021

Choose a reason for hiding this comment

velo left a comment

Choose a reason for hiding this comment

velo Jul 20, 2021

Choose a reason for hiding this comment

velo Jul 20, 2021

Choose a reason for hiding this comment

fabiocarvalho777 Jul 20, 2021

Choose a reason for hiding this comment

velo commented Jul 21, 2021

fabiocarvalho777 commented Jul 21, 2021

velo commented Jul 21, 2021

velo commented Jul 20, 2021 •

edited

Loading

fabiocarvalho777 commented Jul 20, 2021 •

edited

Loading