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

[fabiocarvalho777]

Currently Feign will not resolve methods with BuildEncodedTemplateFromArgs if they have 0 or more than 1 body parameters, which prevents the usage of custom Feign encoders. There are use cases though where an application (or library) needs a client with a custom encoder and needs that encoder triggered even if the client method has 0 or more than 1 "body parameters" (parameters not annotated with Feign annotations or custom annotations registered in Feign using DeclarativeContract.registerParameterAnnotation).

A few examples of this use case:

1. An application needs a Feign client whose request payload is defined entirely by a custom Feign encoder even if no parameter is declared at the client method. In this case, even with no presence of body parameter the encoder will know how to define the request payload (based on the method name, method return type, and other optional metadata provided by annotations).
2. An application needs a Feign client whose request payload is defined by a custom Feign encoder plus one or more parameters, all of them annotated with custom annotation(s) registered in Feign using DeclarativeContract.registerParameterAnnotation. In this case the encoder will use all the parameters, plus metadata in their respective annotations, to properly define the request message payload.

To be more specific, this issue is required to support certain use cases in Mocca. This improvement would help to simplify Mocca's API a lot.

We are also open to contributing a PR to implement this feature, if Feign team first accesses this issue and let us know we should do so.

By the way, we tried first to address this use case by creating a custom Feign Contract implementation, but that was not possible because overwriting a parseAndValidateMetadata requires instantiating MethodMetadata and that class has a package-private constructor (besides being final too).

Please let us know your thoughts on this and, if you believe this is an useful enhancement to have, if we can go ahead and provide a PR addressing this (or if you prefer to do so yourselves, please let us know the timeline to provide it).

Thanks.

[fabiocarvalho777]

By the way, regarding how to implement this, I was thinking about providing a subclass of DeclarativeContract. That would be also nice in terms of backwards compatibility. Let me know if that makes sense or if you have another idea to recommend. Thanks.

[kdavisk6]

@fabiocarvalho777 Looks like we work together. Look me up at work and we can chat.

[velo]

HI Fabio,

0 bodies should be fine. Let's see if I got it correctly. By ZERO body you mean a client like this:

interface Bank {
  @RequestLine("POST /account/{id}")
  Account getAccountInfo(@Param("id") String id);
}

Just for checking sake, multi-body like this:

interface Bank {
  @RequestLine("POST /account/")
  Account getAccountInfo(String id, String name, int count);
}

Multiple bodies, can be trick for 2 main reasons:
1 - Detecting what is a body, right now, any arguments that feign can't fit into something else, is set as body. Could use same approach.
2 - Encoder has a single input Object object. We could assume that when MULTIPLE bodies are present, we could "auto cast" to Map<String, Object>, key been the attribute name and value the actual value.

Does my initial pointers make sense?

[fabiocarvalho777]

Hello Marvin,

Your ZERO body example is right, that is what we mean. The problem with it is that with a method like the one in the example, Feign skips any custom encoder set in the client (Feign will use BuildTemplateByResolvingArgs instead of BuildEncodedTemplateFromArgs). That is a problem for us because we need to have our encoder used even for "ZERO body" cases.

And yes, your multi-body example is also correct.

Detecting what is a body, right now, any arguments that feign can't fit into something else, is set as body. Could use same approach.

That is right, any arguments that feign can't fit into something else, is set as body. However, that is only true for one single parameter. If the method has more than one parameter, then this exception is thrown (that is the roadblock we were hitting when we tried that approach).

Encoder has a single input Object object. We could assume that when MULTIPLE bodies are present, we could "auto cast" to Map<String, Object>, key been the attribute name and value the actual value.

That is actually an awesome idea! I had not thought about that!

[fabiocarvalho777]

The only problem with using the map for the body object(s) is that the parameter name is not always necessarily known. And knowing the name of the parameter is important. That is why we were taking a different approach, providing a custom annotation for the parameters (containing the parameter name), registering them in Feign (so they wouldn't be counted as body), and then using our custom encoder to figure out how to write the body based on those annotated parameters. We need the encoder to be triggered though even if the method has zero body parameters.

[fabiocarvalho777]

The map for the body object would only work if the key of the map is an integer (the order of the parameter), so, knowing that, we can get from another map the annotations of each parameter (using MethodMetadata I believe). Similarly to how HTTP headers are handled currently.

So, I believe there are two possible approaches:

1. Support multiple body objects (using a map whose key is the order of each body object parameter in the method).
2. Suport triggering encoders in the case of zero body objects (in this case, the body would be defined using custom annotations and parameters provided by the client, and writing the body using those annotations would be done by a custom encoder, also provided by the client).

The first approach is your idea, but using integer as key.
These are the two approaches I can think of. Are they making sense?

I think we prefer approach number 2 because the name of the parameter is crucial for us (they would be used to set the GraphQL variable name) but, GraphQL and Java have different naming rules and restrictions, so Java parameter names sometimes would have to be different than what the end user wants for the GraphQL variable.

What are your thoughts on this? Any objections on approach 2?

I think approach 2 would be simpler as well in terms of backwards compatibility. All we would have to do is provide a new type of Feign contract, and only clients registered with that contract could use encoders that can be triggered with a zero body method. Just an idea. Or we can modify Feign DeclarativeContract contract to allow encoders to be triggered with zero body. Whatever makes more sense for you.

[fabiocarvalho777]

Here is an example of what we are trying to achieve:

    @Query
    List<Book> getBooks(@Variable("authorId") int authorId, @Variable("offset") int offset, @Variable("limit") int limit);

In this example @Query is a Mocca annotation to indicate a GraphQL query. Mocca behind the scenes create a request message payload using all parameters in this method, and it does so using a Feign encoder. However, currently Feign doesn't let that to happen because the method has "more than one body". And, if we register our custom @Variable annotation, then the "more than one body" problem doesn't happen anymore, but our encoder never gets called, because Feign things we are trying to set the message body using Feign HTTP annotations (which is not the case). By the way, GraphQL doesn't use URI and query parameters, everything goes inside of the message body.

[velo]

The only problem with using the map for the body object(s) is that the parameter name is not always necessarily known

Yeah, we have that @Param annotation that we can use for labeling fields. But is optional, on the assumption that you will compile with the necessary args to have Parameter#name

I think is a fair assumption to use arg1, arg2, arg3 if you don't include the compiler arg and don't include the annotation

[velo]

Your ZERO body example is right, that is what we mean. The problem with it is that with a method like the one in the example, Feign skips any custom encoder s

Ow, I see, ok, that seems to be a simple change to call the encoders with null.

But, there is a big risk of breaking existing encoders left and right...

Encoder {
  void encode(Object object, Type bodyType, RequestTemplate template) throws EncodeException;

  default void encode(Optional<Object> object, Type bodyType, RequestTemplate template) throws EncodeException {
    if(object.isPresent()) {
      encode(object.get(), bodyType, template);
    }
  }

}

How does that sound?!

[fabiocarvalho777]

But, there is a big risk of breaking existing encoders left and right...

That is why I suggested that we could create a new type of Contract (extending DeclarativeContract, named EncoderDeclarativeContract maybe?) and only that contract would allow encoders to receive a null body object. In fact, that new contract would always delegate to the encoder to write the body, regardless of annotations or parameters. That would guarantee backwards compatibility.

The Optional idea is also good, but that would require a new major version of Feign to be released, because that would break backward compatibility. Also, today Feign decides to use BuildEncodedTemplateFromArgs, BuildTemplateByResolvingArgs or BuildFormEncodedTemplateFromArgs using a criteria based on the presence or absence of certain annotations and method parameters. If we add Optional to the body object, and let encoders have null bodies regardless of the Contract they use, that criteria will also have to change, and that would be a major modification in terms of Feign behavior.

I believe offering a new Contract extending DeclarativeContract (EncoderDeclarativeContract maybe?) that guarantees BuildEncodedTemplateFromArgs is always used (always calling the custom encoder) is cleanest option, it preserves Feign current behavior, guarantees backward compatibility, and establishes that, if clients want their custom encoder to always be in charge of defining the body, all they need to do is use this new "EncoderDeclarativeContract".

What do you think?

By the way, I have been working a prototype of EncoderDeclarativeContract. I can show you how it works after it is ready (if it works, of course).

Thanks Marvin.

[kdavisk6]

@fabiocarvalho777 and I spoke and worked through the use case. The solution we landed on was to create an extension of the Encoder interface that can be called without a body index.

Proposed Solution

/* name TBD */
interface RequestTemplateEncoder extends Encoder {

   default void encode(Object body, Type bodyType, RequestTemplate template) {
      /* delegate */
      this.encode(template);
   }

   /**
    * Encoder that derives the body from the Request Template
    */
   void encode(RequestTemplate template);
}

To support this, we'd need to make adjustments to BuildEncodedTemplateFromArgs, updating the condition to remove the null check.

@Override
    protected RequestTemplate resolve(Object[] argv,
                                      RequestTemplate mutable,
                                      Map<String, Object> variables) {
      Object body = argv[metadata.bodyIndex()];
      try {
        encoder.encode(body, metadata.bodyType(), mutable);
      } catch (EncodeException e) {
        throw e;
      } catch (RuntimeException e) {
        throw new EncodeException(e.getMessage(), e);
      }
      return super.resolve(argv, mutable, variables);
    }

Considerations

Removing the null check changes the expectation that Encoder implementations be able to handle null body and type parameters. We would need to further update the interface to change the expectations. This may be seen as a non-backward compatible, breaking change, but since is not explicitly documented in the interface, it could be seen as a documentation update.

  /* update the interface method to indicate that object and type may be null */
  void encode(@Nullable Object object, @Nullable Type bodyType, RequestTemplate template) throws EncodeException;

Alternative

An alternative is to do an implementation type check on the Encoder and adjust the conditional:

@Override
    protected RequestTemplate resolve(Object[] argv,
                                      RequestTemplate mutable,
                                      Map<String, Object> variables) {
       try {
          if (RequestTemplateEncoder.class.isAssignableFrom(this.encoder.class) {
             RequestTemplateEncoder enc = (RequestTemplateEncoder) this.encoder;
             enc.encode(mutable);
          } else {
             Object body = argv[metadata.bodyIndex()];
             checkArgument(body != null, "Body parameter %s was null", metadata.bodyIndex());
             encoder.encode(body, metadata.bodyType(), mutable);
        }
      } catch (EncodeException e) {
        throw e;
      } catch (RuntimeException e) {
        throw new EncodeException(e.getMessage(), e);
      }
      return super.resolve(argv, mutable, variables);
    }

While this change maintains the existing null check, it does introduce an implementation specific side-effect, which may be a maintenance issue later on.

Recommendation

My recommendation is to go with the second solution, and update the Encoder interface with the @Nullable annotations, allowing us to eventually move to the first solution in a later release.

Thoughts?

[velo]

I think just having nullable arguments on Encoder would be good enough.

  /* update the interface method to indicate that object and type may be null */
  void encode(@Nullable Object object, @Nullable Type bodyType, RequestTemplate template) throws EncodeException;

But, by doing so, we need to make a major release, as that has major impacts on any Encoders out there that assume the object will never be null.

Or, we could expand on the implementation type check idea.

We leave Encoder as is, create a new NullableEncoder interface. It would extends Encoder and override existing method by making fields nullable.

When a NullableEncoder is present, we invoke it even if body is null.

But, I don't think BuildEncodedTemplateFromArgs is invoked when a method has zero args. But, whatever is the workflow in that case, we would call the Encoder only if a NullableEncoder is present

[kdavisk6]

Then maybe we should use the alternative where we do an interface check. That way we can release it now and deprecate/document the changes for the next major release. Will that work?

[velo]

Yes, I just updated my comment with that in mind.

[velo]

instead of doing:

          if (RequestTemplateEncoder.class.isAssignableFrom(this.encoder.class) {
             RequestTemplateEncoder enc = (RequestTemplateEncoder) this.encoder;
             enc.encode(mutable);
          } else {
             Object body = argv[metadata.bodyIndex()];
             checkArgument(body != null, "Body parameter %s was null", metadata.bodyIndex());
             encoder.encode(body, metadata.bodyType(), mutable);
        }

we would do:

          Object body = argv[metadata.bodyIndex()];
          if (this.encoder instanceof NullableEncoder || body != null) {
             encoder.encode(body, metadata.bodyType(), mutable);
          } else {
             checkArgument(body != null, "Body parameter %s was null", metadata.bodyIndex());
          }

The else block is only to throw the exceptions as it used to.

[fabiocarvalho777]

Hello,

Thanks @velo and @kdavisk6 for taking the time to look at this!! We appreciate it!
The recommened solution looks promissing and makes sense to me.

There is just one other detail though that needs to be addressed (Kevin, I hadn't noticed it by the time we had our meeting). Correct if I am wrong, but RequestTemplate does not contain the method arguments, right? Implementations of the new Encoder type proposed in this issue would need access to the method arguments as well. Luckily, they are availabe at the moment the encoder is called (see Object[] argv). So, the call to the encoder would have to pass argv as well.

With that additional change, the new encode method would look like this:

void encode(Object[] arguments, RequestTemplate template);

What are your thoughts about this? Is it ok to add Object[] arguments to the new encode method?

[kdavisk6]

The template should be resolved by then so you can access the values using the query and header methods on Request Template. I’ll verify. If not, it’s simple to change the order so it is resolved.

[velo]

void encode(Object[] arguments, RequestTemplate template);

That would totally break Encoder backwards compatibility

[fabiocarvalho777]

void encode(Object[] arguments, RequestTemplate template);

That would totally break Encoder backwards compatibility

@velo Only if we are modifying the original encode method. But that is not the idea here, and is not what Kevin documented here a few hours ago. The idea is to add a second encode method. That is the one I am referring to. See Kevins Proposed solution above (his first comment today).

[fabiocarvalho777]

The template should be resolved by then so you can access the values using the query and header methods on Request Template. I’ll verify. If not, it’s simple to change the order so it is resolved.

I see. I believe there are a few issues (see below) with relying on queries and headers from RequestTemplate to access the parameters (for this particular use case we are discussing here):

1. Aren't queries and headers populated based on Feign annotations for queries and headers? In this use case, those parameters wouldn't be annotated with those annotations.
2. The order of the parameters is important because the parameters custom annotations are provided in order as well, so it is important that those orders match (which is possible if an array of parameters is passed, but not possible if using queries and headers).
3. Semantically speaking, those parameters from the encoder point of view, are not really queries or headers.

[velo]

hrmmm, RequestTemplateEncoder was suggested when no body is present... and a new method with a single arg was proposed.

I suggest preserving the original signature, but using @Nullables as a way to be more compatible with existing code and avoid spliting the code flow.

That way, the same code runs when body is present or when it's missing, but the Encoder allow nulls.

Multiple bodies arguments could be handled by using a Map<ArgumentID, Object>... where ArgumentID has both the index and name for each body argument.

Also, we could just use an empty map and avoid the issue of nulls... but still, that would change behaviour for all existing Encoders out here.

[velo]

@fabiocarvalho777 when you propose Object[] arguments, would that be a copy of all arguments for a given method?

so, if I have
void removeCall(@Header String arg1, @PathParam String arg2, @QueryParam String arg3, @Body arg4, @Body arg5), what would the Object[] arguments looks like?

[kdavisk6]

@fabiocarvalho777 If I recall you mentioned you were using a custom Contract. I assumed that you could manage mapping your annotations and the resulting Method Metadata would have the method arguments mapped to query parameters.

I’m sorry if I misunderstood, but that would be the best way forward fo you here. I’ll write up an example that explains how to achieve what you are looking for with the approach outlined above.

I’m certain we can find a solution that works without breaking the current assumptions around Encoder.

[fabiocarvalho777]

@fabiocarvalho777 when you propose Object[] arguments, would that be a copy of all arguments for a given method?

so, if I have
void removeCall(@Header String arg1, @PathParam String arg2, @QueryParam String arg3, @Body arg4, @Body arg5), what would the Object[] arguments looks like?

If a client is using those Feign annotations (Header, PathParam, etc) then it would not need the special encoder being dicussed here, nor to have more than one Body parameter. In those cases the current behavior should be preserved and enforced (not allowing more than one Body parameter).

The proposed solution Kevin posted here (the one with an extension of Encoder interface with an additional encode method), even with the inclusion of the parameters Object array, still guarantees backwards compatibility. The same API and behavior for all existing clients and their custom contracts and encoders (if existent), would be preserved.

[fabiocarvalho777]

@fabiocarvalho777 If I recall you mentioned you were using a custom Contract.

Correct. That was before your suggestion to have a new Encoder interface. With your idea to have a new Encoder interface the custom contract wouldn't be necessary anymore. And I agree, adding a new Encoder type, instead of Contract, is a cleaner solution.

I assumed that you could manage mapping your annotations and the resulting Method Metadata would have the method arguments mapped to query parameters.

MethodMetadata is not part of Feign SPI. It is final and has a package-private constructor. Are you suggesting to change it in Feign itself?

I’m sorry if I misunderstood, but that would be the best way forward fo you here. I’ll write up an example that explains how to achieve what you are looking for with the approach outlined above.

[velo]

The proposed solution Kevin posted here (the one with an extension of Encoder interface with an additional encode method), even with the inclusion of the parameters Object array, still guarantees backwards compatibility. The same API and behavior for all existing clients and their custom contracts and encoders (if existent), would be preserved.

Yes but create two code paths... and then it raises the question of what should happen if both code paths could be executed.

Regardless of what Contract is been used, I'm curious to understand what the Encoder object[] would look like.

Let's say I'm using my own home brew Contract. Arg1 is a path param, Arg2 is a header, Arg3 and Arg4 are bodies. What would the object[] look like?

[fabiocarvalho777]

what should happen if both code paths could be executed.

The contract for the new Encoder interface should make it clear that the Feign annotations are ignored when that second encode method is called. The user should know then, when he or she explicitly chooses to use that Encoder interface, he or she will gain access to a different integration experience. As long as the contract is clear, and documented in the javadoc, it shouldn't be a problem.

Also, please correct me if I am wrong, but isn't that similar to the situation today when defining the body based on BuildTemplateByResolvingArgs, BuildFormEncodedTemplateFromArgs and BuildEncodedTemplateFromArgs. What I mean is, not all annotations are applicable to all of them, right? (I am not sure about this, please correct me if I am wrong)

[velo]

So, effectively, this should be an Encoder that disables/bypasses Contract?!

[fabiocarvalho777]

No, the contract is not being bypassed. We use a Contract to register all our custom annotations.

[velo]

But still, the new Encoder, would have access to all data contract has access to in order to create a body?!

[fabiocarvalho777]

Correct.

[velo]

I'm wondering if we shouldn't fix this on the Contract then... if Contract is able to properly assemble the body, the Encoder would be free to only encode the body

[fabiocarvalho777]

Yes, actually that is what I thought too at first. Kevin then recommended to use the Encoder, instead of Contract. I think both options might work and have pros and cons. Please let me know which one you guys prefer and I will test in a prototype. :-)

In fact, I already finished a prototype for the Encoder approach and it worked. You can see it here.

If you want, I can try a prototype solving the problem in the Contract instead. Just let me know.

[velo]

Now that I understand the problem a bit better, seems Contract is the correct place to go.

@kdavisk6 I guess is just a matter of understanding why you prefer Encoder over Contract =)

[fabiocarvalho777]

I am in the process of developing a prototype based on a new Contract, instead of a new Encoder type

However, just to save time, @kdavisk6, is that second prototype worth it? Or do you still think the Encoder solution is the best regardless?

I don't have a preference at this point. The Encoder solution works. If the Contract solution also works I guess any decision you guys make I will be glad with.

[fabiocarvalho777]

Hello @velo @kdavisk6 ,

I have just finished the second prototype (based on modifying Contract, instead of adding a new Encoder type). You can see both options below:

1. Adding new Encoder type
2. Modifying BaseContract

I tested them both and they both work as expected.

Obviously anything can be changed in both options, just let me know and I will do it.
Also, those are just prototypes. Once one is chosen, I will improve them as needed and add tests.

So, which of the two options do you prefer?

Thanks.

[kdavisk6]

@velo and @fabiocarvalho777

The contract part was something we spoke about in our discussion, but I felt it would be something that could end up being another decodeSlash, but after looking at what @fabiocarvalho777 I think that would be a better approach. It meets all the criteria and is good MVP for the use case.

Let's go with adding a new parameter, something like alwaysEncodeBody to indicate that encoding will always be called, regardless of whether a @Body annotation, or null body object is present.

Should we see about adding to a method level or is the Contract/Builder level OK for now? Since it will apply to every method?

[fabiocarvalho777]

@velo @kdavisk6 which branch should I create the PR against? master or feign-11?

[fabiocarvalho777]

@velo @kdavisk6 here is the PR #1459.

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.