servicetalk

servicetalk copied to clipboard

The header implementations have commonalities between cookie parsing. Can we share code and consolidate?

I'll work on it.

I think I can solve it by creating a class like AbstractHttpHeaders. And I need to check a little more detail. 😄

@heowc great to know that you are interested in working on this.

Sharing code between DefaultHttpHeaders and ReadOnlyHttpHeaders may be easier to do than sharing with NettyH2HeadersToHttpHeaders as NettyH2HeadersToHttpHeaders is in a different module and we tend to avoid publically exposing classes which are not intended to be consumed directly by users.

I would suggest trying code reuse between DefaultHttpHeaders and ReadOnlyHttpHeaders first to see how would the common abstractions look like.

Good luck 😄

I was readed code about DefaultHttpHeaders and ReadOnlyHttpHeaders.

So, I thought about creating the following code from HeaderUtils:

@Nullable
public static HttpCookiePair getCookie(Iterator<Map.Entry<CharSequence, CharSequence>> iterator,
                                       Predicate<Map.Entry<CharSequence, CharSequence>> predicate,
                                       final CharSequence name) {
    return StreamSupport.stream(Spliterators.spliteratorUnknownSize(iterator, Spliterator.ORDERED), false)
                        .filter(predicate)
                        .map(entry -> parseCookiePair(entry.getValue(), name))
                        .findFirst().orElse(null);
}

If apply the code,

@Nullable
@Override
public HttpCookiePair getCookie(final CharSequence name) {
    // ...

    final Iterator<Map.Entry<CharSequence, CharSequence>> iterator = iterator();
    return HeaderUtils.getCookie(iterator, entry -> hashCode(entry.getKey()) == nameHash && contentEqualsIgnoreCase(entry.getKey(), COOKIE), name);
}

What do you think? Please give feedback if you have a better opinion.

@heowc for header iteration/modification, we prefer utilizing the internal datastructure (associative array) over using the iterator() as creating an Iterator for each lookup has allocation overhead.

However, the approach to share code through HeaderUtils seems reasonable to me. For the purpose of sharing code between DefaultHttpHeaders and ReadOnlyHttpHeaders, you can make the new method in HeaderUtils package private. Once you start working on the changes for NettyH2HeadersToHttpHeaders, we can make the method(s) public as required.

Are you also looking at modifying other cookie related methods also? eg: getSetCookie(), getCookiesIterator(), etc.

Sorry, the answer is late. I understand the above. But I do not come up with other ideas. Can you give me a hint? :sob:

I changed the code almost similarly:

# ReadOnlyHttpHeaders
int i = 0;
do {
    final CharSequence currentName = keyValuePairs[i];
    final CharSequence currentValue = keyValuePairs[i + 1];
    if (nameHash == hashCode(currentName) && equals(currentName, COOKIE)) {
        HttpCookiePair cookiePair = parseCookiePair(currentValue, name);
        if (cookiePair != null) {
            return cookiePair;
        }
    }
    i += 2;
} while (i < end);

# DefaultHttpHeaders
MultiMapEntry<CharSequence, CharSequence> e = bucketHead.entry;
assert e != null;
do {
    final CharSequence currentName = e.getKey();
    final CharSequence currentValue = e.getValue();
    if (e.keyHash == nameHash && contentEqualsIgnoreCase(currentName, COOKIE)) {
        HttpCookiePair cookiePair = parseCookiePair(currentValue, name);
        if (cookiePair != null) {
            return cookiePair;
        }
    }
    e = e.bucketNext;
} while (e != null);

Let me ping @Scottmitch about the intent of sharing code here, he might have better suggestions.