Skip to content

Commit

Permalink
sql: Switch recovery module to use SQLITE_ERROR on bad queries.
Browse files Browse the repository at this point in the history 
//sql/recovery_module used SQLITE_MISUSE to signal receiving incorrect
SQL statements. However, SQLITE_MISUSE is intended for incorrect use of
the SQLite C API, and is mostly used to signal buggy lifecycle
management.

This CL switches //sql/recovery_module away from SQLITE_MISUSE.
SQLITE_ERROR is used instead, matching SQLite's behavior on SQL
statement issues such as incorrect row names and bad syntax.

Change-Id: I9b879c3351801d475898576bf7f6b3d6c5755676
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3536962
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983500}
  • Loading branch information
@pwnall
pwnall authored and Chromium LUCI CQ committed Mar 21, 2022
1 parent aa392a1 commit 1d764e6
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 14 deletions.
10 changes: 5 additions & 5 deletions sql/recover_module/module.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ int ModuleCreate(sqlite3* sqlite_db,
DCHECK(sqlite_db != nullptr);
if (argc <= kFirstColumnArgument) {
// The recovery module needs at least one column specification.
return SQLITE_MISUSE;
return SQLITE_ERROR;
}
DCHECK(argv != nullptr);
DCHECK(result_sqlite_table != nullptr);
Expand All @@ -76,29 +76,29 @@ int ModuleCreate(sqlite3* sqlite_db,
// temporary database (ATTACH '' AS other_temp). However, there is no easy
// way to determine if an attachment point corresponds to a temporary
// database, and "temp" is sufficient for Chrome's purposes.
return SQLITE_MISUSE;
return SQLITE_ERROR;
}

base::StringPiece table_name(argv[kVirtualTableNameArgument]);
if (!base::StartsWith(table_name, "recover_")) {
// In the future, we may deploy UMA metrics that use the virtual table name
// to attribute recovery events to Chrome features. In preparation for that
// future, require all recovery table names to start with "recover_".
return SQLITE_MISUSE;
return SQLITE_ERROR;
}

TargetTableSpec backing_table_spec =
ParseTableSpec(argv[kBackingTableSpecArgument]);
if (!backing_table_spec.IsValid()) {
// The parser concluded that the string specifying the backing table is
// invalid. This is definitely an error in the SQL using the virtual table.
return SQLITE_MISUSE;
return SQLITE_ERROR;
}

std::vector<RecoveredColumnSpec> column_specs = ParseColumnSpecs(argc, argv);
if (column_specs.empty()) {
// The column specifications were invalid.
return SQLITE_MISUSE;
return SQLITE_ERROR;
}

auto [sqlite_status, table] = VirtualTable::Create(
Expand Down
18 changes: 9 additions & 9 deletions sql/recover_module/module_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnNonTempTable) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(db_.Execute(
"CREATE VIRTUAL TABLE recover_backing USING recover(backing, t TEXT)"));
EXPECT_TRUE(error_expecter.SawExpectedErrors());
Expand Down Expand Up @@ -106,7 +106,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingTableName) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(main., t TEXT)"));
Expand All @@ -117,7 +117,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingSchemaSpec) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(backing)"));
Expand All @@ -128,7 +128,7 @@ TEST_F(RecoverModuleTest, CreateVtableFailsOnMissingDbName) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(.backing)"));
Expand Down Expand Up @@ -168,7 +168,7 @@ TEST_F(RecoverModuleTest, ColumnTypeMappingAnyStrict) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(backing, t ANY STRICT)"));
Expand All @@ -180,7 +180,7 @@ TEST_F(RecoverModuleTest, ColumnTypeExtraKeyword) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(backing, t INTEGER SOMETHING)"));
Expand All @@ -191,7 +191,7 @@ TEST_F(RecoverModuleTest, ColumnTypeNotNullExtraKeyword) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(backing, t INTEGER NOT NULL SOMETHING)"));
Expand All @@ -202,7 +202,7 @@ TEST_F(RecoverModuleTest, ColumnTypeDoubleTypes) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing "
"USING recover(backing, t INTEGER FLOAT)"));
Expand All @@ -213,7 +213,7 @@ TEST_F(RecoverModuleTest, ColumnTypeNotNullDoubleTypes) {
ASSERT_TRUE(db_.Execute("CREATE TABLE backing(t TEXT)"));
{
sql::test::ScopedErrorExpecter error_expecter;
error_expecter.ExpectError(SQLITE_MISUSE);
error_expecter.ExpectError(SQLITE_ERROR);
EXPECT_FALSE(
db_.Execute("CREATE VIRTUAL TABLE temp.recover_backing USING recover("
"backing, t INTEGER NOT NULL TEXT)"));
Expand Down

0 comments on commit 1d764e6

Please sign in to comment.