[sql] Refactor to remove use of const_cast<>.

sql/connection.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sql/connection.h"
 
 #include <string.h>
 
 #include "base/file_path.h"
 #include "base/logging.h"
@@ skipping 43 matching lines @@
 }
 
 ErrorDelegate::~ErrorDelegate() {
 }
 
 Connection::StatementRef::StatementRef()
     : connection_(NULL),
       stmt_(NULL) {
 }
 
+Connection::StatementRef::StatementRef(sqlite3_stmt* stmt)
+    : connection_(NULL),
+      stmt_(stmt) {
+}
+
 Connection::StatementRef::StatementRef(Connection* connection,
                                        sqlite3_stmt* stmt)
     : connection_(connection),
       stmt_(stmt) {
   connection_->StatementRefCreated(this);
 }
 
 Connection::StatementRef::~StatementRef() {
   if (connection_)
     connection_->StatementRefDeleted(this);
@@ skipping 262 matching lines @@
 
   scoped_refptr<StatementRef> statement = GetUniqueStatement(sql);
   if (statement->is_valid())
     statement_cache_[id] = statement;  // Only cache valid statements.
   return statement;
 }
 
 scoped_refptr<Connection::StatementRef> Connection::GetUniqueStatement(
     const char* sql) {
   if (!db_)
-    return new StatementRef(this, NULL);  // Return inactive statement.
+    return new StatementRef();  // Return inactive statement.
 
   sqlite3_stmt* stmt = NULL;
   if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK) {
     // This is evidence of a syntax error in the incoming SQL.
     DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
-    return new StatementRef(this, NULL);
+    return new StatementRef();
   }
   return new StatementRef(this, stmt);
 }
 
+scoped_refptr<Connection::StatementRef> Connection::GetUntrackedStatement(
+    const char* sql) const {
+  if (!db_)
+    return new StatementRef();  // Return inactive statement.
+
+  sqlite3_stmt* stmt = NULL;
+  int rc = sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL);
+  if (rc != SQLITE_OK) {
+    // This is evidence of a syntax error in the incoming SQL.
+    DLOG(FATAL) << "SQL compile error " << GetErrorMessage();
+    return new StatementRef();
+  }
+  return new StatementRef(stmt);
+}
+
 bool Connection::IsSQLValid(const char* sql) {
   sqlite3_stmt* stmt = NULL;
   if (sqlite3_prepare_v2(db_, sql, -1, &stmt, NULL) != SQLITE_OK)
     return false;
 
   sqlite3_finalize(stmt);
   return true;
 }
 
 bool Connection::DoesTableExist(const char* table_name) const {
   return DoesTableOrIndexExist(table_name, "table");
 }
 
 bool Connection::DoesIndexExist(const char* index_name) const {
   return DoesTableOrIndexExist(index_name, "index");
 }
 
 bool Connection::DoesTableOrIndexExist(
     const char* name, const char* type) const {
-  // GetUniqueStatement can't be const since statements may modify the
-  // database, but we know ours doesn't modify it, so the cast is safe.
-  Statement statement(const_cast<Connection*>(this)->GetUniqueStatement(
-      "SELECT name FROM sqlite_master "
-      "WHERE type=? AND name=?"));
+  const char* kSql = "SELECT name FROM sqlite_master WHERE type=? AND name=?";
+  Statement statement(GetUntrackedStatement(kSql));
   statement.BindString(0, type);
   statement.BindString(1, name);
 
   return statement.Step();  // Table exists if any row was returned.
 }
 
 bool Connection::DoesColumnExist(const char* table_name,
                                  const char* column_name) const {
   std::string sql("PRAGMA TABLE_INFO(");
   sql.append(table_name);

[Greg Billock, 2012/07/12 21:08:17]

Is this OK to pass unescaped? (I don't know whether pragmas can accept bound
params, but if so, I think that'd be better. If not, we should make sure this is
escaped.)

[Scott Hess - ex-Googler, 2012/07/12 21:53:08]

[Insert fingers into ears.]  "La la la, I can't hear you, la la la!"

I agree with you that this is a bad thing to do.  PRAGMA does not work with bind
parameters, unfortunately.  sqlite3_table_column_metadata() seems like a better
option, but I'm guessing it didn't exist when this code was written.  Since I
haven't used it before, I'm not entirely confident in dropping it right in
there.  In part this is because I consider DoesColumnExist() and
DoesTableOrIndexExist() to be anti-patterns that are probably being used
inappropriately in our code (I generally consider "test the schema and then
change it" code to be a sign that the author wasn't fully familiar with SQL or
SQLite in the first place, and thus often a source of interesting bugs).

Long story short - how valuable do you think such a change would be?  As-is, I
expect that people don't use table names that require quotes because if they do
their SQL looks weird.  If anyone is using user input for table names we're
already screwed on that front.  I could completely see adding a DCHECK that
table_name looks like /^[A-Za-z_]+$/.

[Greg Billock, 2012/07/14 21:41:25]

Usage is all migration stuff. I wonder if we should write some more
general-purpose migration DSL-type manager... There's quite a bit strewn around,
with some variation in approach.



On 2012/07/12 21:53:08, shess wrote:

[Scott Hess - ex-Googler, 2012/07/24 00:19:54]

I don't like the "does this table exist" approach because basically it's a race
condition.  Usually it doesn't cause problems in Chromium because of the profile
lock, but even so it makes me assume the developer using it didn't really think
things through (and use CREATE IF NOT EXISTS or something like that).

I kind of wish that our migration code was all "Create a new database, migrate
the data over", and never "Based on the current database, either create a new
table or add columns to an existing table".  Then we could potentially make the
version into something strong, like a hash over the schema :-).

-scott

   sql.append(")");
 
-  // Our SQL is non-mutating, so this cast is OK.
-  Statement statement(const_cast<Connection*>(this)->GetUniqueStatement(
-      sql.c_str()));
-
+  Statement statement(GetUntrackedStatement(sql.c_str()));
   while (statement.Step()) {
     if (!statement.ColumnString(1).compare(column_name))
       return true;
   }
   return false;
 }
 
 int64 Connection::GetLastInsertRowId() const {
   if (!db_) {
     DLOG(FATAL) << "Illegal use of connection without a db";
@@ skipping 142 matching lines @@
 
 int Connection::OnSqliteError(int err, sql::Statement *stmt) {
   if (error_delegate_.get())
     return error_delegate_->OnError(err, this, stmt);
   // The default handling is to assert on debug and to ignore on release.
   DLOG(FATAL) << GetErrorMessage();
   return err;
 }
 
 }  // namespace sql