Support one-time configuration of expression parameters

[hellproxy]

Problem

Let's say I have a request template that looks like this:

@RequestLine("GET /{env}/{resource}")
void getResource(@Param("env") String env, @Param("resource") String resource);

In the application that defines this template, env is configured at startup and will only ever have 1 value (e.g. "production" or "qa") during the application's lifespan. It would make sense to not have to pass this same value into each call of getResource().

Default methods are of no help here because env still needs to be dynamically set.

Proposal

Support on-time configuration of expression parameters via configuration methods on the client interface:

@Configuration
void setEnv(@Param("env") String env);

@RequestLine("GET /{env}/{resource}")
void getResource(@Param("resource") String resource);

Implementing this is pretty easy (~150 lines).

N.B. this proposal is somewhat similar to https://github.com/OpenFeign/feign/issues/1359.

[velo]

Sounds awesome, I would love to see this done.

Feel free to ping me once you have a PR.

Maybe, call it @Shared instead of configuration

[kdavisk6]

I've built something like this in feignx already. I rely on the Expander concept. Maybe we could consider the same here. We could register an Expander without having to tie it to the Param annotation:

@Expander(value = "env", expander = EnvironmentExpander.class)
@RequestLine("GET /{env}/{resource}")
void getResource(@Param("resource") String resource);

We would need to modify now is the Contract to add the Expander during method parsing. This would provide the capability quickly without really changing the contract. One drawback I see though is there were more than one parameter that would apply. For that, I think we may want to consider adding an ExpanderRegister, where users can register Expander instances by Class or Name. Again, I've done this in feignx:

https://github.com/OpenFeign/feignx/blob/main/core/src/main/java/feign/template/ExpanderRegistry.java

I could see about backporting it. What do you think?

[hellproxy]

@velo - Thanks for the enthusiasm! Here's the PR: https://github.com/OpenFeign/feign/pull/1413

@kdavisk6 - Good point, I haven't considered expanders in my implementation yet. I do prefer having explicit @Param-annotated configuration methods though. Maybe your context injections could hook into these?

[kdavisk6]

The more I think about this, the more I like the idea of expanding @Param to be used at the method and class level. Here's an example:

@Param(value = "tenant", expander = TenantExpander.class)
interface MultiTenantResourceApi {

   @Param(value = "env", expander = EnvironmentExpander.class)
   @RequestLine("GET /{tenant}/{env}/{resource}")
   void getResource(@Param("resource") String resource);
}

This approach meets the initial request, can be included by Contract extensions already out in the ecosystem without modification, and provides full control over how the parameter is resolved.

Changes to make this possible would be limited refactoring the current @Param annotation processor into a dual purpose process that can be registered on the class, method, and parameter:

class ParamAnnotationProcessor implements ParameterAnnotationProcessor, AnnotationProcessor {

   public void process(E annotation, MethodMetadata metadata, int paramIndex) {
   
   }

   public void process(E annotation, MethodMetadata metadata) {

   }
}

@hellproxy does this meet your needs? @velo any feedback?

[velo]

Problem is if EnvironmentExpander needs to read some configuration from a database or need some other classes access....

Let's say I have a client that I authenticate and I get a Bearer token that expires in 20 minutes....

It would be a very good use case for this, so, when I create the client instance, I also authenticate and run setBearerToken()

What the present code doesn't deal very well is expiry the token.

But using an expander would make harder to inject configurations or services or other clients necessary to get the value

[velo]

Another thing we need too consider is: Do we want this to affect ALL contracts or just the default contract?!

[velo]

Wondering if we should do something like this instead:

var client = Feign.builder()
.decoder(...)
.encoder(...)
.contract(...)
.configuration("env", "staging")
.target(...);

[kdavisk6]

Problem is if EnvironmentExpander needs to read some configuration from a database or need some other classes access....

Let's say I have a client that I authenticate and I get a Bearer token that expires in 20 minutes....

It would be a very good use case for this, so, when I create the client instance, I also authenticate and run setBearerToken()

What the present code doesn't deal very well is expiry the token.

But using an expander would make harder to inject configurations or services or other clients necessary to get the value

I think that is more of a limitation of the Expander than the proposed solution. If we supported Expander singleton instances through the annotations, that would solve that problem.

---

Another thing we need too consider is: Do we want this to affect ALL contracts or just the default contract?!

Almost every other Contract extends from Default. This change wouldn't necessarily change those, but make this usage available to it.

---

Wondering if we should do something like this instead:

var client = Feign.builder()
   .decoder(...)
   .encoder(...)
   .contract(...)
   .configuration("env", "staging")
   .target(...);

I don't see why we can't support both.

Feign.builder()
   .decoder()
   .encoder()
   .contract()
   .parameter(name, new Expander()) // singleton dynamic use case
   .parameter(name, "static value") // constant use case
   .build()

[velo]

I like this:

Feign.builder()
   .decoder()
   .encoder()
   .contract()
   .parameter(name, new Expander()) // singleton dynamic use case
   .parameter(name, "static value") // constant use case
   .build()

I just don't see the point on using the Expander for this

  interface Expander {

    /**
     * Expands the value into a string. Does not accept or return null.
     */
    String expand(Object value);
  }

What would be the value when this was called?

We could add the following to the builder:

    public Builder parameter(String staticValue)
      return this.parameter(() -> staticValue);
    }

    public Builder parameter(Integer staticValue)
      return this.parameter(() -> staticValue);
    }

    public Builder parameter(Object?! staticParameter)
      return this.parameter(() -> staticValue);
    }

    public Builder parameter(Supplier<Object> dynamicParameter) {
      // magic goes here
      return this;
    }

[kdavisk6]

Agreed. I was confusing Expander with something I've been working on in feignx. We can add a supplier to the annotation too.

@Target({ElementType.PARAMETER, ElementType.TYPE, ElementType.METHOD})
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface Param {

  /**
   * Name of the Parameter.
   *
   * @return parameter name.
   */
  String value();

  /**
   * Expander instance to use.
   *
   * @return the expression expander type.
   */
  Class<? extends Expander> expander() default DefaultExpander.class;

  /**
   * Supplier instance that provides the value for the parameter.  Optional.
   *
   * @return a Supplier type.
   */ 
  Class<? extends Supplier<String>> supplier() default EmptyValueSupplier.class;
}

@Param(value = "tenant", supplier= TenantSupplier.class)
interface MultiTenantResourceApi {

   @Param(value = "env", supplier= EnvironmentSupplier.class)
   @RequestLine("GET /{tenant}/{env}/{resource}")
   void getResource(@Param("resource") String resource);
}

[velo]

I don't see any point on having the @Param if we include this on the builder.

Am I missing something?

[kdavisk6]

I don't see any point on having the @Param if we include this on the builder.

Am I missing something?

I'm looking towards the annotation processor, but we can figure that out later. Let's move forward with your suggestions.