enh(Data): SQLParser test coverage and integration assertions #5370

Adds tests that lock down the parser-layer behaviour POCO's production
code relies on, plus reproductions of recent fix motivations so any
upstream sql-parser sync that regresses them fails loudly in the test
layer rather than silently in MemoryDB / Utility.

Data/testsuite/src/SQLParserTest:
- testResetClearsParameters: reparse loop with reset() between
  iterations - the lifecycle the upstream sval-leak fix hardened.
- testNamedParameter: :name placeholders parse to kExprParameterNamed
  with names {user, age}.
- testAlterDropColumnIfExists: down-casts to AlterStatement +
  DropColumnAction and asserts columnName, ifExists, table name.
- testDropDiscrimination: DROP TABLE vs DROP INDEX dispatch.
- testDeleteShape: DeleteStatement::expr nullptr distinguishes truncate
  from filtered delete - what MemoryDB branches on for the truncate
  optimization.
- testComments: -- line comments survive parsing across statements.
- testTokenize: backstops the SQLParser::tokenize loop the upstream
  sval-leak fix rewrote.
- Inline fix to the pre-existing testSQLParser failure formatter: L13
  was rendering with no separator between line and column; replaced
  with L<line>:C<col> and switched bare fail() to failmsg() so the
  formatted message actually surfaces on the CppUnit report.

Data/SQLite/testsuite/src/UtilityTest:
- testBoundSQLNamed: pins the current pass-through behaviour of :name
  placeholders in Utility::boundSQL (Utility.cpp kExprParameterNamed
  branch sets paramIndex=npos so the span is copied verbatim and the
  supplied args are silently ignored). If this branch is ever taught
  to substitute values, the test must be rewritten.

Data/SQLite/testsuite/src/MemoryDBTest:
- testCommentPrefixedWithoutRowidRejected: covers both operator<< and
  session() bypass paths with a leading -- comment. Pins the motivation
  for the leading-comment-skip rewrite in onStatement; the previous
  single-char trigger-marker check let -- prefixed CREATE bypass the
  trace-hook backstop.
- testDropTableClassified: DROP TABLE survives close-reopen via the
  schema log, proving the kStmtDrop + kDropTable dispatch.
- testAlterTableClassified: ALTER TABLE ADD COLUMN survives close-
  reopen. ADD COLUMN is not modelled as an AlterAction by hsql so this
  exercises the first-keyword DDL fallback in onStatement.

Author: aleks-f

Data/SQLite/testsuite/src/MemoryDBTest.cpp

@@ -1249,6 +1249,102 @@ void MemoryDBTest::testConcurrentAccess()
 }
 
 
