Java - RequestConfiguration gets applied to empty headers and options

[andreaTP]

Description

Currently, to be able to manually operate on the headers and options parameters for a given endpoint call the only viable way is to patch the already constructed RequestInformation object.

Current situation

An example RequestInformation looks as follows:

clientV2.groups().byGroupId(groupId).artifacts().byArtifactId(artifactId).versions().toPostRequestInformation(content, config -> {
    config.headers.add("X-Registry-Name", name);
});

but any change to already default headers is going to be ignored, e.g.:

clientV2.groups().byGroupId(groupId).artifacts().byArtifactId(artifactId).versions().toPostRequestInformation(content, config -> {
    config.headers.clear();
    config.headers.add("Content-Type", "application/xml");
});

unexpectedly the headers are not cleaned up and the additional Content-Type is appended as an extra value instead of replacing the default one.

Proposal

The config object passed to the closure should be the original one(or "a view of it") exposing the original headers and options fields so that a user does have idiomatic access to manipulate them.

At the moment, it's possible to workaround the issue by creating the RequestInformation object and operating on it, such as:

var postReq = clientV2.groups().byGroupId(groupId).artifacts().byArtifactId(artifactId).versions().toPostRequestInformation(content);
postReq.headers.clear();
postReq.headers.add("Content-Type", "application/xml");

NOTE: given the fact that a viable workaround exists, the priority of this request should be low.

[baywet]

Thanks for reporting this.
The generation is effectively setting only two request headers:

• Content-Type
• Accept

For both it'd be easy to add a tryadd method in the headers class and then update the request information and as well as the generation to make use of that new method.

Is this something you'd be willing to submit a couple of pull requests for?

[andreaTP]

Is this something you'd be willing to submit a couple of pull requests for?

Slowly, I probably can work on this next week or the following, instead of doing it on a case-by-case, I think that a more robust approach will be to rely on the maps already being generated instead of applying the lambda on empty parameters, does it make sense to you?

[baywet]

The reason why we're working on an empty map of headers instead of the original one is because we don't want the client code to maintain a reference to the headers somehow and potentially mess things up.
Switching the order of the statements and using a try add instead should accomplish the goal you want with minimal impact on the design.

[darrelmiller]

            var requestInfo = new RequestInformation {
                HttpMethod = Method.POST,
                UrlTemplate = UrlTemplate,
                PathParameters = PathParameters,
            };
            requestInfo.Headers.Add("Accept", "application/json");
            requestInfo.SetContentFromParsable(RequestAdapter, "application/x-www-form-urlencoded", body);
            if (requestConfiguration != null) {
                var requestConfig = new AppsRequestBuilderPostRequestConfiguration();
                requestConfiguration.Invoke(requestConfig);
                requestInfo.AddRequestOptions(requestConfig.Options);
                requestInfo.AddHeaders(requestConfig.Headers);
            }
            return requestInfo;

In the above code we shouldn't be defaulting values in requestInfo if they also exist in requestConfig. We should default in the requestConfig and allow the dev to change the values and then copy back to requestInfo.

[baywet]

how can they already exist in the request info if we new it up in the line right above? can you clarify please @darrelmiller?

[andreaTP]

I was thinking along the lines of @darrelmiller something like:

            var requestConfig = new AppsRequestBuilderPostRequestConfiguration();
            requestConfig.Headers.Add("Accept", "application/json");
            ...
            if (requestConfiguration != null) {
                requestConfiguration.Invoke(requestConfig);
            }
            
            requestInfo.AddRequestOptions(requestConfig.Options);
            requestInfo.AddHeaders(requestConfig.Headers);

[baywet]

Thanks for the precision here.
This approach would only partly address the issue since this line sets the content type header, directly on the request information.
requestInfo.SetContentFromParsable(RequestAdapter, "application/x-www-form-urlencoded", body)

[andreaTP]

The least invasive change would be to change the method SetContentFromParsable to accept an additional argument, which will be the target Dictionary to be modified, in this case: requestConfig.Headers.

wdyt?

[baywet]

looking at request information itself it'd seem odd, why is it setting the body to a property of the request information but the header on an external map even though request information has one? Plus it'd be a breaking change for Go.

I'm not sure what's the issue with changing the order of statements and adding try add instead like I proposed earlier?

var requestInfo = new RequestInformation {
    HttpMethod = Method.POST,
    UrlTemplate = UrlTemplate,
    PathParameters = PathParameters,
};
if (requestConfiguration != null) {
    var requestConfig = new AppsRequestBuilderPostRequestConfiguration();
    requestConfiguration.Invoke(requestConfig);
    requestInfo.AddRequestOptions(requestConfig.Options);
    requestInfo.AddHeaders(requestConfig.Headers);
 }
requestInfo.SetContentFromParsable(RequestAdapter, "application/x-www-form-urlencoded", body); // switched order, uses try add
requestInfo.Headers.TryAdd("Accept", "application/json"); // switched order, uses try add
return requestInfo;

[andreaTP]

I'm, personally, not enthusiastic about this proposal.
I think that it simply pushes the limits of the current design without actually solving the underlying issue.

The "solution" I would like to see here, is to be in complete control, from the user code, of the generated defaults in Headers and Options.

With this last proposal, the user is not in control, but it's allowed to "hack back" the current implementation which is not ideal.

More specifically:

1. the user is not in control of replacing the Content-Type header e.g. looking up the current value, which is one of the objectives
2. the user cannot control the Accept header
3. any additional header and option coming from the code generation might interfere in future implementations

Instead of:

1. asking the user to provide a configuration from blank
2. performing opinionated patches on top of it

I think that a more solid design(and possibly more resilient to code-generator changes) would be to:

1. fully form the headers and options from the generation
2. let the user modify them

[baywet]

To clarify things with the approach I'm proposing:

• We let the user provide a full set of headers.
• The generated code sets values for two headers (Accept/Content-Type) only if no value has been provided in the requests configuration

Yes that doesn't let them set the defaults as they are setting new values, but this design also prevents the defaults from being cleared and not set afterwards, which would derail the pipeline.

Lastly, if the user wants full control over the headers, they can implement a middleware handler (with or without options)

[andreaTP]

I agree there are workarounds and here we are discussing "how to make it more convenient".

I understand your motivation and tradeoffs, I think that the different bias comes from the perspective.

I see this usage as "advanced" where the user deliberately wants full control.

@baywet wants to prevent possible abuses in standard usage of the functionality.

A third opinion would break the tie, otherwise we'd have to go with your proposed approach as it's, anyhow, an improvement over the current encoding.

[baywet]

FYI we briefly talked about this one too during our brainstorming session yesterday and I was able to rally @darrelmiller to the proposal I made. If people want full control over the headers for a very advanced scenario, they have the middleware option, they can also get the request information, tweak it, and send it manually.

[andreaTP]

ok, deal, will look into this next week likely.