From 24fedb94c233e2eed21a50453b8d6878add2cfec Mon Sep 17 00:00:00 2001 From: dan Date: Sat, 18 Aug 2018 17:35:38 +0000 Subject: [PATCH] Improvements to error handling in ALTER TABLE RENAME COLUMN. FossilOrigin-Name: 7fa1faeaff30b74b68ee6f4b363d837f21cf313d8262361c901bda884df139a2 --- manifest | 14 ++++---- manifest.uuid | 2 +- src/alter.c | 90 +++++++++++++++++++++++++++++++++------------- test/altercol.test | 53 +++++++++++++++++++++------ 4 files changed, 117 insertions(+), 42 deletions(-) diff --git a/manifest b/manifest index 7287245cfa..94dcb835ce 100644 --- a/manifest +++ b/manifest @@ -1,5 +1,5 @@ -C Allow\san\sALTER\sTABLE\sRENAME\sCOLUMN\sto\sproceed\seven\sif\sthe\sschema\scontains\sa\nvirtual\stable\sfor\swhich\sthe\smodule\sis\sunavailable. -D 2018-08-17T18:08:28.458 +C Improvements\sto\serror\shandling\sin\sALTER\sTABLE\sRENAME\sCOLUMN. +D 2018-08-18T17:35:38.195 F .fossil-settings/empty-dirs dbb81e8fc0401ac46a1491ab34a7f2c7c0452f2f06b54ebb845d024ca8283ef1 F .fossil-settings/ignore-glob 35175cdfcf539b2318cb04a9901442804be81cd677d8b889fcc9149c21f239ea F Makefile.in 0a3a6c81e6fcb969ff9106e882f0a08547014ba463cb6beca4c4efaecc924ee6 @@ -432,7 +432,7 @@ F spec.template 86a4a43b99ebb3e75e6b9a735d5fd293a24e90ca F sqlite.pc.in 42b7bf0d02e08b9e77734a47798d1a55a9e0716b F sqlite3.1 fc7ad8990fc8409983309bb80de8c811a7506786 F sqlite3.pc.in 48fed132e7cb71ab676105d2a4dc77127d8c1f3a -F src/alter.c c81f6d8d4f07ba89b2aa237bf8aaa4cb63b43b716eec8291f70eeea145a66a09 +F src/alter.c 3342dba26f9023b334e6ab634ef45d0d8b333023c80070a57f22f85f64bbeabb F src/analyze.c 3dc6b98cf007b005af89df165c966baaa48e8124f38c87b4d2b276fe7f0b9eb9 F src/attach.c 4bd5b92633671d3e8ce431153ebb1893b50335818423b5373f3f27969f79769a F src/auth.c 32a5bbe3b755169ab6c66311c5225a3cd4f75a46c041f7fb117e0cbb68055114 @@ -599,7 +599,7 @@ F test/alter.test b820ab9dcf85f8e3a65bc8326accb2f0c7be64ef F test/alter2.test 7ea05c7d92ac99349a802ef7ada17294dd647060 F test/alter3.test 4d79934d812eaeacc6f22781a080f8cfe012fdc3 F test/alter4.test b6d7b86860111864f6cddb54af313f5862dda23b -F test/altercol.test cac9395932f9fa18b3c074559c33c557352db5d0fdf81aec28b1513828cb855f +F test/altercol.test 7e22d63b1adb3f66e695c5dfb32d102644943ceeb88bc81bf74fa6d65dca5eba F test/altermalloc.test e81ac9657ed25c6c5bb09bebfa5a047cd8e4acfc F test/amatch1.test b5ae7065f042b7f4c1c922933f4700add50cdb9f F test/analyze.test b3a9c67d00e1df7588a5b7be9a0292899f94fe8cac1f94a017277474ca2e59df @@ -1756,7 +1756,7 @@ F vsixtest/vsixtest.tcl 6a9a6ab600c25a91a7acc6293828957a386a8a93 F vsixtest/vsixtest.vcxproj.data 2ed517e100c66dc455b492e1a33350c1b20fbcdc F vsixtest/vsixtest.vcxproj.filters 37e51ffedcdb064aad6ff33b6148725226cd608e F vsixtest/vsixtest_TemporaryKey.pfx e5b1b036facdb453873e7084e1cae9102ccc67a0 -P 540014efd6a048373313c6cd9413de10d5d7114daf537cf5999ccf3c5c3f9358 -R f20218c0b275f1c838232dab3f0beae8 +P 7b72b2360a70eb1f788b3c4d745967dfedb2c80af6b2e146ba940a3741fd51a1 +R 4b2d31defa56ef33764a237b1d928d0d U dan -Z 4df0525d7f8fcece703661753e166268 +Z ea61ccced068208007aae6f195a47978 diff --git a/manifest.uuid b/manifest.uuid index 5786d9265b..20186e110c 100644 --- a/manifest.uuid +++ b/manifest.uuid @@ -1 +1 @@ -7b72b2360a70eb1f788b3c4d745967dfedb2c80af6b2e146ba940a3741fd51a1 \ No newline at end of file +7fa1faeaff30b74b68ee6f4b363d837f21cf313d8262361c901bda884df139a2 \ No newline at end of file diff --git a/src/alter.c b/src/alter.c index 84640f3ab3..613293182b 100644 --- a/src/alter.c +++ b/src/alter.c @@ -964,11 +964,15 @@ static void renameTokenFind(Parse *pParse, struct RenameCtx *pCtx, void *pPtr){ } } +/* +** This is a Walker select callback. It does nothing. It is only required +** because without a dummy callback, sqlite3WalkExpr() and similar do not +** descend into sub-select statements. +*/ static int renameColumnSelectCb(Walker *pWalker, Select *p){ return WRC_Continue; } - /* ** This is a Walker expression callback. ** @@ -996,10 +1000,10 @@ static int renameColumnExprCb(Walker *pWalker, Expr *pExpr){ /* ** The RenameCtx contains a list of tokens that reference a column that -** is being renamed by an ALTER TABLE statement. Return the "first" +** is being renamed by an ALTER TABLE statement. Return the "last" ** RenameToken in the RenameCtx and remove that RenameToken from the -** RenameContext. "First" means the first RenameToken encountered when -** the input SQL from left to right. Repeated calls to this routine +** RenameContext. "Last" means the last RenameToken encountered when +** the input SQL is parsed from left to right. Repeated calls to this routine ** return all column name tokens in the order that they are encountered ** in the SQL statement. */ @@ -1017,6 +1021,33 @@ static RenameToken *renameColumnTokenNext(RenameCtx *pCtx){ return pBest; } +/* +** An error occured while parsing or otherwise processing a database +** object (either pParse->pNewTable, pNewIndex or pNewTrigger) as part of an +** ALTER TABLE RENAME COLUMN program. The error message emitted by the +** sub-routine is currently stored in pParse->zErrMsg. This function +** adds context to the error message and then stores it in pCtx. +*/ +static void renameColumnParseError(sqlite3_context *pCtx, Parse *pParse){ + const char *zT; + const char *zN; + char *zErr; + if( pParse->pNewTable ){ + zT = pParse->pNewTable->pSelect ? "view" : "table"; + zN = pParse->pNewTable->zName; + }else if( pParse->pNewIndex ){ + zT = "index"; + zN = pParse->pNewIndex->zName; + }else{ + assert( pParse->pNewTrigger ); + zT = "trigger"; + zN = pParse->pNewTrigger->zName; + } + zErr = sqlite3_mprintf("error processing %s %s: %s", zT, zN, pParse->zErrMsg); + sqlite3_result_error(pCtx, zErr, -1); + sqlite3_free(zErr); +} + /* ** SQL function: ** @@ -1083,12 +1114,18 @@ static void renameColumnFunc( memset(&sCtx, 0, sizeof(sCtx)); sCtx.iCol = ((iCol==pTab->iPKey) ? -1 : iCol); + /* Parse the SQL statement passed as the first argument. If no error + ** occurs and the parse does not result in a new table, index or + ** trigger object, the database must be corrupt. */ memset(&sParse, 0, sizeof(sParse)); sParse.eParseMode = PARSE_MODE_RENAME_COLUMN; sParse.db = db; sParse.nQueryLoop = 1; rc = sqlite3RunParser(&sParse, zSql, &zErr); - assert( sParse.pNewTable==0 || sParse.pNewIndex==0 ); + assert( sParse.zErrMsg==0 ); + assert( rc!=SQLITE_OK || zErr==0 ); + assert( (!!sParse.pNewTable)+(!!sParse.pNewIndex)+(!!sParse.pNewTrigger)<2 ); + sParse.zErrMsg = zErr; if( db->mallocFailed ) rc = SQLITE_NOMEM; if( rc==SQLITE_OK && sParse.pNewTable==0 && sParse.pNewIndex==0 && sParse.pNewTrigger==0 @@ -1096,6 +1133,23 @@ static void renameColumnFunc( rc = SQLITE_CORRUPT_BKPT; } +#ifdef SQLITE_DEBUG + /* Ensure that all mappings in the Parse.pRename list really do map to + ** a part of the input string. */ + assert( sqlite3Strlen30(zSql)==nSql ); + if( rc==SQLITE_OK ){ + RenameToken *pToken; + for(pToken=sParse.pRename; pToken; pToken=pToken->pNext){ + assert( pToken->t.z>=zSql && &pToken->t.z[pToken->t.n]<=&zSql[nSql] ); + } + } +#endif + + /* Set zQuot to point to a buffer containing a quoted copy of the + ** identifier zNew. If the corresponding identifier in the original + ** ALTER TABLE statement was quoted (bQuote==1), then set zNew to + ** point to zQuot so that all substitutions are made using the + ** quoted version of the new column name. */ if( rc==SQLITE_OK ){ zQuot = sqlite3_mprintf("\"%w\"", zNew); if( zQuot==0 ){ @@ -1104,22 +1158,11 @@ static void renameColumnFunc( nQuot = sqlite3Strlen30(zQuot); } } - if( bQuote ){ zNew = zQuot; nNew = nQuot; } -#ifdef SQLITE_DEBUG - assert( sqlite3Strlen30(zSql)==nSql ); - if( rc==SQLITE_OK ){ - RenameToken *pToken; - for(pToken=sParse.pRename; pToken; pToken=pToken->pNext){ - assert( pToken->t.z>=zSql && &pToken->t.z[pToken->t.n]<=&zSql[nSql] ); - } - } -#endif - /* Find tokens that need to be replaced. */ memset(&sWalker, 0, sizeof(Walker)); sWalker.pParse = &sParse; @@ -1137,12 +1180,6 @@ static void renameColumnFunc( rc = (db->mallocFailed ? SQLITE_NOMEM : sParse.rc); if( rc==SQLITE_OK ){ sqlite3WalkSelect(&sWalker, pSelect); - }else if( rc==SQLITE_ERROR ){ - /* Failed to resolve all symbols in the view. This is not an - ** error, but it will not be edited. */ - sqlite3DbFree(db, sParse.zErrMsg); - sParse.zErrMsg = 0; - rc = SQLITE_OK; } if( rc!=SQLITE_OK ) goto renameColumnFunc_done; }else{ @@ -1294,6 +1331,10 @@ static void renameColumnFunc( } } + /* At this point sCtx.pList contains a list of RenameToken objects + ** corresponding to all tokens in the input SQL that must be replaced + ** with the new column name. All that remains is to construct and + ** return the edited SQL string. */ assert( rc==SQLITE_OK ); assert( nQuot>=nNew ); zOut = sqlite3DbMallocZero(db, nSql + sCtx.nList*nQuot + 1); @@ -1334,8 +1375,8 @@ static void renameColumnFunc( renameColumnFunc_done: if( rc!=SQLITE_OK ){ - if( zErr ){ - sqlite3_result_error(context, zErr, -1); + if( sParse.zErrMsg ){ + renameColumnParseError(context, &sParse); }else{ sqlite3_result_error_code(context, rc); } @@ -1349,6 +1390,7 @@ renameColumnFunc_done: sqlite3DeleteTrigger(db, sParse.pNewTrigger); renameTokenFree(db, sParse.pRename); renameTokenFree(db, sCtx.pList); + sqlite3DbFree(db, sParse.zErrMsg); sqlite3ParserReset(&sParse); sqlite3_free(zQuot); } diff --git a/test/altercol.test b/test/altercol.test index a48a1444d7..de025ac61c 100644 --- a/test/altercol.test +++ b/test/altercol.test @@ -26,6 +26,20 @@ ifcapable !altertable { return } +# Drop all the tables and views in the 'main' database of database connect +# [db]. Sort the objects by name before dropping them. +# +proc drop_all_tables_and_views {db} { + set SQL { + SELECT name, type FROM sqlite_master + WHERE type IN ('table', 'view') AND name NOT LIKE 'sqlite_%' + ORDER BY 1 + } + foreach {z t} [db eval $SQL] { + db eval "DROP $t $z" + } +} + foreach {tn before after} { 1 {CREATE TABLE t1(a INTEGER, b TEXT, c BLOB)} {CREATE TABLE t1(a INTEGER, d TEXT, c BLOB)} @@ -324,23 +338,17 @@ do_execsql_test 8.4.4 { SELECT sql FROM sqlite_master WHERE name='xxx'; } {{CREATE VIEW xxx AS SELECT a FROM b1 UNION SELECT hello FROM b2 ORDER BY 1 COLLATE nocase}} -do_execsql_test 8.4.5 { +do_catchsql_test 8.4.5 { CREATE VIEW zzz AS SELECT george, ringo FROM b1; ALTER TABLE b1 RENAME a TO aaa; - SELECT sql FROM sqlite_master WHERE name = 'zzz' -} {{CREATE VIEW zzz AS SELECT george, ringo FROM b1}} +} {1 {error processing view zzz: no such column: george}} #------------------------------------------------------------------------- # More triggers. # proc do_rename_column_test {tn old new lSchema} { - for {set i 0} {$i < 2} {incr i} { - # DROP all tables and views in database. - set sql "SELECT name FROM sqlite_master WHERE type='table' ORDER BY 1" - foreach nm [db eval $sql] { db eval "DROP TABLE $nm" } - set sql "SELECT name FROM sqlite_master WHERE type='view' ORDER BY 1" - foreach nm [db eval $sql] { db eval "DROP VIEW $nm" } + drop_all_tables_and_views db set lSorted [list] foreach sql $lSchema { @@ -353,7 +361,7 @@ proc do_rename_column_test {tn old new lSchema} { SELECT sql FROM sqlite_master WHERE sql!='' ORDER BY 1 } $lSorted - if {0 && $i==1} { + if {$i==1} { db close sqlite3 db test.db } @@ -432,5 +440,30 @@ foreach {tn old new lSchema} { do_rename_column_test 10.$tn $old $new $lSchema } +#-------------------------------------------------------------------------- +# Test that if a view or trigger refers to a virtual table for which the +# module is not available, RENAME COLUMN cannot proceed. +# +reset_db +register_echo_module db +do_execsql_test 11.0 { + CREATE TABLE x1(a, b, c); + CREATE VIRTUAL TABLE e1 USING echo(x1); +} +db close +sqlite3 db test.db + +do_execsql_test 11.1 { + ALTER TABLE x1 RENAME b TO bbb; + SELECT sql FROM sqlite_master; +} { {CREATE TABLE x1(a, bbb, c)} {CREATE VIRTUAL TABLE e1 USING echo(x1)} } + +do_execsql_test 11.2 { + CREATE VIEW v1 AS SELECT e1.*, x1.c FROM e1, x1; +} + +do_catchsql_test 11.3 { + ALTER TABLE x1 RENAME c TO ccc; +} {1 {error processing view v1: no such module: echo}} finish_test -- 2.39.5