Skip to content

bazel: ban boost::property_tree::get<ss::sstring> via force-include#30375

Draft
nvartolomei wants to merge 1 commit intoredpanda-data:devfrom
nvartolomei:nv/ban-ptree-sstring
Draft

bazel: ban boost::property_tree::get<ss::sstring> via force-include#30375
nvartolomei wants to merge 1 commit intoredpanda-data:devfrom
nvartolomei:nv/ban-ptree-sstring

Conversation

@nvartolomei
Copy link
Copy Markdown
Contributor

boost::property_tree::ptree::get<ss::sstring>(path, default) silently
returns the supplied default for non-empty trees because
seastar::sstring lacks a stream extractor; boost's stream_translator
falls through and the failure is invisible at both compile time and
runtime — the call site looks correct.

This PR makes the pattern a hard compile error.

What's wired

  • src/v/base/ptree_ban.h: forward-declares seastar::basic_sstring and
    partially specializes boost::property_tree::translator_between for
    it with a static_assert(false, …) carrying a pointed message.
  • //src/v/base:ptree_ban: header-only cc_library (uses the native
    rule directly to avoid the redpanda_cc_library macro recursing into
    itself).
  • bazel/internal.bzl: redpanda_copts() appends
    -include base/ptree_ban.h; new redpanda_implicit_deps() returns
    ["//src/v/base:ptree_ban"] so the header is visible under
    layering_check.
  • bazel/build.bzl + bazel/test.bzl: thread redpanda_implicit_deps()
    through redpanda_cc_library, redpanda_cc_binary,
    _redpanda_cc_test, _redpanda_cc_fuzz_test,
    redpanda_cc_btest_no_seastar, redpanda_test_cc_library, and
    redpanda_cc_bench.

Third-party cc_library targets are unaffected — only Redpanda's own
macros inject the force-include.

Diagnostic

ptree_ban.h:38:7: error: static assertion failed:
  boost::property_tree::ptree::get<seastar::sstring> silently returns
  the default for non-empty trees because seastar::sstring lacks a
  stream extractor. Use std::string and convert at the call site.
note: in instantiation of … translator_between<std::string,
      seastar::basic_sstring<char, unsigned int, 15>> requested here
src/v/cloud_storage_clients/abs_client.cc:122:26: note: in instantiation
  of … resp.get<ss::sstring>("Error.Code", "Unknown");

Existing breakage surfaced

This change does not compile against dev as-is — it surfaces what
appear to be real, latent silent failures in S3/Azure error parsing
that need follow-up fixes:

  • src/v/cloud_storage_clients/abs_client.cc:122-123
  • src/v/cloud_storage_clients/s3_client.cc:550-553, 1401-1404, 1837

Those call sites parse cloud error responses and have been resolving
Error.Code / Error.Message / Error.RequestId to the supplied
defaults ("Unknown" / "") regardless of what the server returned.
A follow-up PR will switch them to std::string and adapt the call
sites; this PR is filed as a draft to discuss the approach before that
follow-up lands.

Backports Required

  • none - not a bug fix

Release Notes

  • none

ptree::get<ss::sstring>(path, default) silently returns the default
for non-empty trees because seastar::sstring lacks a stream extractor;
boost's stream_translator falls through and the failure is invisible
at compile time and at runtime.

Force-include base/ptree_ban.h via redpanda_copts(), which partially
specializes boost::property_tree::translator_between for basic_sstring
with a static_assert. The new //src/v/base:ptree_ban target is wired
as an implicit dep of redpanda_cc_library / redpanda_cc_binary and the
test/bench rules so the header resolves under layering_check.

Existing call sites in cloud_storage_clients fail to build with this
change and need to be migrated to std::string in a follow-up.
@nvartolomei nvartolomei force-pushed the nv/ban-ptree-sstring branch from 14d2557 to 301a56b Compare May 6, 2026 18:09
@nvartolomei
Copy link
Copy Markdown
Contributor Author

/rp-unit-test
debug

@nvartolomei
Copy link
Copy Markdown
Contributor Author

/ci-repeat 1
debug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant