From 2c26debe967920e0f2a74d430fe6410a8a9a0513 Mon Sep 17 00:00:00 2001 From: vshepard Date: Tue, 3 Dec 2024 01:30:03 +0100 Subject: [PATCH 1/5] Fix set_auto_conf with single quotes --- testgres/node.py | 13 +++++++++---- tests/test_simple.py | 27 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/testgres/node.py b/testgres/node.py index 4ae30908..be5019a4 100644 --- a/testgres/node.py +++ b/testgres/node.py @@ -1627,17 +1627,22 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}): name, var = line.partition('=')[::2] name = name.strip() var = var.strip() - var = var.strip('"') - var = var.strip("'") - # remove options specified in rm_options list + # Handle quoted values and remove escaping + if var.startswith("'") and var.endswith("'"): + var = var[1:-1].replace("''", "'") + + # Remove options specified in rm_options list if name in rm_options: continue current_options[name] = var for option in options: - current_options[option] = options[option] + value = options[option] + if isinstance(value, str): + value = value.replace("'", "\\'") + current_options[option] = value auto_conf = '' for option in current_options: diff --git a/tests/test_simple.py b/tests/test_simple.py index 41203a65..a11d7932 100644 --- a/tests/test_simple.py +++ b/tests/test_simple.py @@ -1061,6 +1061,33 @@ def test_simple_with_bin_dir(self): except FileNotFoundError: pass # Expected error + def test_set_auto_conf(self): + with get_new_node() as node: + node.init().start() + + options = { + "archive_command": "cp '%p' \"/mnt/server/archivedir/%f\"", + 'restore_command': 'cp "/mnt/server/archivedir/%f" \'%p\'', + } + + node.set_auto_conf(options) + node.stop() + node.slow_start() + + auto_conf_path = f"{node.data_dir}/postgresql.auto.conf" + with open(auto_conf_path, "r") as f: + content = f.read() + self.assertIn( + "archive_command = 'cp \\'%p\\' \"/mnt/server/archivedir/%f\"", + content, + "archive_command stored wrong" + ) + self.assertIn( + "restore_command = 'cp \"/mnt/server/archivedir/%f\" \\'%p\\''", + content, + "restore_command stored wrong" + ) + if __name__ == '__main__': if os.environ.get('ALT_CONFIG'): From a4092af44fae5a1a92e9c9fbfec449fd99c8e520 Mon Sep 17 00:00:00 2001 From: "d.kovalenko" Date: Tue, 3 Dec 2024 16:50:47 +0300 Subject: [PATCH 2/5] Node.set_auto_conf is improved - we do not touch existing values - escaping of '\n', '\r', '\t', '\b' and '\\' is added - translation of bool into 'on|off' is added test_set_auto_conf is updated. --- testgres/node.py | 39 +++++++++++++++++++++++++++--------- tests/test_simple.py | 47 +++++++++++++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 23 deletions(-) diff --git a/testgres/node.py b/testgres/node.py index be5019a4..7469b0d6 100644 --- a/testgres/node.py +++ b/testgres/node.py @@ -1626,11 +1626,6 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}): name, var = line.partition('=')[::2] name = name.strip() - var = var.strip() - - # Handle quoted values and remove escaping - if var.startswith("'") and var.endswith("'"): - var = var[1:-1].replace("''", "'") # Remove options specified in rm_options list if name in rm_options: @@ -1640,14 +1635,18 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}): for option in options: value = options[option] - if isinstance(value, str): - value = value.replace("'", "\\'") + valueType = type(value) + + if valueType == str: + value = __class__._escape_config_value(value) + elif valueType == bool: + value = "on" if value else "off" + current_options[option] = value auto_conf = '' for option in current_options: - auto_conf += "{0} = '{1}'\n".format( - option, current_options[option]) + auto_conf += option + " = " + str(current_options[option]) + "\n" for directive in current_directives: auto_conf += directive + "\n" @@ -1695,6 +1694,28 @@ def _get_bin_path(self, filename): bin_path = get_bin_path(filename) return bin_path + def _escape_config_value(value): + result = "'" + + for ch in value: + if (ch == "'"): + result += "\\'" + elif (ch == "\n"): + result += "\\n" + elif (ch == "\r"): + result += "\\r" + elif (ch == "\t"): + result += "\\t" + elif (ch == "\b"): + result += "\\b" + elif (ch == "\\"): + result += "\\\\" + else: + result += ch + + result += "'" + return result + class NodeApp: diff --git a/tests/test_simple.py b/tests/test_simple.py index a11d7932..ffefda6c 100644 --- a/tests/test_simple.py +++ b/tests/test_simple.py @@ -1062,13 +1062,35 @@ def test_simple_with_bin_dir(self): pass # Expected error def test_set_auto_conf(self): + # elements contain [property id, value, storage value] + testData = [ + ["archive_command", + "cp '%p' \"/mnt/server/archivedir/%f\"", + "'cp \\'%p\\' \"/mnt/server/archivedir/%f\""], + ["restore_command", + 'cp "/mnt/server/archivedir/%f" \'%p\'', + "'cp \"/mnt/server/archivedir/%f\" \\'%p\\''"], + ["log_line_prefix", + "'\n\r\t\b\\\"", + "'\\\'\\n\\r\\t\\b\\\\\""], + ["log_connections", + True, + "on"], + ["log_disconnections", + False, + "off"], + ["autovacuum_max_workers", + 3, + "3"] + ] + with get_new_node() as node: node.init().start() - options = { - "archive_command": "cp '%p' \"/mnt/server/archivedir/%f\"", - 'restore_command': 'cp "/mnt/server/archivedir/%f" \'%p\'', - } + options = {} + + for x in testData: + options[x[0]] = x[1] node.set_auto_conf(options) node.stop() @@ -1077,16 +1099,13 @@ def test_set_auto_conf(self): auto_conf_path = f"{node.data_dir}/postgresql.auto.conf" with open(auto_conf_path, "r") as f: content = f.read() - self.assertIn( - "archive_command = 'cp \\'%p\\' \"/mnt/server/archivedir/%f\"", - content, - "archive_command stored wrong" - ) - self.assertIn( - "restore_command = 'cp \"/mnt/server/archivedir/%f\" \\'%p\\''", - content, - "restore_command stored wrong" - ) + + for x in testData: + self.assertIn( + x[0] + " = " + x[2], + content, + x[0] + " stored wrong" + ) if __name__ == '__main__': From ee7dc91f2f56d80ca975f091c4dd60a004770177 Mon Sep 17 00:00:00 2001 From: "d.kovalenko" Date: Tue, 3 Dec 2024 19:01:12 +0300 Subject: [PATCH 3/5] Asserts in PostgresNode.set_auto_conf are added Let's control a correct usage of this function. Plus one assert was added in _escape_config_value. --- testgres/node.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testgres/node.py b/testgres/node.py index 7469b0d6..74a18cdf 100644 --- a/testgres/node.py +++ b/testgres/node.py @@ -1634,6 +1634,10 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}): current_options[name] = var for option in options: + assert type(option) == str + assert option != "" + assert option.strip() == option + value = options[option] valueType = type(value) @@ -1695,6 +1699,8 @@ def _get_bin_path(self, filename): return bin_path def _escape_config_value(value): + assert type(value) == str + result = "'" for ch in value: From 9dd8dfec44ef56f732761d31c94cf98764a820a8 Mon Sep 17 00:00:00 2001 From: "d.kovalenko" Date: Tue, 3 Dec 2024 21:11:00 +0300 Subject: [PATCH 4/5] Flake8 E721 is suppressed --- testgres/node.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testgres/node.py b/testgres/node.py index 74a18cdf..d9fb0604 100644 --- a/testgres/node.py +++ b/testgres/node.py @@ -1634,7 +1634,7 @@ def set_auto_conf(self, options, config='postgresql.auto.conf', rm_options={}): current_options[name] = var for option in options: - assert type(option) == str + assert type(option) == str # noqa: E721 assert option != "" assert option.strip() == option @@ -1699,7 +1699,7 @@ def _get_bin_path(self, filename): return bin_path def _escape_config_value(value): - assert type(value) == str + assert type(value) == str # noqa: E721 result = "'" From 1335e517e6d8bc16f2d628446c02a03078c5957e Mon Sep 17 00:00:00 2001 From: "d.kovalenko" Date: Wed, 4 Dec 2024 10:00:58 +0300 Subject: [PATCH 5/5] PostgresNode::_escape_config_value is updated (code style) --- testgres/node.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/testgres/node.py b/testgres/node.py index d9fb0604..1706de11 100644 --- a/testgres/node.py +++ b/testgres/node.py @@ -1704,17 +1704,17 @@ def _escape_config_value(value): result = "'" for ch in value: - if (ch == "'"): + if ch == "'": result += "\\'" - elif (ch == "\n"): + elif ch == "\n": result += "\\n" - elif (ch == "\r"): + elif ch == "\r": result += "\\r" - elif (ch == "\t"): + elif ch == "\t": result += "\\t" - elif (ch == "\b"): + elif ch == "\b": result += "\\b" - elif (ch == "\\"): + elif ch == "\\": result += "\\\\" else: result += ch