From b902f5d045a70153ab49f34cf31850323ff2f9c8 Mon Sep 17 00:00:00 2001 From: kssenii Date: Sat, 17 Jun 2023 01:54:38 +0200 Subject: [PATCH] Small style corrections --- src/Databases/DatabaseFilesystem.cpp | 94 +++++++++++++++------------- src/Databases/DatabaseFilesystem.h | 14 +++-- src/Databases/DatabaseHDFS.cpp | 50 +++++++-------- src/Databases/DatabaseHDFS.h | 12 ++-- src/Databases/DatabaseS3.cpp | 44 ++++++------- src/Databases/DatabaseS3.h | 9 +-- 6 files changed, 114 insertions(+), 109 deletions(-) diff --git a/src/Databases/DatabaseFilesystem.cpp b/src/Databases/DatabaseFilesystem.cpp index cf45240a5f0..001aa1f9ef6 100644 --- a/src/Databases/DatabaseFilesystem.cpp +++ b/src/Databases/DatabaseFilesystem.cpp @@ -31,21 +31,22 @@ namespace ErrorCodes DatabaseFilesystem::DatabaseFilesystem(const String & name_, const String & path_, ContextPtr context_) : IDatabase(name_), WithContext(context_->getGlobalContext()), path(path_), log(&Poco::Logger::get("DatabaseFileSystem(" + name_ + ")")) { - fs::path user_files_path; - const auto & application_type = context_->getApplicationType(); - - if (application_type != Context::ApplicationType::LOCAL) - user_files_path = fs::canonical(fs::path(getContext()->getUserFilesPath())); + bool is_local = context_->getApplicationType() == Context::ApplicationType::LOCAL; + fs::path user_files_path = is_local ? "" : fs::canonical(getContext()->getUserFilesPath()); if (fs::path(path).is_relative()) + { path = user_files_path / path; - else if (application_type != Context::ApplicationType::LOCAL && !pathStartsWith(fs::path(path), user_files_path)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Path must be inside user-files path ({})", user_files_path.string()); - - path = fs::absolute(path).lexically_normal().string(); + } + else if (!is_local && !pathStartsWith(fs::path(path), user_files_path)) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "Path must be inside user-files path: {}", user_files_path.string()); + } + path = fs::absolute(path).lexically_normal(); if (!fs::exists(path)) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Path does not exist ({})", path); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Path does not exist: {}", path); } std::string DatabaseFilesystem::getTablePath(const std::string & table_name) const @@ -62,19 +63,17 @@ void DatabaseFilesystem::addTable(const std::string & table_name, StoragePtr tab throw Exception( ErrorCodes::LOGICAL_ERROR, "Table with name `{}` already exists in database `{}` (engine {})", - table_name, - getDatabaseName(), - getEngineName()); + table_name, getDatabaseName(), getEngineName()); } bool DatabaseFilesystem::checkTableFilePath(const std::string & table_path, ContextPtr context_, bool throw_on_error) const { - // If run in Local mode, no need for path checking. - bool need_check_path = context_->getApplicationType() != Context::ApplicationType::LOCAL; - std::string user_files_path = fs::canonical(fs::path(context_->getUserFilesPath())).string(); + /// If run in Local mode, no need for path checking. + bool check_path = context_->getApplicationType() != Context::ApplicationType::LOCAL; + const auto & user_files_path = context_->getUserFilesPath(); - // Check access for file before checking its existence - if (need_check_path && !fileOrSymlinkPathStartsWith(table_path, user_files_path)) + /// Check access for file before checking its existence. + if (check_path && !fileOrSymlinkPathStartsWith(table_path, user_files_path)) { if (throw_on_error) throw Exception(ErrorCodes::DATABASE_ACCESS_DENIED, "File is not inside {}", user_files_path); @@ -82,11 +81,20 @@ bool DatabaseFilesystem::checkTableFilePath(const std::string & table_path, Cont return false; } - // Check if the corresponding file exists - if (!fs::exists(table_path) || !fs::is_regular_file(table_path)) + /// Check if the corresponding file exists. + if (!fs::exists(table_path)) { if (throw_on_error) - throw Exception(ErrorCodes::FILE_DOESNT_EXIST, "File does not exist ({})", table_path); + throw Exception(ErrorCodes::FILE_DOESNT_EXIST, "File does not exist: {}", table_path); + else + return false; + } + + if (!fs::is_regular_file(table_path)) + { + if (throw_on_error) + throw Exception(ErrorCodes::FILE_DOESNT_EXIST, + "File is directory, but expected a file: {}", table_path); else return false; } @@ -104,7 +112,7 @@ StoragePtr DatabaseFilesystem::tryGetTableFromCache(const std::string & name) co table = it->second; } - // invalidate cache if file no longer exists + /// Invalidate cache if file no longer exists. if (table && !fs::exists(getTablePath(name))) { std::lock_guard lock(mutex); @@ -120,29 +128,26 @@ bool DatabaseFilesystem::isTableExist(const String & name, ContextPtr context_) if (tryGetTableFromCache(name)) return true; - fs::path table_file_path(getTablePath(name)); - - return checkTableFilePath(table_file_path, context_, false); + return checkTableFilePath(getTablePath(name), context_, /* throw_on_error */false); } StoragePtr DatabaseFilesystem::getTableImpl(const String & name, ContextPtr context_) const { - // Check if table exists in loaded tables map + /// Check if table exists in loaded tables map. if (auto table = tryGetTableFromCache(name)) return table; auto table_path = getTablePath(name); + checkTableFilePath(table_path, context_, /* throw_on_error */true); - checkTableFilePath(table_path, context_, true); - - // If the file exists, create a new table using TableFunctionFile and return it. + /// If the file exists, create a new table using TableFunctionFile and return it. auto args = makeASTFunction("file", std::make_shared(table_path)); auto table_function = TableFunctionFactory::instance().get(args, context_); if (!table_function) return nullptr; - // TableFunctionFile throws exceptions, if table cannot be created + /// TableFunctionFile throws exceptions, if table cannot be created. auto table_storage = table_function->execute(args, context_, name); if (table_storage) addTable(name, table_storage); @@ -152,10 +157,12 @@ StoragePtr DatabaseFilesystem::getTableImpl(const String & name, ContextPtr cont StoragePtr DatabaseFilesystem::getTable(const String & name, ContextPtr context_) const { - // rethrow all exceptions from TableFunctionFile to show correct error to user + /// getTableImpl can throw exceptions, do not catch them to show correct error to user. if (auto storage = getTableImpl(name, context_)) return storage; - throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); + + throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", + backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); } StoragePtr DatabaseFilesystem::tryGetTable(const String & name, ContextPtr context_) const @@ -166,15 +173,14 @@ StoragePtr DatabaseFilesystem::tryGetTable(const String & name, ContextPtr conte } catch (const Exception & e) { - // Ignore exceptions thrown by TableFunctionFile, which indicate that there is no table - // see tests/02722_database_filesystem.sh for more details - if (e.code() == ErrorCodes::BAD_ARGUMENTS) + /// Ignore exceptions thrown by TableFunctionFile, which indicate that there is no table + /// see tests/02722_database_filesystem.sh for more details. + if (e.code() == ErrorCodes::BAD_ARGUMENTS + || e.code() == ErrorCodes::DATABASE_ACCESS_DENIED + || e.code() == ErrorCodes::FILE_DOESNT_EXIST) + { return nullptr; - if (e.code() == ErrorCodes::DATABASE_ACCESS_DENIED) - return nullptr; - if (e.code() == ErrorCodes::FILE_DOESNT_EXIST) - return nullptr; - + } throw; } } @@ -187,10 +193,10 @@ bool DatabaseFilesystem::empty() const ASTPtr DatabaseFilesystem::getCreateDatabaseQuery() const { - auto settings = getContext()->getSettingsRef(); - ParserCreateQuery parser; - + const auto & settings = getContext()->getSettingsRef(); const String query = fmt::format("CREATE DATABASE {} ENGINE = Filesystem('{}')", backQuoteIfNeed(getDatabaseName()), path); + + ParserCreateQuery parser; ASTPtr ast = parseQuery(parser, query.data(), query.data() + query.size(), "", 0, settings.max_parser_depth); if (const auto database_comment = getDatabaseComment(); !database_comment.empty()) @@ -238,4 +244,4 @@ DatabaseTablesIteratorPtr DatabaseFilesystem::getTablesIterator(ContextPtr, cons return std::make_unique(Tables{}, getDatabaseName()); } -} // DB +} diff --git a/src/Databases/DatabaseFilesystem.h b/src/Databases/DatabaseFilesystem.h index 350ebfe37a3..5e32342d58f 100644 --- a/src/Databases/DatabaseFilesystem.h +++ b/src/Databases/DatabaseFilesystem.h @@ -12,11 +12,13 @@ namespace DB class Context; /** - * DatabaseFilesystem allows to interact with files stored on the file system - * Uses TableFunctionFile to implicitly load file when a user requests the table, and provides read-only access to the data in the file + * DatabaseFilesystem allows to interact with files stored on the local filesystem. + * Uses TableFunctionFile to implicitly load file when a user requests the table, + * and provides a read-only access to the data in the file. * Tables are cached inside the database for quick access * - * Used in clickhouse-local to access local files + * Used in clickhouse-local to access local files. + * For clickhouse-server requires allows to access file only fron user_files directory. */ class DatabaseFilesystem : public IDatabase, protected WithContext { @@ -31,8 +33,7 @@ public: StoragePtr tryGetTable(const String & name, ContextPtr context) const override; - // Contains only temporary tables - bool shouldBeEmptyOnDetach() const override { return false; } + bool shouldBeEmptyOnDetach() const override { return false; } /// Contains only temporary tables. bool empty() const override; @@ -43,6 +44,7 @@ public: void shutdown() override; std::vector> getTablesForBackup(const FilterByNameFunction &, const ContextPtr &) const override; + DatabaseTablesIteratorPtr getTablesIterator(ContextPtr, const FilterByNameFunction &) const override; protected: @@ -62,4 +64,4 @@ private: Poco::Logger * log; }; -} // DB +} diff --git a/src/Databases/DatabaseHDFS.cpp b/src/Databases/DatabaseHDFS.cpp index 34cb337cdbe..1a0145b9015 100644 --- a/src/Databases/DatabaseHDFS.cpp +++ b/src/Databases/DatabaseHDFS.cpp @@ -49,7 +49,9 @@ DatabaseHDFS::DatabaseHDFS(const String & name_, const String & source_url, Cont if (!source.empty()) { if (!re2::RE2::FullMatch(source, std::string(HDFS_HOST_REGEXP))) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bad hdfs host: {}. It should have structure 'hdfs://:'", source); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bad hdfs host: {}. " + "It should have structure 'hdfs://:'", source); + context_->getGlobalContext()->getRemoteHostFilter().checkURL(Poco::URI(source)); } } @@ -62,18 +64,19 @@ void DatabaseHDFS::addTable(const std::string & table_name, StoragePtr table_sto throw Exception( ErrorCodes::LOGICAL_ERROR, "Table with name `{}` already exists in database `{}` (engine {})", - table_name, - getDatabaseName(), - getEngineName()); + table_name, getDatabaseName(), getEngineName()); } std::string DatabaseHDFS::getTablePath(const std::string & table_name) const { if (table_name.starts_with("hdfs://")) return table_name; + if (source.empty()) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bad hdfs url: {}. It should have structure 'hdfs://:/path'", table_name); - return (fs::path(source) / table_name).string(); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Bad hdfs url: {}. " + "It should have structure 'hdfs://:/path'", table_name); + + return fs::path(source) / table_name; } bool DatabaseHDFS::checkUrl(const std::string & url, ContextPtr context_, bool throw_on_error) const @@ -104,7 +107,7 @@ bool DatabaseHDFS::isTableExist(const String & name, ContextPtr context_) const StoragePtr DatabaseHDFS::getTableImpl(const String & name, ContextPtr context_) const { - // Check if the table exists in the loaded tables map + /// Check if the table exists in the loaded tables map. { std::lock_guard lock(mutex); auto it = loaded_tables.find(name); @@ -116,14 +119,13 @@ StoragePtr DatabaseHDFS::getTableImpl(const String & name, ContextPtr context_) checkUrl(url, context_, true); - // call TableFunctionHDFS auto args = makeASTFunction("hdfs", std::make_shared(url)); auto table_function = TableFunctionFactory::instance().get(args, context_); if (!table_function) return nullptr; - // TableFunctionHDFS throws exceptions, if table cannot be created + /// TableFunctionHDFS throws exceptions, if table cannot be created. auto table_storage = table_function->execute(args, context_, name); if (table_storage) addTable(name, table_storage); @@ -133,10 +135,12 @@ StoragePtr DatabaseHDFS::getTableImpl(const String & name, ContextPtr context_) StoragePtr DatabaseHDFS::getTable(const String & name, ContextPtr context_) const { - // rethrow all exceptions from TableFunctionHDFS to show correct error to user + /// Rethrow all exceptions from TableFunctionHDFS to show correct error to user. if (auto storage = getTableImpl(name, context_)) return storage; - throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); + + throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", + backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); } StoragePtr DatabaseHDFS::tryGetTable(const String & name, ContextPtr context_) const @@ -148,20 +152,16 @@ StoragePtr DatabaseHDFS::tryGetTable(const String & name, ContextPtr context_) c catch (const Exception & e) { // Ignore exceptions thrown by TableFunctionHDFS, which indicate that there is no table - if (e.code() == ErrorCodes::BAD_ARGUMENTS) - return nullptr; - if (e.code() == ErrorCodes::ACCESS_DENIED) - return nullptr; - if (e.code() == ErrorCodes::DATABASE_ACCESS_DENIED) - return nullptr; - if (e.code() == ErrorCodes::FILE_DOESNT_EXIST) - return nullptr; - if (e.code() == ErrorCodes::UNACCEPTABLE_URL) - return nullptr; - if (e.code() == ErrorCodes::HDFS_ERROR) - return nullptr; - if (e.code() == ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE) + if (e.code() == ErrorCodes::BAD_ARGUMENTS + || e.code() == ErrorCodes::ACCESS_DENIED + || e.code() == ErrorCodes::DATABASE_ACCESS_DENIED + || e.code() == ErrorCodes::FILE_DOESNT_EXIST + || e.code() == ErrorCodes::UNACCEPTABLE_URL + || e.code() == ErrorCodes::HDFS_ERROR + || e.code() == ErrorCodes::CANNOT_EXTRACT_TABLE_STRUCTURE) + { return nullptr; + } throw; } catch (const Poco::URISyntaxException &) @@ -178,7 +178,7 @@ bool DatabaseHDFS::empty() const ASTPtr DatabaseHDFS::getCreateDatabaseQuery() const { - auto settings = getContext()->getSettingsRef(); + const auto & settings = getContext()->getSettingsRef(); ParserCreateQuery parser; const String query = fmt::format("CREATE DATABASE {} ENGINE = HDFS('{}')", backQuoteIfNeed(getDatabaseName()), source); diff --git a/src/Databases/DatabaseHDFS.h b/src/Databases/DatabaseHDFS.h index c7071370b5e..957b2080135 100644 --- a/src/Databases/DatabaseHDFS.h +++ b/src/Databases/DatabaseHDFS.h @@ -16,9 +16,10 @@ namespace DB class Context; /** - * DatabaseHDFS allows to interact with files stored on the file system - * Uses TableFunctionHDFS to implicitly load file when a user requests the table, and provides read-only access to the data in the file - * Tables are cached inside the database for quick access + * DatabaseHDFS allows to interact with files stored on the file system. + * Uses TableFunctionHDFS to implicitly load file when a user requests the table, + * and provides read-only access to the data in the file. + * Tables are cached inside the database for quick access. */ class DatabaseHDFS : public IDatabase, protected WithContext { @@ -33,8 +34,7 @@ public: StoragePtr tryGetTable(const String & name, ContextPtr context) const override; - // Contains only temporary tables - bool shouldBeEmptyOnDetach() const override { return false; } + bool shouldBeEmptyOnDetach() const override { return false; } /// Contains only temporary tables. bool empty() const override; @@ -63,6 +63,6 @@ private: Poco::Logger * log; }; -} // DB +} #endif diff --git a/src/Databases/DatabaseS3.cpp b/src/Databases/DatabaseS3.cpp index 46f8a67687d..11655f5f100 100644 --- a/src/Databases/DatabaseS3.cpp +++ b/src/Databases/DatabaseS3.cpp @@ -60,15 +60,13 @@ void DatabaseS3::addTable(const std::string & table_name, StoragePtr table_stora throw Exception( ErrorCodes::LOGICAL_ERROR, "Table with name `{}` already exists in database `{}` (engine {})", - table_name, - getDatabaseName(), - getEngineName()); + table_name, getDatabaseName(), getEngineName()); } std::string DatabaseS3::getFullUrl(const std::string & name) const { if (!config.url_prefix.empty()) - return (fs::path(config.url_prefix) / name).string(); + return fs::path(config.url_prefix) / name; return name; } @@ -100,7 +98,7 @@ bool DatabaseS3::isTableExist(const String & name, ContextPtr context_) const StoragePtr DatabaseS3::getTableImpl(const String & name, ContextPtr context_) const { - // Check if the table exists in the loaded tables map + /// Check if the table exists in the loaded tables map. { std::lock_guard lock(mutex); auto it = loaded_tables.find(name); @@ -109,12 +107,9 @@ StoragePtr DatabaseS3::getTableImpl(const String & name, ContextPtr context_) co } auto url = getFullUrl(name); + checkUrl(url, context_, /* throw_on_error */true); - checkUrl(url, context_, true); - - // call TableFunctionS3 auto function = std::make_shared(); - function->name = "s3"; function->arguments = std::make_shared(); function->children.push_back(function->arguments); @@ -134,7 +129,7 @@ StoragePtr DatabaseS3::getTableImpl(const String & name, ContextPtr context_) co if (!table_function) return nullptr; - // TableFunctionS3 throws exceptions, if table cannot be created + /// TableFunctionS3 throws exceptions, if table cannot be created. auto table_storage = table_function->execute(function, context_, name); if (table_storage) addTable(name, table_storage); @@ -144,10 +139,12 @@ StoragePtr DatabaseS3::getTableImpl(const String & name, ContextPtr context_) co StoragePtr DatabaseS3::getTable(const String & name, ContextPtr context_) const { - // rethrow all exceptions from TableFunctionS3 to show correct error to user + /// Rethrow all exceptions from TableFunctionS3 to show correct error to user. if (auto storage = getTableImpl(name, context_)) return storage; - throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); + + throw Exception(ErrorCodes::UNKNOWN_TABLE, "Table {}.{} doesn't exist", + backQuoteIfNeed(getDatabaseName()), backQuoteIfNeed(name)); } StoragePtr DatabaseS3::tryGetTable(const String & name, ContextPtr context_) const @@ -158,15 +155,14 @@ StoragePtr DatabaseS3::tryGetTable(const String & name, ContextPtr context_) con } catch (const Exception & e) { - // Ignore exceptions thrown by TableFunctionS3, which indicate that there is no table - if (e.code() == ErrorCodes::BAD_ARGUMENTS) - return nullptr; - if (e.code() == ErrorCodes::S3_ERROR) - return nullptr; - if (e.code() == ErrorCodes::FILE_DOESNT_EXIST) - return nullptr; - if (e.code() == ErrorCodes::UNACCEPTABLE_URL) + /// Ignore exceptions thrown by TableFunctionS3, which indicate that there is no table. + if (e.code() == ErrorCodes::BAD_ARGUMENTS + || e.code() == ErrorCodes::S3_ERROR + || e.code() == ErrorCodes::FILE_DOESNT_EXIST + || e.code() == ErrorCodes::UNACCEPTABLE_URL) + { return nullptr; + } throw; } catch (const Poco::URISyntaxException &) @@ -183,7 +179,7 @@ bool DatabaseS3::empty() const ASTPtr DatabaseS3::getCreateDatabaseQuery() const { - auto settings = getContext()->getSettingsRef(); + const auto & settings = getContext()->getSettingsRef(); ParserCreateQuery parser; std::string creation_args; @@ -236,8 +232,8 @@ DatabaseS3::Configuration DatabaseS3::parseArguments(ASTs engine_args, ContextPt result.url_prefix = collection.getOrDefault("url", ""); result.no_sign_request = collection.getOrDefault("no_sign_request", false); - auto key_id = collection.getOrDefault("access_key_id", ""); - auto secret_key = collection.getOrDefault("secret_access_key", ""); + auto key_id = collection.getOrDefault("access_key_id", ""); + auto secret_key = collection.getOrDefault("secret_access_key", ""); if (!key_id.empty()) result.access_key_id = key_id; @@ -311,6 +307,6 @@ DatabaseTablesIteratorPtr DatabaseS3::getTablesIterator(ContextPtr, const Filter return std::make_unique(Tables{}, getDatabaseName()); } -} // DB +} #endif diff --git a/src/Databases/DatabaseS3.h b/src/Databases/DatabaseS3.h index f494925b09b..8297ae4e02d 100644 --- a/src/Databases/DatabaseS3.h +++ b/src/Databases/DatabaseS3.h @@ -16,9 +16,10 @@ namespace DB class Context; /** - * DatabaseS3 provides access to data stored in S3 - * Uses TableFunctionS3 to implicitly load file when a user requests the table, and provides read-only access to the data in the file - * Tables are cached inside the database for quick access + * DatabaseS3 provides access to data stored in S3. + * Uses TableFunctionS3 to implicitly load file when a user requests the table, + * and provides read-only access to the data in the file. + * Tables are cached inside the database for quick access. */ class DatabaseS3 : public IDatabase, protected WithContext { @@ -75,6 +76,6 @@ private: Poco::Logger * log; }; -} // DB +} #endif