+void MemoryDBTest::testCommentPrefixedWithoutRowidRejected()
+{
+	// Two paths. (a) The operator<< prophylactic check mentionsWithoutRowid()
+	// requires CREATE as the first non-whitespace token and therefore does NOT
+	// see through leading line comments - the CREATE statement is allowed to
+	// reach the in-memory SQLite. (b) The session() bypass skips even that
+	// check. In both cases the reactive trace-hook backstop must catch the
+	// WITHOUT ROWID and poison the instance so subsequent operations throw.
+	// This is what the leading-comment-skip rewrite in onStatement was added
+	// to guarantee; previously the single-char "if (sql[0] == '-')" treated
+	// any --prefixed user SQL as a SQLite trigger marker and skipped it.
+
+	// (a) operator<< path with leading comment
+	{
+		MemoryDB db(_dir);
+		db << "-- a leading comment\nCREATE TABLE t(id INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID", now;
+		try
+		{
+			std::string h("hi");
+			db << "INSERT INTO t(v) VALUES(:v)", use(h), now;
+			failmsg("trace-hook backstop must reject comment-prefixed WITHOUT ROWID on operator<< path");
+		}
+		catch (Poco::NotImplementedException&)
+		{
+		}
+	}
+
+	// (b) session() bypass with leading comment
+	{
+		MemoryDB db(_dir);
+		db.session() << "-- a leading comment\nCREATE TABLE t(id INTEGER PRIMARY KEY, v TEXT) WITHOUT ROWID", now;
+		try
+		{
+			std::string h("hi");
+			db << "INSERT INTO t(v) VALUES(:v)", use(h), now;
+			failmsg("trace-hook backstop must reject comment-prefixed WITHOUT ROWID via session() bypass");
+		}
+		catch (Poco::NotImplementedException&)
+		{
+		}
+	}
+}
+
+
+void MemoryDBTest::testDropTableClassified()
+{
+	// DROP TABLE must be classified by the trace hook as kStmtDrop + kDropTable
+	// and replayed via the schema log on reopen so the table is genuinely absent,
+	// not just absent from main while still in a sealed shard. Pins the dispatch
+	// at the kStmtDrop branch in MemoryDB::onStatement.
+	{
+		MemoryDB db(_dir);
+		db << "CREATE TABLE t(id INTEGER PRIMARY KEY, v TEXT)", now;
+		std::string x("x");
+		db << "INSERT INTO t(v) VALUES(:v)", use(x), now;
+		db.flush();
+
+		db << "DROP TABLE t", now;
+		db.flush();
+	}
+
+	MemoryDB db(_dir);
+	int tcnt = -1;
+	db << "SELECT count(*) FROM sqlite_master WHERE type='table' AND name='t'",
+		into(tcnt), now;
+	assertTrue (tcnt == 0);
+}
+
+
+void MemoryDBTest::testAlterTableClassified()
+{
+	// ALTER TABLE ADD COLUMN: hsql parses this but does not currently model
+	// ADD COLUMN as an AlterAction (only DROP COLUMN is in the enum), so the
+	// trace hook's parser branch reports !isValid() and the keyword-fallback
+	// path classifies it as DDL via the first-keyword check. The schema log
+	// records the ALTER either way; on reopen the column must be present.
+	{
+		MemoryDB db(_dir);
+		db << "CREATE TABLE t(id INTEGER PRIMARY KEY, a TEXT)", now;
+		std::string a1("a1");
+		db << "INSERT INTO t(a) VALUES(:a)", use(a1), now;
+
+		db << "ALTER TABLE t ADD COLUMN b TEXT", now;
+
+		std::string a2("a2"), b2("b2");
+		db << "INSERT INTO t(a, b) VALUES(:a, :b)", use(a2), use(b2), now;
+		db.flush();
+	}
+
+	MemoryDB db(_dir);
+	std::string b;
+	db << "SELECT b FROM t WHERE id=2", into(b), now;
+	assertTrue (b == "b2");
+}
+
+
 CppUnit::Test* MemoryDBTest::suite()
 {
 	CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("MemoryDBTest");
@@ -1262,10 +1358,13 @@ CppUnit::Test* MemoryDBTest::suite()
 	CppUnit_addTest(pSuite, MemoryDBTest, testTruncate);
 	CppUnit_addTest(pSuite, MemoryDBTest, testWithoutRowidRejected);
 	CppUnit_addTest(pSuite, MemoryDBTest, testWithoutRowidRejectedViaSession);
+	CppUnit_addTest(pSuite, MemoryDBTest, testCommentPrefixedWithoutRowidRejected);
 	CppUnit_addTest(pSuite, MemoryDBTest, testLoadArchivedFalse);
 	CppUnit_addTest(pSuite, MemoryDBTest, testIdleFlush);
 	CppUnit_addTest(pSuite, MemoryDBTest, testCustomShardNamer);
 	CppUnit_addTest(pSuite, MemoryDBTest, testIndexPreservedAcrossReload);
+	CppUnit_addTest(pSuite, MemoryDBTest, testDropTableClassified);
+	CppUnit_addTest(pSuite, MemoryDBTest, testAlterTableClassified);
 	CppUnit_addTest(pSuite, MemoryDBTest, testHistoryView);
 	CppUnit_addTest(pSuite, MemoryDBTest, testDeleteSealedShardLoaded);
 	CppUnit_addTest(pSuite, MemoryDBTest, testDeleteSealedShardUnloaded);

Data/SQLite/testsuite/src/MemoryDBTest.h

@@ -34,10 +34,13 @@ class MemoryDBTest: public CppUnit::TestCase
 	void testTruncate();
 	void testWithoutRowidRejected();
 	void testWithoutRowidRejectedViaSession();
+	void testCommentPrefixedWithoutRowidRejected();
 	void testLoadArchivedFalse();
 	void testIdleFlush();
 	void testCustomShardNamer();
 	void testIndexPreservedAcrossReload();
+	void testDropTableClassified();
+	void testAlterTableClassified();
 	void testHistoryView();
 	void testDeleteSealedShardLoaded();
 	void testDeleteSealedShardUnloaded();

Data/SQLite/testsuite/src/UtilityTest.cpp

@@ -252,6 +252,25 @@ void UtilityTest::testBoundSQLMixedStyles()
 }
 
 
