Skip to content

Commit

Permalink
Remove deprecated sql::Database::RazeAndClose method
Browse files Browse the repository at this point in the history 
It has been replaced by the identically behaving albeit more precisely
named RazeAndPoison.

Bug: 1428036
Change-Id: I949b411bd4fcc6b7ae06ed3c11f6f519d7cc4175
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377358
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123105}
  • Loading branch information
Andrew Paseltiner authored and Chromium LUCI CQ committed Mar 28, 2023
1 parent 3d7a94c commit d309fec
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 72 deletions.
10 changes: 3 additions & 7 deletions sql/database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ void Database::CloseInternal(bool forced) {

void Database::Close() {
TRACE_EVENT0("sql", "Database::Close");
// If the database was already closed by RazeAndClose(), then no
// If the database was already closed by RazeAndPoison(), then no
// need to close again. Clear the |poisoned_| bit so that incorrect
// API calls are caught.
if (poisoned_) {
Expand Down Expand Up @@ -1093,11 +1093,7 @@ bool Database::Raze() {
}

bool Database::RazeAndPoison() {
return RazeAndClose();
}

bool Database::RazeAndClose() {
TRACE_EVENT0("sql", "Database::RazeAndClose");
TRACE_EVENT0("sql", "Database::RazeAndPoison");

if (!db_) {
DCHECK(poisoned_) << "Cannot raze null db";
Expand Down Expand Up @@ -1801,7 +1797,7 @@ bool Database::OpenInternal(const std::string& db_file_path,
EnsureSqliteInitialized();

// If |poisoned_| is set, it means an error handler called
// RazeAndClose(). Until regular Close() is called, the caller
// RazeAndPoison(). Until regular Close() is called, the caller
// should be treating the database as open, but is_open() currently
// only considers the sqlite3 handle's state.
// TODO(shess): Revise is_open() to consider poisoned_, and review
Expand Down
12 changes: 4 additions & 8 deletions sql/database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,10 +447,6 @@ class COMPONENT_EXPORT(SQL) Database {
// Close() should still be called at some point.
void Poison();

// Deprecated: Renamed to `RazeAndPoison()`.
// TODO(apaseltiner): Remove this once all callers have been migrated.
bool RazeAndClose();

// `Raze()` the database and `Poison()` the handle. Returns the return
// value from `Raze()`.
bool RazeAndPoison();
Expand Down Expand Up @@ -724,7 +720,7 @@ class COMPONENT_EXPORT(SQL) Database {
kNone = 0,

// Retry if the database error handler is invoked and closes the database.
// Database error handlers that call RazeAndClose() take advantage of this.
// Database error handlers that call RazeAndPoison() take advantage of this.
kRetryOnPoision = 1,

// Open an in-memory database. Used by OpenInMemory().
Expand All @@ -749,7 +745,7 @@ class COMPONENT_EXPORT(SQL) Database {
// called on the object.
void ConfigureSqliteDatabaseObject();

// Internal close function used by Close() and RazeAndClose().
// Internal close function used by Close() and RazeAndPoison().
// |forced| indicates that orderly-shutdown checks should not apply.
void CloseInternal(bool forced);

Expand Down Expand Up @@ -817,7 +813,7 @@ class COMPONENT_EXPORT(SQL) Database {

// Destroys the compiled statement and sets it to nullptr. The statement
// will no longer be active. |forced| is used to indicate if
// orderly-shutdown checks should apply (see Database::RazeAndClose()).
// orderly-shutdown checks should apply (see Database::RazeAndPoison()).
void Close(bool forced);

// Construct a ScopedBlockingCall to annotate IO calls, but only if
Expand Down Expand Up @@ -982,7 +978,7 @@ class COMPONENT_EXPORT(SQL) Database {
// with Open().
bool in_memory_ = false;

// |true| if the Database was closed using RazeAndClose(). Used
// |true| if the Database was closed using RazeAndPoison(). Used
// to enable diagnostics to distinguish calls to never-opened
// databases (incorrect use of the API) from calls to once-valid
// databases.
Expand Down
105 changes: 51 additions & 54 deletions sql/database_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ void RazeErrorCallback(Database* db,
// Nothing here needs extended errors at this time.
EXPECT_EQ(expected_error, expected_error & 0xff);
EXPECT_EQ(expected_error, error & 0xff);
db->RazeAndClose();
db->RazeAndPoison();
}

#if BUILDFLAG(IS_POSIX)
Expand Down Expand Up @@ -1029,182 +1029,179 @@ TEST_P(SQLDatabaseTest, RazeCallbackReopen) {
base::BindRepeating(&RazeErrorCallback, db_.get(), SQLITE_CORRUPT));

// When the PRAGMA calls in Open() raise SQLITE_CORRUPT, the error
// callback will call RazeAndClose(). Open() will then fail and be
// callback will call RazeAndPoison(). Open() will then fail and be
// retried. The second Open() on the empty database will succeed
// cleanly.
ASSERT_TRUE(db_->Open(db_path_));
ASSERT_TRUE(db_->Execute("PRAGMA auto_vacuum"));
EXPECT_EQ(0, SqliteSchemaCount(db_.get()));
}

TEST_P(SQLDatabaseTest, RazeAndClose_DeletesData) {
TEST_P(SQLDatabaseTest, RazeAndPoison_DeletesData) {
ASSERT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"));
ASSERT_TRUE(db_->Execute("INSERT INTO rows(id) VALUES(12)"));
ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());

// RazeAndClose() actually Poison()s. We need to call Close() in order to
// re-Open(). crbug.com/1311771 tracks renaming RazeAndClose().
// We need to call Close() in order to re-Open().
db_->Close();
ASSERT_TRUE(db_->Open(db_path_))
<< "RazeAndClose() did not produce a healthy database";
<< "RazeAndPoison() did not produce a healthy database";
EXPECT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"))
<< "RazeAndClose() did not produce a healthy empty database";
<< "RazeAndPoison() did not produce a healthy empty database";
}

TEST_P(SQLDatabaseTest, RazeAndClose_IsOpen) {
TEST_P(SQLDatabaseTest, RazeAndPoison_IsOpen) {
ASSERT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"));
ASSERT_TRUE(db_->Execute("INSERT INTO rows(id) VALUES(12)"));
ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());

EXPECT_FALSE(db_->is_open())
<< "RazeAndClose() did not mark the database as closed";
<< "RazeAndPoison() did not mark the database as closed";
}

TEST_P(SQLDatabaseTest, RazeAndClose_Reopen_NoChanges) {
ASSERT_TRUE(db_->RazeAndClose());
TEST_P(SQLDatabaseTest, RazeAndPoison_Reopen_NoChanges) {
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"))
<< "Execute() should return false after RazeAndClose()";
<< "Execute() should return false after RazeAndPoison()";

// RazeAndClose() actually Poison()s. We need to call Close() in order to
// re-Open(). crbug.com/1311771 tracks renaming RazeAndClose().
// We need to call Close() in order to re-Open().
db_->Close();
ASSERT_TRUE(db_->Open(db_path_))
<< "RazeAndClose() did not produce a healthy database";
<< "RazeAndPoison() did not produce a healthy database";
EXPECT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"))
<< "Execute() returned false but went through after RazeAndClose()";
<< "Execute() returned false but went through after RazeAndPoison()";
}

TEST_P(SQLDatabaseTest, RazeAndClose_OpenTransaction) {
TEST_P(SQLDatabaseTest, RazeAndPoison_OpenTransaction) {
ASSERT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"));
ASSERT_TRUE(db_->Execute("INSERT INTO rows(id) VALUES(12)"));

Transaction transaction(db_.get());
ASSERT_TRUE(transaction.Begin());
ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());

EXPECT_FALSE(db_->is_open())
<< "RazeAndClose() did not mark the database as closed";
<< "RazeAndPoison() did not mark the database as closed";
EXPECT_FALSE(transaction.Commit())
<< "RazeAndClose() did not cancel the transaction";
<< "RazeAndPoison() did not cancel the transaction";

// RazeAndClose() actually Poison()s. We need to call Close() in order to
// re-Open(). crbug.com/1311771 tracks renaming RazeAndClose().
// We need to call Close() in order to re-Open().
db_->Close();

ASSERT_TRUE(db_->Open(db_path_));
EXPECT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"))
<< "RazeAndClose() did not produce a healthy empty database";
<< "RazeAndPoison() did not produce a healthy empty database";
}

TEST_P(SQLDatabaseTest, RazeAndClose_Preload_NoCrash) {
TEST_P(SQLDatabaseTest, RazeAndPoison_Preload_NoCrash) {
db_->Preload();
db_->RazeAndClose();
db_->RazeAndPoison();
db_->Preload();
}

TEST_P(SQLDatabaseTest, RazeAndClose_DoesTableExist) {
TEST_P(SQLDatabaseTest, RazeAndPoison_DoesTableExist) {
ASSERT_TRUE(
db_->Execute("CREATE TABLE rows(id INTEGER PRIMARY KEY NOT NULL)"));
ASSERT_TRUE(db_->DoesTableExist("rows")) << "Incorrect test setup";

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(db_->DoesTableExist("rows"))
<< "DoesTableExist() should return false after RazeAndClose()";
<< "DoesTableExist() should return false after RazeAndPoison()";
}

TEST_P(SQLDatabaseTest, RazeAndClose_IsSQLValid) {
TEST_P(SQLDatabaseTest, RazeAndPoison_IsSQLValid) {
ASSERT_TRUE(db_->IsSQLValid("SELECT 1")) << "Incorrect test setup";

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(db_->IsSQLValid("SELECT 1"))
<< "IsSQLValid() should return false after RazeAndClose()";
<< "IsSQLValid() should return false after RazeAndPoison()";
}

TEST_P(SQLDatabaseTest, RazeAndClose_Execute) {
TEST_P(SQLDatabaseTest, RazeAndPoison_Execute) {
ASSERT_TRUE(db_->Execute("SELECT 1")) << "Incorrect test setup";

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(db_->Execute("SELECT 1"))
<< "Execute() should return false after RazeAndClose()";
<< "Execute() should return false after RazeAndPoison()";
}

TEST_P(SQLDatabaseTest, RazeAndClose_GetUniqueStatement) {
TEST_P(SQLDatabaseTest, RazeAndPoison_GetUniqueStatement) {
{
Statement select(db_->GetUniqueStatement("SELECT 1"));
ASSERT_TRUE(select.Step()) << "Incorrect test setup";
}

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
{
Statement select(db_->GetUniqueStatement("SELECT 1"));
EXPECT_FALSE(select.Step())
<< "GetUniqueStatement() should return an invalid Statement after "
<< "RazeAndClose()";
<< "RazeAndPoison()";
}
}

TEST_P(SQLDatabaseTest, RazeAndClose_GetCachedStatement) {
TEST_P(SQLDatabaseTest, RazeAndPoison_GetCachedStatement) {
{
Statement select(db_->GetCachedStatement(SQL_FROM_HERE, "SELECT 1"));
ASSERT_TRUE(select.Step()) << "Incorrect test setup";
}

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
{
Statement select(db_->GetCachedStatement(SQL_FROM_HERE, "SELECT 1"));
EXPECT_FALSE(select.Step())
<< "GetCachedStatement() should return an invalid Statement after "
<< "RazeAndClose()";
<< "RazeAndPoison()";
}
}

TEST_P(SQLDatabaseTest, RazeAndClose_InvalidatesUniqueStatement) {
TEST_P(SQLDatabaseTest, RazeAndPoison_InvalidatesUniqueStatement) {
Statement select(db_->GetUniqueStatement("SELECT 1"));
ASSERT_TRUE(select.is_valid()) << "Incorrect test setup";
ASSERT_TRUE(select.Step()) << "Incorrect test setup";
select.Reset(/*clear_bound_vars=*/true);

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(select.is_valid())
<< "RazeAndClose() should invalidate live Statements";
<< "RazeAndPoison() should invalidate live Statements";
EXPECT_FALSE(select.Step())
<< "RazeAndClose() should invalidate live Statements";
<< "RazeAndPoison() should invalidate live Statements";
}

TEST_P(SQLDatabaseTest, RazeAndClose_InvalidatesCachedStatement) {
TEST_P(SQLDatabaseTest, RazeAndPoison_InvalidatesCachedStatement) {
Statement select(db_->GetCachedStatement(SQL_FROM_HERE, "SELECT 1"));
ASSERT_TRUE(select.is_valid()) << "Incorrect test setup";
ASSERT_TRUE(select.Step()) << "Incorrect test setup";
select.Reset(/*clear_bound_vars=*/true);

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
EXPECT_FALSE(select.is_valid())
<< "RazeAndClose() should invalidate live Statements";
<< "RazeAndPoison() should invalidate live Statements";
EXPECT_FALSE(select.Step())
<< "RazeAndClose() should invalidate live Statements";
<< "RazeAndPoison() should invalidate live Statements";
}

TEST_P(SQLDatabaseTest, RazeAndClose_TransactionBegin) {
TEST_P(SQLDatabaseTest, RazeAndPoison_TransactionBegin) {
{
Transaction transaction(db_.get());
ASSERT_TRUE(transaction.Begin()) << "Incorrect test setup";
ASSERT_TRUE(transaction.Commit()) << "Incorrect test setup";
}

ASSERT_TRUE(db_->RazeAndClose());
ASSERT_TRUE(db_->RazeAndPoison());
{
Transaction transaction(db_.get());
EXPECT_FALSE(transaction.Begin())
<< "Transaction::Begin() should return false after RazeAndClose()";
<< "Transaction::Begin() should return false after RazeAndPoison()";
EXPECT_FALSE(transaction.IsActiveForTesting())
<< "RazeAndClose() should block transactions from starting";
<< "RazeAndPoison() should block transactions from starting";
}
}

Expand Down
2 changes: 1 addition & 1 deletion sql/recovery.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ void Recovery::Shutdown(Recovery::Disposition raze) {

recover_db_.Close();
if (raze == RAZE_AND_POISON) {
db_->RazeAndClose();
db_->RazeAndPoison();
} else if (raze == POISON) {
db_->Poison();
}
Expand Down
4 changes: 2 additions & 2 deletions sql/recovery.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace sql {
// }
// }
//
// If Recovered() is not called, then RazeAndClose() is called on
// If Recovered() is not called, then RazeAndPoison() is called on
// orig_db.

class COMPONENT_EXPORT(SQL) Recovery {
Expand Down Expand Up @@ -95,7 +95,7 @@ class COMPONENT_EXPORT(SQL) Recovery {
//
// TODO(shess): At this time, this function can fail while leaving
// the original database intact. Figure out which failure cases
// should go to RazeAndClose() instead.
// should go to RazeAndPoison() instead.
[[nodiscard]] static bool Recovered(std::unique_ptr<Recovery> r);

// Indicate that the database is unrecoverable. The original
Expand Down

0 comments on commit d309fec

Please sign in to comment.