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.