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
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.common;

import static com.google.common.base.Preconditions.checkArgument;
import static com.linecorp.armeria.common.StringMultimap.DEFAULT_SIZE_HINT;
import static java.util.Objects.requireNonNull;

import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

import com.google.common.collect.ImmutableList;

import com.linecorp.armeria.common.annotation.Nullable;

import io.netty.util.AsciiString;

/**
* The composite container implementation which wraps {@link HttpHeadersBase}s
* to avoid expensive copy operations.
*/
class CompositeHttpHeadersBase
extends CompositeStringMultimap</* IN_NAME */ CharSequence, /* NAME */ AsciiString>
implements HttpHeaderGetters {

private static final Supplier<StringMultimap<CharSequence, AsciiString>> DEFAULT_APPENDER_SUPPLIER =
() -> new HttpHeadersBase(DEFAULT_SIZE_HINT);

CompositeHttpHeadersBase(HttpHeaderGetters... parents) {
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?

List<HttpHeaderGetters> parents,
@Nullable List<HttpHeaderGetters> defaults) {
super(additionals != null ? from(additionals) : null,
from(parents),
defaults != null ? from(defaults) : null,
DEFAULT_APPENDER_SUPPLIER);
}

private static List<StringMultimap<CharSequence, AsciiString>> from(HttpHeaderGetters... headers) {
if (headers.length == 0) {
return ImmutableList.of();

Check warning on line 59 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L59

Added line #L59 was not covered by tests
}

final ImmutableList.Builder<StringMultimap<CharSequence, AsciiString>> builder =
ImmutableList.builder();
for (HttpHeaderGetters header : headers) {
if (header instanceof HttpHeadersBase) {
builder.add((HttpHeadersBase) header);
} else {
builder.add(new HttpHeadersBase(header));
}
}

return builder.build();
}

private static List<StringMultimap<CharSequence, AsciiString>> from(List<HttpHeaderGetters> headers) {
if (headers.isEmpty()) {
return ImmutableList.of();

Check warning on line 77 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L77

Added line #L77 was not covered by tests
}

final ImmutableList.Builder<StringMultimap<CharSequence, AsciiString>> builder =
ImmutableList.builder();
for (HttpHeaderGetters header : headers) {
if (header instanceof HttpHeadersBase) {
builder.add((HttpHeadersBase) header);
} else {
builder.add(new HttpHeadersBase(header));

Check warning on line 86 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L86

Added line #L86 was not covered by tests
}
}

return builder.build();
}

final void contentLength(long contentLength) {
checkArgument(contentLength >= 0, "contentLength: %s (expected: >= 0)", contentLength);
remove0(HttpHeaderNames.CONTENT_LENGTH);
((HttpHeadersBase) appender()).contentLength(contentLength);
}

@Override
public long contentLength() {
return getLong(HttpHeaderNames.CONTENT_LENGTH, -1);
}

final void contentLengthUnknown() {
remove0(HttpHeaderNames.CONTENT_LENGTH);
((HttpHeadersBase) appender()).contentLengthUnknown();
}

@Override
public boolean isContentLengthUnknown() {
return ((HttpHeaderGetters) appender()).isContentLengthUnknown();
}

final void contentType(MediaType contentType) {
requireNonNull(contentType, "contentType");
remove0(HttpHeaderNames.CONTENT_TYPE);
((HttpHeadersBase) appender()).contentType(contentType);
}

@Override
public @Nullable MediaType contentType() {
final String contentTypeString = get(HttpHeaderNames.CONTENT_TYPE);
if (contentTypeString == null) {
return null;
}

try {
return MediaType.parse(contentTypeString);
} catch (IllegalArgumentException ex) {
return null;

Check warning on line 130 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L129-L130

Added lines #L129 - L130 were not covered by tests
}
}

final void contentDisposition(ContentDisposition contentDisposition) {
requireNonNull(contentDisposition, "contentDisposition");
remove0(HttpHeaderNames.CONTENT_DISPOSITION);
((HttpHeadersBase) appender()).contentDisposition(contentDisposition);
}

@Override
public @Nullable ContentDisposition contentDisposition() {
final String contentDispositionString = get(HttpHeaderNames.CONTENT_DISPOSITION);
if (contentDispositionString == null) {
return null;
}

try {
return ContentDisposition.parse(contentDispositionString);
} catch (IllegalArgumentException ex) {
return null;

Check warning on line 150 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L149-L150

Added lines #L149 - L150 were not covered by tests
}
}

final void endOfStream(boolean endOfStream) {
((HttpHeadersBase) appender()).endOfStream(endOfStream);
}

@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. πŸ˜‰

if (((HttpHeaderGetters) delegate).isEndOfStream()) {
return true;
}
}
return false;
}

@Override
public final int hashCode() {
final int hashCode = super.hashCode();

Check warning on line 170 in core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/common/CompositeHttpHeadersBase.java#L170

Added line #L170 was not covered by tests
return isEndOfStream() ? ~hashCode : hashCode;
}

@Override
public boolean equals(@Nullable Object o) {
if (this == o) {
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)

return false;
}

// `contentLengthUnknown` is excluded from the comparison since it is not a field expressing headers
// data.
return isEndOfStream() == ((HttpHeaderGetters) o).isEndOfStream() && super.equals(o);
}

@Override
public final String toString() {
final int size = size();
final boolean isEndOfStream = isEndOfStream();
if (size == 0) {
return isEndOfStream ? "[EOS]" : "[]";
}

final StringBuilder sb = new StringBuilder(7 + size * 20);
if (isEndOfStream) {
sb.append("[EOS, ");
} else {
sb.append('[');
}

for (Map.Entry<AsciiString, String> e : this) {
sb.append(e.getKey()).append('=').append(e.getValue()).append(", ");
}

final int length = sb.length();
sb.setCharAt(length - 2, ']');
return sb.substring(0, length - 1);
}
}
Loading
Loading