FYI) to handle modifying operations (add, set, remove) efficiently without expensive copy, I apply append-only data structure.

add() only on appender, remove() -> just mark as removed on removed (like tombstone on kafka). 🙇

Question) To match the current merge logic, should this loop deduplicate such that only one key is returned?

i.e.

final CompositeStringMultimap<CharSequence, AsciiString> headers =
        new CompositeHttpHeadersBase(HttpHeaders.of("k1", "v1"),
                                     HttpHeaders.of("k1", "v2"));
for (Entry<AsciiString, String> entry: headers) {
    System.out.println(entry); // ("k1", "v2") only should be printed
}

ditto for the other iterating methods (forEach, forEachValue, etc..)

Oh you're right! based on current merge logic, we don't add duplicated key on mergeHeaders(). I miss it 😅

I think CompositeHeaders can support merge (remove duplicated key) and just composite options.
(or maybe we can explicitly introduce CompositeHeader and MergedHeader class separately,
or Iwe can just simply introduce additionalHeaders, defaultHeaders field on CompositeHeader!)

Anyway, I'll think more about this design and share to you soon~! 🙇

// 1) Merge headers same with HttpHeadersUtil.merge(additionalHeaders, headers, defaultHeaders)
CompositeHeader additionals = new CompositeHeader(add1, add2, ..);
for (Name name : additional.names() {
  if (isPsedoHeader(name) ||
      ADDITIONAL_REQUEST_HEADER_DISALLOWED_LIST.contains(name)) {
    additional.remove(name) // Only mark as removed, without deep copy
  }
}

..

// Can composite default and internal header like this without deep copy
HttpHeaders internals = HttpHeaders.of(HttpHeaderNames.SERVER, ArmeriaHttpUtil.SERVER_HEADER);
CompositeHeader defaults = new CompositeHeader(default1, internals);

// 2) Also, can composite headers
CompositeHeader headers = new CompositeHeader(header1, header2, ..);

✅ To respect current HttpHeadersUtil.merge(additionals, headers, defaults) logic,
I make CompositeHeader can merge additional, header, default too with same logic, without deep copy~!

It seems like the additionals and defaults CompositeStringMultimaps are tightly coupled to the logic that merges response headers and I think they complicates this class.
How about removing those and just taking the list of StringMultimap? The additionalHeaders, headers, and defaultHeaders are specified in that order so that the headers in additionalHeaders take precedence.
new CompositeHttpHeadersBase(additionalHeaders, headers, defaultHeaders)

How about removing those and just taking the list of StringMultimap?

✅ updated, nice suggestion! this way looks much simpler 👍

I'm not sure about aggregating all the Content-Length headers as it's unlikely that it accurately represents the actual content length. How about using the length of the first encountered header as we do for others?

How about using the length of the first encountered header as we do for others?

✅ updated! agree with this way.

Similarly, I'm not sure how we should handle on isEndOfStream case 🤔

• e.g. If only some of the parents are endOfStream: true, what should we return on isEndOfStream()?

PTAL 🙇

That's a good question.
Based on previous usage, simply using the flag of the first header may not be sufficient.

I see two potential options:

• If there's a headers with isEndOfStream`` set to true``, consider the composite headers as true as well. This approach seems reasonable, as the composite headers should reflect the end-of-stream status if any of them does.
• Ignore the isEndOfStream flag in all headers objects and allow the user to specify it after composition.

What do you think of it?
Of course, others might have different opinions. cc @line/dx

If there's a headers with isEndOfStream set to true, consider the composite headers as true as well.

I think option1 looks better so updated code and test~! thank you! 🙇

Is this intentional not using CompositeHttpHeadersBase?

// CompositeStingMultimap
        if (that instanceof CompositeStringMultimap ||
            that instanceof StringMultimap) { // allow StringMultimap to compare too
            for (NAME name : names()) {
                if (!getAll((IN_NAME) name.toString())
                        .equals(that.getAll((IN_NAME) name.toString()))) {
                    return false;
                }
            }

Aha right. on CompositeStingMultimap#equal, I think we may can compare CompositeStingMultimap with StringMultimap too.

So i intended to just check !(o instanceof HttpHeaderGetters) here to allow both CompositeStingMultimap and StringMultimap.

Hmm should we allow only o instanceof CompositeHttpHeadersBase to check equality here?
I'm not sure so please share your opinion~! thanks 🙇

I think it's okay because they are equal if they have the same headers no matter what the type is.
We have the similar case for ByteArrayHttpData:

I got it! I added related test case :) (compare equality with CompositeHeader and just Header)

I think is should be Multi'M'ap.

Oh I just copy&paste it from StringMultimap. should we use Multi'M'ap here?

Oh, I don't know what I was thinking when I made that comment. Sorry. Let's just leave it as it is.

Considering the current usage, which involves three different headers in HttpHeadersUtil, it seems that this constructor isn't necessary. Do you have a specific reason for adding it?

in HttpHeadersUtil, it seems that this constructor isn't necessary.

If we don't have this constructor, how can we pass additioanls headers to CompositeStringMultimap? 🤔

Also, this constructor is needed for next PR, to add CompositeHttpHeaderBuilder similar with HttpHeaderBuilder!
It uses new HttpHeaderBase(..) constuructor.

If we don't have this constructor, how can we pass additioanls headers to CompositeStringMultimap?

CompositeHttpHeadersBase(HttpHeaderGetters... parents) {...}

This constructor accepts multiple HttpHeaders. Can't users simply specify the additional headers that take precedence as the first argument?

/**
 * Merges the given {@link ResponseHeaders}. The headers have priority in the following order.
 * <pre>{@code
 * additional headers (highest priority) > headers > default headers > framework headers (lowest priority)
 * }</pre>
 */
public static ResponseHeaders mergeResponseHeaders(ResponseHeaders headers,
                                                   HttpHeaders additionalHeaders,
                                                   HttpHeaders defaultHeaders) {
    ...
    new CompositeHttpHeadersBase(additionalHeaders, headers, defaultHeaders);
    ...
}

this constructor is needed for next PR, to add CompositeHttpHeaderBuilder similar with HttpHeaderBuilder!
It uses new HttpHeaderBase(..) constructor

I might miss this. Could you share an example of how it will be used?

Shouldn't we check the appender first?
Additionally, we need to ensure that the appender isn't created if it's null.

#5340 (comment) If there's a headers with isEndOfStream set to true, consider the composite headers as true as well.

As we discussed like above, I think it's not mandatory to always check appender first on isEndOfStream !

Cause if one of headers is end-of-stream, we return composite headers' isEndOfStream as true.
For optimization, we can check appender first 😄

Additionally, we need to ensure that the appender isn't created if it's null.

    protected final StringMultimap<IN_NAME, NAME> appender() {
        if (appender != null) {
            return appender;
        }
        appender = requireNonNull(appenderSupplier.get(), "appender");

        final ImmutableList.Builder<StringMultimapGetters<IN_NAME, NAME>> builder = ImmutableList.builder();

        // 👈👈  If appender is created, add appender on delegates.
        delegates = builder.addAll(delegates()) 
                           .add(appender)
                           .build();
        return appender;
    }

    protected final List<StringMultimapGetters<IN_NAME, NAME>> delegates() {
        if (delegates != null) {  // 👈👈 if appender is created or delegates() is called before, return it.
            return delegates;
        }
    }

Nice point! I considered this case and on delegates(), it doesn't contain appender if appender is not created yet~!

So we don't have to check appender is null or not when using delegates()!

As we discussed like above, I think it's not mandatory to always check appender first on isEndOfStream !

I apologize for my previous comment. It doesn't contain all the information
I thought we still needed to check the appender if endOfStream is set after the CompositeHttpHeadersBase is created.
I imagined a case something like:

CompositeHttpHeadersBase headers =
  new CompositeHttpHeadersBase(additionalHeaders, endOfStreamTrueHeaders);
headers.endOfStream(false);

Additionally, we need to ensure that the appender isn't created if it's null.

What I meant was that we need to ensure that the appender is not created if we change the logic to check for the appender first. 😉