Conversation
Replace the old start(m, buffers) buffer-sequence API and never-implemented source mechanism with a clean two-mode sink: WriteSink (caller-owned buffers via write/write_eof) and BufferSink (zero-copy via prepare/commit/commit_eof). Add set_message() for decoupled message association, start_writes() and start_buffers() entry points with lazy start from the sink, and remove cbs_gen, max_type_erase, and all message-taking overloads.
Eliminate the two-step router+flat_router workflow by merging the flattened dispatch directly into any_router. Routes are now inlined into contiguous arrays at registration time, so dispatch is always cache-friendly without a separate flattening step.
📝 WalkthroughWalkthroughReworks routing into a templated router and public route_params, consolidates bcrypt headers, adds json_body/accepts/serve_index/http_worker, refactors serializer/parser streaming, introduces a compile-time DB schema, and removes legacy flat/basic router files and several bcrypt headers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Worker as http_worker
participant Parser as request_parser
participant Router as router
participant Middleware as json_body
participant Handler as Handler
participant Serializer as serializer
Client->>Worker: TCP request bytes
Worker->>Parser: read_header() / parse request
Parser-->>Worker: parsed request, url, headers
Worker->>Router: dispatch(method, url, route_params)
Router->>Middleware: invoke(route_params) (if matched)
Middleware->>Parser: push body into json_sink
Middleware-->>Router: store parsed json in route_data / next
Router->>Handler: call resolved handler (dynamic_invoke may resolve args)
Handler->>route_params: status(...) / send(body)
route_params->>Serializer: write/write_eof(body)
Serializer->>Client: write response bytes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
No actionable comments were generated in the recent review. 🎉 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://261.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-07 22:03:20 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/config.cpp (1)
35-42:⚠️ Potential issue | 🟡 MinorStale layout diagram —
Tis no longer allocated.The ASCII layout still shows a
Tregion (| fb | cb0 | cb1 | T | f |), butmax_type_erasehas been removed from the space calculation andThas no legend entry. This will mislead future readers about what the buffer actually contains.Suggested fix
- /* - | fb | cb0 | cb1 | T | f | - - fb flat_dynamic_buffer headers.max_size - cb0 circular_buffer min_buffer - cb1 circular_buffer min_buffer - f table max_table_space - */ + /* + | fb | cb0 | cb1 | f | + + fb flat_dynamic_buffer headers.max_size + cb0 circular_buffer min_buffer + cb1 circular_buffer min_buffer + f table max_table_space + */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… Warn if… the overview appears stale relative to the code."include/boost/http/parser.hpp (1)
467-476:⚠️ Potential issue | 🟡 MinorDocstring for
pulldoesn't mentioncapy::error::eofon body completion.Line 610 now returns
capy::error::eofwhen the body is complete, but the docstring at lines 471 only mentions "returns an empty span" without documenting the error code. Thereadmethod's docstring (line 347) correctly documents this behavior —pullshould be consistent.Suggested docstring update
/** Pull buffer data from the body. On first invocation, reads headers if not yet parsed. Returns buffer descriptors pointing to internal parser - memory. When the body is complete, returns an empty span. + memory. When the body is complete, returns + `capy::error::eof` with an empty span. `@param` dest Span of const_buffer to fill. `@return` An awaitable yielding `(error_code,std::span<const_buffer>)`. */As per coding guidelines, "Docstrings should include a brief description of purpose, document all parameters, return values, and any preconditions, postconditions, or exceptions."
src/server/mime_types.cpp (1)
28-81:⚠️ Potential issue | 🔴 Critical
"7z"is out of sort order — binary search will never find it.The array is documented as "sorted by extension for binary search," but
"7z"is placed last. Since'7'(0x37) precedes'a'(0x61) in ASCII (andcompare_icasedoesn't change digit ordering),"7z"must be the first entry. As-is,lookup("file.7z")will silently return an empty string.🐛 Proposed fix
constexpr ext_entry ext_db[] = { + { "7z", "application/x-7z-compressed" }, { "aac", "audio/aac" }, { "avif", "image/avif" }, ... { "xml", "application/xml" }, { "zip", "application/zip" }, - { "7z", "application/x-7z-compressed" }, };src/server/detail/any_router.hpp (1)
57-68:⚠️ Potential issue | 🔴 Critical
noexcepton a constructor that allocates astd::string— willstd::terminateon allocation failure.The third
entryconstructor is markednoexcept, butverb_str = verb_str_(line 67) copies astd::string_viewinto astd::string, which allocates memory and can throwstd::bad_alloc. If that happens,std::terminatewill be called.🐛 Proposed fix: remove noexcept
- // string verb match entry( std::string_view verb_str_, - handler_ptr h_) noexcept + handler_ptr h_) : h(std::move(h_)) , verb(http::string_to_method(verb_str_)) , all(false)src/serializer.cpp (1)
273-280:⚠️ Potential issue | 🟡 MinorRemove the unused two-argument
implconstructor at lines 273–280.The constructor
impl(cfg, message_base const& msg)is never called. The codebase instantiatesimplonly with a single argument (line 765), and the message is set separately viaset_message()(line 782). This dead code should be removed.include/boost/http/server/route_handler.hpp (1)
270-282:⚠️ Potential issue | 🟠 MajorRemove
noexceptfromroute_erroror change the contract to precondition.Line 279 declares
route_error(system::error_code ec) noexcept, but its docstring at line 277 claims@throw std::invalid_argument if !ec. The private constructor implementation (insrc/server/router_types.cpplines 85–92) confirms it does throwstd::invalid_argumentwhen the error code does not represent a failure. Anoexceptfunction that throws will callstd::terminate()instead of propagating the exception, violating the semantic contract ofnoexcept. Either remove thenoexceptspecifier or change the documentation to specify a precondition (e.g., "requiresecto be a failing code").
🤖 Fix all issues with AI agents
In `@include/boost/http/db/schema.hpp`:
- Around line 413-437: for_each_field and field_count call tag_invoke(fields,
T{}) which requires T be default-constructible, but HasMapping doesn't enforce
that; constrain these utilities to only accept default-initializable types by
adding std::default_initializable<T> (or equivalent) to the template requirement
list for template<HasMapping T, ...> in for_each_field and template<HasMapping
T> in field_count so the compiler will reject non-default-constructible T
instead of producing a hard-to-diagnose error when constructing T{}.
In `@include/boost/http/json/json_body.hpp`:
- Around line 32-39: The Content-Type check in the json_body middleware is too
permissive because it uses starts_with("application/json") and will match media
types like "application/json-patch+json"; update the check in the json_body
handler (the routine that verifies request method and content type before
creating a json_sink and storing json::value in the datastore) to accept
"application/json" only when it is followed by end-of-string, a semicolon, or
whitespace (i.e., parse the header token and allow optional parameters),
otherwise yield to the next handler; keep the rest of the flow (reading into
json_sink and storing under the "json::value" key) unchanged.
In `@include/boost/http/server/detail/router_base.hpp`:
- Line 28: Remove the stale forward declaration "template<class> class router;"
and replace nothing; instead rely on the existing correct two-parameter template
declaration of router (template<class P = route_params, class HT = identity>
class router) that the friend declaration already references; locate and delete
the single-parameter forward declaration in router_base.hpp so only the correct
router template (as defined in the router.hpp header) is visible and there are
no conflicting/unused forward declarations.
In `@include/boost/http/server/http_worker.hpp`:
- Around line 43-71: The example in the class docstring is stale: it calls
http_worker(router, parser_cfg, serializer_cfg) but the actual constructor is
http_worker(Stream& stream_, router, parser_cfg, serializer_cfg) and it already
wires rp.req_body, rp.res_body and stream; update the example's my_worker
constructor to call http_worker(sock, router, parser_cfg, serializer_cfg) (or
otherwise pass the stream object as the first argument) and remove the manual
assignments to rp.req_body, rp.res_body, and stream so the example compiles and
reflects the real http_worker(Stream& ...) constructor and behavior.
- Around line 10-11: The include guard macro is misnamed; change the opening
macro from BOOST_HTTP_SERVER_WORKER_HPP to BOOST_HTTP_SERVER_HTTP_WORKER_HPP and
update the corresponding closing `#endif` comment (if present) to match; locate
the macros in http_worker.hpp (the existing BOOST_HTTP_SERVER_WORKER_HPP) and
replace both the `#ifndef/`#define and the trailing `#endif` comment to use
BOOST_HTTP_SERVER_HTTP_WORKER_HPP so the guard matches the filename and Boost
naming conventions.
- Around line 89-110: The constructor docblock and template constraint need
updating: add a `@param` entry describing the first parameter stream_ (the stream
used for both input and output) and change the template constraint on
http_worker from template<capy::WriteStream Stream> to require both read and
write capabilities (e.g., use a requires-clause or combined concept so Stream
satisfies both capy::ReadStream and capy::WriteStream); this ensures
parser.source_for(stream_) and serializer.sink_for(stream_) are valid.
Reference: the http_worker constructor, the template parameter Stream, and the
calls parser.source_for(stream_) and serializer.sink_for(stream_).
In `@include/boost/http/server/route_handler.hpp`:
- Around line 478-483: The code creates a "soft slash" view by computing
p.priv_.decoded_path_.data() + p.priv_.decoded_path_.size() - 1 which is
undefined when decoded_path_ is empty; update the logic in the route handler
that sets p.path to first check p.priv_.decoded_path_.empty() (or assert
!empty()) and handle the empty case by assigning the literal "/" view (or
otherwise avoid subtracting from size()), otherwise perform the current pointer
arithmetic; reference p.path and p.priv_.decoded_path_ (and the block currently
asserting BOOST_ASSERT(p.path == "/")) when making the change.
In `@include/boost/http/server/router.hpp`:
- Around line 619-625: The converting constructor template for router
incorrectly requires std::derived_from<OtherP, P>, allowing constructing
router<P> from router<Derived> which leads to unsafe static_casts in handler
invoke(); change the constraint on the template<class OtherP> constructor to
require std::derived_from<P, OtherP> (i.e. P derives from OtherP) so only
routers whose params type is a base of P can be moved into router<P>, and verify
the constructor initializer still delegates to
detail::router_base(std::move(other)) and keeps noexcept semantics.
In `@src/serializer.cpp`:
- Around line 794-810: The two methods serializer::start_stream and
serializer::start_writes are identical but intended as distinct entry points;
either implement start_writes to call the correct backend (e.g.,
impl_->start_writes(*impl_->msg_) or another appropriate method on impl_) or, if
this synonymy is intentional during a transition, add a clear comment above
serializer::start_writes stating it intentionally delegates to
impl_->start_stream and why; update the call site accordingly and ensure the
unique symbols serializer::start_writes and impl_->start_writes (or the correct
impl method) are used to make the intent explicit.
In `@src/server/any_router.cpp`:
- Around line 36-37: The build_allow_header function currently returns a
hardcoded string when methods == ~0ULL which omits verbs in the known[] array
and any custom verbs; change it to build the Allow header dynamically: when
methods == ~0ULL treat it as "include all known methods" by iterating the
known[] array (and the custom verbs collection used elsewhere) to append every
known method name plus any registered custom verbs, join them with commas, and
return that string so CONNECT, TRACE, PROPFIND and custom verbs are included;
ensure you use the same known[] and custom storage identifiers as in the file so
behavior matches non-wildcard bitmask construction.
- Around line 614-623: The OPTIONS "*" early-return is invoking
impl_->options_handler_->invoke with p before p (route_params) is initialized;
move the route_params initialization (the block that sets p.kind_, p.verb_,
p.decoded_path_, p.params, etc.) to occur before the OPTIONS '*' check so p is
fully populated when passed to impl_->options_handler_->invoke, and apply the
same reordering in the dispatch overload that takes std::string_view verb (the
other dispatch block that calls impl_->options_handler_->invoke) so both code
paths initialize route_params prior to calling the options handler.
In `@src/server/http_worker.cpp`:
- Around line 66-73: The code calls rv.value() unconditionally after
urls::parse_uri_reference(rp.req.target()), which will throw if rv.has_error();
update the error path in the parsing block so that after setting
rp.status(http::status::bad_request) you skip using rv (e.g., send the response
and continue or return from the surrounding loop/coroutine) and only call rp.url
= rv.value() when rv.has_value() (or !rv.has_error()); locate the parsing block
around urls::parse_uri_reference, the local variable rv, and the
rp.status/rp.url assignments and ensure control flow exits the error case before
accessing rv.value().
In `@src/server/serve_index.cpp`:
- Around line 399-410: The canonicalization block for root_canonical and
dir_canonical silently ignores errors via a reused ec2, which can leave
non-canonical paths and make the show_up decision incorrect; update the code in
serve_index.cpp to use separate std::error_code variables for each
std::filesystem::canonical call (e.g., ec_root and ec_dir), check each error
after its call, and if either canonicalization fails set show_up = false (or
otherwise short-circuit the parent-link logic) before comparing dir_canonical
and root_canonical; ensure you still respect impl_->opts.show_parent when
deciding the final value of show_up.
- Around line 366-395: The code constructs a std::filesystem::directory_iterator
named it but then creates a second iterator in the for loop, causing the
directory to be opened twice and the error code from the second open to be
ignored; replace the second iterator with the already-created it (and use it in
the for loop) so you only open the directory once and check fec before
iterating, keeping the existing behavior around skipping hidden files,
populating dir_entry (e.name, e.is_dir, e.size, e.mtime) and pushing into
entries.
- Around line 230-244: The JSON string escaping in the loop that builds body
(see the loop iterating over e.name and calls like body.append and
body.push_back) only handles '"', '\\', '\n', '\r', '\t' and leaves other
control characters U+0000–U+001F unescaped; update that loop to escape all
control characters per RFC 8259 by mapping '\b' and '\f' to "\\b" and "\\f" and
encoding any other control codepoints (<= 0x1F) as "\\u00XX" (hex-escaped)
before appending to body so the generated JSON is always valid.
🧹 Nitpick comments (32)
src/server/fresh.cpp (1)
10-16: Add a high-level overview comment after the includes.This file contains non-trivial implementation logic for HTTP cache freshness validation (ETag matching with weak/strong validator handling, Last-Modified date comparison). A block comment after the includes should explain the overall strategy, the simplifications made (e.g., lexicographic date comparison, simple ETag parsing), and reference to the HTTP caching specification being implemented (RFC 7232 freshness validation).
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. This should highlight anything tricky or non-obvious that a maintainer might need to know in order to reason effectively about the code."📝 Example overview comment
`#include` <boost/http/server/fresh.hpp> `#include` <boost/http/field.hpp> +/* + Implementation of HTTP cache freshness validation per RFC 7232. + + The is_fresh() function checks if a cached response can be reused based on + conditional request headers (If-None-Match, If-Modified-Since) and response + validators (ETag, Last-Modified). + + Strategy: + - ETag matching is checked first as the stronger validator + - Falls back to Last-Modified date comparison if ETags not present + - Handles weak/strong ETag distinction with W/ prefix stripping + + Simplifications: + - ETag matching uses simple substring search, not full list parsing + - Date comparison is lexicographic (works for RFC 7231 format dates) + - Does not fully parse comma-separated ETag lists +*/ namespace boost { namespace http {include/boost/http/server/detail/dynamic_invoke.hpp (2)
13-18: Missing high-level overview block comment after includes.This file implements non-trivial template metaprogramming (type-uniqueness checking, polystore-driven argument resolution, coroutine handler wrapping). A
/* */block comment after the includes explaining the overall design—how callables are resolved from a polystore, the role offind_key_t, and why duplicate types are disallowed—would help maintainers reason about this code.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."
37-40: Nit: considerstd::remove_cvref_t(C++20).Given the project already uses C++20 features (coroutines, fold expressions,
std::bool_constant, etc.),std::remove_cvref_t<T>is a direct drop-in replacement forstd::remove_cv_t<std::remove_reference_t<T>>.♻️ Suggested simplification
template<class T> using find_key_t = - std::remove_cv_t< - std::remove_reference_t<T>>; + std::remove_cvref_t<T>;src/server/mime_types.cpp (1)
10-14: Consider adding a brief overview block comment after the includes.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."include/boost/http/json/json_sink.hpp (1)
115-129:write_someandwriteare functionally identical — document the intended semantic difference.Both
write_someandwritedelegate towrite_impl(buffers, false). Since JSON parsing is synchronous and always consumes all provided data, they behave identically. If the distinction exists purely to satisfy theWriteSinkconcept's surface area, a brief inline comment onwrite_some(e.g., "// satisfies WriteSink; equivalent to write() for synchronous sinks") would help future maintainers understand why both exist.include/boost/http/db/schema.hpp (1)
1-18: Missing high-level overview comment and repository URL.Per coding guidelines, files with non-trivial implementation logic should include a
/* */block comment after the includes providing a high-level overview. This file introduces a complete compile-time schema system with several interacting abstractions (fields, embeds, relationships, CPOs, concepts). A brief architectural overview would help maintainers orient themselves.Also, unlike other headers in the project, the copyright block omits the
// Official repository: ...line.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/router_types.cpp (1)
9-17: Missing implementation overview block comment.This file contains non-trivial logic (error category definition,
route_result,route_params::is_method,route_params::send). Consider adding a/* */block comment after the includes summarizing the purpose and key behaviors. As per coding guidelines, "Files containing non-trivial implementation logic should include a/* */block comment after the includes that provides a high-level overview".src/json/json_body.cpp (1)
9-16: Missing implementation overview block comment.This file implements a coroutine-based JSON body parser middleware. A brief
/* */block comment after the includes would help future maintainers understand the flow (POST+JSON content-type guard → stream to json_sink → store in route_data). As per coding guidelines, "Files containing non-trivial implementation logic should include a/* */block comment after the includes that provides a high-level overview".src/server/http_worker.cpp (3)
52-53:std::cerrin library code is not ideal for production.Library code typically should not write to
stderrdirectly — callers have no control over this output. Consider routing through a configurable logging mechanism or the error code propagation path.
22-46: Guard destructor duplicates the per-iteration reset at the top of the loop.The guard's destructor (lines 31-36) performs
parser.reset(); parser.start(); rp.session_data.clear();, which is identical to lines 44-46 at the start of each loop iteration. The guard provides safety for mid-coroutine teardown, which is good, but the duplication is worth a brief inline comment explaining why both exist (the guard covers abnormal exit / cancellation).
9-14: Missing implementation overview block comment.This coroutine drives the entire HTTP session lifecycle (read-header → dispatch → response loop). A
/* */overview after the includes would help a maintainer quickly understand the flow, especially the guard's role and the URL-parse → dispatch sequence. As per coding guidelines, "Files containing non-trivial implementation logic should include a/* */block comment after the includes that provides a high-level overview".include/boost/http/server/serve_index.hpp (1)
39-93: Consider documenting thread-safety guarantees.Other classes in this PR (e.g.,
http_worker) document thread-safety ("Distinct objects: Safe. Shared objects: Unsafe."). Sinceserve_indexholds mutable state behind a pointer and isconst-callable viaoperator(), users would benefit from knowing whether it's safe to share across threads. As per coding guidelines, "Document thread-safety guarantees and exception specifications where applicable, particularly for classes intended for concurrent use".include/boost/http/server/http_worker.hpp (1)
82-87: Public data members lack documentation.The five public data members (
fr,rp,stream,parser,serializer) are undocumented. Since this is a public header and users interact with these directly (as shown in the example), brief///doc comments would help clarify ownership semantics and expected usage. As per coding guidelines, "Docstrings are required for all classes and functions in public headers in non-detail namespaces".Proposed addition
public: + /// The router for dispatching requests to handlers. http::router<route_params> fr; + /// Per-request state: request, response, and route data. http::route_params rp; + /// The underlying read stream for the connection. capy::any_read_stream stream; + /// The HTTP request parser. http::request_parser parser; + /// The HTTP response serializer. http::serializer serializer;include/boost/http/server/accepts.hpp (1)
22-41: Well-documented public API.The class-level and per-method documentation is thorough — parameters, return values, and behavior for edge cases (empty Accept header, no match) are all covered. One minor omission per the coding guidelines: the class stores a
const&tofields_base, so a brief thread-safety and lifetime note would be helpful (e.g., "the caller must ensurefieldsoutlives this object; const member functions may be called concurrently").As per coding guidelines, "Document thread-safety guarantees and exception specifications where applicable, particularly for classes intended for concurrent use or functions that may throw."
src/server/serve_index.cpp (2)
26-55:path_catis duplicated fromsrc/server/serve_static.cpp.The relevant code snippets show an identical
path_catfunction insrc/server/serve_static.cpp(lines 23-51). Consider extracting it into a shared internal header to avoid maintaining two copies.
10-19: Missing implementation overview block comment.This file contains non-trivial implementation logic (path assembly, directory reading, content negotiation, multiple rendering formats) but lacks a
/* */block comment after the includes providing a high-level overview for maintainers.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/serializer.cpp (1)
622-632:is_done()andis_start()have identical bodies.Both return
state_ == state::start. While they serve different semantic purposes at the API level (checking "has serialization completed" vs. "has serialization not yet begun"), having identical implementations means they cannot be distinguished in practice. If the state machine genuinely resets tostartafter completion, consider adding a brief inline comment explaining why both map to the same state.src/server/detail/any_router.hpp (1)
109-114:const_castfor lazy finalization is functional but fragile.The
const_cast<impl*>(this)->finalize_pending()pattern insidestd::call_onceworks because theimplobject is never trulyconst(it's held byshared_ptr<impl>), but it bypasses const-correctness guarantees. Sincefinalized_is alreadymutable, consider makingfinalize_pending()itself callable on a const impl (or makingensure_finalized()non-const and adjusting call sites via the shared_ptr).src/server/accepts.cpp (1)
10-14: Missing implementation overview block comment.This file implements a full HTTP content negotiation engine with multiple parsing strategies (media types, simple tokens, language prefixes) and a generic negotiation template. A brief
/* */block comment after the includes summarizing the approach (parse Accept-* headers into scored entries, then negotiate against offered values using specificity + q-value + order) would help maintainers orient quickly.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/any_router.cpp (3)
296-302: Redundantconst_cast—pis already a non-const reference.
dispatch_looptakesroute_params& p, soconst_cast<route_params&>(p)at lines 298 and 317 is a no-op. This appears to be a leftover from when the parameter may have been const.♻️ Proposed fix
if(m.end_ && !e.match_method( - const_cast<route_params&>(p))) + p)) {rv = co_await e.h->invoke( - const_cast<route_params&>(p)); + p);
200-374: Core dispatch loop is well-structured but would benefit from an overview comment.The dispatch loop implements a complex hierarchical matching algorithm with ancestor validation, depth tracking, OPTIONS aggregation, exception capture, and multiple continuation modes (next, next_route, done, close, error). A block comment at the top of the function summarizing this flow would greatly help maintainability.
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."
625-648: Duplicate initialization logic across bothdispatchoverloads.The two
dispatchmethods share ~20 lines of identical route_params initialization and path setup (decoding, trailing slash handling, base_path/path setup). Consider extracting this into a private helper to reduce duplication and prevent the two overloads from diverging.Also applies to: 675-701
include/boost/http/server/route_handler.hpp (2)
440-446:reset(),status(), andsend()lack sufficient documentation.
reset()should document what state it clears and its postconditions.status(http::status)should clarify that it sets the response status code and returns*thisfor chaining.send()has a one-liner but should mention that it writes the response throughres_bodyand what happens if no body is provided (empty body vs. no body). These are public API methods on a central type.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."
458-508:match_resultis a public nested struct accessing private state — consider restricting visibility.
match_resultaccessespriv_members directly (lines 480–481, 490–493), yet it is declaredpublicat line 383. Users could construct and calladjust_path/restore_pathon arbitraryroute_paramsobjects, corrupting internal state. If this type is only used internally by the router, it should be in aprivateordetailscope and befriended as needed.include/boost/http/serializer.hpp (3)
217-238:start_stream()coexists withstart_writes()— clarify or deprecate.The docstring for
start_stream()says it is a "Low-level entry point equivalent to@refstart_writes." If they are truly equivalent, having both creates confusion about which one to call. Consider deprecatingstart_stream()in favor ofstart_writes()(e.g., with[[deprecated]]), or document why both exist (backward compatibility?).
647-680: Drain-loop logic is duplicated ~6 times acrosssinkmethods.The pattern
while(!is_done()) { prepare → check error/need_data → check empty → write_some → consume }is repeated incommit,commit_eof,write(twice: header drain + body drain),write_eof(buffers), andwrite_eof(). The only variations are whetherneed_datatriggersbreakvscontinue. Extracting a private helper coroutine (e.g.,drain(bool until_done)) would significantly reduce duplication and the risk of inconsistent fixes.Also applies to: 696-730, 752-774, 800-824, 856-878, 902-927
610-635:prepare()is synchronous but has no error reporting path.If
stream_prepare()throws (e.g., because the serializer is not in streaming mode), the exception propagates uncaught. The docstring doesn't mention exceptions. Since this is synchronous (not a coroutine), consider documenting which exceptions may propagate, or wrapping with asystem::result.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."
include/boost/http/server/router.hpp (5)
61-130: Bit-packing inrouter_optionsis non-obvious — add a comment documenting the encoding.The bit layout across
merge_params(bit 0),case_sensitive(bits 1–2 for tri-state: 2 = set-true, 4 = set-false), andstrict(bits 3–4 for tri-state: 8 = set-true, 16 = set-false) uses magic numbers. A brief comment next tounsigned int v_ = 0explaining the bit layout would help maintainers.As per coding guidelines, "Files containing non-trivial implementation logic should include a
/* */block comment ... This should highlight anything tricky or non-obvious that a maintainer might need to know."Proposed comment
private: template<class, class> friend class router; + // Bit layout: + // bit 0 : merge_params (0=false, 1=true) + // bits 1-2 : case_sensitive tri-state (0=inherit, 2=true, 4=false) + // bits 3-4 : strict tri-state (0=inherit, 8=true, 16=false) unsigned int v_ = 0; };
506-545:handlers_implstores a dangling reference to the transform after construction.
T const& htat line 509 is a reference to the transform. It is only used during the constructor'sassign()calls (lines 522–539). After construction,htis dangling if the originalht_is a temporary or moves. Since thehandler_ptrobjects are fully populated during construction and moved out byadd_middleware, this likely works in practice. However, the dangling reference is fragile. Consider storing the reference only within the constructor scope, or using a pointer set tonullptrafter construction.
456-478:handler_impl::invokerelies on dead-codestd::terminate()branch.The
elsebranch at line 474–477 callsstd::terminate(), which should be unreachable given thathandler_kindstatically classifies handlers at registration. This is a reasonable safety net. Consider annotating withBOOST_UNREACHABLE()or a compiler hint for the optimiser, if available in the Boost ecosystem.
891-902:except()passesht_tomake_handlersbut transforms are never applied to exception handlers.The class docstring states "Error handlers and exception handlers are never transformed." Inside
handlers_impl::assign, exception handlers match the firstif constexprbranch (lines 525–531) which does not invoke the transform, so this is functionally correct. However, passinght_is misleading to readers. Consider passing a no-op oridentity{}explicitly, or adding a comment clarifying that the transform is unused for these handler kinds.
1190-1202:fluent_routestores a reference torouter— document lifetime requirements.The
router&member at line 1201 means thefluent_routeis only valid while the owningrouteris alive. Sinceroute()at line 1058 returns by value and is typically used in chained calls (e.g.,r.route("/x").get(...)), this is safe in normal usage. The docstring forfluent_route(line 559) could briefly note that the route must not outlive the router.
| template <HasMapping T, typename F> | ||
| constexpr void for_each_field(F&& f) | ||
| { | ||
| constexpr auto fs = tag_invoke(fields, T{}); | ||
| detail::for_each_impl( | ||
| fs, | ||
| static_cast<F&&>(f), | ||
| std::make_index_sequence< | ||
| std::tuple_size_v<decltype(fs)>>{}); | ||
| } | ||
|
|
||
| /** Return the number of fields in a mapped type. | ||
|
|
||
| @tparam T A type satisfying @ref HasMapping. | ||
|
|
||
| @return The compile-time field count. | ||
|
|
||
| @see for_each_field | ||
| */ | ||
| template <HasMapping T> | ||
| constexpr std::size_t field_count() | ||
| { | ||
| constexpr auto fs = tag_invoke(fields, T{}); | ||
| return std::tuple_size_v<decltype(fs)>; | ||
| } |
There was a problem hiding this comment.
for_each_field and field_count require default-constructible T, but HasMapping does not enforce this.
Both for_each_field<T> and field_count<T> invoke tag_invoke(fields, T{}), which requires T to be default-constructible. However, the HasMapping concept only checks that tag_invoke(fields, v) is valid for T const& v — it does not require default-constructibility. A type that satisfies HasMapping but has a deleted/private default constructor would pass the concept check but fail to compile when used with these utilities.
Consider either:
- Adding a
std::default_initializable<T>constraint to these functions, or - Using
std::declval<T const&>()in adecltypecontext instead ofT{}(though this complicates constexpr usage).
🤖 Prompt for AI Agents
In `@include/boost/http/db/schema.hpp` around lines 413 - 437, for_each_field and
field_count call tag_invoke(fields, T{}) which requires T be
default-constructible, but HasMapping doesn't enforce that; constrain these
utilities to only accept default-initializable types by adding
std::default_initializable<T> (or equivalent) to the template requirement list
for template<HasMapping T, ...> in for_each_field and template<HasMapping T> in
field_count so the compiler will reject non-default-constructible T instead of
producing a hard-to-diagnose error when constructing T{}.
| /** Route handler middleware that parses a JSON request body. | ||
|
|
||
| This middleware checks that the request is a POST with | ||
| Content-Type `application/json`, reads the entire request | ||
| body into a @ref json_sink, and stores the parsed | ||
| `json::value` in the route's @ref datastore under the | ||
| key `json::value`. If the request does not match, the | ||
| middleware yields to the next handler. |
There was a problem hiding this comment.
Content-Type prefix match may be too broad.
The implementation (in src/json/json_body.cpp) uses starts_with("application/json") to check the Content-Type. This would also match types like application/json-patch+json or application/jsonlines, which are distinct media types that shouldn't be parsed as plain JSON bodies.
Consider checking for an exact match or verifying that the character following "application/json" is either end-of-string, ;, or whitespace.
Proposed fix in src/json/json_body.cpp
- if( ! p.is_method(method::post) ||
- ! p.req.value_or(
- field::content_type, "")
- .starts_with("application/json"))
+ auto ct = p.req.value_or(
+ field::content_type, "");
+ bool is_json =
+ ct.starts_with("application/json") &&
+ (ct.size() == 16 ||
+ ct[16] == ';' ||
+ ct[16] == ' ');
+ if( ! p.is_method(method::post) || ! is_json)🤖 Prompt for AI Agents
In `@include/boost/http/json/json_body.hpp` around lines 32 - 39, The Content-Type
check in the json_body middleware is too permissive because it uses
starts_with("application/json") and will match media types like
"application/json-patch+json"; update the check in the json_body handler (the
routine that verifies request method and content type before creating a
json_sink and storing json::value in the datastore) to accept "application/json"
only when it is followed by end-of-string, a semicolon, or whitespace (i.e.,
parse the header token and allow optional parameters), otherwise yield to the
next handler; keep the rest of the flow (reading into json_sink and storing
under the "json::value" key) unchanged.
| template<class> | ||
| class basic_router; | ||
| class flat_router; | ||
| template<class> class router; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all usages of detail::router
rg -n 'detail::router' --type=cpp --type=hppRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
# Also search for just "router" in the detail namespace to see if it's referenced
rg -n '::router<' include/boost/http/server/detail/ --type=cpp --type=hppRepository: cppalliance/http
Length of output: 87
🏁 Script executed:
# Check the actual content of the file around lines 28 and 55
sed -n '25,60p' include/boost/http/server/detail/router_base.hppRepository: cppalliance/http
Length of output: 972
🏁 Script executed:
# Correct rg syntax - search for detail::router or router< pattern
rg 'detail::router' --type-list | grep -i hppRepository: cppalliance/http
Length of output: 449
🏁 Script executed:
# Try simpler search without file type specification
rg 'detail::router' -A 2 -B 2Repository: cppalliance/http
Length of output: 4667
🏁 Script executed:
# Search for router references in detail namespace
rg 'router<' include/boost/http/server/detail/ -A 2 -B 2Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check the entire router_base.hpp file for any reference to the forward declaration
cat include/boost/http/server/detail/router_base.hpp | grep -n 'router'Repository: cppalliance/http
Length of output: 1073
🏁 Script executed:
# Verify: search explicitly for any single-parameter router usage in detail namespace
rg 'router<[^,>]+>' include/boost/http/server/detail/Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Check if there's any other definition of detail::router with one parameter
rg 'detail::router<' -A 3Repository: cppalliance/http
Length of output: 42
🏁 Script executed:
# Verify the actual router template definition
sed -n '1,50p' include/boost/http/server/router.hppRepository: cppalliance/http
Length of output: 1543
Remove stale forward declaration with incorrect template arity.
Line 28 forward-declares template<class> class router; with a single template parameter, but the actual router template defined in include/boost/http/server/router.hpp takes two parameters: template<class P = route_params, class HT = identity> class router. The friend declaration at line 55 correctly references the two-parameter version. The stale single-parameter forward declaration is not referenced anywhere in the codebase and should be removed.
🤖 Prompt for AI Agents
In `@include/boost/http/server/detail/router_base.hpp` at line 28, Remove the
stale forward declaration "template<class> class router;" and replace nothing;
instead rely on the existing correct two-parameter template declaration of
router (template<class P = route_params, class HT = identity> class router) that
the friend declaration already references; locate and delete the
single-parameter forward declaration in router_base.hpp so only the correct
router template (as defined in the router.hpp header) is visible and there are
no conflicting/unused forward declarations.
| #ifndef BOOST_HTTP_SERVER_WORKER_HPP | ||
| #define BOOST_HTTP_SERVER_WORKER_HPP |
There was a problem hiding this comment.
Include guard doesn't match the filename.
The file is http_worker.hpp but the guard is BOOST_HTTP_SERVER_WORKER_HPP. For consistency with Boost conventions (and to avoid collisions if a worker.hpp ever exists), this should be BOOST_HTTP_SERVER_HTTP_WORKER_HPP.
Proposed fix
-#ifndef BOOST_HTTP_SERVER_WORKER_HPP
-#define BOOST_HTTP_SERVER_WORKER_HPP
+#ifndef BOOST_HTTP_SERVER_HTTP_WORKER_HPP
+#define BOOST_HTTP_SERVER_HTTP_WORKER_HPPAnd at the end of the file:
-#endif
+#endif // BOOST_HTTP_SERVER_HTTP_WORKER_HPP🤖 Prompt for AI Agents
In `@include/boost/http/server/http_worker.hpp` around lines 10 - 11, The include
guard macro is misnamed; change the opening macro from
BOOST_HTTP_SERVER_WORKER_HPP to BOOST_HTTP_SERVER_HTTP_WORKER_HPP and update the
corresponding closing `#endif` comment (if present) to match; locate the macros in
http_worker.hpp (the existing BOOST_HTTP_SERVER_WORKER_HPP) and replace both the
`#ifndef/`#define and the trailing `#endif` comment to use
BOOST_HTTP_SERVER_HTTP_WORKER_HPP so the guard matches the filename and Boost
naming conventions.
| @par Example | ||
| @code | ||
| struct my_worker | ||
| : tcp_server::worker_base | ||
| , http_worker | ||
| { | ||
| corosio::tcp_socket sock; | ||
|
|
||
| my_worker( | ||
| corosio::io_context& ctx, | ||
| http::router<route_params> const& router, | ||
| http::shared_parser_config parser_cfg, | ||
| http::shared_serializer_config serializer_cfg) | ||
| : http_worker(router, parser_cfg, serializer_cfg) | ||
| , sock(ctx) | ||
| { | ||
| sock.open(); | ||
| rp.req_body = capy::any_buffer_source(parser.source_for(sock)); | ||
| rp.res_body = capy::any_buffer_sink(serializer.sink_for(sock)); | ||
| stream = capy::any_read_stream(&sock); | ||
| } | ||
|
|
||
| corosio::tcp_socket& socket() override { return sock; } | ||
|
|
||
| void run(launcher launch) override | ||
| { | ||
| launch(sock.get_executor(), do_http_session()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Example in class docstring won't compile — constructor signature mismatch.
The example constructs http_worker(router, parser_cfg, serializer_cfg) with 3 arguments, but the actual constructor (line 97) takes 4: http_worker(Stream& stream_, router, parser_cfg, serializer_cfg). The example also manually wires rp.req_body, rp.res_body, and stream, but the constructor already does this internally — the example appears to be stale.
🤖 Prompt for AI Agents
In `@include/boost/http/server/http_worker.hpp` around lines 43 - 71, The example
in the class docstring is stale: it calls http_worker(router, parser_cfg,
serializer_cfg) but the actual constructor is http_worker(Stream& stream_,
router, parser_cfg, serializer_cfg) and it already wires rp.req_body,
rp.res_body and stream; update the example's my_worker constructor to call
http_worker(sock, router, parser_cfg, serializer_cfg) (or otherwise pass the
stream object as the first argument) and remove the manual assignments to
rp.req_body, rp.res_body, and stream so the example compiles and reflects the
real http_worker(Stream& ...) constructor and behavior.
| // Handle OPTIONS * before normal dispatch | ||
| if(verb == http::method::options && | ||
| url.encoded_path() == "*") | ||
| { | ||
| if(impl_->options_handler_) | ||
| { | ||
| return impl_->options_handler_->invoke( | ||
| p, impl_->global_allow_header_); | ||
| } | ||
| } |
There was a problem hiding this comment.
route_params used before initialization in OPTIONS * early-return path.
When verb == method::options and the path is "*", the handler is invoked at line 620-621 with p — but p hasn't been initialized yet (the initialization block starts at line 626). This means route_params fields like kind_, verb_, decoded_path_, params, etc. contain whatever state the caller passed in, which could be stale or default values. The same issue exists in the second dispatch overload at lines 665-673.
🐛 Proposed fix: move initialization before the OPTIONS * check
impl_->ensure_finalized();
- // Handle OPTIONS * before normal dispatch
- if(verb == http::method::options &&
- url.encoded_path() == "*")
- {
- if(impl_->options_handler_)
- {
- return impl_->options_handler_->invoke(
- p, impl_->global_allow_header_);
- }
- }
-
// Initialize params
auto& pv = *route_params_access{p};
pv.kind_ = is_plain;
pv.verb_ = verb;
pv.verb_str_.clear();
pv.ec_.clear();
pv.ep_ = nullptr;
p.params.clear();
+
+ // Handle OPTIONS * before normal dispatch
+ if(verb == http::method::options &&
+ url.encoded_path() == "*")
+ {
+ if(impl_->options_handler_)
+ {
+ return impl_->options_handler_->invoke(
+ p, impl_->global_allow_header_);
+ }
+ }
+
pv.decoded_path_ = pct_decode_path(url.encoded_path());Apply the same reordering in the std::string_view verb overload (lines 665-673).
🤖 Prompt for AI Agents
In `@src/server/any_router.cpp` around lines 614 - 623, The OPTIONS "*"
early-return is invoking impl_->options_handler_->invoke with p before p
(route_params) is initialized; move the route_params initialization (the block
that sets p.kind_, p.verb_, p.decoded_path_, p.params, etc.) to occur before the
OPTIONS '*' check so p is fully populated when passed to
impl_->options_handler_->invoke, and apply the same reordering in the dispatch
overload that takes std::string_view verb (the other dispatch block that calls
impl_->options_handler_->invoke) so both code paths initialize route_params
prior to calling the options handler.
| { | ||
| auto rv = urls::parse_uri_reference(rp.req.target()); | ||
| if(rv.has_error()) | ||
| { | ||
| rp.status(http::status::bad_request); | ||
| } | ||
| rp.url = rv.value(); | ||
| } |
There was a problem hiding this comment.
Accessing rv.value() after an error will throw system_error.
When urls::parse_uri_reference fails, the code sets the response status to bad_request but falls through to rv.value() on line 72. Since rv holds an error, .value() will throw boost::system::system_error, likely terminating the coroutine with an unhandled exception.
The error path needs to skip the .value() call — either continue to the next iteration (after sending an error response) or break.
🐛 Proposed fix
// Parse the URL
{
auto rv = urls::parse_uri_reference(rp.req.target());
if(rv.has_error())
{
rp.status(http::status::bad_request);
+ // TODO: send the 400 response before continuing
+ break;
}
- rp.url = rv.value();
+ else
+ rp.url = rv.value();
}🤖 Prompt for AI Agents
In `@src/server/http_worker.cpp` around lines 66 - 73, The code calls rv.value()
unconditionally after urls::parse_uri_reference(rp.req.target()), which will
throw if rv.has_error(); update the error path in the parsing block so that
after setting rp.status(http::status::bad_request) you skip using rv (e.g., send
the response and continue or return from the surrounding loop/coroutine) and
only call rp.url = rv.value() when rv.has_value() (or !rv.has_error()); locate
the parsing block around urls::parse_uri_reference, the local variable rv, and
the rp.status/rp.url assignments and ensure control flow exits the error case
before accessing rv.value().
| body.append("{\"name\":\""); | ||
|
|
||
| // Escape JSON string | ||
| for(auto c : e.name) | ||
| { | ||
| switch(c) | ||
| { | ||
| case '"': body.append("\\\""); break; | ||
| case '\\': body.append("\\\\"); break; | ||
| case '\n': body.append("\\n"); break; | ||
| case '\r': body.append("\\r"); break; | ||
| case '\t': body.append("\\t"); break; | ||
| default: body.push_back(c); break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete JSON string escaping — control characters emitted raw.
RFC 8259 §7 requires all control characters (U+0000–U+001F) to be escaped. This code only handles ", \\, \n, \r, \t. Characters like \b (\x08), \f (\x0C), and other control chars will be emitted raw, producing invalid JSON. While unlikely in filenames on most OSes, it's a correctness gap.
🐛 Proposed fix: escape remaining control characters
for(auto c : e.name)
{
switch(c)
{
case '"': body.append("\\\""); break;
case '\\': body.append("\\\\"); break;
+ case '\b': body.append("\\b"); break;
+ case '\f': body.append("\\f"); break;
case '\n': body.append("\\n"); break;
case '\r': body.append("\\r"); break;
case '\t': body.append("\\t"); break;
- default: body.push_back(c); break;
+ default:
+ if(static_cast<unsigned char>(c) < 0x20)
+ {
+ char buf[7];
+ std::snprintf(buf, sizeof(buf),
+ "\\u%04x",
+ static_cast<unsigned>(
+ static_cast<unsigned char>(c)));
+ body.append(buf);
+ }
+ else
+ {
+ body.push_back(c);
+ }
+ break;
}
}🤖 Prompt for AI Agents
In `@src/server/serve_index.cpp` around lines 230 - 244, The JSON string escaping
in the loop that builds body (see the loop iterating over e.name and calls like
body.append and body.push_back) only handles '"', '\\', '\n', '\r', '\t' and
leaves other control characters U+0000–U+001F unescaped; update that loop to
escape all control characters per RFC 8259 by mapping '\b' and '\f' to "\\b" and
"\\f" and encoding any other control codepoints (<= 0x1F) as "\\u00XX"
(hex-escaped) before appending to body so the generated JSON is always valid.
| std::vector<dir_entry> entries; | ||
| { | ||
| std::filesystem::directory_iterator it(path, fec); | ||
| if(fec) | ||
| co_return route_next; | ||
|
|
||
| for(auto const& de : | ||
| std::filesystem::directory_iterator(path, fec)) | ||
| { | ||
| auto name = de.path().filename().string(); | ||
|
|
||
| // Skip hidden files unless configured | ||
| if(! impl_->opts.hidden && | ||
| ! name.empty() && name[0] == '.') | ||
| continue; | ||
|
|
||
| dir_entry e; | ||
| e.name = std::move(name); | ||
|
|
||
| std::error_code sec; | ||
| e.is_dir = de.is_directory(sec); | ||
| if(! e.is_dir) | ||
| e.size = de.file_size(sec); | ||
| auto lwt = de.last_write_time(sec); | ||
| if(! sec) | ||
| e.mtime = to_epoch(lwt); | ||
|
|
||
| entries.push_back(std::move(e)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: unused directory_iterator — directory is iterated twice.
it is constructed at line 368 but never used. A second directory_iterator is created at line 373 for the actual loop. This means the directory is opened twice (once to check for errors, once to iterate), and the error code from the second open is not checked before entering the loop.
🐛 Proposed fix: use a single iterator
// Read directory entries
std::vector<dir_entry> entries;
{
std::filesystem::directory_iterator it(path, fec);
if(fec)
co_return route_next;
- for(auto const& de :
- std::filesystem::directory_iterator(path, fec))
+ for(auto const& de : it)
{📝 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.
| std::vector<dir_entry> entries; | |
| { | |
| std::filesystem::directory_iterator it(path, fec); | |
| if(fec) | |
| co_return route_next; | |
| for(auto const& de : | |
| std::filesystem::directory_iterator(path, fec)) | |
| { | |
| auto name = de.path().filename().string(); | |
| // Skip hidden files unless configured | |
| if(! impl_->opts.hidden && | |
| ! name.empty() && name[0] == '.') | |
| continue; | |
| dir_entry e; | |
| e.name = std::move(name); | |
| std::error_code sec; | |
| e.is_dir = de.is_directory(sec); | |
| if(! e.is_dir) | |
| e.size = de.file_size(sec); | |
| auto lwt = de.last_write_time(sec); | |
| if(! sec) | |
| e.mtime = to_epoch(lwt); | |
| entries.push_back(std::move(e)); | |
| } | |
| } | |
| std::vector<dir_entry> entries; | |
| { | |
| std::filesystem::directory_iterator it(path, fec); | |
| if(fec) | |
| co_return route_next; | |
| for(auto const& de : it) | |
| { | |
| auto name = de.path().filename().string(); | |
| // Skip hidden files unless configured | |
| if(! impl_->opts.hidden && | |
| ! name.empty() && name[0] == '.') | |
| continue; | |
| dir_entry e; | |
| e.name = std::move(name); | |
| std::error_code sec; | |
| e.is_dir = de.is_directory(sec); | |
| if(! e.is_dir) | |
| e.size = de.file_size(sec); | |
| auto lwt = de.last_write_time(sec); | |
| if(! sec) | |
| e.mtime = to_epoch(lwt); | |
| entries.push_back(std::move(e)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/server/serve_index.cpp` around lines 366 - 395, The code constructs a
std::filesystem::directory_iterator named it but then creates a second iterator
in the for loop, causing the directory to be opened twice and the error code
from the second open to be ignored; replace the second iterator with the
already-created it (and use it in the for loop) so you only open the directory
once and check fec before iterating, keeping the existing behavior around
skipping hidden files, populating dir_entry (e.name, e.is_dir, e.size, e.mtime)
and pushing into entries.
| // Determine ".." display | ||
| std::filesystem::path root_canonical(impl_->root); | ||
| std::filesystem::path dir_canonical(path); | ||
| { | ||
| std::error_code ec2; | ||
| root_canonical = | ||
| std::filesystem::canonical(root_canonical, ec2); | ||
| dir_canonical = | ||
| std::filesystem::canonical(dir_canonical, ec2); | ||
| } | ||
| bool show_up = impl_->opts.show_parent && | ||
| dir_canonical != root_canonical; |
There was a problem hiding this comment.
Canonicalization silently ignores errors on both paths.
If std::filesystem::canonical fails for root_canonical (e.g., the root was deleted between the directory check and here), ec2 is set but ignored, and the subsequent dir_canonical call reuses the same ec2 variable. The comparison dir_canonical != root_canonical at line 410 then operates on potentially non-canonical paths, which could incorrectly show or hide the parent link.
Consider checking ec2 and falling back to show_parent = false (or true) on failure.
🤖 Prompt for AI Agents
In `@src/server/serve_index.cpp` around lines 399 - 410, The canonicalization
block for root_canonical and dir_canonical silently ignores errors via a reused
ec2, which can leave non-canonical paths and make the show_up decision
incorrect; update the code in serve_index.cpp to use separate std::error_code
variables for each std::filesystem::canonical call (e.g., ec_root and ec_dir),
check each error after its call, and if either canonicalization fails set
show_up = false (or otherwise short-circuit the parent-link logic) before
comparing dir_canonical and root_canonical; ensure you still respect
impl_->opts.show_parent when deciding the final value of show_up.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@include/boost/http/server/detail/dynamic_invoke.hpp`:
- Around line 130-140: The single-arg invoke_impl (template<class First>)
wrongly assumes f(p) yields route_task; update it to mirror the multi-arg
overload's return-type dispatch: compute the callable's return type for
invocation with route_params& (use std::invoke_result_t or equivalent) and use
if constexpr to detect whether it is route_task or route_result, returning f(p)
directly for route_task and returning wrap_result(f(p)) when the callable
returns route_result; keep the existing static_assert on First accepting
route_params& and keep calling f(p) otherwise.
In `@include/boost/http/server/router.hpp`:
- Around line 629-634: The converting and default constructors for router are
currently marked unconditional noexcept but may call HT move/default
constructors (ht_ or ht_(std::move(other.ht_))) which can throw; change those
noexcept specifiers to conditional ones: for the converting constructor make it
noexcept when HT is nothrow-constructible from OtherHT and detail::router_base
is nothrow-move-constructible, and for the default constructor make it noexcept
when HT is nothrow-default-constructible and detail::router_base is
nothrow-move-constructible (use the std::is_nothrow_* traits with HT, OtherHT
and detail::router_base to implement these conditions).
- Around line 523-547: The template assign<I, H1, HN...> can SFINAE-fail when HT
(T) is not invocable with H1 because the decltype in the second constexpr is
instantiated unconditionally; fix by guarding that branch with an invocability
check (e.g. std::is_invocable_v or std::invocable) so the decltype is only
considered if T const& is callable with H1, and add an explicit fallback branch
for direct plain handlers (the case where detail::returns_route_task<H1, P&,
...> is true but HT is not invocable) that assigns v[I] =
make_handler(std::forward<H1>(h1)); ensure you update the conditions around
detail::returns_route_task, the decltype check, and the fallback so assign,
make_handler, ht, and the detail::returns_route_task checks behave correctly
without causing hard errors.
🧹 Nitpick comments (1)
include/boost/http/server/detail/dynamic_invoke.hpp (1)
13-18: Consider adding a high-level overview comment after the includes.Per coding guidelines, files with non-trivial implementation logic should include a
/* */block comment after the includes providing a high-level overview. A brief note explaining the dependency-injection-style dispatch pattern (resolve callable args from a polystore, returnroute_nexton miss) would help future maintainers. 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."
| private: | ||
| template<std::size_t I, class H1, class... HN> | ||
| void assign(H1&& h1, HN&&... hn) | ||
| { | ||
| if constexpr ( | ||
| detail::returns_route_task< | ||
| H1, P&, system::error_code> || | ||
| detail::returns_route_task< | ||
| H1, P&, std::exception_ptr>) | ||
| { | ||
| v[I] = make_handler(std::forward<H1>(h1)); | ||
| } | ||
| else if constexpr (detail::returns_route_task< | ||
| decltype(std::declval<T const&>()(std::declval<H1>())), P&>) | ||
| { | ||
| v[I] = make_handler(ht(std::forward<H1>(h1))); | ||
| } | ||
| assign<I+1>(std::forward<HN>(hn)...); | ||
| } | ||
|
|
||
| template<std::size_t> | ||
| void assign(int = 0) | ||
| { | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing branch for direct plain handlers causes hard compile error with custom transforms.
When HT is a custom transform (not identity), a handler that already satisfies route_task(P&) directly will skip the error/exception branch (line 527) and reach line 535. There, decltype(std::declval<T const&>()(std::declval<H1>())) is a hard error if HT is not invocable with H1 — if constexpr does not make the decltype SFINAE-friendly.
This contradicts handler_kind (line 414), which correctly classifies direct plain handlers before checking the transform, so handler_check static asserts pass but instantiation of assign fails.
Proposed fix — guard invocability and add a direct-plain fallback
template<std::size_t I, class H1, class... HN>
void assign(H1&& h1, HN&&... hn)
{
if constexpr (
detail::returns_route_task<
H1, P&, system::error_code> ||
detail::returns_route_task<
H1, P&, std::exception_ptr>)
{
v[I] = make_handler(std::forward<H1>(h1));
}
- else if constexpr (detail::returns_route_task<
- decltype(std::declval<T const&>()(std::declval<H1>())), P&>)
+ else if constexpr (
+ std::is_invocable_v<T const&, H1> &&
+ detail::returns_route_task<
+ std::invoke_result_t<T const&, H1>, P&>)
{
v[I] = make_handler(ht(std::forward<H1>(h1)));
}
+ else if constexpr (detail::returns_route_task<H1, P&>)
+ {
+ v[I] = make_handler(std::forward<H1>(h1));
+ }
+ else
+ {
+ static_assert(sizeof(H1) == 0,
+ "handler has no valid dispatch path");
+ }
assign<I+1>(std::forward<HN>(hn)...);
}🤖 Prompt for AI Agents
In `@include/boost/http/server/router.hpp` around lines 523 - 547, The template
assign<I, H1, HN...> can SFINAE-fail when HT (T) is not invocable with H1
because the decltype in the second constexpr is instantiated unconditionally;
fix by guarding that branch with an invocability check (e.g. std::is_invocable_v
or std::invocable) so the decltype is only considered if T const& is callable
with H1, and add an explicit fallback branch for direct plain handlers (the case
where detail::returns_route_task<H1, P&, ...> is true but HT is not invocable)
that assigns v[I] = make_handler(std::forward<H1>(h1)); ensure you update the
conditions around detail::returns_route_task, the decltype check, and the
fallback so assign, make_handler, ht, and the detail::returns_route_task checks
behave correctly without causing hard errors.
| router( | ||
| router<OtherP, OtherHT>&& other) noexcept | ||
| : detail::router_base(std::move(other)) | ||
| , ht_(std::move(other.ht_)) | ||
| { | ||
| } |
There was a problem hiding this comment.
Unconditional noexcept may call std::terminate if HT construction throws.
Line 630 and 642: both converting constructors are marked noexcept, but ht_(std::move(other.ht_)) (move-constructing HT from OtherHT) and default-initializing HT are not guaranteed noexcept. If either throws, the program terminates.
Consider using a conditional noexcept:
noexcept(std::is_nothrow_constructible_v<HT, OtherHT>
&& std::is_nothrow_move_constructible_v<detail::router_base>)🤖 Prompt for AI Agents
In `@include/boost/http/server/router.hpp` around lines 629 - 634, The converting
and default constructors for router are currently marked unconditional noexcept
but may call HT move/default constructors (ht_ or ht_(std::move(other.ht_)))
which can throw; change those noexcept specifiers to conditional ones: for the
converting constructor make it noexcept when HT is nothrow-constructible from
OtherHT and detail::router_base is nothrow-move-constructible, and for the
default constructor make it noexcept when HT is nothrow-default-constructible
and detail::router_base is nothrow-move-constructible (use the std::is_nothrow_*
traits with HT, OtherHT and detail::router_base to implement these conditions).
2 participants
Summary by CodeRabbit
New Features
API Changes
Removals
Bug Fixes