Headers behavior in ResponseBuilder

[regbo]

I'm moving some code over from OkHttp and had a gotcha moment today that ate up a lot of my day. Out client was sending multiple copys of request headers and we couldn't figire it out.

In the ResponseBuilder class "headers" is used for adding headers. However, in other parts of the same class, methods with no prefixes are used for setting values. For example version(v) is used for setting.

I understand that there is a setHeader method for the headers, but I would recommend flip flopping the behavior to make it more intuitive. I think the "no-prefix" methods should be for setting, with "add" prefix methods used for adding. For example:

Current com.github.mizosoft.methanol.internal.extensions.ResponseBuilder:

 public ResponseBuilder<T> header(String name, String value) {
    headersBuilder.add(name, value);
    return this;
  }

  public ResponseBuilder<T> setHeader(String name, String value) {
    headersBuilder.set(name, value);
    return this;
  }

  public ResponseBuilder<T> headers(HttpHeaders headers) {
    headersBuilder.addAll(headers);
    return this;
  }

Suggested com.github.mizosoft.methanol.internal.extensions.ResponseBuilder:

  public ResponseBuilder<T> header(String name, String value) {
    headersBuilder.set(name, value);
    return this;
  }

  public ResponseBuilder<T> addHeader(String name, String value) {
    headersBuilder.add(name, value);
    return this;
  }

  public ResponseBuilder<T> headers(HttpHeaders headers) {
    clearHeaders();
    addHeaders(headers);
    return this;
  }

  public ResponseBuilder<T> addHeaders(HttpHeaders headers) {
    headersBuilder.addAll(headers);
    return this;
  }

This primarily came about because we are used to OkHttp client which uses the following logic for setting all headers:

    /** Removes all headers on this builder and adds {@code headers}. */
    public Builder headers(Headers headers) {
      this.headers = headers.newBuilder();
      return this;
    }

[regbo]

Eh... I take it back, it looks like the default JDK Http client uses the behavior your library uses. Sorry about that.

[mizosoft]

Hi @regbo!

As you noticed, I'm trying to be consistent with JDK's HTTP client. The naming convention is also used elsewhere in the library (e.g. MutableRequest). I believe the rationale is that adding fields to a property that represents a collection (e.g., headers) is the common action, so prefixing it as addHeader might be API noise. In case of other non-collection fields (e.g., version), the only action is to set them, so setVersion is also API noise. So the thing is to add a verbal prefix for the less common action. But I understand how this might be confusing, particularly that you're coming from another library with a different convention.

BTW, it's interesting to see you're using ResponseBuilder! Which is an internal API. What's your use case? I can document it and move it (perhaps along with HeadersBuilder) to public API.

[regbo]

BTW, it's interesting to see you're using ResponseBuilder! Which is an internal API. What's your use case? I can document it and move it (perhaps along with HeadersBuilder) to public API.

At the moment it's just being used to chain commands with legacy code as we try to transition more to your project. Also there are a couple of cases where we needed to modify response headers in an intercepter, and the ResponseBuilder is helpful for that.

For example, we have a proxy app that intercepts WebP responses, writes them to disk, converts them to PNG, and then sends it along to the client with modified headers. There may be a better way to go about that, but ResponseBuilder was helpful.

[mizosoft]

I see. Rewriting/modifying responses is a common use case for interceptors, and ResponseBuilder is certainly useful for such purpose. I will open another issue to track moving ResponseBuilder to public API.