From 1f80918ac95da2f9f0afdce92612f2a9592915f2 Mon Sep 17 00:00:00 2001 From: Nektarios Katakis Date: Mon, 27 Jan 2020 22:55:37 +0000 Subject: [PATCH 1/4] Don't default to an invalid sqlite config #6573 Signed-off-by: Nektarios Katakis --- changelog.d/6249.bugfix | 1 + synapse/config/database.py | 58 ++++++++++++++++++++++++-------------- 2 files changed, 38 insertions(+), 21 deletions(-) create mode 100644 changelog.d/6249.bugfix diff --git a/changelog.d/6249.bugfix b/changelog.d/6249.bugfix new file mode 100644 index 000000000000..29113a2af959 --- /dev/null +++ b/changelog.d/6249.bugfix @@ -0,0 +1 @@ +Don't default to an `invalid` sqlite config if no database configuration is provided. diff --git a/synapse/config/database.py b/synapse/config/database.py index 219b32f67084..6fa2e32b656e 100644 --- a/synapse/config/database.py +++ b/synapse/config/database.py @@ -56,6 +56,11 @@ def __init__(self, name: str, db_config: dict): class DatabaseConfig(Config): section = "database" + def __init__(self, *args, **kwargs): + self.databases = [] + + super().__init__(*args, **kwargs) + def read_config(self, config, **kwargs): self.event_cache_size = self.parse_size(config.get("event_cache_size", "10K")) @@ -76,12 +81,13 @@ def read_config(self, config, **kwargs): multi_database_config = config.get("databases") database_config = config.get("database") + database_path = config.get("database_path") if multi_database_config and database_config: raise ConfigError("Can't specify both 'database' and 'datbases' in config") if multi_database_config: - if config.get("database_path"): + if database_path: raise ConfigError("Can't specify 'database_path' with 'databases'") self.databases = [ @@ -89,13 +95,13 @@ def read_config(self, config, **kwargs): for name, db_conf in multi_database_config.items() ] - else: - if database_config is None: - database_config = {"name": "sqlite3", "args": {}} - + elif database_config: self.databases = [DatabaseConnectionConfig("master", database_config)] - self.set_databasepath(config.get("database_path")) + elif database_path: + database_config = {"name": "sqlite3", "args": {}} + self.databases = [DatabaseConnectionConfig("master", database_config)] + self.set_databasepath(database_path) def generate_config_section(self, data_dir_path, database_conf, **kwargs): if not database_conf: @@ -127,27 +133,37 @@ def generate_config_section(self, data_dir_path, database_conf, **kwargs): ) def read_arguments(self, args): - self.set_databasepath(args.database_path) + """ + Cases for the cli input: + - If no databases are configured and no path available raise. + - No databases and only path available ==> sqlite3 db. + - If there are multiple databases and a path raise an error. + - If the database set in the config file is sqlite then + overwrite with the command line argument. + """ - def set_databasepath(self, database_path): - if database_path is None: + if args.database_path is None: + if len(self.databases) == 0: + raise ConfigError("No database config provided") return - if database_path != ":memory:": - database_path = self.abspath(database_path) - - # We only support setting a database path if we have a single sqlite3 - # database. - if len(self.databases) != 1: - raise ConfigError("Cannot specify 'database_path' with multiple databases") + if len(self.databases) == 0: + database_config = {"name": "sqlite3", "args": {}} + self.databases = [DatabaseConnectionConfig("master", database_config)] + self.set_databasepath(args.database_path) + return - database = self.get_single_database() - if database.config["name"] != "sqlite3": - # We don't raise here as we haven't done so before for this case. + if self.get_single_database().name == "sqlite3": + self.set_databasepath(args.database_path) + else: logger.warn("Ignoring 'database_path' for non-sqlite3 database") - return - database.config["args"]["database"] = database_path + def set_databasepath(self, database_path): + + if database_path != ":memory:": + database_path = self.abspath(database_path) + + self.databases[0].config["args"]["database"] = database_path @staticmethod def add_arguments(parser): From 2b14e7e8e480efe4fd1e8b2f200ca8b5238afa90 Mon Sep 17 00:00:00 2001 From: Nektarios Katakis Date: Wed, 5 Feb 2020 10:13:32 +0000 Subject: [PATCH 2/4] Overwrite database_path from global scope in config file --- synapse/config/database.py | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/synapse/config/database.py b/synapse/config/database.py index 6fa2e32b656e..1ff8b57c1d16 100644 --- a/synapse/config/database.py +++ b/synapse/config/database.py @@ -57,10 +57,10 @@ class DatabaseConfig(Config): section = "database" def __init__(self, *args, **kwargs): - self.databases = [] - super().__init__(*args, **kwargs) + self.databases = [] + def read_config(self, config, **kwargs): self.event_cache_size = self.parse_size(config.get("event_cache_size", "10K")) @@ -95,10 +95,14 @@ def read_config(self, config, **kwargs): for name, db_conf in multi_database_config.items() ] - elif database_config: + if database_config: self.databases = [DatabaseConnectionConfig("master", database_config)] - elif database_path: + if database_path: + if self.databases and self.databases[0].name != "sqlite3": + logger.warn("Ignoring 'database_path' for non-sqlite3 database") + return + database_config = {"name": "sqlite3", "args": {}} self.databases = [DatabaseConnectionConfig("master", database_config)] self.set_databasepath(database_path) @@ -135,15 +139,15 @@ def generate_config_section(self, data_dir_path, database_conf, **kwargs): def read_arguments(self, args): """ Cases for the cli input: - - If no databases are configured and no path available raise. - - No databases and only path available ==> sqlite3 db. - - If there are multiple databases and a path raise an error. + - If no databases are configured and no database_path is set, raise. + - No databases and only database_path available ==> sqlite3 db. + - If there are multiple databases and a database_path raise an error. - If the database set in the config file is sqlite then overwrite with the command line argument. """ if args.database_path is None: - if len(self.databases) == 0: + if not self.databases: raise ConfigError("No database config provided") return @@ -178,7 +182,7 @@ def add_arguments(parser): def get_single_database(self) -> DatabaseConnectionConfig: """Returns the database if there is only one, useful for e.g. tests """ - if len(self.databases) != 1: + if not self.databases: raise Exception("More than one database exists") return self.databases[0] From b05d0d938fbb766d9d812f4127109299e8186b16 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 7 Mar 2020 08:57:55 +0000 Subject: [PATCH 3/4] Make warning a bit more visible --- synapse/config/database.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/synapse/config/database.py b/synapse/config/database.py index 1ff8b57c1d16..aef7530e5fdc 100644 --- a/synapse/config/database.py +++ b/synapse/config/database.py @@ -22,6 +22,11 @@ logger = logging.getLogger(__name__) +NON_SQLITE_DATABASE_PATH_WARNING = """\ +Ignoring 'database_path' setting: not using a sqlite3 database. +-------------------------------------------------------------------------------- +""" + class DatabaseConnectionConfig: """Contains the connection config for a particular database. @@ -100,7 +105,7 @@ def read_config(self, config, **kwargs): if database_path: if self.databases and self.databases[0].name != "sqlite3": - logger.warn("Ignoring 'database_path' for non-sqlite3 database") + logger.warning(NON_SQLITE_DATABASE_PATH_WARNING) return database_config = {"name": "sqlite3", "args": {}} @@ -160,7 +165,7 @@ def read_arguments(self, args): if self.get_single_database().name == "sqlite3": self.set_databasepath(args.database_path) else: - logger.warn("Ignoring 'database_path' for non-sqlite3 database") + logger.warning(NON_SQLITE_DATABASE_PATH_WARNING) def set_databasepath(self, database_path): From bbbea8a07060750d330cb42d154eb8ce4ef7cf7f Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Sat, 7 Mar 2020 09:09:32 +0000 Subject: [PATCH 4/4] Update changelog --- changelog.d/6249.bugfix | 1 - changelog.d/6573.bugfix | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 changelog.d/6249.bugfix create mode 100644 changelog.d/6573.bugfix diff --git a/changelog.d/6249.bugfix b/changelog.d/6249.bugfix deleted file mode 100644 index 29113a2af959..000000000000 --- a/changelog.d/6249.bugfix +++ /dev/null @@ -1 +0,0 @@ -Don't default to an `invalid` sqlite config if no database configuration is provided. diff --git a/changelog.d/6573.bugfix b/changelog.d/6573.bugfix new file mode 100644 index 000000000000..1bb8014db795 --- /dev/null +++ b/changelog.d/6573.bugfix @@ -0,0 +1 @@ +Don't attempt to use an invalid sqlite config if no database configuration is provided. Contributed by @nekatak.