Skip to content

Commit

Permalink
alternator: optimize validate_table_name call
Browse files Browse the repository at this point in the history
Prior to this `table_name` was validated for every request in `find_table_name` leading to unnecessary overhead (although small, but unnecessary). Now, the `table_name` is only validated while creation reqeust and in other requests iff the table does not exist (to keep compatibility with DynamoDB's exception).

Fixes: scylladb#12538
  • Loading branch information
harsh020 committed May 23, 2023
1 parent 05be5e9 commit dcd01b2
Showing 1 changed file with 17 additions and 4 deletions.
21 changes: 17 additions & 4 deletions alternator/executor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -188,9 +188,8 @@ static std::string lsi_name(const std::string& table_name, std::string_view inde

/** Extract table name from a request.
* Most requests expect the table's name to be listed in a "TableName" field.
* This convenience function returns the name, with appropriate validation
* and api_error in case the table name is missing or not a string, or
* doesn't pass validate_table_name().
* This convenience function returns the name or api_error in case the
* table name is missing or not a string.
*/
static std::optional<std::string> find_table_name(const rjson::value& request) {
const rjson::value* table_name_value = rjson::find(request, "TableName");
Expand All @@ -201,7 +200,6 @@ static std::optional<std::string> find_table_name(const rjson::value& request) {
throw api_error::validation("Non-string TableName field in request");
}
std::string table_name = table_name_value->GetString();
validate_table_name(table_name);
return table_name;
}

Expand All @@ -228,6 +226,11 @@ schema_ptr executor::find_table(service::storage_proxy& proxy, const rjson::valu
try {
return proxy.data_dictionary().find_schema(sstring(executor::KEYSPACE_NAME_PREFIX) + sstring(*table_name), *table_name);
} catch(data_dictionary::no_such_column_family&) {
// DynamoDB returns validation error even when table does not exist
// and the table name is invalid.
validate_table_name(table_name);

// if table_name is valid, raise resource not found exception.
throw api_error::resource_not_found(
format("Requested resource not found: Table: {} not found", *table_name));
}
Expand Down Expand Up @@ -278,6 +281,10 @@ get_table_or_view(service::storage_proxy& proxy, const rjson::value& request) {
try {
return { proxy.data_dictionary().find_schema(sstring(internal_ks_name), sstring(internal_table_name)), type };
} catch (data_dictionary::no_such_column_family&) {
// DynamoDB returns validation error even when table does not exist
// and the table name is invalid.
validate_table_name(table_name);

throw api_error::resource_not_found(
format("Requested resource not found: Internal table: {}.{} not found", internal_ks_name, internal_table_name));
}
Expand Down Expand Up @@ -519,6 +526,10 @@ future<executor::request_return_type> executor::delete_table(client_state& clien
elogger.trace("Deleting table {}", request);

std::string table_name = get_table_name(request);
// DynamoDB returns validation error even when table does not exist
// and the table name is invalid.
validate_table_name(table_name);

std::string keyspace_name = executor::KEYSPACE_NAME_PREFIX + table_name;
tracing::add_table_name(trace_state, keyspace_name, table_name);
auto& p = _proxy.container();
Expand Down Expand Up @@ -864,6 +875,8 @@ static future<executor::request_return_type> create_table_on_shard0(tracing::tra
// (e.g., verify that this table doesn't already exist) - we can only
// do this further down - after taking group0_guard.
std::string table_name = get_table_name(request);
validate_table_name(table_name);

if (table_name.find(executor::INTERNAL_TABLE_PREFIX) == 0) {
co_return api_error::validation(format("Prefix {} is reserved for accessing internal tables", executor::INTERNAL_TABLE_PREFIX));
}
Expand Down

0 comments on commit dcd01b2

Please sign in to comment.