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

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

The comments in these cases used incorrect reasoning for why the
const_cast<> was safe.  Whether the SQL statements modify the database
is irrelevant.  Add an internal helper to allow them to correctly
accomplish things in a const situation.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148031

[Scott Hess - ex-Googler, 2012-07-11 20:33:13]

In reviewing code for an unrelated change, I saw these const_cast<> calls and
thought "WTF?"  Probably weren't dangerous in the end, but still poor form.

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

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc#n...
sql/connection.cc:407: sql.append(table_name);
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.)

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.h
File sql/connection.h (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.h#ne...
sql/connection.h:396: scoped_refptr<StatementRef> GetUntrackedStatement(const
char* sql) const;
I am reading this as being private, right? It looks like a bug magnet if not. Is
there anyway to keep it fully in the .cc file?

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

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc#n...
sql/connection.cc:407: sql.append(table_name);
On 2012/07/12 21:08:17, Greg Billock wrote:
> 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.)

[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_]+$/.

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.h
File sql/connection.h (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.h#ne...
sql/connection.h:396: scoped_refptr<StatementRef> GetUntrackedStatement(const
char* sql) const;
On 2012/07/12 21:08:17, Greg Billock wrote:
> I am reading this as being private, right? It looks like a bug magnet if not.
Is
> there anyway to keep it fully in the .cc file?

Yeah, it's private.  Looks to me like pulling it into the anonymous namespace is
pretty trivial.  Then I noticed that StatementRef is private to Connection.

I've elaborated on the usage comment a bit.

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

lgtm

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc#n...
sql/connection.cc:407: sql.append(table_name);
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:
> On 2012/07/12 21:08:17, Greg Billock wrote:
> > 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.)
> 
> [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_]+$/.

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

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://chromiumcodereview.appspot.com/10736042/diff/2001/sql/connection.cc#n...
sql/connection.cc:407: sql.append(table_name);
On 2012/07/14 21:41:25, Greg Billock wrote:
> 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.

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

[commit-bot: I haz the power, 2012-07-24 00:20:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/10736042/10001

[commit-bot: I haz the power, 2012-07-24 01:59:59]

Change committed as 148031