This is an automated email from the ASF dual-hosted git repository.
gehafearless pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-pegasus.git
The following commit(s) were added to refs/heads/master by this push:
new 2c9b5ed5d refactor(ddl_client): Simplify app_name validate code (#1931)
2c9b5ed5d is described below
commit 2c9b5ed5d95ad89e2cd50ac3e34f57149788b9ce
Author: Yingchun Lai <[email protected]>
AuthorDate: Thu Mar 7 14:17:46 2024 +0800
refactor(ddl_client): Simplify app_name validate code (#1931)
Add a new static function error_s validate_app_name(const std::string
&app_name)
to replication_ddl_client then it can be reused freely.
---
src/client/replication_ddl_client.cpp | 85 +++++++++++------------------------
src/client/replication_ddl_client.h | 2 +
src/client/test/ddl_client_test.cpp | 23 ++++++++++
src/utils/errors.h | 18 ++++++++
4 files changed, 68 insertions(+), 60 deletions(-)
diff --git a/src/client/replication_ddl_client.cpp
b/src/client/replication_ddl_client.cpp
index cbdb1ee92..15dc4fec6 100644
--- a/src/client/replication_ddl_client.cpp
+++ b/src/client/replication_ddl_client.cpp
@@ -70,17 +70,19 @@ DSN_DEFINE_uint32(ddl_client,
namespace dsn {
namespace replication {
-#define VALIDATE_TABLE_NAME(app_name)
\
- do {
\
- if (app_name.empty() ||
\
- !std::all_of(app_name.cbegin(),
\
- app_name.cend(),
\
- (bool
(*)(int))replication_ddl_client::valid_app_char)) \
- return FMT_ERR(ERR_INVALID_PARAMETERS, "Invalid name. Only
0-9a-zA-Z.:_ are valid!"); \
- } while (false)
-
using tp_output_format = ::dsn::utils::table_printer::output_format;
+error_s replication_ddl_client::validate_app_name(const std::string &app_name)
+{
+ if (app_name.empty() || !std::all_of(app_name.cbegin(), app_name.cend(),
[](const char c) {
+ return static_cast<bool>(std::isalnum(c)) || c == '_' || c == '.'
|| c == ':';
+ })) {
+ return FMT_ERR(ERR_INVALID_PARAMETERS, "Invalid name: Only
0-9a-zA-Z.:_ are valid.");
+ }
+
+ return error_s::ok();
+}
+
replication_ddl_client::replication_ddl_client(const
std::vector<dsn::rpc_address> &meta_servers)
{
_meta_server.assign_group("meta-servers");
@@ -164,21 +166,8 @@ dsn::error_code replication_ddl_client::create_app(const
std::string &app_name,
return ERR_INVALID_PARAMETERS;
}
- if (app_name.empty() ||
- !std::all_of(app_name.cbegin(),
- app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char)) {
- std::cout << "create app " << app_name << " failed: invalid app_name"
<< std::endl;
- return ERR_INVALID_PARAMETERS;
- }
-
- if (app_type.empty() ||
- !std::all_of(app_type.cbegin(),
- app_type.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char)) {
- std::cout << "create app " << app_name << " failed: invalid app_type"
<< std::endl;
- return ERR_INVALID_PARAMETERS;
- }
+ RETURN_EC_NOT_OK_MSG(validate_app_name(app_name), "invalid app_name:
'{}'", app_name);
+ RETURN_EC_NOT_OK_MSG(validate_app_name(app_type), "invalid app_type:
'{}'", app_type);
auto req = std::make_shared<configuration_create_app_request>();
req->app_name = app_name;
@@ -214,11 +203,7 @@ dsn::error_code replication_ddl_client::create_app(const
std::string &app_name,
dsn::error_code replication_ddl_client::drop_app(const std::string &app_name,
int reserve_seconds)
{
- if (app_name.empty() ||
- !std::all_of(app_name.cbegin(),
- app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char))
- return ERR_INVALID_PARAMETERS;
+ RETURN_EC_NOT_OK_MSG(validate_app_name(app_name), "invalid app_name:
'{}'", app_name);
auto req = std::make_shared<configuration_drop_app_request>();
req->app_name = app_name;
@@ -241,10 +226,8 @@ dsn::error_code replication_ddl_client::drop_app(const
std::string &app_name, in
dsn::error_code replication_ddl_client::recall_app(int32_t app_id, const
std::string &new_app_name)
{
- if (!std::all_of(new_app_name.cbegin(),
- new_app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char))
- return ERR_INVALID_PARAMETERS;
+ RETURN_EC_NOT_OK_MSG(
+ validate_app_name(new_app_name), "invalid new_app_name: '{}'",
new_app_name);
auto req = std::make_shared<configuration_recall_app_request>();
req->app_id = app_id;
@@ -824,11 +807,7 @@ dsn::error_code replication_ddl_client::list_app(const
std::string &app_name,
int32_t &partition_count,
std::vector<partition_configuration> &partitions)
{
- if (app_name.empty() ||
- !std::all_of(app_name.cbegin(),
- app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char))
- return ERR_INVALID_PARAMETERS;
+ RETURN_EC_NOT_OK_MSG(validate_app_name(app_name), "invalid app_name:
'{}'", app_name);
auto req = std::make_shared<query_cfg_request>();
req->app_name = app_name;
@@ -965,21 +944,10 @@ dsn::error_code replication_ddl_client::do_restore(const
std::string &backup_pro
bool skip_bad_partition,
const std::string
&restore_path)
{
- if (old_app_name.empty() ||
- !std::all_of(old_app_name.cbegin(),
- old_app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char)) {
- std::cout << "restore app " << old_app_name << " failed: invalid
old_app_name" << std::endl;
- return ERR_INVALID_PARAMETERS;
- }
-
- if (new_app_name.empty() ||
- !std::all_of(new_app_name.cbegin(),
- new_app_name.cend(),
- (bool (*)(int))replication_ddl_client::valid_app_char)) {
- std::cout << "restore app " << new_app_name << " failed: invalid
new_app_name" << std::endl;
- return ERR_INVALID_PARAMETERS;
- }
+ RETURN_EC_NOT_OK_MSG(
+ validate_app_name(old_app_name), "invalid old_app_name: '{}'",
old_app_name);
+ RETURN_EC_NOT_OK_MSG(
+ validate_app_name(new_app_name), "invalid new_app_name: '{}'",
new_app_name);
auto req = std::make_shared<configuration_restore_request>();
@@ -1396,11 +1364,6 @@ error_with<duplication_query_response>
replication_ddl_client::query_dup(std::st
return call_rpc_sync(duplication_query_rpc(std::move(req),
RPC_CM_QUERY_DUPLICATION));
}
-bool replication_ddl_client::valid_app_char(int c)
-{
- return (bool)std::isalnum(c) || c == '_' || c == '.' || c == ':';
-}
-
namespace {
bool need_retry(uint32_t attempt_count, const dsn::error_code &err)
@@ -1753,8 +1716,10 @@ replication_ddl_client::set_max_replica_count(const
std::string &app_name,
error_with<configuration_rename_app_response>
replication_ddl_client::rename_app(const std::string &old_app_name, const
std::string &new_app_name)
{
- VALIDATE_TABLE_NAME(old_app_name);
- VALIDATE_TABLE_NAME(new_app_name);
+ RETURN_ES_NOT_OK_MSG(
+ validate_app_name(old_app_name), "invalid old_app_name: '{}'",
old_app_name);
+ RETURN_ES_NOT_OK_MSG(
+ validate_app_name(new_app_name), "invalid new_app_name: '{}'",
new_app_name);
auto req = std::make_unique<configuration_rename_app_request>();
req->__set_old_app_name(old_app_name);
diff --git a/src/client/replication_ddl_client.h
b/src/client/replication_ddl_client.h
index 7a32dc851..38b94a6e1 100644
--- a/src/client/replication_ddl_client.h
+++ b/src/client/replication_ddl_client.h
@@ -265,6 +265,8 @@ public:
void set_max_wait_app_ready_secs(uint32_t max_wait_secs) { _max_wait_secs
= max_wait_secs; }
+ static error_s validate_app_name(const std::string &app_name);
+
private:
bool static valid_app_char(int c);
diff --git a/src/client/test/ddl_client_test.cpp
b/src/client/test/ddl_client_test.cpp
index 0659cf7b3..e0e6b9099 100644
--- a/src/client/test/ddl_client_test.cpp
+++ b/src/client/test/ddl_client_test.cpp
@@ -19,6 +19,7 @@
#include <stdint.h>
#include <deque>
#include <memory>
+#include <string>
#include <vector>
#include "client/replication_ddl_client.h"
@@ -30,8 +31,10 @@
#include "runtime/task/task.h"
#include "utils/autoref_ptr.h"
#include "utils/error_code.h"
+#include "utils/errors.h"
#include "utils/fail_point.h"
#include "utils/flags.h"
+#include "utils/fmt_logging.h"
DSN_DECLARE_uint32(ddl_client_max_attempt_count);
DSN_DECLARE_uint32(ddl_client_retry_interval_ms);
@@ -39,6 +42,26 @@ DSN_DECLARE_uint32(ddl_client_retry_interval_ms);
namespace dsn {
namespace replication {
+TEST(DDLClientTest, ValidateAppName)
+{
+ struct test_case
+ {
+ std::string app_name;
+ bool valid;
+ } tests[] = {{"", false},
+ {"abc!", false},
+ {"abc-", false},
+ {"abc@", false},
+ {"abc", true},
+ {"abc1", true},
+ {"abc_", true},
+ {"abc.", true},
+ {"abc:", true}};
+ for (const auto &test : tests) {
+ CHECK_EQ(test.valid,
replication_ddl_client::validate_app_name(test.app_name).is_ok());
+ }
+}
+
TEST(DDLClientTest, RetryMetaRequest)
{
const auto reserved_ddl_client_max_attempt_count =
FLAGS_ddl_client_max_attempt_count;
diff --git a/src/utils/errors.h b/src/utils/errors.h
index 342621151..17ee51092 100644
--- a/src/utils/errors.h
+++ b/src/utils/errors.h
@@ -233,6 +233,24 @@ USER_DEFINED_STRUCTURE_FORMATTER(::dsn::error_s);
}
\
} while (false)
+#define RETURN_ES_NOT_OK_MSG(s, ...)
\
+ do {
\
+ const ::dsn::error_s &_s = (s);
\
+ if (dsn_unlikely(!_s)) {
\
+ fmt::print(stderr, "{}: {}\n", _s.description(),
fmt::format(__VA_ARGS__)); \
+ return _s;
\
+ }
\
+ } while (false)
+
+#define RETURN_EC_NOT_OK_MSG(s, ...)
\
+ do {
\
+ const ::dsn::error_s &_s = (s);
\
+ if (dsn_unlikely(!_s)) {
\
+ fmt::print(stderr, "{}: {}\n", _s.description(),
fmt::format(__VA_ARGS__)); \
+ return _s.code();
\
+ }
\
+ } while (false)
+
#define CHECK_OK(s, ...)
\
do {
\
const ::dsn::error_s &_s = (s);
\
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
