From 210c119907e0ab5823e0e72261b081e7564d0e9b Mon Sep 17 00:00:00 2001 From: feiiiiii5 Date: Tue, 29 Sep 2026 00:24:06 +0800 Subject: [PATCH] insert --convert: validate the result instead of falling back on falsy The JSON/CSV branch used 'fn(doc) or doc', so any falsy result was discarded and the unconverted row inserted. Exit code 0, no warning: $ echo '[{"id": 1, "name": "Cleo"}]' | sqlite-utils insert d.db t - --convert '{}' $ sqlite-utils 'select * from d.db' # -> the original row, not {} The realistic shape of this is a filter: --convert 'row if row["ok"] else {}' # meant to drop the bad rows # every row inserted, including the ones meant to be dropped '--lines' runs a different expression twelve lines up and has no such fallback, so the same --convert code is rejected there and silently accepted here: --convert 0 JSON input -> exit 0, rows inserted --convert 0 --lines -> exit 1, Rows must all be dictionaries, got: 0 test_insert_convert_error_messages already asserts that a non-dict return is rejected (--convert 1); 0, False, [] and "" were the outliers, purely because they are falsy. None is the one falsy result that has to keep meaning 'modify doc in place' - a --convert that is a bare statement returns it, which test_insert_convert_row_modifying_in_place covers - so the fallback is now keyed on 'is None' rather than truthiness. Tests: 7 new cases in tests/test_cli_insert.py, all failing on 6bc1d33. Suite 1505 passed, 16 skipped. black, flake8, mypy clean. pyright reports one error and ty two diagnostics, both identical on unmodified main (tests/test_constructor.py:74 and two unused-type-ignore comments in db.py/utils.py) and none in the files this touches. --- sqlite_utils/cli.py | 14 ++++++++- tests/test_cli_insert.py | 67 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/sqlite_utils/cli.py b/sqlite_utils/cli.py index c23090283..2eaf497d1 100644 --- a/sqlite_utils/cli.py +++ b/sqlite_utils/cli.py @@ -1312,7 +1312,19 @@ def _insert_docs(docs, tracker=None): "--convert must return dict or iterator" ) else: - docs = (fn(doc) or doc for doc in docs) + + def convert_doc(doc, fn=fn): + # A --convert expression that is a statement returns + # None, and means "modify doc in place", so None is + # the one falsy result that falls back to the + # original row. Any other falsy result is a value + # the caller returned deliberately and has to be + # validated like --lines already does, rather than + # silently inserting the row unconverted. + result = fn(doc) + return doc if result is None else result + + docs = (convert_doc(doc) for doc in docs) _insert_docs(docs, tracker=tracker) diff --git a/tests/test_cli_insert.py b/tests/test_cli_insert.py index 011786259..3f4e23904 100644 --- a/tests/test_cli_insert.py +++ b/tests/test_cli_insert.py @@ -530,6 +530,73 @@ def test_insert_convert_row_modifying_in_place(db_path): assert rows == [{"name": "Azi", "is_chicken": 1}] +@pytest.mark.parametrize( + "convert,expected_value", + ( + ("0", "0"), + ('""', "''"), + ("[]", "[]"), + ("False", "False"), + ), +) +def test_insert_convert_falsy_result_is_rejected(db_path, convert, expected_value): + # A --convert expression that is a statement returns None and is + # documented to modify row in place. Every other falsy value used to + # be treated the same way, so the row was inserted unconverted. + result = CliRunner().invoke( + cli.cli, + ["insert", db_path, "rows", "-", "--convert", convert], + input='{"name": "Azi"}', + ) + assert result.exit_code == 1 + assert result.output == ( + f"Error: Rows must all be dictionaries, got: {expected_value}\n" + ) + + +def test_insert_convert_returning_none_still_inserts_the_row(db_path): + # None stays the in-place signal: a --convert that is a bare + # statement returns it, and the row is inserted as it arrived. + result = CliRunner().invoke( + cli.cli, + ["insert", db_path, "rows", "-", "--convert", "None"], + input='{"name": "Azi"}', + ) + assert result.exit_code == 0, result.output + db = Database(db_path) + assert list(db.query("select * from rows")) == [{"name": "Azi"}] + + +def test_insert_convert_returning_empty_dict_does_not_insert_the_original(db_path): + # {} is a dict, so it is accepted - but it used to be discarded as + # falsy and the unconverted row inserted in its place. + result = CliRunner().invoke( + cli.cli, + ["insert", db_path, "rows", "-", "--convert", 'row if row["id"] else {}'], + input='[{"id": 1, "name": "Azi"}, {"id": 0, "name": "Not me"}]', + ) + assert result.exit_code == 0, result.output + db = Database(db_path) + # no ORDER BY: the empty row sorts first under "order by id" + assert list(db.query("select * from rows")) == [ + {"id": 1, "name": "Azi"}, + {"id": None, "name": None}, + ] + + +@pytest.mark.parametrize("convert", ("0", "{}")) +def test_lines_and_json_convert_reject_the_same_falsy_result(db_path, convert): + # The two input formats run different expressions, so the same + # --convert code has to behave the same way in both + for options in ([], ["--lines"]): + result = CliRunner().invoke( + cli.cli, + ["insert", db_path, "rows", "-", *options, "--convert", convert], + input='{"name": "Azi"}', + ) + assert result.exit_code == 1, (options, convert, result.output) + + @pytest.mark.parametrize( "options,expected_error", (