Client decorator for appending custom request headers

[heshamMassoud]

Description

As mentioned in a previous issues: #1602 It would be great to be able to add additional user- specified headers to the sphere client that it could append to any of the requests it makes.

It was suggested by you guys (@acbeni & @katmatt) to use the SolutionInfo by injecting our own service implementation which would be loaded in the JVM SDK using the service loader.

However, there are some shortcomings using the SolutionInfo this way:

1. In the case of a library that's using the JVM SDK and want to inject extra header information in the userAgent) (e.g. the commercetools-sync-java). As soon as an application that uses library as a dependency in their project and decides to use it's own custom SolutionInfo as well and then packs the application as a far/über jar. The original resources/META-INF/services/io.sphere.sdk.client.SolutionInfo (added by the library) will be overwritten by the one of the application, causing the implementation expected by the library to be neglected.

2. Header additions are only added to the userAgent header.

3. It is limited / not very flexible with regards to the information that can be added to the header -> (It only provided these information to be implemented: name, version, website, emergencyContact) in addition to the constraints on these fields.

Proposed Solution

Be able to flexibly specify additional headers to the client. This could be implemented with a design consistent with the decorator pattern used in the JVM SDK. So have a new client decorator (e.g. HeadersSphereClientDecorator) where it has a KV map in which the users can specify additional headers that would be made with every request the client makes.

The decorator's implementation would be similar to the standard SphereClientImpl's implementation, except that when creating a request it will use this user-specified KV map of additional headers and simply add it the sphereRequest instance (i.e chain the extra headers to the intent. Similar to how it's done here: https://github.com/commercetools/commercetools-jvm-sdk/blob/f5d3d06397de68ace17d008797022888ed8c389d/commercetools-java-client-core/src/main/java/io/sphere/sdk/client/SphereClientImpl.java#L109-L117)

With this proposal, the headers will be appended to the request of the client at runtime, which means it the headers cannot be removed by the application using the library. It also means that users of the JVM SDK would be able to as much headers as they want to the key that they desire (not limited to the userAgent).

We could provide a PR such proposal as soon as we have time ;) But decided to create this issue so that we can discuss with you guys the proposal if you agree to it.

[katmatt]

Basically you are suggesting to avoid using our solution info service because you're afraid that someone might use multiple solution info enabled jars in a fat jar 😉 Or am I wrong? Do you have any customer that is using such a setup?

And with your solution every user of our JVM SDK has to implement his own info solution provider and could even suppress sending of the user agent header at all. And which additional headers do you want to send? And why should we make our JVM SDK more complex with a feature that very few of our users will ever use? and they still can use our SphereClientDecorator.

That doesn't sound very convincing to me, but may be you have a more specific real use case that could convince us? 😉 😄

[heshamMassoud]

@katmatt

Basically you are suggesting to avoid using our solution info service because you're afraid that someone might use multiple solution info enabled jars in a fat jar 😉 Or am I wrong? Do you have any customer that is using such a setup?

Yes, indeed that is my concern. Well, as an example, any user depending on the commercetools-sync-java library in his sync application, packs the application in a fat jar. Consequently, if any of such users add their own implementation of solution info, it would override the solution info of the commercetools-sync-java, which means that now we we lost track of such a user of the library from the logs - making it an unreliable way for tracking.

And with your solution every user of our JVM SDK has to implement his own info solution provider and could even suppress sending of the user agent header at all.

No, not every user needs to implement it. There are some information in the header that we can keep as obligatory (e.g. commercetools-jvm-sdk/1.27.0 (Apache-CloseableHttpAsyncClient/unkown) Java/1.8.0_151-b12 (Linux; amd64)) and the rest of information is optionally appended to it in the same userAgent header or any other headers, as the user desires. So basically it's not allowed to suppress the userAgent header because by default it has this info.

And which additional headers do you want to send?

I think it would be much more flexible and easier for the users to specify the headers with their own keys and values, for easier log filtering for example. (cc: I remember @andrii-kovalenko-ct & @butenkor mentioned that it would be good to have such feature in some application) And in any case, I think it's valid to ask the question, if we already give the user the option to customise the header, then why limit them with such constraint? (name, version, website, emergencyContact and all on the userAgent header)

And why should we make our JVM SDK more complex with a feature that very few of our users will ever use? and they still can use our SphereClientDecorator

I honestly can't see how this makes it more complex. It's an additional feature that could be used or not at all 😊 And that's why it follows the Decorator Pattern -> additional behaviour that only affects the behaviour of an instance but doesn't affect other instances from the same class.

[andrii-kovalenko-ct]

@katmatt

And which additional headers do you want to send?

For example: Ctp

"Ctp-Service-Name: commercetools-payone-integration"
"Ctp-Service-Version: 1.X.X-RC.1"

And why should we make our JVM SDK more complex with a feature that very few of our users will ever use?

It is just an adding optional feature to the library. If one doesn't need it - the just ignore the new decorator implementation.

and they still can use our SphereClientDecorator.

Not really: SphereClientImpl#createHttpRequest() has private access, and actually it returns already built HttpRequest instance. Our proposition is:

1. Either re-factor SphereClientImpl#createHttpRequest() adding smth like:

    .httpRequestIntent()
    .withHeaders(createExtraHttpHeaders())
    .plusHeader(HttpHeaders.X_CORRELATION_ID, correlationId)
    ....
   

   and make overridable protected HttpHeaders createExtraHttpHeaders() returning empty headers by default

2. Split SphereClientImpl#createHttpRequest() so it returns HttpRequestIntent and then SDK consumers could decorate the intent before .toHttpRequest() is called by SDK in execute() method. Smth like this:

     private<T> HttpRequestIntent createHttpRequest(final SphereRequest<T> sphereRequest,final String token){
         final String correlationId=correlationIdGenerator.get();
         HttpRequestIntent intent = sphereRequest
             .httpRequestIntent()
             .plusHeader(HttpHeaders.X_CORRELATION_ID,correlationId)
             .plusHeader(HttpHeaders.USER_AGENT,userAgent)
             .plusHeader(HttpHeaders.ACCEPT_ENCODING,"gzip")
             .plusHeader(HttpHeaders.AUTHORIZATION,"Bearer "+token)
             .prefixPath("/"+config.getProjectKey());
   
             return decorateIntent(HttpRequestIntent);
     }
   
     private<T> CompletionStage<T> execute(final SphereRequest<T> sphereRequest,final String token,final int ttl){
         final HttpRequest httpRequest=createHttpRequestIntent(sphereRequest,token)
             .toHttpRequest(config.getApiUrl())
         .....
     }
   
     /**
      * By default do nothing
      */
     protected HttpRequestIntent decorateIntent(HttpRequestIntent intent) {
         return intent;
     }

   This approach is more flexible, but, as usual, more flexibility makes shooting to the foot easier (for example, SDK consumers could remove default mandatory headers or so on)

3. (the easiest, but the strictest one) introduce just one additional header (consider Ctp-Custom-Header) and if SDK consumers sets/overrides some sphere client value - set it:

    private <T> HttpRequest createHttpRequest(final SphereRequest<T> sphereRequest, final String token) {
      final String correlationId = correlationIdGenerator.get();
      HttpIntent intent = sphereRequest
           .httpRequestIntent()
           .plusHeader(HttpHeaders.X_CORRELATION_ID, correlationId)
           .plusHeader(HttpHeaders.USER_AGENT, userAgent)
           .plusHeader(HttpHeaders.ACCEPT_ENCODING, "gzip")
           .plusHeader(HttpHeaders.AUTHORIZATION, "Bearer " + token)
           .prefixPath("/" + config.getProjectKey());
       String customHeader = getCustomHeader();
       if (isNotBlank(customHeader)) {
         intent = intent.plusHeader("Ctp-Custom-Header", customHeader);
       }
       return intent.toHttpRequest(config.getApiUrl());
    }

   In this case shooting to the foot is the most difficult, but header name is hardcoded

[heshamMassoud]

@andrii-kovalenko-ct We could also have it in a way that is not as strict as 3 but also flexible in way that doesn't allow the users to remove the default mandatory headers as you mentioned in point 2:

The user would give a map of header key to value to the decorator and the decorator would simply add the provided headers without affecting the mandatory ones. This way you have:

1. More than one custom header (More flexible/less strict than point 3)
2. You cannot remove the mandatory headers (so no shooting in the foot as in point 2)

[andrii-kovalenko-ct]

@heshamMassoud that's exactly way #1 i described, isn't it?

[heshamMassoud]

@andrii-kovalenko-ct ah ok, if you meant that, then yes 😊

The .withHeaders(createExtraHttpHeaders()) made it seem that it overwrites the previously set headers. Like the difference between .withPredicates and .addPredicates in the JVM SDK query building -> with overwrites and add just adds 😉

[andrii-kovalenko-ct]

ha, you are right, .withHeaders() should be before all plusHeader()

[schleichardt]

You can create super flexible decorators yourself:

public final class HeaderSphereClientDecorator extends SphereClientDecorator {
    protected HeaderSphereClientDecorator(SphereClient delegate) {
        super(delegate);
    }

    @Override
    public <T> CompletionStage<T> execute(SphereRequest<T> sphereRequest) {
        return super.execute(new HeaderSphereRequestDecorator<>(sphereRequest));
    }

    private static final class HeaderSphereRequestDecorator<T> extends SphereRequestDecorator<T> {
        protected HeaderSphereRequestDecorator(SphereRequest<T> delegate) {
            super(delegate);
        }

        @Override
        public HttpRequestIntent httpRequestIntent() {
            return super.httpRequestIntent().plusHeader("foo", "bar");
        }
    }
}

[andrii-kovalenko-ct]

@schleichardt Thanks, we didn't know we can decorate the requests themselves! We knew only SphereClient decoration feature.

[heshamMassoud]

Using the super.httpRequestIntent().plusHeader("foo", "bar"); in https://github.com/commercetools/commercetools-jvm-sdk/issues/1691#issuecomment-365202896 actually would add a <"foo", "bar"> entry to the list of NameValuePairs even if there is a NameValuePair with the same key already existing e.g. <"foo", "another-bar"> rather than change the value. This is due to the implementation of it here:

https://github.com/commercetools/commercetools-jvm-sdk/blob/2080b5b5d827ce8760d2c6937b1220c415dff920/sdk-http/src/main/java/io/sphere/sdk/http/HttpHeaders.java#L74-L79

@schleichardt is this on purpose? or should it set the value of the same key?

Because with this way, decorating the requests won't help in setting the "User-Agent", as it would lead to having 2 headers with the "User-Agent" key in the same request.

[schleichardt]

Yes, it is intentionally because HTTP headers could be repeated: https://stackoverflow.com/a/4371395/5320693

If you want to change existing headers, don't use plus but getHeadersAsMap() and create a new HttpHeaders instance with a Map that has the new value.

[heshamMassoud]

Hmm, ok. Thanks for your reply. Unfortunately, this means that decorating the request won't help in changing the headers because in the decorator the headers are not "yet" attached to the request. They are rather attached in SphereClientImpl (which kicks in after the decorator logic) https://github.com/commercetools/commercetools-jvm-sdk/blob/2080b5b5d827ce8760d2c6937b1220c415dff920/commercetools-java-client-core/src/main/java/io/sphere/sdk/client/SphereClientImpl.java#L110-L117