doc work #262
doc work #262
Conversation
📝 WalkthroughWalkthroughThe pull request modifies HTTP routing and error handling across four files. It introduces a default OPTIONS handler in the router constructor, refactors method-tracking variable names in the router base implementation, implements 405 Method Not Allowed responses for unsupported methods on matched paths, and adds result-aware error response handling in the HTTP worker for 404 and 500 scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Worker as HTTP Worker
participant Router as Router
participant Response as Response Handler
Client->>Worker: HTTP Request
Worker->>Router: Dispatch Route
alt Route Matched
Router->>Router: Check Method Allowed
alt Method Allowed
Router-->>Worker: route_done
Worker->>Response: Send Response
else Method Not Allowed
Router->>Router: Build Allow Header<br/>from matched methods
Router->>Response: 405 Method Not Allowed
Response-->>Worker: Response Sent
end
else Route Not Matched
Router-->>Worker: next (no match)
Worker->>Response: 404 Not Found
Response-->>Worker: Response Sent
else Dispatch Failed
Router-->>Worker: failed
Worker->>Response: 500 Internal Server Error
Response-->>Worker: Response Sent
end
Worker->>Client: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://262.http.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-09 01:34:00 UTC |
![coderabbitai[bot]](https://avatars-githubusercontent-com-gh.computerqwq.top/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@include/boost/http/server/router.hpp`:
- Around line 603-609: The OPTIONS handler currently ignores the result of
co_await rp.send(), so send failures are discarded; update the lambda passed to
set_options_handler (the handler taking P& rp) to capture the outcome of
co_await rp.send(), check for an error, and only co_return route_done on
success—otherwise propagate or return an error/failed route result so callers
know the response wasn't sent (i.e., handle rp.send()'s failure instead of
discarding it after rp.status(status::no_content) and rp.res.set(field::allow,
allow)).
In `@src/server/detail/router_base.cpp`:
- Around line 291-297: matched_custom_verbs is being appended from every
matching end-route matcher (inside the m.end_ branch) and can accumulate
duplicates, causing duplicate verbs in the Allow header because
build_allow_header doesn't deduplicate; update the logic after collecting from
matchers (or immediately before calling build_allow_header) to deduplicate
matched_custom_verbs (e.g., sort and std::unique or use an unordered_set) so
only unique custom verbs are passed to build_allow_header; keep collecting
matched_methods as-is but ensure matched_custom_verbs is de-duplicated similarly
to rebuild_global_allow_header to avoid repeated verbs in the header.
- Around line 375-384: The 405 fallback fires even when the incoming request's
method is present in matched_methods or matched_custom_verbs; change the
condition in the block that builds the Allow header to also verify the current
request method is NOT present before returning 405. Concretely, compute the
method bit for the current request (e.g. method_bit =
method_to_bit(p.req.method) or similar) and only trigger the 405 when
!is_options && (matched_methods != 0 || !matched_custom_verbs.empty()) &&
((matched_methods & method_bit) == 0) && (matched_custom_verbs does not contain
the request method string); then call build_allow_header(matched_methods,
matched_custom_verbs), p.res.set_status(status::method_not_allowed) and send as
before (return route_done).
In `@src/server/http_worker.cpp`:
- Line 84: The two discarded send results of (co_await rp.send()) must be
checked and handled: capture the await result into an error variable when
calling rp.send(), and if it indicates failure, break out of the request loop or
return early (e.g., co_return route_error(ec)) instead of continuing to check
keep_alive() and reuse the stream; apply this change to both occurrences of
rp.send() so failed sends do not allow another request/response cycle on a
broken connection.
🧹 Nitpick comments (3)
src/server/http_worker.cpp (1)
78-91: Use anelse ifchain for mutually exclusive dispatch outcomes.The three conditions (
close,next,failed) are semantically mutually exclusive, but they're written as independentifblocks. After thenextbranch sends a 404 response, execution falls through to therv.failed()check (which should be false, but it's fragile). Anelse ifchain would make the intent explicit and guard against subtle bugs ifroute_resultsemantics evolve.Additionally, both the 404 and 500 paths send an empty body via
rp.send(), whereas other handlers in this codebase (e.g.,serve_static.cppLine 243, Line 306) send descriptive text like"Not Found"and"Internal Server Error". Consider adding body text for consistency and to help HTTP clients that render error pages.Proposed refactor
- if(rv.what() == route_what::close) - break; - - if(rv.what() == route_what::next) + if(rv.what() == route_what::close) + break; + else if(rv.what() == route_what::next) { rp.status(http::status::not_found); - (void)(co_await rp.send()); + (void)(co_await rp.send("Not Found")); } - - if(rv.failed()) + else if(rv.failed()) { rp.status(http::status::internal_server_error); - (void)(co_await rp.send()); + (void)(co_await rp.send("Internal Server Error")); }include/boost/http/server/router.hpp (1)
586-609: Constructor docstring doesn't mention the default OPTIONS handler.The constructor now installs a default OPTIONS handler (lines 603–609) that responds with
204 No Contentand anAllowheader, but the docstring (lines 586–596) only discusses option inheritance. Since this is a public header and the behavior is user-visible (automatic OPTIONS responses), it should be documented.As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces. Docstrings should include a brief description of purpose, document all parameters, return values, and any preconditions, postconditions, or exceptions."
src/server/detail/router_base.cpp (1)
375-384: The 405 path sends a response directly from the dispatch loop, bypassing the caller.Unlike other dispatch outcomes that return
route_next/route_done/route_errorand let the caller (http_worker) decide the response, the 405 branch directly sets status, headers, and sends the response withindispatch_loop. This means the caller has no opportunity to customize the 405 response (e.g., add CORS headers, custom body). This is consistent with the OPTIONS fallback pattern (line 372), but worth noting as an architectural choice — the worker'sroute_what::next→ 404 handling becomes unreachable for 405-eligible paths.
| set_options_handler( | ||
| [](P& rp, std::string_view allow) -> route_task { | ||
| rp.status(status::no_content); | ||
| rp.res.set(field::allow, allow); | ||
| (void)(co_await rp.send()); | ||
| co_return route_done; | ||
| }); |
There was a problem hiding this comment.
Default OPTIONS handler discards send error.
Same pattern as in http_worker.cpp — if rp.send() fails, the error is silently discarded and route_done is returned. This means the caller won't know the response wasn't actually sent. Low severity since OPTIONS responses are lightweight, but worth noting for consistency.
🤖 Prompt for AI Agents
In `@include/boost/http/server/router.hpp` around lines 603 - 609, The OPTIONS
handler currently ignores the result of co_await rp.send(), so send failures are
discarded; update the lambda passed to set_options_handler (the handler taking
P& rp) to capture the outcome of co_await rp.send(), check for an error, and
only co_return route_done on success—otherwise propagate or return an
error/failed route result so callers know the response wasn't sent (i.e., handle
rp.send()'s failure instead of discarding it after rp.status(status::no_content)
and rp.res.set(field::allow, allow)).
| // Collect methods from matching end-route matchers | ||
| if(m.end_) | ||
| { | ||
| options_methods |= m.allowed_methods_; | ||
| matched_methods |= m.allowed_methods_; | ||
| for(auto const& v : m.custom_verbs_) | ||
| options_custom_verbs.push_back(v); | ||
| matched_custom_verbs.push_back(v); | ||
| } |
There was a problem hiding this comment.
Method collection runs unconditionally for every matching end-route matcher.
matched_methods and matched_custom_verbs accumulate from every end-route matcher whose path matches, even when the handler subsequently matches the method and runs. This is fine for the Allow header (you want all methods), but matched_custom_verbs is a std::vector<std::string> that may accumulate duplicates across matchers. The build_allow_header function doesn't deduplicate (unlike rebuild_global_allow_header which sorts/uniques). For routes with many custom verbs or overlapping matchers, the Allow header could contain duplicates.
Proposed fix — deduplicate before building
if(is_options && matched_methods != 0 && options_handler_)
{
+ std::sort(matched_custom_verbs.begin(), matched_custom_verbs.end());
+ matched_custom_verbs.erase(
+ std::unique(matched_custom_verbs.begin(), matched_custom_verbs.end()),
+ matched_custom_verbs.end());
std::string allow = build_allow_header(matched_methods, matched_custom_verbs);
co_return co_await options_handler_->invoke(p, allow);
}
// 405 fallback: path matched but method didn't
if(!is_options &&
(matched_methods != 0 || !matched_custom_verbs.empty()))
{
+ std::sort(matched_custom_verbs.begin(), matched_custom_verbs.end());
+ matched_custom_verbs.erase(
+ std::unique(matched_custom_verbs.begin(), matched_custom_verbs.end()),
+ matched_custom_verbs.end());
std::string allow = build_allow_header(matched_methods, matched_custom_verbs);🤖 Prompt for AI Agents
In `@src/server/detail/router_base.cpp` around lines 291 - 297,
matched_custom_verbs is being appended from every matching end-route matcher
(inside the m.end_ branch) and can accumulate duplicates, causing duplicate
verbs in the Allow header because build_allow_header doesn't deduplicate; update
the logic after collecting from matchers (or immediately before calling
build_allow_header) to deduplicate matched_custom_verbs (e.g., sort and
std::unique or use an unordered_set) so only unique custom verbs are passed to
build_allow_header; keep collecting matched_methods as-is but ensure
matched_custom_verbs is de-duplicated similarly to rebuild_global_allow_header
to avoid repeated verbs in the header.
| // 405 fallback: path matched but method didn't | ||
| if(!is_options && | ||
| (matched_methods != 0 || !matched_custom_verbs.empty())) | ||
| { | ||
| std::string allow = build_allow_header(matched_methods, matched_custom_verbs); | ||
| p.res.set(field::allow, allow); | ||
| p.res.set_status(status::method_not_allowed); | ||
| (void)(co_await p.send()); | ||
| co_return route_done; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/server/detail/router_base.cpp | sed -n '350,400p'Repository: cppalliance/http
Length of output: 1718
🏁 Script executed:
# Search for matched_methods initialization and usage
rg -n 'matched_methods' src/server/detail/router_base.cpp -B2 -A2Repository: cppalliance/http
Length of output: 1049
🏁 Script executed:
# Look for route_next and 405 related logic
rg -n 'route_next\|method_not_allowed' src/server/detail/router_base.cpp -B3 -A3Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Search for any tests related to this scenario
find . -name "*test*.cpp" -o -name "*_test.cpp" | xargs grep -l "route_next\|method_not_allowed" 2>/dev/null | head -5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
cat -n src/server/detail/router_base.cpp | sed -n '200,310p'Repository: cppalliance/http
Length of output: 4102
🏁 Script executed:
# Search for where the current request method is checked against allowed methods
rg -n 'pv\.verb_|allowed_methods' src/server/detail/router_base.cpp -B2 -A2Repository: cppalliance/http
Length of output: 1227
🏁 Script executed:
# Look for the handler invocation and route_next handling
rg -n 'handler.*invoke|route_next' src/server/detail/router_base.cpp -B3 -A3Repository: cppalliance/http
Length of output: 913
🏁 Script executed:
cat -n src/server/detail/router_base.cpp | sed -n '300,375p'Repository: cppalliance/http
Length of output: 2412
🏁 Script executed:
# Look for where handlers are invoked in the loop
rg -n '\.invoke\(p' src/server/detail/router_base.cpp -B5 -A5Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
find . -type f -name "*.cpp" | xargs grep -l "405\|method_not_allowed" 2>/dev/null | head -10Repository: cppalliance/http
Length of output: 327
🏁 Script executed:
# Search for test files related to router
find . -type f \( -name "*test*.cpp" -o -name "test_*.cpp" \) | head -10Repository: cppalliance/http
Length of output: 88
🏁 Script executed:
# Look for tests that cover route_next behavior
rg -l 'route_next' --type=cpp | grep -i test | head -5Repository: cppalliance/http
Length of output: 227
🏁 Script executed:
# Search for 405 tests in router test files
rg -n '405\|method_not_allowed' test/unit/server/router.cpp -B3 -A3Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look for tests with route_next and method handling
rg -n 'route_next' test/unit/server/router.cpp -B5 -A5 | head -50Repository: cppalliance/http
Length of output: 1615
🏁 Script executed:
# Search for test cases that test 405 or method_not_allowed behavior
rg -n 'TEST\|BOOST_AUTO_TEST\|method_not_allowed\|405' test/unit/server/router.cpp | head -30Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Look for tests combining route_next with method-specific routes
rg -n 'on.*GET\|on.*POST.*h_next\|\.get\(' test/unit/server/router.cpp -B2 -A2 | head -40Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Get an overview of router test structure
cat -n test/unit/server/router.cpp | head -150Repository: cppalliance/http
Length of output: 5283
🏁 Script executed:
# Search for any tests involving method selection and route_next
rg -n 'get\(|post\(|put\(' test/unit/server/router.cpp | head -20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
cat -n test/unit/server/router.cpp | sed -n '150,400p'Repository: cppalliance/http
Length of output: 10382
🏁 Script executed:
# Look for test methods that test method-based routing
rg -n 'BOOST_FIXTURE_TEST_CASE|void|auto.*test' test/unit/server/router.cpp | grep -A2 'test' | head -30Repository: cppalliance/http
Length of output: 604
🏁 Script executed:
cat -n test/unit/server/router.cpp | sed -n '394,470p'Repository: cppalliance/http
Length of output: 2933
🏁 Script executed:
# Look for any 405-related test cases
rg -n 'status.*method\|method.*status\|405\|method_not_allowed' test/unit/server/router.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
rg -n '405\|method_not_allowed' test/unit/server/detail/router_base.cppRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if there are tests that specifically test the scenario described in the bug
# (handler matches both path and method, returns route_next, then falls through)
cat -n test/unit/server/detail/router_base.cpp | head -100Repository: cppalliance/http
Length of output: 3321
🏁 Script executed:
# Let me trace through the exact scenario: what is pv.verb_ and how is it set?
rg -n 'pv.verb_.*=' src/server/detail/router_base.cppRepository: cppalliance/http
Length of output: 190
🏁 Script executed:
# Examine the match_method function to understand how method matching works
rg -n 'match_method' src/server/detail/router_base.cpp -B5 -A10Repository: cppalliance/http
Length of output: 509
🏁 Script executed:
# Let me understand the complete picture of route entry and what e.match_method does
rg -n 'match_method' src/server/ -B2 -A5 --type=cpp | head -50Repository: cppalliance/http
Length of output: 967
🏁 Script executed:
# Find the router_entry structure to understand match_method
rg -n 'struct.*router_entry\|bool match_method' src/server/ --type=cpp -A5 | head -40Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
cat -n src/server/detail/router_base.hpp | sed -n '68,100p'Repository: cppalliance/http
Length of output: 1066
🏁 Script executed:
# Get the complete router_entry structure
rg -n 'struct router_entry' src/server/detail/router_base.hpp -A20Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Now let me verify the complete picture: after handler returns route_next, is there any way
# the current request's method could be saved or flagged?
rg -n 'route_what::next' src/server/detail/router_base.cpp -B10 -A10Repository: cppalliance/http
Length of output: 807
🏁 Script executed:
# Check if there's any tracking of whether current method was matched
rg -n 'current.*method\|method.*matched' src/server/detail/router_base.cpp -iRepository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Now let me verify the logic of the proposed fix by understanding pv.verb_ and matched_methods structure
cat -n src/server/detail/router_base.cpp | sed -n '206,220p'Repository: cppalliance/http
Length of output: 625
🏁 Script executed:
# Understand the bit manipulation: how are methods stored in matched_methods?
rg -n '1ULL << static_cast' src/server/ --type=cpp -B2 -A2Repository: cppalliance/http
Length of output: 1237
🏁 Script executed:
# Verify the complete scenario by creating a simple trace
cat > /tmp/verify_405.txt << 'EOF'
Scenario: GET handler at "/" returns route_next
1. Request: GET /
2. Path matching succeeds
3. matched_methods gets 0 (initially)
4. Loop iteration 1: find handler for GET /
- Path matcher matches (end_ = true)
- Line 294: matched_methods |= m.allowed_methods_ (now has GET bit set)
- Line 299-304: e.match_method() returns true (GET == GET)
- Line 319: Handler invoked, returns route_next
- Line 330-333: Loop continues (++i)
5. No more handlers match
6. Loop exits
7. Line 376-384: 405 check fires
- matched_methods != 0 (true - has GET bit)
- is_options is false (true)
- Condition is true, sends 405 Method Not Allowed
- BUT: the request method WAS in matched_methods!
The bug: The 405 check never verifies that the CURRENT REQUEST'S METHOD
is absent from matched_methods. It only checks that SOME method was matched.
EOF
cat /tmp/verify_405.txtRepository: cppalliance/http
Length of output: 910
🏁 Script executed:
# Verify the proposed fix checks the correct logic
cat > /tmp/verify_fix.txt << 'EOF'
Proposed fix logic:
1. method_is_matched = check if pv.verb_ is in matched_methods
2. For custom verbs: check if pv.verb_str_ is in matched_custom_verbs
3. Handle "all methods" case: matched_methods == ~0ULL
4. Only fire 405 if:
- NOT is_options AND
- NOT method_is_matched AND <-- NEW: ensure current method isn't matched
- (matched_methods != 0 OR !matched_custom_verbs.empty())
This is correct!
EOF
cat /tmp/verify_fix.txtRepository: cppalliance/http
Length of output: 468
Bug: 405 fires incorrectly even when the request method IS in the matched set.
The 405 fallback at lines 376-384 checks only that matched_methods != 0 || !matched_custom_verbs.empty() but never verifies that the current request's method is absent from these sets. Consider a scenario where a GET handler for "/" returns route_next: the path matches, matched_methods accumulates the GET bit (line 294), the handler passes the method check (line 299), returns route_next (line 330), and the loop continues. After all entries are processed, the 405 check fires even though GET was in matched_methods — the response incorrectly sends 405 Method Not Allowed instead of falling through to route_next.
The condition must additionally verify that the current request's method is not in the matched set before returning 405.
Proposed fix
// 405 fallback: path matched but method didn't
- if(!is_options &&
- (matched_methods != 0 || !matched_custom_verbs.empty()))
+ bool method_is_matched = false;
+ if(pv.verb_ != http::method::unknown)
+ method_is_matched = (matched_methods & (1ULL << static_cast<unsigned>(pv.verb_))) != 0;
+ else
+ {
+ for(auto const& v : matched_custom_verbs)
+ {
+ if(v == pv.verb_str_)
+ {
+ method_is_matched = true;
+ break;
+ }
+ }
+ }
+ // Also treat "all methods" as matching
+ if(matched_methods == ~0ULL)
+ method_is_matched = true;
+
+ if(!is_options && !method_is_matched &&
+ (matched_methods != 0 || !matched_custom_verbs.empty()))
{🤖 Prompt for AI Agents
In `@src/server/detail/router_base.cpp` around lines 375 - 384, The 405 fallback
fires even when the incoming request's method is present in matched_methods or
matched_custom_verbs; change the condition in the block that builds the Allow
header to also verify the current request method is NOT present before returning
405. Concretely, compute the method bit for the current request (e.g. method_bit
= method_to_bit(p.req.method) or similar) and only trigger the 405 when
!is_options && (matched_methods != 0 || !matched_custom_verbs.empty()) &&
((matched_methods & method_bit) == 0) && (matched_custom_verbs does not contain
the request method string); then call build_allow_header(matched_methods,
matched_custom_verbs), p.res.set_status(status::method_not_allowed) and send as
before (return route_done).
| if(rv.what() == route_what::next) | ||
| { | ||
| rp.status(http::status::not_found); | ||
| (void)(co_await rp.send()); |
There was a problem hiding this comment.
Discarded send errors may leave the connection in a bad state.
Both (void)(co_await rp.send()) calls discard the error code. If the send fails (e.g., broken pipe), the loop proceeds to check keep_alive() and may attempt another request/response cycle on a broken stream. Consider breaking out of the loop on send failure, similar to how route handlers use co_return route_error(ec).
Also applies to: 90-90
🤖 Prompt for AI Agents
In `@src/server/http_worker.cpp` at line 84, The two discarded send results of
(co_await rp.send()) must be checked and handled: capture the await result into
an error variable when calling rp.send(), and if it indicates failure, break out
of the request loop or return early (e.g., co_return route_error(ec)) instead of
continuing to check keep_alive() and reuse the stream; apply this change to both
occurrences of rp.send() so failed sends do not allow another request/response
cycle on a broken connection.
|
GCOVR code coverage report https://262.http.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-09 01:43:53 UTC |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes