Copilot commented on code in PR #3606:
URL: https://github.com/apache/brpc/pull/3606#discussion_r4290679230
##########
src/brpc/details/http_message.cpp:
##########
@@ -186,6 +186,9 @@ int HttpMessage::on_headers_complete(http_parser *parser) {
parser->http_major = 1;
}
HttpHeader& headers = http_message->header();
+ if (headers.HasSetCookieHeaders()) {
+ headers.SyncSetCookieHeaders();
+ }
Review Comment:
`HttpMessage` calls `HttpHeader::HasSetCookieHeaders()` and
`HttpHeader::SyncSetCookieHeaders()`. In `http_header.h` these helpers appear
to be internal (defined alongside other non-public helpers), so this may fail
to compile due to access control. Make these APIs publicly accessible in a
controlled way (e.g., a public `FinalizeHeadersParsing()` that performs the
sync), or explicitly grant friendship to the caller that needs it.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -879,9 +900,65 @@ TEST(HttpMessageTest, http_header) {
}
ASSERT_EQ(1, set_cookie_value2_count);
ASSERT_EQ(1, set_cookie_value3_count);
+ brpc::HttpHeader copied_header(header);
+ copied_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = copied_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ brpc::HttpHeader assigned_header;
+ assigned_header = header;
+ assigned_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = assigned_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
header.RemoveHeader(brpc::HttpHeader::SET_COOKIE);
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
- ASSERT_EQ(header._first_set_cookie, nullptr);
+ ASSERT_TRUE(header.GetAllSetCookieHeader().empty());
+
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ const std::string* first_set_cookie_after_reset =
+ header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_NE(nullptr, first_set_cookie_after_reset);
+ header.AppendHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ const size_t bucket_count = header._headers.bucket_count();
+ for (int i = 0; i < 4096 &&
+ header._headers.bucket_count() == bucket_count; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ ASSERT_NE(bucket_count, header._headers.bucket_count());
Review Comment:
This test forces a `HeaderMap` growth by inserting up to 4096 headers and
asserting `bucket_count()` changes. That assumption can be
implementation-dependent (initial bucket counts / growth factors can vary),
which may make the test flaky across platforms or future container changes. A
more robust approach is to force rehash explicitly (if the container supports
it), or to loop until a growth condition that is guaranteed by the container
API, while keeping an upper bound and asserting based on an explicit rehash
trigger rather than a hardcoded insert count.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -879,9 +900,65 @@ TEST(HttpMessageTest, http_header) {
}
ASSERT_EQ(1, set_cookie_value2_count);
ASSERT_EQ(1, set_cookie_value3_count);
+ brpc::HttpHeader copied_header(header);
+ copied_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = copied_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ brpc::HttpHeader assigned_header;
+ assigned_header = header;
+ assigned_header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value3);
+ value = assigned_header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value2);
header.RemoveHeader(brpc::HttpHeader::SET_COOKIE);
ASSERT_FALSE(header.GetHeader(brpc::HttpHeader::SET_COOKIE));
- ASSERT_EQ(header._first_set_cookie, nullptr);
+ ASSERT_TRUE(header.GetAllSetCookieHeader().empty());
+
+ header.SetHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value1);
+ const std::string* first_set_cookie_after_reset =
+ header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_NE(nullptr, first_set_cookie_after_reset);
+ header.AppendHeader(brpc::HttpHeader::SET_COOKIE, set_cookie_value2);
+ const size_t bucket_count = header._headers.bucket_count();
+ for (int i = 0; i < 4096 &&
+ header._headers.bucket_count() == bucket_count; ++i) {
+ header.SetHeader("header" + std::to_string(i), "value");
+ }
+ ASSERT_NE(bucket_count, header._headers.bucket_count());
+ ASSERT_EQ(set_cookie_value1, *first_set_cookie_after_reset);
+ value = header.GetHeader(brpc::HttpHeader::SET_COOKIE);
+ ASSERT_TRUE(value && *value == set_cookie_value1);
+ all_set_cookie = header.GetAllSetCookieHeader();
+ ASSERT_EQ(2u, all_set_cookie.size());
+ ASSERT_EQ(set_cookie_value1, *all_set_cookie[0]);
+ ASSERT_EQ(set_cookie_value2, *all_set_cookie[1]);
+ int set_cookie_value1_count = 0;
+ set_cookie_value2_count = 0;
Review Comment:
The counting variables are handled inconsistently here:
`set_cookie_value1_count` is declared locally, but `set_cookie_value2_count` is
assigned without a local declaration in this block, which makes the code harder
to follow and more sensitive to surrounding scope changes. Prefer declaring
both counters in the same local scope (and similarly for any others) to keep
the test self-contained and avoid accidental reuse of earlier variables.
##########
src/brpc/http_header.cpp:
##########
@@ -128,28 +161,29 @@ std::string& HttpHeader::GetOrAddHeader(const
std::string& key) {
return _content_type;
}
- bool is_set_cookie = IsSetCookie(key);
- // Only returns the first Set-Cookie header field for compatibility.
- if (is_set_cookie && nullptr != _first_set_cookie) {
- return *_first_set_cookie;
- }
-
std::string* val = _headers.seek(key);
if (nullptr == val) {
val = _headers.insert({ key, "" });
- if (is_set_cookie) {
- _first_set_cookie = val;
- }
}
return *val;
}
std::string& HttpHeader::AddHeader(const std::string& key) {
- std::string* val = _headers.insert({ key, "" });
- if (IsSetCookie(key) && nullptr == _first_set_cookie) {
- _first_set_cookie = val;
+ if (!_set_cookie_headers) {
+ _set_cookie_headers.reset(new SetCookieHeaders);
+ }
+ _set_cookie_headers->push_back(std::make_pair(key, std::string()));
+ SyncSetCookieHeaders();
+ return _set_cookie_headers->back().second;
+}
Review Comment:
`AddHeader()` now unconditionally appends into `_set_cookie_headers` and
syncs `SET_COOKIE`, regardless of `key`. If `AddHeader()` is ever used for
non-`Set-Cookie` headers, this will incorrectly route those headers into the
`Set-Cookie` store and also mutate the `SET_COOKIE` entries in `_headers`. Fix
by making `AddHeader()` conditional: only use `_set_cookie_headers` when
`IsSetCookie(key)` is true; otherwise preserve the prior behavior of inserting
into `_headers` and returning that value slot. Alternatively, split into
`AddSetCookieHeader()` (new) and keep `AddHeader()` generic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
