From 8d1a07975c0ea46fc9bde6adec256d95b44d39c4 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Thu, 12 Aug 2021 11:03:50 +0100 Subject: [PATCH 1/9] Allow using several custom template directories --- synapse/config/_base.py | 27 ++++++++++++++------------- synapse/config/account_validity.py | 4 ++-- synapse/config/emailconfig.py | 6 +++--- synapse/config/sso.py | 2 +- synapse/module_api/__init__.py | 2 +- tests/config/test_base.py | 6 +++--- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index d6ec618f8f52..70ac9c5e39c4 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -237,7 +237,7 @@ def read_template(self, filename: str) -> jinja2.Template: def read_templates( self, filenames: List[str], - custom_template_directory: Optional[str] = None, + custom_template_directories: Optional[List[str]] = None, ) -> List[jinja2.Template]: """Load a list of template files from disk using the given variables. @@ -251,8 +251,8 @@ def read_templates( Args: filenames: A list of template filenames to read. - custom_template_directory: A directory to try to look for the templates - before using the default Synapse template directory instead. + custom_template_directories: A list of directories to try to look for the + templates before using the default Synapse template directory instead. Raises: ConfigError: if the file's path is incorrect or otherwise cannot be read. @@ -262,18 +262,19 @@ def read_templates( """ search_directories = [self.default_template_dir] - # The loader will first look in the custom template directory (if specified) for the + # The loader will first look in the custom template directories (if specified) for the # given filename. If it doesn't find it, it will use the default template dir instead - if custom_template_directory: - # Check that the given template directory exists - if not self.path_exists(custom_template_directory): - raise ConfigError( - "Configured template directory does not exist: %s" - % (custom_template_directory,) - ) + if custom_template_directories: + for custom_template_directory in custom_template_directories: + # Check that the given template directory exists + if not self.path_exists(custom_template_directory): + raise ConfigError( + "Configured template directory does not exist: %s" + % (custom_template_directory,) + ) - # Search the custom template directory as well - search_directories.insert(0, custom_template_directory) + # Search the custom template directory as well + search_directories.insert(0, custom_template_directory) # TODO: switch to synapse.util.templates.build_jinja_env loader = jinja2.FileSystemLoader(search_directories) diff --git a/synapse/config/account_validity.py b/synapse/config/account_validity.py index 6be4eafe5582..dcf419cf5152 100644 --- a/synapse/config/account_validity.py +++ b/synapse/config/account_validity.py @@ -86,7 +86,7 @@ def read_config(self, config, **kwargs): [ account_renewed_template_filename, "account_previously_renewed.html", - invalid_token_template_filename, + [invalid_token_template_filename], ], - account_validity_template_dir, + [account_validity_template_dir], ) diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index 42526502f0e2..f51c1f71123e 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -257,7 +257,7 @@ def read_config(self, config, **kwargs): registration_template_success_html, add_threepid_template_success_html, ], - template_dir, + [template_dir], ) # Render templates that do not contain any placeholders @@ -297,7 +297,7 @@ def read_config(self, config, **kwargs): self.email_notif_template_text, ) = self.read_templates( [notif_template_html, notif_template_text], - template_dir, + [template_dir], ) self.email_notif_for_new_users = email_config.get( @@ -320,7 +320,7 @@ def read_config(self, config, **kwargs): self.account_validity_template_text, ) = self.read_templates( [expiry_template_html, expiry_template_text], - template_dir, + [template_dir], ) subjects_config = email_config.get("subjects", {}) diff --git a/synapse/config/sso.py b/synapse/config/sso.py index d0f04cf8e6b2..a616bf9661b9 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -63,7 +63,7 @@ def read_config(self, config, **kwargs): "sso_auth_success.html", "sso_auth_bad_user.html", ], - self.sso_template_dir, + [self.sso_template_dir], ) # These templates have no placeholders, so render them here diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 1cc13fc97b22..06562d54119c 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -677,7 +677,7 @@ def read_templates( A list containing the loaded templates, with the orders matching the one of the filenames parameter. """ - return self._hs.config.read_templates(filenames, custom_template_directory) + return self._hs.config.read_templates(filenames, [custom_template_directory]) class PublicRoomListManager: diff --git a/tests/config/test_base.py b/tests/config/test_base.py index 84ae3b88ae9b..b940a086a682 100644 --- a/tests/config/test_base.py +++ b/tests/config/test_base.py @@ -30,7 +30,7 @@ def test_loading_missing_templates(self): # contain template files with tempfile.TemporaryDirectory() as tmp_dir: # Attempt to load an HTML template from our custom template directory - template = self.hs.config.read_templates(["sso_error.html"], tmp_dir)[0] + template = self.hs.config.read_templates(["sso_error.html"], [tmp_dir])[0] # If no errors, we should've gotten the default template instead @@ -60,7 +60,7 @@ def test_loading_custom_templates(self): # Attempt to load the template from our custom template directory template = ( - self.hs.config.read_templates([template_filename], tmp_dir) + self.hs.config.read_templates([template_filename], [tmp_dir]) )[0] # Render the template @@ -77,5 +77,5 @@ def test_loading_custom_templates(self): def test_loading_template_from_nonexistent_custom_directory(self): with self.assertRaises(ConfigError): self.hs.config.read_templates( - ["some_filename.html"], "a_nonexistent_directory" + ["some_filename.html"], ["a_nonexistent_directory"] ) From 9e9539617a019e1a75ac4e09a71a68e39f78ed36 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 13 Aug 2021 14:00:53 +0100 Subject: [PATCH 2/9] Allow None paths --- synapse/config/_base.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 70ac9c5e39c4..30cbe9aab193 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -237,7 +237,7 @@ def read_template(self, filename: str) -> jinja2.Template: def read_templates( self, filenames: List[str], - custom_template_directories: Optional[List[str]] = None, + custom_template_directories: List[Optional[str]] = [], ) -> List[jinja2.Template]: """Load a list of template files from disk using the given variables. @@ -251,7 +251,7 @@ def read_templates( Args: filenames: A list of template filenames to read. - custom_template_directories: A list of directories to try to look for the + custom_template_directories: A list of directory to try to look for the templates before using the default Synapse template directory instead. Raises: @@ -264,8 +264,8 @@ def read_templates( # The loader will first look in the custom template directories (if specified) for the # given filename. If it doesn't find it, it will use the default template dir instead - if custom_template_directories: - for custom_template_directory in custom_template_directories: + for custom_template_directory in custom_template_directories: + if custom_template_directory: # Check that the given template directory exists if not self.path_exists(custom_template_directory): raise ConfigError( From 466f184b653e57d7f3b94615822e44ca1151660c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 13 Aug 2021 14:39:33 +0100 Subject: [PATCH 3/9] Lint --- synapse/config/_base.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index 30cbe9aab193..ce4afa6de747 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -237,7 +237,7 @@ def read_template(self, filename: str) -> jinja2.Template: def read_templates( self, filenames: List[str], - custom_template_directories: List[Optional[str]] = [], + custom_template_directories: Optional[List[Optional[str]]] = None, ) -> List[jinja2.Template]: """Load a list of template files from disk using the given variables. @@ -262,19 +262,23 @@ def read_templates( """ search_directories = [self.default_template_dir] - # The loader will first look in the custom template directories (if specified) for the - # given filename. If it doesn't find it, it will use the default template dir instead - for custom_template_directory in custom_template_directories: - if custom_template_directory: - # Check that the given template directory exists - if not self.path_exists(custom_template_directory): - raise ConfigError( - "Configured template directory does not exist: %s" - % (custom_template_directory,) - ) + # The loader will first look in the custom template directories (if specified) + # for the given filename. If it doesn't find it, it will use the default + # template dir instead. + if custom_template_directories is not None: + for custom_template_directory in custom_template_directories: + # Elements in the list might be None if they were retrieved from the + # configuration dict using config_dict.get(...). + if custom_template_directory: + # Check that the given template directory exists + if not self.path_exists(custom_template_directory): + raise ConfigError( + "Configured template directory does not exist: %s" + % (custom_template_directory,) + ) - # Search the custom template directory as well - search_directories.insert(0, custom_template_directory) + # Search the custom template directory as well + search_directories.insert(0, custom_template_directory) # TODO: switch to synapse.util.templates.build_jinja_env loader = jinja2.FileSystemLoader(search_directories) From d35cc311a0726fd6a3d9fd85e0ae4e7fcd4b83ba Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 13 Aug 2021 14:40:15 +0100 Subject: [PATCH 4/9] Changelog --- changelog.d/10587.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10587.misc diff --git a/changelog.d/10587.misc b/changelog.d/10587.misc new file mode 100644 index 000000000000..4c6167977c6c --- /dev/null +++ b/changelog.d/10587.misc @@ -0,0 +1 @@ +Allow multiple custom directories in `read_templates`. From fb68544a4dcace49fc60443dc1c3e07656e31585 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Fri, 13 Aug 2021 14:59:48 +0100 Subject: [PATCH 5/9] Typo --- synapse/config/account_validity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/account_validity.py b/synapse/config/account_validity.py index dcf419cf5152..f76f5fe35d98 100644 --- a/synapse/config/account_validity.py +++ b/synapse/config/account_validity.py @@ -86,7 +86,7 @@ def read_config(self, config, **kwargs): [ account_renewed_template_filename, "account_previously_renewed.html", - [invalid_token_template_filename], + invalid_token_template_filename, ], [account_validity_template_dir], ) From 952d1e1accd11a6238b6c5fe2797c6f063e004dd Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 18:24:26 +0100 Subject: [PATCH 6/9] Incorporate review --- synapse/config/_base.py | 32 ++++++++-------- synapse/config/account_validity.py | 2 +- synapse/config/emailconfig.py | 6 +-- synapse/config/sso.py | 2 +- synapse/module_api/__init__.py | 5 ++- tests/config/test_base.py | 59 ++++++++++++++++++++++++++++-- 6 files changed, 82 insertions(+), 24 deletions(-) diff --git a/synapse/config/_base.py b/synapse/config/_base.py index ce4afa6de747..2cc242782add 100644 --- a/synapse/config/_base.py +++ b/synapse/config/_base.py @@ -237,13 +237,14 @@ def read_template(self, filename: str) -> jinja2.Template: def read_templates( self, filenames: List[str], - custom_template_directories: Optional[List[Optional[str]]] = None, + custom_template_directories: Optional[Iterable[str]] = None, ) -> List[jinja2.Template]: """Load a list of template files from disk using the given variables. This function will attempt to load the given templates from the default Synapse - template directory. If `custom_template_directory` is supplied, that directory - is tried first. + template directory. If `custom_template_directories` is supplied, any directory + in this list is tried (in the order they appear in the list) before trying + Synapse's default directory. Files read are treated as Jinja templates. The templates are not rendered yet and have autoescape enabled. @@ -260,25 +261,26 @@ def read_templates( Returns: A list of jinja2 templates. """ - search_directories = [self.default_template_dir] + search_directories = [] # The loader will first look in the custom template directories (if specified) # for the given filename. If it doesn't find it, it will use the default # template dir instead. if custom_template_directories is not None: for custom_template_directory in custom_template_directories: - # Elements in the list might be None if they were retrieved from the - # configuration dict using config_dict.get(...). - if custom_template_directory: - # Check that the given template directory exists - if not self.path_exists(custom_template_directory): - raise ConfigError( - "Configured template directory does not exist: %s" - % (custom_template_directory,) - ) + # Check that the given template directory exists + if not self.path_exists(custom_template_directory): + raise ConfigError( + "Configured template directory does not exist: %s" + % (custom_template_directory,) + ) + + # Search the custom template directory as well + search_directories.append(custom_template_directory) - # Search the custom template directory as well - search_directories.insert(0, custom_template_directory) + # Append the default directory at the end of the list so Jinja can fallback on it + # if a template is missing from any custom directory. + search_directories.append(self.default_template_dir) # TODO: switch to synapse.util.templates.build_jinja_env loader = jinja2.FileSystemLoader(search_directories) diff --git a/synapse/config/account_validity.py b/synapse/config/account_validity.py index f76f5fe35d98..9acce5996ec2 100644 --- a/synapse/config/account_validity.py +++ b/synapse/config/account_validity.py @@ -88,5 +88,5 @@ def read_config(self, config, **kwargs): "account_previously_renewed.html", invalid_token_template_filename, ], - [account_validity_template_dir], + (td for td in (account_validity_template_dir,) if td), ) diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index f51c1f71123e..7087e575de5f 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -257,7 +257,7 @@ def read_config(self, config, **kwargs): registration_template_success_html, add_threepid_template_success_html, ], - [template_dir], + (td for td in (template_dir,) if td), # Filter out template_dir if not provided ) # Render templates that do not contain any placeholders @@ -297,7 +297,7 @@ def read_config(self, config, **kwargs): self.email_notif_template_text, ) = self.read_templates( [notif_template_html, notif_template_text], - [template_dir], + (td for td in (template_dir,) if td), ) self.email_notif_for_new_users = email_config.get( @@ -320,7 +320,7 @@ def read_config(self, config, **kwargs): self.account_validity_template_text, ) = self.read_templates( [expiry_template_html, expiry_template_text], - [template_dir], + (td for td in (template_dir,) if td), ) subjects_config = email_config.get("subjects", {}) diff --git a/synapse/config/sso.py b/synapse/config/sso.py index a616bf9661b9..4b590e05356a 100644 --- a/synapse/config/sso.py +++ b/synapse/config/sso.py @@ -63,7 +63,7 @@ def read_config(self, config, **kwargs): "sso_auth_success.html", "sso_auth_bad_user.html", ], - [self.sso_template_dir], + (td for td in (self.sso_template_dir,) if td), ) # These templates have no placeholders, so render them here diff --git a/synapse/module_api/__init__.py b/synapse/module_api/__init__.py index 06562d54119c..82725853bc6c 100644 --- a/synapse/module_api/__init__.py +++ b/synapse/module_api/__init__.py @@ -677,7 +677,10 @@ def read_templates( A list containing the loaded templates, with the orders matching the one of the filenames parameter. """ - return self._hs.config.read_templates(filenames, [custom_template_directory]) + return self._hs.config.read_templates( + filenames, + (td for td in (custom_template_directory,) if td), + ) class PublicRoomListManager: diff --git a/tests/config/test_base.py b/tests/config/test_base.py index b940a086a682..e4be8a170fd9 100644 --- a/tests/config/test_base.py +++ b/tests/config/test_base.py @@ -30,7 +30,7 @@ def test_loading_missing_templates(self): # contain template files with tempfile.TemporaryDirectory() as tmp_dir: # Attempt to load an HTML template from our custom template directory - template = self.hs.config.read_templates(["sso_error.html"], [tmp_dir])[0] + template = self.hs.config.read_templates(["sso_error.html"], (tmp_dir,))[0] # If no errors, we should've gotten the default template instead @@ -60,7 +60,7 @@ def test_loading_custom_templates(self): # Attempt to load the template from our custom template directory template = ( - self.hs.config.read_templates([template_filename], [tmp_dir]) + self.hs.config.read_templates([template_filename], (tmp_dir,)) )[0] # Render the template @@ -74,8 +74,61 @@ def test_loading_custom_templates(self): "Template file did not contain our test string", ) + def test_multiple_custom_template_directories(self): + """Tests that directories are searched in the right order if multiple custom + template directories are provided. + """ + # Create two temporary directories on the filesystem. + tempdirs = [ + tempfile.TemporaryDirectory(), + tempfile.TemporaryDirectory(), + ] + + # Create one template in each directory, which content is the index of the + # directory in the list. + template_filename = "my_template.html.j2" + for i in range(len(tempdirs)): + tempdir = tempdirs[i] + template_path = os.path.join(tempdir.name, template_filename) + + with open(template_path, "w") as fp: + fp.write(str(i)) + fp.flush() + + # Retrieve the template. + template = ( + self.hs.config.read_templates( + [template_filename], + (td.name for td in tempdirs), + ) + )[0] + + # Test that we got the template we dropped in the first directory in the list. + self.assertEqual(template.render(), "0") + + # Add another template, this one only in the second directory in the list, so we + # can test that the second directory is still searched into when no matching file + # could be found in the first one. + other_template_name = "my_other_template.html.j2" + other_template_path = os.path.join(tempdirs[1].name, other_template_name) + + with open(other_template_path, "w") as fp: + fp.write("hello world") + fp.flush() + + # Retrieve the template. + template = ( + self.hs.config.read_templates( + [other_template_name], + (td.name for td in tempdirs), + ) + )[0] + + # Test that the file has the expected content. + self.assertEqual(template.render(), "hello world") + def test_loading_template_from_nonexistent_custom_directory(self): with self.assertRaises(ConfigError): self.hs.config.read_templates( - ["some_filename.html"], ["a_nonexistent_directory"] + ["some_filename.html"], ("a_nonexistent_directory",) ) From bba6e5de6cb1adf71ca01f92c7c6ea38a6977002 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 18:26:20 +0100 Subject: [PATCH 7/9] Cleanup temp dirs --- tests/config/test_base.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/config/test_base.py b/tests/config/test_base.py index e4be8a170fd9..cabb66ddd104 100644 --- a/tests/config/test_base.py +++ b/tests/config/test_base.py @@ -127,6 +127,11 @@ def test_multiple_custom_template_directories(self): # Test that the file has the expected content. self.assertEqual(template.render(), "hello world") + # Cleanup the temporary directories manually since we're not using a context + # manager. + for td in tempdirs: + td.cleanup() + def test_loading_template_from_nonexistent_custom_directory(self): with self.assertRaises(ConfigError): self.hs.config.read_templates( From 9d90235446fe4a4e49a5e35498456439b33923c8 Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Mon, 16 Aug 2021 18:35:21 +0100 Subject: [PATCH 8/9] Lint --- synapse/config/emailconfig.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/config/emailconfig.py b/synapse/config/emailconfig.py index 7087e575de5f..fc74b4a8b939 100644 --- a/synapse/config/emailconfig.py +++ b/synapse/config/emailconfig.py @@ -257,7 +257,9 @@ def read_config(self, config, **kwargs): registration_template_success_html, add_threepid_template_success_html, ], - (td for td in (template_dir,) if td), # Filter out template_dir if not provided + ( + td for td in (template_dir,) if td + ), # Filter out template_dir if not provided ) # Render templates that do not contain any placeholders From a72c92c4a2b6e67cd5aa23897328b2b34b35509c Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 17 Aug 2021 11:57:25 +0200 Subject: [PATCH 9/9] Update tests/config/test_base.py Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- tests/config/test_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/config/test_base.py b/tests/config/test_base.py index cabb66ddd104..baa5313fb3cc 100644 --- a/tests/config/test_base.py +++ b/tests/config/test_base.py @@ -84,7 +84,7 @@ def test_multiple_custom_template_directories(self): tempfile.TemporaryDirectory(), ] - # Create one template in each directory, which content is the index of the + # Create one template in each directory, whose content is the index of the # directory in the list. template_filename = "my_template.html.j2" for i in range(len(tempdirs)):