Odds and Ends. Mostly ends. #260
Conversation
📝 WalkthroughWalkthroughReplaces the legacy route grammar with a new ABNF-based parser/matcher, adds captured route-parameter storage, and implements pluggable automatic OPTIONS handling with generated Allow headers and a user-configurable options handler. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router
participant Matcher as Matcher
participant OptionsHandler as OptionsHandler
Client->>Router: SEND OPTIONS (path or "*")
Router->>Matcher: match_route(path, pattern, opts)
Matcher->>Matcher: collect allowed methods & params
Matcher-->>Router: match_params + matched_length
Router->>Router: build Allow header from collected methods/verbs
alt options_handler_ configured
Router->>OptionsHandler: invoke(route_params, Allow header)
OptionsHandler->>Client: respond OPTIONS with Allow header
else fallback
Router->>Client: 404 or default response
end
sequenceDiagram
participant Caller as API Caller
participant Parser as parse_route_pattern
participant Tokenizer as Tokenizer
participant Pattern as RoutePattern
Caller->>Parser: parse_route_pattern(":id/*rest{.json}")
Parser->>Tokenizer: tokenize pattern
Tokenizer->>Pattern: emit tokens (param,wildcard,group)
Parser->>Caller: return route_pattern
Note over Pattern, Tokenizer: Later during request
Caller->>Pattern: match_route("/42/extra.json", options)
Pattern->>Pattern: iterate tokens, capture params
Pattern->>Caller: return match_params (params, matched_length)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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)
Comment |
|
An automated preview of the documentation is available at https://260.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-02 14:43:53 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/server/detail/route_match.cpp`:
- Around line 34-39: The code currently ignores errors from
parse_route_pattern(decoded_pat_) leaving pattern_.tokens empty; update the
route initialization in the block guarded by if(!slash_) to handle parse errors
explicitly: call parse_route_pattern(decoded_pat_), check rv.has_error() and on
error either log the parsing error (including decoded_pat_ and rv.error()) and
mark the route invalid or throw an exception so registration fails, otherwise
assign pattern_ = std::move(rv.value()); also ensure any callers using pattern_
(and flags like end_ and slash_) validate the route's parsed state before
accepting registration or matching.
In `@src/server/flat_router.cpp`:
- Around line 502-512: When handling the special-case OPTIONS * branch, p
(route_params_base) is still uninitialized before invoking
impl_->options_handler_, so initialize its critical members (e.g., p.path,
p.decoded_path_, p.params and any other fields the handler might read) to
safe/empty defaults before the early return; update the branch in
flat_router.cpp (the block that checks verb == http::method::options &&
url.encoded_path() == "*") to set those fields on p prior to calling
impl_->options_handler_->invoke(p, impl_->global_allow_header_) so handlers
never observe uninitialized or stale path-related data.
- Around line 73-74: The current special-case check if(methods == ~0ULL) returns
a hardcoded 7-method string which is inconsistent with the 32-entry known[]
method table; update the logic so the Allow header is consistent: either replace
the ~0ULL shortcut with a named constant (e.g., COMMON_REST_METHODS) and
document that it intentionally maps to the common subset, or remove the shortcut
entirely and compute the header by iterating the known[] array when methods ==
~0ULL (or when methods has all bits set) so the returned string always reflects
the actual known[] entries; update the branch handling the methods variable and
any comment/docs in flat_router.cpp accordingly.
- Around line 213-231: The per-matcher collection path currently pushes custom
verb strings into m.custom_verbs_ without deduplication, causing duplicated
entries in the Allow header; update the code that adds to m.custom_verbs_ (the
branch inside the if(m.end_) block where e.verb_str is used) to check for and
avoid inserting duplicates (same approach as global_custom_verbs_ /
build_allow_headers()), so that m.custom_verbs_ contains unique verb strings
before headers are constructed.
In `@src/server/route_abnf.hpp`:
- Around line 39-54: The default constructor route_token() leaves the enum
member route_token_type type uninitialized; update the default construction so
type is initialized to a sensible default (for example route_token_type::text)
either by replacing the = default with an explicit default constructor that
initializes type, or by providing an in-class initializer for type, and keep the
existing parameterized constructor (route_token(route_token_type, std::string))
unchanged so all tokens are safely initialized.
🧹 Nitpick comments (7)
include/boost/http/server/router_types.hpp (1)
399-400: Consider documentingparams_member.The
params_member inmatch_resultlacks documentation, unlike its counterpartparamsinroute_params_base. A brief comment explaining its role (e.g., temporary storage during matching that gets transferred toroute_params_base::params) would help maintainers understand the data flow.src/server/route_abnf.cpp (2)
10-11: Consider adding an implementation overview comment.Per coding guidelines, files containing non-trivial implementation logic should include a block comment after the includes providing a high-level overview of how the implementation works. This parser has multiple interacting components (tokenizer, matcher, backtracking for groups) that would benefit from such documentation.
109-125:peek()andget()can dereference past end.These methods dereference
it_without checkingat_end(). While callers currently checkat_end()before calling, this makes the API fragile. Thepeek()call at line 112 inparse_text()(line 218) is protected by the loop condition, but a future refactor could introduce bugs.Consider adding assertions or defensive checks:
Proposed defensive checks
char peek() const noexcept { + BOOST_ASSERT(!at_end()); return *it_; } // ... char get() noexcept { + BOOST_ASSERT(!at_end()); return *it_++; }src/server/detail/route_match.cpp (1)
10-16: Missing implementation overview comment.Per coding guidelines, files with non-trivial implementation logic should include a block comment after the includes providing a high-level overview. This file now uses
parse_route_patternandmatch_routefrom the new ABNF-based routing infrastructure, which would benefit from a brief explanation of the approach.As per coding guidelines: "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works."src/server/flat_router.cpp (3)
122-128: Custom verbs appended without maintaining alphabetical order.Known methods are listed alphabetically, but custom verbs are simply appended at the end. While HTTP doesn't require any particular ordering in the Allow header, maintaining consistency could improve readability for debugging.
469-475: Runtime Allow header construction for OPTIONS fallback.The
build_allow_headerfunction is called during request handling for OPTIONS fallback. While OPTIONS requests are typically infrequent, if they become common, this repeated construction could be optimized by caching per-path Allow headers during flattening.Additionally,
options_custom_verbscollected during the loop (lines 377-378) may contain duplicates that will appear in the generated header.
547-559: Same uninitialized params concern as the method-based dispatch overload.The same issue applies here:
route_params_base& pmay not be fully initialized whenoptions_handler_is invoked forOPTIONS *requests.Additionally, there's code duplication between the two dispatch overloads for OPTIONS handling. Consider extracting the OPTIONS
*handling into a shared helper function.
| if(methods == ~0ULL) | ||
| return "DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT"; |
There was a problem hiding this comment.
Inconsistency between ~0ULL shortcut and actual method enumeration.
The hardcoded string for ~0ULL returns "DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT" (7 methods), but the known array contains 32 methods. If someone explicitly registers handlers for all methods individually, the generated Allow header would be different from the ~0ULL case.
Is the ~0ULL sentinel intended to mean "common REST methods" rather than "all methods"? If so, consider using a named constant and documenting this behavior. Otherwise, consider removing the shortcut to ensure consistency.
🤖 Prompt for AI Agents
In `@src/server/flat_router.cpp` around lines 73 - 74, The current special-case
check if(methods == ~0ULL) returns a hardcoded 7-method string which is
inconsistent with the 32-entry known[] method table; update the logic so the
Allow header is consistent: either replace the ~0ULL shortcut with a named
constant (e.g., COMMON_REST_METHODS) and document that it intentionally maps to
the common subset, or remove the shortcut entirely and compute the header by
iterating the known[] array when methods == ~0ULL (or when methods has all bits
set) so the returned string always reflects the actual known[] entries; update
the branch handling the methods variable and any comment/docs in flat_router.cpp
accordingly.
| // Collect methods for OPTIONS (only for end routes) | ||
| if(m.end_) | ||
| { | ||
| // Per-matcher collection | ||
| if(e.all) | ||
| m.allowed_methods_ = ~0ULL; | ||
| else if(e.verb != http::method::unknown) | ||
| m.allowed_methods_ |= (1ULL << static_cast<unsigned>(e.verb)); | ||
| else if(!e.verb_str.empty()) | ||
| m.custom_verbs_.push_back(e.verb_str); | ||
|
|
||
| // Global collection (for OPTIONS *) | ||
| if(e.all) | ||
| global_methods_ = ~0ULL; | ||
| else if(e.verb != http::method::unknown) | ||
| global_methods_ |= (1ULL << static_cast<unsigned>(e.verb)); | ||
| else if(!e.verb_str.empty()) | ||
| global_custom_verbs_.push_back(e.verb_str); | ||
| } |
There was a problem hiding this comment.
Per-matcher custom verbs may contain duplicates.
Global custom verbs are deduplicated in build_allow_headers(), but per-matcher m.custom_verbs_ are not. If multiple entries on the same end-route matcher register the same custom verb, the Allow header will contain duplicates.
Consider applying the same deduplication when building per-matcher headers at lines 168-169.
🔧 Proposed fix
// Build per-matcher Allow header strings
for(auto& m : matchers)
{
if(m.end_)
+ {
+ std::sort(m.custom_verbs_.begin(), m.custom_verbs_.end());
+ m.custom_verbs_.erase(
+ std::unique(m.custom_verbs_.begin(), m.custom_verbs_.end()),
+ m.custom_verbs_.end());
m.allow_header_ = build_allow_header(
m.allowed_methods_, m.custom_verbs_);
+ }
}🤖 Prompt for AI Agents
In `@src/server/flat_router.cpp` around lines 213 - 231, The per-matcher
collection path currently pushes custom verb strings into m.custom_verbs_
without deduplication, causing duplicated entries in the Allow header; update
the code that adds to m.custom_verbs_ (the branch inside the if(m.end_) block
where e.verb_str is used) to check for and avoid inserting duplicates (same
approach as global_custom_verbs_ / build_allow_headers()), so that
m.custom_verbs_ contains unique verb strings before headers are constructed.
| struct route_token | ||
| { | ||
| route_token_type type; | ||
| std::string value; // text content or param name | ||
| std::vector<route_token> children; // group contents | ||
|
|
||
| route_token() = default; | ||
|
|
||
| route_token( | ||
| route_token_type t, | ||
| std::string v) | ||
| : type(t) | ||
| , value(std::move(v)) | ||
| { | ||
| } | ||
| }; |
There was a problem hiding this comment.
Default constructor leaves type uninitialized.
The default constructor route_token() = default; leaves the type member uninitialized since route_token_type is an enum class without a default value. Consider initializing type to a sensible default (e.g., text) or removing the default constructor if all tokens should be explicitly constructed.
Proposed fix
struct route_token
{
- route_token_type type;
+ route_token_type type = route_token_type::text;
std::string value; // text content or param name
std::vector<route_token> children; // group contents
route_token() = default;🤖 Prompt for AI Agents
In `@src/server/route_abnf.hpp` around lines 39 - 54, The default constructor
route_token() leaves the enum member route_token_type type uninitialized; update
the default construction so type is initialized to a sensible default (for
example route_token_type::text) either by replacing the = default with an
explicit default constructor that initializes type, or by providing an in-class
initializer for type, and keep the existing parameterized constructor
(route_token(route_token_type, std::string)) unchanged so all tokens are safely
initialized.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/flat_router.cpp (2)
10-18:⚠️ Potential issue | 🟡 MinorAdd a post-include implementation overview comment.
This file now contains substantial routing logic; please add a/* ... */block after the includes describing the overall flow (flattening, matcher traversal, OPTIONS Allow assembly, and dispatch).As per coding guidelines: Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works.
268-367:⚠️ Potential issue | 🟠 MajorCaptured params aren’t restored on backtracking.
Line 351–356 appends params top.params, but there’s no restore when siblings or ancestors are re-tried, so params can leak between routes.🛠️ Proposed fix (track param sizes per depth)
// Stack of base_path lengths at each depth level. // path_stack[d] = base_path.size() before any matcher at depth d was tried. std::size_t path_stack[detail::router_base::max_path_depth]; path_stack[0] = 0; + std::size_t params_stack[detail::router_base::max_path_depth]; + params_stack[0] = 0; @@ if(cm.depth_ <= current_depth && current_depth > 0) { restore_path(p, path_stack[cm.depth_]); + p.params.resize(params_stack[cm.depth_]); } @@ // Save path state before trying this matcher if(cm.depth_ < detail::router_base::max_path_depth) path_stack[cm.depth_] = p.base_path.size(); + if(cm.depth_ < detail::router_base::max_path_depth) + params_stack[cm.depth_] = p.params.size(); @@ if(!cm(p, mr)) { + if(cm.depth_ < detail::router_base::max_path_depth) + p.params.resize(params_stack[cm.depth_]); // Clear matched_at_depth for this depth and deeper @@ if(!mr.params_.empty()) { for(auto& param : mr.params_) p.params.push_back(std::move(param)); } @@ if(current_depth < detail::router_base::max_path_depth) - path_stack[current_depth] = p.base_path.size(); + { + path_stack[current_depth] = p.base_path.size(); + params_stack[current_depth] = p.params.size(); + }
🤖 Fix all issues with AI agents
In `@include/boost/http/server/basic_router.hpp`:
- Around line 374-391: Add a brief docstring to the public class template
options_handler_impl explaining its purpose (an adapter that wraps a handler H
to implement the options_handler interface), the expectation for the template
parameter H (callable taking (P&, std::string_view) and returning route_task),
and note behaviour of its constructor and invoke method (that the constructor
stores the handler by decayed type and invoke forwards the call as
h(static_cast<P&>(rp), allow)). Place the comment above the declaration of
template<class H> struct options_handler_impl so it appears in the public
header.
- Around line 797-815: Update the docstring for set_options_handler to state
postconditions and possible exceptions: mention that calling
set_options_handler(H&&) replaces any existing handler stored in
options_handler_ (postcondition: options_handler_ holds a new
options_handler_impl<H> wrapping the provided callable), and document that
allocation or construction via std::make_unique may throw (e.g., std::bad_alloc
or any exception from the handler's construction); reference the function name
set_options_handler, the member options_handler_, and the concrete wrapper type
options_handler_impl to make the behavior explicit.
In `@src/server/flat_router.cpp`:
- Around line 469-474: The fallback for OPTIONS currently requires
options_methods != 0, which skips routes that only define custom verbs; change
the condition so the OPTIONS handler runs when either options_methods is
non-zero or options_custom_verbs is non-empty (i.e. (options_methods != 0 ||
!options_custom_verbs.empty()) ) and then call
build_allow_header(options_methods, options_custom_verbs) and co_await
options_handler_->invoke(p, allow) as before; this ensures custom-verb-only
routes produce an Allow header and reach options_handler_.
| template<class H> | ||
| struct options_handler_impl : options_handler | ||
| { | ||
| std::decay_t<H> h; | ||
|
|
||
| template<class H_> | ||
| explicit options_handler_impl(H_&& h_) | ||
| : h(std::forward<H_>(h_)) | ||
| { | ||
| } | ||
|
|
||
| route_task invoke( | ||
| route_params_base& rp, | ||
| std::string_view allow) const override | ||
| { | ||
| return h(static_cast<P&>(rp), allow); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add a docstring for options_handler_impl.
This is a class in a public header and should have a brief docstring documenting purpose and template parameter expectations.
✍️ Proposed doc comment
+ /** Wrapper that stores the user-provided OPTIONS handler.
+ `@tparam` H A callable invocable as route_task(P&, std::string_view).
+ */
template<class H>
struct options_handler_impl : options_handler📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template<class H> | |
| struct options_handler_impl : options_handler | |
| { | |
| std::decay_t<H> h; | |
| template<class H_> | |
| explicit options_handler_impl(H_&& h_) | |
| : h(std::forward<H_>(h_)) | |
| { | |
| } | |
| route_task invoke( | |
| route_params_base& rp, | |
| std::string_view allow) const override | |
| { | |
| return h(static_cast<P&>(rp), allow); | |
| } | |
| }; | |
| /** Wrapper that stores the user-provided OPTIONS handler. | |
| `@tparam` H A callable invocable as route_task(P&, std::string_view). | |
| */ | |
| template<class H> | |
| struct options_handler_impl : options_handler | |
| { | |
| std::decay_t<H> h; | |
| template<class H_> | |
| explicit options_handler_impl(H_&& h_) | |
| : h(std::forward<H_>(h_)) | |
| { | |
| } | |
| route_task invoke( | |
| route_params_base& rp, | |
| std::string_view allow) const override | |
| { | |
| return h(static_cast<P&>(rp), allow); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@include/boost/http/server/basic_router.hpp` around lines 374 - 391, Add a
brief docstring to the public class template options_handler_impl explaining its
purpose (an adapter that wraps a handler H to implement the options_handler
interface), the expectation for the template parameter H (callable taking (P&,
std::string_view) and returning route_task), and note behaviour of its
constructor and invoke method (that the constructor stores the handler by
decayed type and invoke forwards the call as h(static_cast<P&>(rp), allow)).
Place the comment above the declaration of template<class H> struct
options_handler_impl so it appears in the public header.
| /** Set the handler for automatic OPTIONS responses. | ||
|
|
||
| When an OPTIONS request matches a route but no explicit OPTIONS | ||
| handler is registered, this handler is invoked with the pre-built | ||
| Allow header value. This follows Express.js semantics where | ||
| explicit OPTIONS handlers take priority. | ||
|
|
||
| @param h A callable with signature `route_task(P&, std::string_view)` | ||
| where the string_view contains the pre-built Allow header value. | ||
| */ | ||
| template<class H> | ||
| void set_options_handler(H&& h) | ||
| { | ||
| static_assert( | ||
| std::is_invocable_r_v<route_task, const std::decay_t<H>&, P&, std::string_view>, | ||
| "Handler must have signature: route_task(P&, std::string_view)"); | ||
| this->options_handler_ = std::make_unique<options_handler_impl<H>>( | ||
| std::forward<H>(h)); | ||
| } |
There was a problem hiding this comment.
Document postconditions/exceptions for set_options_handler.
The docstring doesn’t mention that this replaces any existing handler and may throw on allocation.
✍️ Suggested doc additions
/** Set the handler for automatic OPTIONS responses.
@@
`@param` h A callable with signature `route_task(P&, std::string_view)`
where the string_view contains the pre-built Allow header value.
+
+ `@par` Postconditions
+ Replaces any previously configured OPTIONS handler.
+
+ `@par` Exceptions
+ May throw `std::bad_alloc` on allocation failure.
*/🤖 Prompt for AI Agents
In `@include/boost/http/server/basic_router.hpp` around lines 797 - 815, Update
the docstring for set_options_handler to state postconditions and possible
exceptions: mention that calling set_options_handler(H&&) replaces any existing
handler stored in options_handler_ (postcondition: options_handler_ holds a new
options_handler_impl<H> wrapping the provided callable), and document that
allocation or construction via std::make_unique may throw (e.g., std::bad_alloc
or any exception from the handler's construction); reference the function name
set_options_handler, the member options_handler_, and the concrete wrapper type
options_handler_impl to make the behavior explicit.
| // OPTIONS fallback: path matched but no explicit OPTIONS handler | ||
| if(is_options && options_methods != 0 && options_handler_) | ||
| { | ||
| // Build Allow header from collected methods | ||
| std::string allow = build_allow_header(options_methods, options_custom_verbs); | ||
| co_return co_await options_handler_->invoke(p, allow); |
There was a problem hiding this comment.
OPTIONS fallback skips custom-verb-only routes.
If a route has only custom verbs, options_methods stays 0, so the OPTIONS handler never runs.
✅ Proposed fix
- if(is_options && options_methods != 0 && options_handler_)
+ if(is_options && options_handler_ &&
+ (options_methods != 0 || !options_custom_verbs.empty()))
{
// Build Allow header from collected methods
std::string allow = build_allow_header(options_methods, options_custom_verbs);
co_return co_await options_handler_->invoke(p, allow);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // OPTIONS fallback: path matched but no explicit OPTIONS handler | |
| if(is_options && options_methods != 0 && options_handler_) | |
| { | |
| // Build Allow header from collected methods | |
| std::string allow = build_allow_header(options_methods, options_custom_verbs); | |
| co_return co_await options_handler_->invoke(p, allow); | |
| // OPTIONS fallback: path matched but no explicit OPTIONS handler | |
| if(is_options && options_handler_ && | |
| (options_methods != 0 || !options_custom_verbs.empty())) | |
| { | |
| // Build Allow header from collected methods | |
| std::string allow = build_allow_header(options_methods, options_custom_verbs); | |
| co_return co_await options_handler_->invoke(p, allow); | |
| } |
🤖 Prompt for AI Agents
In `@src/server/flat_router.cpp` around lines 469 - 474, The fallback for OPTIONS
currently requires options_methods != 0, which skips routes that only define
custom verbs; change the condition so the OPTIONS handler runs when either
options_methods is non-zero or options_custom_verbs is non-empty (i.e.
(options_methods != 0 || !options_custom_verbs.empty()) ) and then call
build_allow_header(options_methods, options_custom_verbs) and co_await
options_handler_->invoke(p, allow) as before; this ensures custom-verb-only
routes produce an Allow header and reach options_handler_.
|
GCOVR code coverage report https://260.http.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-02 13:45:17 UTC |
|
GCOVR code coverage report https://260.http.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-02 13:51:55 UTC |
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes