sql: Restore strict DCHECK of cached statements SQL.
https://crrev.com/c/1338177 relaxed the DCHECKs for
sql::Database::GetCachedStatment by allowing the SQL statement argument
to have leading and trailing whitespace when compared with the SQL
statement reported by SQLite.
This CL reverts the relaxed restriction. Instead, GetCachedStatement
will also DCHECK that the SQL statement argument matches the SQL
statement reported by SQLite when preparing the statement. This will
help developers discover non-canonical statements on first use.
Bug: 894884
Change-Id: I166f8cda8d5a3980d9858a26507f778bc87ee6cc
Reviewed-on: https://chromium-review.googlesource.com/c/1341493
Commit-Queue: Victor Costan <[email protected]>
Reviewed-by: Joshua Bell <[email protected]>
Cr-Commit-Position: refs/heads/master@{#609605}diff --git a/sql/database.cc b/sql/database.cc
index 8d3bf081..b1a7cdc5 100644
--- a/sql/database.cc
+++ b/sql/database.cc
@@ -1384,13 +1384,11 @@
const char* sql) {
auto it = statement_cache_.find(id);
if (it != statement_cache_.end()) {
- // Statement is in the cache. It should still be active. We're the only
- // one invalidating cached statements, and we remove them from the cache
+ // Statement is in the cache. It should still be valid. We're the only
+ // entity invalidating cached statements, and we remove them from the cache
// when we do that.
DCHECK(it->second->is_valid());
- DCHECK_EQ(base::TrimWhitespaceASCII(sql, base::TRIM_ALL),
- base::TrimWhitespaceASCII(sqlite3_sql(it->second->stmt()),
- base::TRIM_ALL))
+ DCHECK_EQ(std::string(sqlite3_sql(it->second->stmt())), std::string(sql))
<< "GetCachedStatement used with same ID but different SQL";
// Reset the statement so it can be reused.
@@ -1399,8 +1397,11 @@
}
scoped_refptr<StatementRef> statement = GetUniqueStatement(sql);
- if (statement->is_valid())
+ if (statement->is_valid()) {
statement_cache_[id] = statement; // Only cache valid statements.
+ DCHECK_EQ(std::string(sqlite3_sql(statement->stmt())), std::string(sql))
+ << "Input SQL does not match SQLite's normalized version";
+ }
return statement;
}
diff --git a/sql/database.h b/sql/database.h
index 7bee2567..8dc0dc4 100644
--- a/sql/database.h
+++ b/sql/database.h
@@ -376,19 +376,23 @@
// keeping commonly-used ones around for future use is important for
// performance.
//
+ // The SQL_FROM_HERE macro is the recommended way of generating a StatementID.
+ // Code that generates custom IDs must ensure that a StatementID is never used
+ // for different SQL statements. Failing to meet this requirement results in
+ // incorrect behavior, and should be caught by a DCHECK.
+ //
+ // The SQL statement passed in |sql| must match the SQL statement reported
+ // back by SQLite. Mismatches are caught by a DCHECK, so any code that has
+ // automated test coverage or that was manually tested on a DCHECK build will
+ // not exhibit this problem. Mismatches generally imply that the statement
+ // passed in has extra whitespace or comments surrounding it, which waste
+ // storage and CPU cycles.
+ //
// If the |sql| has an error, an invalid, inert StatementRef is returned (and
// the code will crash in debug). The caller must deal with this eventuality,
// either by checking validity of the |sql| before calling, by correctly
// handling the return of an inert statement, or both.
//
- // The StatementID and the SQL must always correspond to one-another. The
- // ID is the lookup into the cache, so crazy things will happen if you use
- // different SQL with the same ID.
- //
- // You will normally use the SQL_FROM_HERE macro to generate a statement
- // ID associated with the current line of code. This gives uniqueness without
- // you having to manage unique names. See StatementID above for more.
- //
// Example:
// sql::Statement stmt(database_.GetCachedStatement(
// SQL_FROM_HERE, "SELECT * FROM foo"));
diff --git a/sql/database_unittest.cc b/sql/database_unittest.cc
index 5bb4c047..38cd6c01 100644
--- a/sql/database_unittest.cc
+++ b/sql/database_unittest.cc
@@ -12,6 +12,7 @@
#include "base/logging.h"
#include "base/macros.h"
#include "base/strings/string_number_conversions.h"
+#include "base/test/gtest_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/simple_test_tick_clock.h"
@@ -230,8 +231,7 @@
TEST_F(SQLDatabaseTest, CachedStatement) {
sql::StatementID id1 = SQL_FROM_HERE;
sql::StatementID id2 = SQL_FROM_HERE;
- // Ensure leading and trailing whitespace doesn't break anything.
- static const char kId1Sql[] = "\n SELECT a FROM foo \n";
+ static const char kId1Sql[] = "SELECT a FROM foo";
static const char kId2Sql[] = "SELECT b FROM foo";
ASSERT_TRUE(db().Execute("CREATE TABLE foo (a, b)"));
@@ -281,6 +281,11 @@
ASSERT_TRUE(from_id2.Step()) << "cached statement was not reset";
EXPECT_EQ(13, from_id2.ColumnInt(0));
}
+
+ EXPECT_DCHECK_DEATH(db().GetCachedStatement(id1, kId2Sql))
+ << "Using a different SQL with the same statement ID should DCHECK";
+ EXPECT_DCHECK_DEATH(db().GetCachedStatement(id2, kId1Sql))
+ << "Using a different SQL with the same statement ID should DCHECK";
}
TEST_F(SQLDatabaseTest, IsSQLValidTest) {
