Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Introduce CompositeHttpHeadersBase to replace HttpHeadersUtil.merge() #5340

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

injae-kim
Copy link
Contributor

related issue: #5146

Motivation:

public static ResponseHeaders mergeResponseHeaders(ResponseHeaders headers,

📌 PRs Overview:

  1. ✅ (this PR) Introduce CompositeHttpHeadersBase to replace HttpHeadersUtil.mergeHeaders(..)
  2. Introduce CompositeHttpBeaderBuilder builder
  3. Introduce DefaultCompositeRequest, ResponseHeaders
  4. (finally) Replace HttpHeadersUtil.mergeHeaders(..) to Composite*Headers

Modifications:

  • Introduce CompositeHttpHeadersBase and CompositeStringMultiMap to replace HttpHeadersUtil.merge()

Result:

  • Now we have CompositeHttpHeadersBase so can implement DefaultCompositeRequest, ResponseHeaders

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: Patch coverage is 84.47552% with 111 lines in your changes missing coverage. Please review.

Project coverage is 74.21%. Comparing base (75d5af1) to head (e9fcd38).
Report is 282 commits behind head on main.

Files with missing lines Patch % Lines
...necorp/armeria/common/CompositeStringMultimap.java 84.67% 52 Missing and 45 partials ⚠️
...ecorp/armeria/common/CompositeHttpHeadersBase.java 82.92% 9 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5340       +/-   ##
===========================================
+ Coverage        0   74.21%   +74.21%     
- Complexity      0    21119    +21119     
===========================================
  Files           0     1803     +1803     
  Lines           0    77283    +77283     
  Branches        0     9948     +9948     
===========================================
+ Hits            0    57354    +57354     
- Misses          0    15254    +15254     
- Partials        0     4675     +4675     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 64 to 70
@Nullable
private StringMultimap<IN_NAME, NAME> appender;
@Nullable
private List<StringMultimap<IN_NAME, NAME>> delegates;

private final Set<IN_NAME> removed = new HashSet<>();
private final Map<IN_NAME, Integer> removedSizeCache = new HashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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). 🙇

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, left a preliminary comment 🙇

}

@Override
public Iterator<Entry<NAME, String>> iterator() {
Copy link
Contributor

@jrhee17 jrhee17 Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..)

Copy link
Contributor Author

@injae-kim injae-kim Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static ResponseHeaders mergeResponseHeaders(ResponseHeaders headers,
HttpHeaders additionalHeaders,
HttpHeaders defaultHeaders,

for (AsciiString name : defaultHeaders.names()) {
if (!isPseudoHeader(name) && !builder.contains(name)) {
defaultHeaders.forEachValue(name, value -> builder.add(name, value));

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~! 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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~!

@minwoox minwoox added this to the 1.27.0 milestone Jan 10, 2024
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the late review. 😓
Basically, looks good and left some suggestions. 👍

Comment on lines 87 to 89
CompositeStringMultimap(@Nullable CompositeStringMultimap<IN_NAME, NAME> additionals,
List<StringMultimap<IN_NAME, NAME>> parents,
@Nullable CompositeStringMultimap<IN_NAME, NAME> defaults,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

public static ResponseHeaders mergeResponseHeaders(ResponseHeaders headers,
HttpHeaders additionalHeaders,
HttpHeaders defaultHeaders,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (contentLength < 0) {
contentLength = 0;
}
contentLength += length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@injae-kim injae-kim Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@injae-kim injae-kim Mar 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙇

return true;
}

if (!(o instanceof HttpHeaderGetters)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional not using CompositeHttpHeadersBase?

Copy link
Contributor Author

@injae-kim injae-kim Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

final HttpData that = (HttpData) o;
if (length() != that.length()) {
return false;
}
return Arrays.equals(array(), that.array());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ikhoon ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024
@injae-kim
Copy link
Contributor Author

Really sorry for late, I'll update this PR within this weekend 🙇

@injae-kim injae-kim force-pushed the composite-http-headers branch 2 times, most recently from 3f7688f to 1480260 Compare February 4, 2024 17:05
*
* @see CompositeHttpHeadersBase
*/
abstract class CompositeStringMultimap<IN_NAME extends CharSequence, NAME extends IN_NAME>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is should be Multi'M'ap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

super(from(parents), DEFAULT_APPENDER_SUPPLIER);
}

CompositeHttpHeadersBase(@Nullable List<HttpHeaderGetters> additionals,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (AsciiString name : additionalHeaders.names()) {
if (!isPseudoHeader(name)) {
builder.remove(name);
additionalHeaders.forEachValue(name, value -> builder.add(name, value));
}
}

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

@Override
final HttpHeadersBase newSetters(int sizeHint) {
return new HttpHeadersBase(sizeHint);
}
@Override
final HttpHeadersBase newSetters(HttpHeadersBase parent, boolean shallowCopy) {
return new HttpHeadersBase(parent, shallowCopy);
}

DefaultHttpHeadersBuilder(HttpHeadersBase parent) {
super(parent);
}
@Override
public HttpHeaders build() {
final HttpHeadersBase delegate = delegate();

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


@Override
public boolean isEndOfStream() {
for (StringMultimapGetters<CharSequence, AsciiString> delegate : delegates()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@injae-kim injae-kim Mar 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()!

Copy link
Member

@minwoox minwoox Mar 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😉

@jrhee17 jrhee17 modified the milestones: 1.28.0, 1.29.0 Apr 2, 2024
@github-actions github-actions bot added the Stale label May 3, 2024
@minwoox minwoox modified the milestones: 1.29.0, 1.30.0 May 21, 2024
@github-actions github-actions bot removed the Stale label May 22, 2024
@github-actions github-actions bot added Stale and removed Stale labels Jun 21, 2024
@injae-kim injae-kim changed the title Introduce CompositeHttpHeadersBase to replace HttpHeadersUtil.merge() [WIP] Introduce CompositeHttpHeadersBase to replace HttpHeadersUtil.merge() Jul 4, 2024
@injae-kim
Copy link
Contributor Author

Really sorry for late, I think we can reduce complexity of CompositeHeader by supporting only add with zero-copy.

  • If we support zero-copy on add, set, remove, logic is too complex and easy to make bug
    • If we usually don't set, remove after mergeHeaders, we can minimize complexity of CompositeHeader

I found that spring has similar CompositeAttribute impl but it has many bugs.

✅ So, I'll check mergeHeaders's usage and prepare benchmark test too~! 🙇

@ikhoon ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 1, 2024
@github-actions github-actions bot added the Stale label Sep 9, 2024
@jrhee17 jrhee17 modified the milestones: 1.31.0, 1.32.0 Nov 5, 2024
@github-actions github-actions bot removed the Stale label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants