From 1ac7b4c35e70fda18d0f7be609b06b3d94fa7d8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Tue, 16 Sep 2025 17:37:01 +0300 Subject: [PATCH] treewide: move away from accessing httpd::request::query_parameters Acecssing this member directly is deprecated, migrate code to use {get,set}_query_param() and friends instead. Fixes: https://github.com/scylladb/scylladb/issues/26023 --- api/column_family.cc | 2 +- api/raft.cc | 6 +-- api/storage_service.cc | 19 ++++----- api/task_manager.cc | 16 ++++---- api/task_manager_test.cc | 29 ++++++-------- test/boost/aws_error_injection_test.cc | 4 +- test/boost/encryption_at_rest_test.cc | 6 +-- tools/scylla-nodetool.cc | 6 ++- utils/s3/client.cc | 40 +++++++++---------- .../sts_assume_role_credentials_provider.cc | 10 ++--- 10 files changed, 66 insertions(+), 72 deletions(-) diff --git a/api/column_family.cc b/api/column_family.cc index 59569d52ea..4ba2ad28e7 100644 --- a/api/column_family.cc +++ b/api/column_family.cc @@ -1066,7 +1066,7 @@ void set_column_family(http_context& ctx, routes& r, sharded& }); cf::force_major_compaction.set(r, [&ctx, &db](std::unique_ptr req) -> future { - if (req->query_parameters.contains("split_output")) { + if (!req->get_query_param("split_output").empty()) { fail(unimplemented::cause::API); } auto [ks, cf] = parse_fully_qualified_cf_name(req->get_path_param("name")); diff --git a/api/raft.cc b/api/raft.cc index 2ac4b83452..5edda908ca 100644 --- a/api/raft.cc +++ b/api/raft.cc @@ -71,7 +71,7 @@ void set_raft(http_context&, httpd::routes& r, sharded req) -> future { - if (!req->query_parameters.contains("group_id")) { + if (req->get_query_param("group_id").empty()) { const auto leader_id = co_await raft_gr.invoke_on(0, [] (service::raft_group_registry& raft_gr) { auto& srv = raft_gr.group0(); return srv.current_leader(); @@ -100,7 +100,7 @@ void set_raft(http_context&, httpd::routes& r, sharded req) -> future { auto timeout = get_request_timeout(*req); - if (!req->query_parameters.contains("group_id")) { + if (req->get_query_param("group_id").empty()) { // Read barrier on group 0 by default co_await raft_gr.invoke_on(0, [timeout] (service::raft_group_registry& raft_gr) -> future<> { co_await raft_gr.group0_with_timeouts().read_barrier(nullptr, timeout); @@ -131,7 +131,7 @@ void set_raft(http_context&, httpd::routes& r, shardedquery_parameters.contains("group_id")) { + if (req->get_query_param("group_id").empty()) { // Stepdown on group 0 by default co_await raft_gr.invoke_on(0, [timeout_dur] (service::raft_group_registry& raft_gr) { apilog.info("Triggering stepdown for group0"); diff --git a/api/storage_service.cc b/api/storage_service.cc index acf482623f..2ccdcc1be2 100644 --- a/api/storage_service.cc +++ b/api/storage_service.cc @@ -174,9 +174,7 @@ std::vector parse_table_infos(const sstring& ks_name, const http_con std::pair> parse_table_infos(const http_context& ctx, const http::request& req, sstring cf_param_name) { auto keyspace = validate_keyspace(ctx, req); - const auto& query_params = req.query_parameters; - auto it = query_params.find(cf_param_name); - auto tis = parse_table_infos(keyspace, ctx, it != query_params.end() ? it->second : ""); + auto tis = parse_table_infos(keyspace, ctx, req.get_query_param(cf_param_name)); return std::make_pair(std::move(keyspace), std::move(tis)); } @@ -331,7 +329,7 @@ void set_repair(http_context& ctx, routes& r, sharded& repair, s // Nodetool still sends those unsupported options. Ignore them to avoid failing nodetool repair. static std::unordered_set legacy_options_to_ignore = {"pullRepair", "ignoreUnreplicatedKeyspaces"}; - for (auto& x : req->query_parameters) { + for (auto& x : req->get_query_params()) { if (legacy_options_to_ignore.contains(x.first)) { continue; } @@ -574,9 +572,8 @@ rest_toppartitions_generic(http_context& ctx, std::unique_ptr req bool filters_provided = false; std::unordered_set, utils::tuple_hash> table_filters {}; - if (req->query_parameters.contains("table_filters")) { + if (auto filters = req->get_query_param("table_filters"); !filters.empty()) { filters_provided = true; - auto filters = req->get_query_param("table_filters"); std::stringstream ss { filters }; std::string filter; while (!filters.empty() && ss.good()) { @@ -586,9 +583,8 @@ rest_toppartitions_generic(http_context& ctx, std::unique_ptr req } std::unordered_set keyspace_filters {}; - if (req->query_parameters.contains("keyspace_filters")) { + if (auto filters = req->get_query_param("keyspace_filters"); !filters.empty()) { filters_provided = true; - auto filters = req->get_query_param("keyspace_filters"); std::stringstream ss { filters }; std::string filter; while (!filters.empty() && ss.good()) { @@ -1676,8 +1672,7 @@ rest_drop_quarantined_sstables(http_context& ctx, shardedquery_parameters.find("tables"); - auto table_infos = parse_table_infos(keyspace, ctx, it != req->query_parameters.end() ? it->second : ""); + auto table_infos = parse_table_infos(keyspace, ctx, req->get_query_param("tables")); co_await ctx.db.invoke_on_all([&table_infos](replica::database& db) -> future<> { return parallel_for_each(table_infos, [&db](const auto& table) -> future<> { @@ -1933,7 +1928,7 @@ void set_snapshot(http_context& ctx, routes& r, sharded& snap_ }); ss::take_snapshot.set(r, [&snap_ctl](std::unique_ptr req) -> future { - apilog.info("take_snapshot: {}", req->query_parameters); + apilog.info("take_snapshot: {}", req->get_query_params()); auto tag = req->get_query_param("tag"); auto column_families = split(req->get_query_param("cf"), ","); auto sfopt = req->get_query_param("sf"); @@ -1960,7 +1955,7 @@ void set_snapshot(http_context& ctx, routes& r, sharded& snap_ }); ss::del_snapshot.set(r, [&snap_ctl](std::unique_ptr req) -> future { - apilog.info("del_snapshot: {}", req->query_parameters); + apilog.info("del_snapshot: {}", req->get_query_params()); auto tag = req->get_query_param("tag"); auto column_family = req->get_query_param("cf"); diff --git a/api/task_manager.cc b/api/task_manager.cc index ba2772bd6b..4577abc5b4 100644 --- a/api/task_manager.cc +++ b/api/task_manager.cc @@ -107,11 +107,11 @@ void set_task_manager(http_context& ctx, routes& r, sharded throw bad_param_exception(fmt::format("{}", std::current_exception())); } - if (auto it = req->query_parameters.find("keyspace"); it != req->query_parameters.end()) { - keyspace = it->second; + if (auto param = req->get_query_param("keyspace"); !param.empty()) { + keyspace = param; } - if (auto it = req->query_parameters.find("table"); it != req->query_parameters.end()) { - table = it->second; + if (auto param = req->get_query_param("table"); !param.empty()) { + table = param; } return module->get_stats(internal, [keyspace = std::move(keyspace), table = std::move(table)] (std::string& ks, std::string& t) { @@ -175,8 +175,8 @@ void set_task_manager(http_context& ctx, routes& r, sharded auto id = tasks::task_id{utils::UUID{req->get_path_param("task_id")}}; tasks::task_status status; std::optional timeout = std::nullopt; - if (auto it = req->query_parameters.find("timeout"); it != req->query_parameters.end()) { - timeout = std::chrono::seconds(boost::lexical_cast(it->second)); + if (auto param = req->get_query_param("timeout"); !param.empty()) { + timeout = std::chrono::seconds(boost::lexical_cast(param)); } try { auto task = tasks::task_handler{tm.local(), id}; @@ -217,7 +217,7 @@ void set_task_manager(http_context& ctx, routes& r, sharded tm::get_and_update_ttl.set(r, [&cfg] (std::unique_ptr req) -> future { uint32_t ttl = cfg.task_ttl_seconds(); try { - co_await cfg.task_ttl_seconds.set_value_on_all_shards(req->query_parameters["ttl"], utils::config_file::config_source::API); + co_await cfg.task_ttl_seconds.set_value_on_all_shards(req->get_query_param("ttl"), utils::config_file::config_source::API); } catch (...) { throw bad_param_exception(fmt::format("{}", std::current_exception())); } @@ -232,7 +232,7 @@ void set_task_manager(http_context& ctx, routes& r, sharded tm::get_and_update_user_ttl.set(r, [&cfg] (std::unique_ptr req) -> future { uint32_t user_ttl = cfg.user_task_ttl_seconds(); try { - co_await cfg.user_task_ttl_seconds.set_value_on_all_shards(req->query_parameters["user_ttl"], utils::config_file::config_source::API); + co_await cfg.user_task_ttl_seconds.set_value_on_all_shards(req->get_query_param("user_ttl"), utils::config_file::config_source::API); } catch (...) { throw bad_param_exception(fmt::format("{}", std::current_exception())); } diff --git a/api/task_manager_test.cc b/api/task_manager_test.cc index 44046d48b6..b23dc93467 100644 --- a/api/task_manager_test.cc +++ b/api/task_manager_test.cc @@ -57,20 +57,16 @@ void set_task_manager_test(http_context& ctx, routes& r, sharded req) -> future { sharded& tms = tm; - auto it = req->query_parameters.find("task_id"); - auto id = it != req->query_parameters.end() ? tasks::task_id{utils::UUID{it->second}} : tasks::task_id::create_null_id(); - it = req->query_parameters.find("shard"); - unsigned shard = it != req->query_parameters.end() ? boost::lexical_cast(it->second) : 0; - it = req->query_parameters.find("keyspace"); - std::string keyspace = it != req->query_parameters.end() ? it->second : ""; - it = req->query_parameters.find("table"); - std::string table = it != req->query_parameters.end() ? it->second : ""; - it = req->query_parameters.find("entity"); - std::string entity = it != req->query_parameters.end() ? it->second : ""; - it = req->query_parameters.find("parent_id"); + const auto id_param = req->get_query_param("task_id"); + auto id = !id_param.empty() ? tasks::task_id{utils::UUID{id_param}} : tasks::task_id::create_null_id(); + const auto shard_param = req->get_query_param("shard"); + unsigned shard = shard_param.empty() ? 0 : boost::lexical_cast(shard_param); + std::string keyspace = req->get_query_param("keyspace"); + std::string table = req->get_query_param("table"); + std::string entity = req->get_query_param("entity"); tasks::task_info data; - if (it != req->query_parameters.end()) { - data.id = tasks::task_id{utils::UUID{it->second}}; + if (auto parent_id = req->get_query_param("parent_id"); !parent_id.empty()) { + data.id = tasks::task_id{utils::UUID{parent_id}}; auto parent_ptr = co_await tasks::task_manager::lookup_task_on_all_shards(tm, data.id); data.shard = parent_ptr->get_status().shard; } @@ -88,7 +84,7 @@ void set_task_manager_test(http_context& ctx, routes& r, sharded req) -> future { - auto id = tasks::task_id{utils::UUID{req->query_parameters["task_id"]}}; + auto id = tasks::task_id{utils::UUID{req->get_query_param("task_id")}}; try { co_await tasks::task_manager::invoke_on_task(tm, id, [] (tasks::task_manager::task_variant task_v, tasks::virtual_task_hint) -> future<> { return std::visit(overloaded_functor{ @@ -109,9 +105,8 @@ void set_task_manager_test(http_context& ctx, routes& r, sharded req) -> future { auto id = tasks::task_id{utils::UUID{req->get_path_param("task_id")}}; - auto it = req->query_parameters.find("error"); - bool fail = it != req->query_parameters.end(); - std::string error = fail ? it->second : ""; + std::string error = req->get_query_param("error"); + bool fail = !error.empty(); try { co_await tasks::task_manager::invoke_on_task(tm, id, [fail, error = std::move(error)] (tasks::task_manager::task_variant task_v, tasks::virtual_task_hint) -> future<> { diff --git a/test/boost/aws_error_injection_test.cc b/test/boost/aws_error_injection_test.cc index b132a597cc..af37fdcdc6 100644 --- a/test/boost/aws_error_injection_test.cc +++ b/test/boost/aws_error_injection_test.cc @@ -51,8 +51,8 @@ static void register_policy(const std::string& key, failure_policy policy) { auto close_client = deferred_close(cln); auto req = http::request::make("PUT", get_address(), "/"); req._headers["Content-Length"] = "0"; - req.query_parameters["Key"] = key; - req.query_parameters["Policy"] = std::to_string(std::to_underlying(policy)); + req.set_query_param("Key", key); + req.set_query_param("Policy", std::to_string(std::to_underlying(policy))); cln.make_request(std::move(req), [](const http::reply&, input_stream&&) -> future<> { return seastar::make_ready_future(); }).get(); } diff --git a/test/boost/encryption_at_rest_test.cc b/test/boost/encryption_at_rest_test.cc index 028f4c0f1c..3daf7f647f 100644 --- a/test/boost/encryption_at_rest_test.cc +++ b/test/boost/encryption_at_rest_test.cc @@ -1807,9 +1807,9 @@ static future<> configure_azure_mock_server(const std::string& host, const unsig auto close_client = deferred_close(cln); auto req = http::request::make("POST", host, "/config/error"); req._headers["Content-Length"] = "0"; - req.query_parameters["service"] = service; - req.query_parameters["error_type"] = error_type; - req.query_parameters["repeat"] = std::to_string(repeat); + req.set_query_param("service", service); + req.set_query_param("error_type", error_type); + req.set_query_param("repeat", std::to_string(repeat)); co_await cln.make_request(std::move(req), [](const http::reply&, input_stream&&) -> future<> { return seastar::make_ready_future(); }); } diff --git a/tools/scylla-nodetool.cc b/tools/scylla-nodetool.cc index 1eb29abbb3..57218be95f 100644 --- a/tools/scylla-nodetool.cc +++ b/tools/scylla-nodetool.cc @@ -163,7 +163,11 @@ class scylla_rest_client { std::optional body = {}) { auto req = http::request::make(type, _host_name, path); auto url = req.get_url(); - req.query_parameters = params; + + for (const auto& [k, v] : params) { + req.set_query_param(k, v); + } + if (body) { req.write_body(body->content_type, body->content); } diff --git a/utils/s3/client.cc b/utils/s3/client.cc index 3b1c0674dc..5f1d668012 100644 --- a/utils/s3/client.cc +++ b/utils/s3/client.cc @@ -181,8 +181,8 @@ future<> client::authorize(http::request& req) { } sstring query_string = ""; std::map query_parameters; - for (const auto& q : req.query_parameters) { - query_parameters[q.first] = q.second; + for (const auto& q : req.get_query_params()) { + query_parameters[q.first] = q.second.back(); } unsigned query_nr = query_parameters.size(); for (const auto& q : query_parameters) { @@ -395,7 +395,7 @@ static tag_set parse_tagging(sstring& body) { future client::get_object_tagging(sstring object_name, seastar::abort_source* as) { // see https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObjectTagging.html auto req = http::request::make("GET", _host, object_name); - req.query_parameters["tagging"] = ""; + req.set_query_param("tagging", ""); s3l.trace("GET {} tagging", object_name); tag_set tags; co_await make_request(std::move(req), @@ -418,7 +418,7 @@ static auto dump_tagging(const tag_set& tags) { future<> client::put_object_tagging(sstring object_name, tag_set tagging, seastar::abort_source* as) { // see https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObjectTagging.html auto req = http::request::make("PUT", _host, object_name); - req.query_parameters["tagging"] = ""; + req.set_query_param("tagging", ""); s3l.trace("PUT {} tagging", object_name); auto body = dump_tagging(tagging); size_t body_size = body.size(); @@ -442,7 +442,7 @@ future<> client::put_object_tagging(sstring object_name, tag_set tagging, seasta future<> client::delete_object_tagging(sstring object_name, seastar::abort_source* as) { // see https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObjectTagging.html auto req = http::request::make("DELETE", _host, object_name); - req.query_parameters["tagging"] = ""; + req.set_query_param("tagging", ""); s3l.trace("DELETE {} tagging", object_name); co_await make_request(std::move(req), ignore_reply, http::reply::status_type::no_content, as); } @@ -659,8 +659,8 @@ private: s3l.trace("PUT part {}, Upload range: {}, Upload ID:", part_number, range, _upload_id); req._headers["x-amz-copy-source-range"] = range; - req.query_parameters.emplace("partNumber", to_sstring(part_number + 1)); - req.query_parameters.emplace("uploadId", _upload_id); + req.set_query_param("partNumber", to_sstring(part_number + 1)); + req.set_query_param("uploadId", _upload_id); // upload the parts in the background for better throughput auto gh = _bg_flushes.hold(); @@ -791,7 +791,7 @@ future<> dump_multipart_upload_parts(output_stream out, const utils::chunk future<> client::multipart_upload::start_upload() { s3l.trace("POST uploads {} (tag {})", _object_name, seastar::value_of([this] { return _tag ? _tag->key + "=" + _tag->value : "none"; })); auto rep = http::request::make("POST", _client->_host, _object_name); - rep.query_parameters["uploads"] = ""; + rep.set_query_param("uploads", ""); if (_tag) { rep._headers["x-amz-tagging"] = seastar::format("{}={}", _tag->key, _tag->value); } @@ -819,8 +819,8 @@ future<> client::multipart_upload::upload_part(memory_data_sink_buffers bufs) { auto req = http::request::make("PUT", _client->_host, _object_name); auto size = bufs.size(); req._headers["Content-Length"] = seastar::format("{}", size); - req.query_parameters["partNumber"] = seastar::format("{}", part_number + 1); - req.query_parameters["uploadId"] = _upload_id; + req.set_query_param("partNumber", seastar::format("{}", part_number + 1)); + req.set_query_param("uploadId", _upload_id); req.write_body("bin", size, [this, part_number, bufs = std::move(bufs), p = std::move(claim)] (output_stream&& out_) mutable -> future<> { auto out = std::move(out_); std::exception_ptr ex; @@ -868,7 +868,7 @@ future<> client::multipart_upload::upload_part(memory_data_sink_buffers bufs) { future<> client::multipart_upload::abort_upload() { s3l.trace("DELETE upload {}", _upload_id); auto req = http::request::make("DELETE", _client->_host, _object_name); - req.query_parameters["uploadId"] = std::exchange(_upload_id, ""); // now upload_started() returns false + req.set_query_param("uploadId", std::exchange(_upload_id, "")); // now upload_started() returns false co_await _client->make_request(std::move(req), ignore_reply, http::reply::status_type::no_content) .handle_exception([this](const std::exception_ptr& ex) -> future<> { // Here we discard whatever exception is thrown when aborting multipart upload since we don't care about cleanly aborting it since there are other @@ -889,7 +889,7 @@ future<> client::multipart_upload::finalize_upload() { s3l.trace("POST upload completion {} parts (upload id {})", _part_etags.size(), _upload_id); auto req = http::request::make("POST", _client->_host, _object_name); - req.query_parameters["uploadId"] = _upload_id; + req.set_query_param("uploadId", _upload_id); req.write_body("xml", parts_xml_len, [this] (output_stream&& out) -> future<> { return dump_multipart_upload_parts(std::move(out), _part_etags); }); @@ -994,8 +994,8 @@ future<> client::multipart_upload::upload_part(std::unique_ptr piec _part_etags.emplace_back(); s3l.trace("PUT part {} from {} (upload id {})", part_number, piece._object_name, _upload_id); auto req = http::request::make("PUT", _client->_host, _object_name); - req.query_parameters["partNumber"] = format("{}", part_number + 1); - req.query_parameters["uploadId"] = _upload_id; + req.set_query_param("partNumber", format("{}", part_number + 1)); + req.set_query_param("uploadId", _upload_id); req._headers["x-amz-copy-source"] = piece._object_name; // See comment in upload_part(memory_data_sink_buffers) overload regarding the @@ -1462,8 +1462,8 @@ class client::do_upload_file : private multipart_upload { _part_etags.emplace_back(); auto req = http::request::make("PUT", _client->_host, _object_name); req._headers["Content-Length"] = to_sstring(part_size); - req.query_parameters.emplace("partNumber", to_sstring(part_number + 1)); - req.query_parameters.emplace("uploadId", _upload_id); + req.set_query_param("partNumber", to_sstring(part_number + 1)); + req.set_query_param("uploadId", _upload_id); s3l.trace("PUT part {}, {} bytes (upload id {})", part_number, part_size, _upload_id); req.write_body("bin", part_size, [f=std::move(f), mem_units=std::move(mem_units), offset, part_size, &progress = _progress] (output_stream&& out_) { auto input = make_file_input_stream(f, offset, part_size, input_stream_options()); @@ -1805,13 +1805,13 @@ future<> client::bucket_lister::start_listing() { do { s3l.trace("GET /?list-type=2 (prefix={})", _prefix); auto req = http::request::make("GET", _client->_host, format("/{}", _bucket)); - req.query_parameters.emplace("list-type", "2"); - req.query_parameters.emplace("max-keys", _max_keys); + req.set_query_param("list-type", "2"); + req.set_query_param("max-keys", _max_keys); if (!continuation_token.empty()) { - req.query_parameters.emplace("continuation-token", std::exchange(continuation_token, "")); + req.set_query_param("continuation-token", std::exchange(continuation_token, "")); } if (!_prefix.empty()) { - req.query_parameters.emplace("prefix", _prefix); + req.set_query_param("prefix", _prefix); } std::vector names; diff --git a/utils/s3/credentials_providers/sts_assume_role_credentials_provider.cc b/utils/s3/credentials_providers/sts_assume_role_credentials_provider.cc index ef6f2540b1..7608ebcd2d 100644 --- a/utils/s3/credentials_providers/sts_assume_role_credentials_provider.cc +++ b/utils/s3/credentials_providers/sts_assume_role_credentials_provider.cc @@ -36,11 +36,11 @@ future<> sts_assume_role_credentials_provider::update_credentials() { auto req = http::request::make("POST", sts_host, "/"); // Just set this version // https://github.com/aws/aws-sdk-cpp/blob/8d68be52dcad85095753e069a4355e241f1edb1c/generated/src/aws-cpp-sdk-sts/source/model/AssumeRoleRequest.cpp#L143 - req.query_parameters["Version"] = "2011-06-15"; - req.query_parameters["DurationSeconds"] = format("{}", session_duration); - req.query_parameters["Action"] = "AssumeRole"; - req.query_parameters["RoleSessionName"] = format("{}", utils::make_random_uuid()); - req.query_parameters["RoleArn"] = role_arn; + req.set_query_param("Version", "2011-06-15"); + req.set_query_param("DurationSeconds", format("{}", session_duration)); + req.set_query_param("Action", "AssumeRole"); + req.set_query_param("RoleSessionName", format("{}", utils::make_random_uuid())); + req.set_query_param("RoleArn", role_arn); auto factory = std::make_unique(sts_host, port, is_secured, sts_logger); retryable_http_client http_client(std::move(factory), 1, retryable_http_client::ignore_exception, http::experimental::client::retry_requests::yes, retry_strategy); co_await http_client.make_request(