+void UtilityTest::testBoundSQLNamed()
+{
+	// Pins the current pass-through behaviour of the kExprParameterNamed
+	// branch in Utility.cpp:131-134: paramIndex is set to npos and srcLen
+	// to 1 + strlen(name), so renderRow at Utility.cpp:162-163 copies the
+	// original :name span verbatim and the Style stays None - the arity
+	// checks at lines 144/146 never fire and the supplied args are
+	// silently ignored.
+	//
+	// Net effect: :name placeholders are a no-op pass-through; the args
+	// are dropped and the returned SQL equals the input SQL. If Utility
+	// is ever taught to actually substitute named placeholders, this
+	// test must be rewritten.
+	const std::string sql = "SELECT * FROM t WHERE u = :user AND a = :age";
+	const std::string s = Utility::boundSQL(sql, std::string("John"), 42);
+	assertTrue (s == sql);
+}
+
+
 void UtilityTest::testExecuteSQLSuccess()
 {
 	Session session(Connector::KEY, ":memory:");
@@ -619,6 +638,7 @@ CppUnit::Test* UtilityTest::suite()
 	CppUnit_addTest(pSuite, UtilityTest, testBoundSQLDollarOutOfRange);
 	CppUnit_addTest(pSuite, UtilityTest, testBoundSQLDollarZero);
 	CppUnit_addTest(pSuite, UtilityTest, testBoundSQLMixedStyles);
+	CppUnit_addTest(pSuite, UtilityTest, testBoundSQLNamed);
 	CppUnit_addTest(pSuite, UtilityTest, testExecuteSQLSuccess);
 	CppUnit_addTest(pSuite, UtilityTest, testExecuteSQLConstraintFailure);
 	CppUnit_addTest(pSuite, UtilityTest, testExecuteSQLMalformed);

Data/SQLite/testsuite/src/UtilityTest.h

@@ -36,6 +36,7 @@ class UtilityTest: public CppUnit::TestCase
 	void testBoundSQLDollarOutOfRange();
 	void testBoundSQLDollarZero();
 	void testBoundSQLMixedStyles();
+	void testBoundSQLNamed();
 	void testExecuteSQLSuccess();
 	void testExecuteSQLConstraintFailure();
 	void testExecuteSQLMalformed();

Data/testsuite/src/SQLParserTest.cpp

@@ -15,7 +15,12 @@
 #include "CppUnit/TestCaller.h"
 #include "CppUnit/TestSuite.h"
 #include <sstream>
+#include <set>
+#include <cstdint>
 #include "SQLParser.h"
+#include "sql/AlterStatement.h"
+#include "sql/DropStatement.h"
+#include "sql/DeleteStatement.h"
 
 
 namespace Poco::Data {
@@ -57,8 +62,8 @@ void SQLParserTest::testSQLParser()
 	{
 		std::ostringstream os;
 		os << "Given string is not a valid SQL query.\n";
-		os << result.errorMsg() << "(L" << result.errorLine() << result.errorColumn() << ")\n";
-		fail(os.str());
+		os << result.errorMsg() << " (L" << result.errorLine() << ":C" << result.errorColumn() << ")\n";
+		failmsg(os.str());
 	}
 	assertTrue(result.isValid());
 	assertEqual(8, result.size());
@@ -93,6 +98,165 @@ void SQLParserTest::testSQLParser()
 }
 
 
+void SQLParserTest::testInvalidSQL()
+{
+	std::string query = "SELECT FROM WHERE ;";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (!result.isValid());
+	assertTrue (result.errorMsg() != nullptr && std::string(result.errorMsg()).length() > 0);
+}
+
+
+void SQLParserTest::testResetClearsParameters()
+{
+	// Reuses the testSQLParser query (8 statements, 2 '?' parameters) and
+	// reparses N times with reset() between. Pins the lifecycle path the
+	// upstream commit 38d1a66 hardened (sval leaks across iterations).
+	std::string query = "INSERT INTO Test VALUES ('1', 2, 3.5);"
+		"SELECT * FROM Test WHERE First = ?;"
+		"UPDATE Test SET value=1 WHERE First = '1';"
+		"DELETE FROM Test WHERE First = ?;"
+		"DROP TABLE table_name;"
+		"ALTER TABLE mytable DROP COLUMN IF EXISTS mycolumn;"
+		"PREPARE prep_inst FROM 'INSERT INTO test VALUES (?, ?, ?)';"
+		"EXECUTE prep_inst(1, 2, 3);";
+
+	SQLParserResult result;
+	for (int i = 0; i < 5; ++i)
+	{
+		SQLParser::parse(query, &result);
+		assertTrue (result.isValid());
+		assertEqual(8, result.size());
+		assertEqual(2, static_cast<int>(result.parameters().size()));
+		result.reset();
+	}
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(8, result.size());
+	assertEqual(2, static_cast<int>(result.parameters().size()));
+}
+
+
+void SQLParserTest::testNamedParameter()
+{
+	std::string query = "SELECT * FROM t WHERE u = :user AND a = :age";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(1, result.size());
+	assertEqual(2, static_cast<int>(result.parameters().size()));
+
+	std::set<std::string> names;
+	for (const Expr* e: result.parameters())
+	{
+		assertTrue (e->type == kExprParameterNamed);
+		assertTrue (e->name != nullptr);
+		names.insert(std::string(e->name));
+	}
+	std::set<std::string> expected;
+	expected.insert("user");
+	expected.insert("age");
+	assertTrue (names == expected);
+}
+
+
+void SQLParserTest::testAlterDropColumnIfExists()
+{
+	std::string query = "ALTER TABLE mytable DROP COLUMN IF EXISTS mycolumn";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(1, result.size());
+
+	const SQLStatement* stmt = result.getStatement(0);
+	assertTrue (stmt->type() == kStmtAlter);
+	const AlterStatement* alter = static_cast<const AlterStatement*>(stmt);
+	assertTrue (alter->name == std::string("mytable"));
+
+	const DropColumnAction* action = static_cast<const DropColumnAction*>(alter->action);
+	assertTrue (action->columnName == std::string("mycolumn"));
+	assertTrue (action->ifExists);
+}
+
+
+void SQLParserTest::testDropDiscrimination()
+{
+	std::string query = "DROP TABLE t; DROP INDEX idx_t;";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(2, result.size());
+
+	const SQLStatement* s0 = result.getStatement(0);
+	assertTrue (s0->type() == kStmtDrop);
+	const DropStatement* d0 = static_cast<const DropStatement*>(s0);
+	assertTrue (d0->type == kDropTable);
+	assertTrue (d0->name == std::string("t"));
+
+	const SQLStatement* s1 = result.getStatement(1);
+	assertTrue (s1->type() == kStmtDrop);
+	const DropStatement* d1 = static_cast<const DropStatement*>(s1);
+	assertTrue (d1->type == kDropIndex);
+}
+
+
+void SQLParserTest::testDeleteShape()
+{
+	// MemoryDB.cpp:1232 specifically branches on d->expr == nullptr for
+	// the truncate-detect optimization. Pin both shapes here so a parser
+	// regression that flipped the meaning would break this test, not
+	// just MemoryDB's truncate behavior under load.
+	std::string query = "DELETE FROM t WHERE id=1; DELETE FROM t;";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(2, result.size());
+
+	const DeleteStatement* d0 = static_cast<const DeleteStatement*>(result.getStatement(0));
+	assertTrue (d0->expr != nullptr);
+
+	const DeleteStatement* d1 = static_cast<const DeleteStatement*>(result.getStatement(1));
+	assertTrue (d1->expr == nullptr);
+	assertTrue (d1->tableName == std::string("t"));
+}
+
+
+void SQLParserTest::testComments()
+{
+	// hsql lexer supports -- line comments; the C-style /* ... */ form is
+	// NOT supported by this lexer build, so we exercise only the line-
+	// comment variant. MemoryDB.cpp's trace-hook skipLeading*Comments is
+	// the production path that must handle both - that path is tested
+	// downstream in MemoryDBTest::testCommentPrefixedWithoutRowidRejected.
+	std::string query = "-- header\nSELECT * FROM t;\n-- trailing\nINSERT INTO t VALUES(1);";
+	SQLParserResult result;
+	SQLParser::parse(query, &result);
+	assertTrue (result.isValid());
+	assertEqual(2, result.size());
+	assertTrue (result.getStatement(0)->type() == kStmtSelect);
+	assertTrue (result.getStatement(1)->type() == kStmtInsert);
+}
+
+
+void SQLParserTest::testTokenize()
+{
+	// Backstops the SQLParser::tokenize loop rewrite landed in upstream
+	// commit 38d1a66 (sval-per-iteration leaks, NAMED_PARAM inclusion).
+	std::string query = "SELECT id FROM t WHERE name = :who AND age = $1";
+	std::vector<int16_t> tokens;
+	try
+	{
+		SQLParser::tokenize(query, &tokens);
+		assertTrue (!tokens.empty());
+	}
+	catch (...)
+	{
+		failmsg("SQLParser::tokenize threw an exception");
+	}
+}
+
+
 void SQLParserTest::setUp()
 {
 }
@@ -108,6 +272,14 @@ CppUnit::Test* SQLParserTest::suite()
 	CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("SQLParserTest");
 
 	CppUnit_addTest(pSuite, SQLParserTest, testSQLParser);
+	CppUnit_addTest(pSuite, SQLParserTest, testInvalidSQL);
+	CppUnit_addTest(pSuite, SQLParserTest, testResetClearsParameters);
+	CppUnit_addTest(pSuite, SQLParserTest, testNamedParameter);
+	CppUnit_addTest(pSuite, SQLParserTest, testAlterDropColumnIfExists);
+	CppUnit_addTest(pSuite, SQLParserTest, testDropDiscrimination);
+	CppUnit_addTest(pSuite, SQLParserTest, testDeleteShape);
+	CppUnit_addTest(pSuite, SQLParserTest, testComments);
+	CppUnit_addTest(pSuite, SQLParserTest, testTokenize);
 
 	return pSuite;
 }

Data/testsuite/src/SQLParserTest.h

@@ -28,6 +28,14 @@ class SQLParserTest: public CppUnit::TestCase
 	~SQLParserTest();
 
 	void testSQLParser();
+	void testInvalidSQL();
+	void testResetClearsParameters();
+	void testNamedParameter();
+	void testAlterDropColumnIfExists();
+	void testDropDiscrimination();
+	void testDeleteShape();
+	void testComments();
+	void testTokenize();
 
 	void setUp();
 	void tearDown();