Bug 132243 - Mysql query fails for collection scanner if directory contains slash
Summary: Mysql query fails for collection scanner if directory contains slash
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-11 08:46 UTC by Martin Burnicki
Modified: 2006-08-12 09:36 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Burnicki 2006-08-11 08:46:32 UTC
Version:           v1.4-SVN rev 571700 (using KDE Devel)
Installed from:    Compiled sources
Compiler:          gcc version 4.0.2 20050901 (prerelease) (SUSE Linux 10) 
OS:                Linux

If the directory for a compilation contains a slash. e.g in

dir/Various/compilation_1 (CD1/2)/title_1.mp3

then the '/' in CD1/2 is not handled correctly.

On the command line I see the following output:
amarok:     [CollectionDB] MYSQL QUERY FAILED: You have an error in your SQL
   syntax; check the manual that corresponds to your MySQL server version 
   for the right syntax to use near '%4, 1125765756 )' at line 1
amarok: FAILED QUERY: REPLACE INTO directories_temp ( dir, deviceid, changedate )
   VALUES ( './dir/Various/compilation_1 (CD12f2)', %4, 1125765756 );

The raw directory string contains (CD1%2f2) which is obviously not converted correctly by the following query:

        query( QString( "REPLACE INTO directories%1 ( dir, deviceid, changedate )      
                  VALUES ( '%3', %4, %2 );" )
                  .arg( temporary ? "_temp" : "" )
                  .arg( datetime )
                  .arg( escapeString( rpath ) )
                  .arg( deviceid ) );

I'm assuming the .arg's don't work correctly since the original string contains a '%', which is removed in the 3rd arg, but the 4th arg is then '%4' instead of the real deviceid.
Comment 1 Maximilian Kossick 2006-08-11 09:40:53 UTC
SVN commit 571931 by mkossick:

fixed more QString placeholder replacement issues
BUG: 132243

 M  +46 -48    collectiondb.cpp  
Comment 2 Martin Burnicki 2006-08-11 13:57:22 UTC
Thanks,

however, after I've removed all tables from the amarok DB it creates new tables, but doesn't scan the lollection at all, neither automatically after startup (because the tables don't exist) nor if I start it from the menu button.

There's no activity, no progress bar, and no messages on the console.

Martin
Comment 3 Maximilian Kossick 2006-08-11 17:17:31 UTC
Can you create a new bug report for this problem and attach the whole
debug output?
Comment 4 Martin Burnicki 2006-08-11 19:25:06 UTC
Hm, I've SVN updated again a few hours later, and now the collection scanner runs again.

However, there's now again a bunch of errors similar to that before (but not the same):

amarok:     [CollectionDB] MYSQL QUERY FAILED: You have an error in your SQL
    syntax; check the manual that corresponds to your MySQL server version for
    the right syntax to use near '%3 AND album.id = tags_temp.album AND
    tags_temp.sampler IS NULL' at line 1
amarok: FAILED QUERY: SELECT DISTINCT album.name FROM tags_temp, album_temp AS
    album WHERE tags_temp.dir = './Various/Oldie Night (CD12f2)' AND
    tags_temp.deviceid = %3 AND album.id = tags_temp.album AND
    tags_temp.sampler IS NULL;

Please note there's now a %3 in the SQL query.

The interesting thing is that after the scan all the tracks can be found in the collection, but if the directory contains a compilation from different artists the tracks are not marked as compilation, and the album is not listed under compilations.

If the directory contains tracks from an album from a single artist then the album is listed properly in the collection.

Please note this happens only if the directory name contains a '/' as in CD1/2 which is translated to CD1%2f2. If I rename a directory and remove the '/' then the compilation appears in the collection (after a re-scan, of course).

Since this is similar to my original report I reopen the bug and hope you agree.

Thanks,

Martin
Comment 5 Maximilian Kossick 2006-08-11 19:51:28 UTC
SVN commit 572128 by mkossick:

*fixed more QString placeholder issues
BUG: 132243
*dialog window is now actually displayed when updating the database, but it still needs some 
more work

 M  +18 -11    collectiondb.cpp  


--- trunk/extragear/multimedia/amarok/src/collectiondb.cpp #572127:572128
@@ -2972,7 +2972,7 @@
     QStringList nonTempURLs = query( QString(
             "SELECT url, uniqueid "
             "FROM uniqueid "
-            "WHERE deviceid = %1 AND url = '%1';" )
+            "WHERE deviceid = %1 AND url = '%2';" )
                 .arg( currdeviceid )
                 .arg( currurl ) );
 
@@ -3005,12 +3005,12 @@
     if( !tempTablesAndInPermanent && urls.empty() && uniqueids.empty() )
     {
         QString insertline = QString( "INSERT INTO uniqueid%1 (deviceid, url, uniqueid, dir) "
-                                      "VALUES ( %2,'%3', '%4', '%5')" )
+                                      "VALUES ( %2,'%3', '%4'" )
                 .arg( tempTables ? "_temp" : "" )
                 .arg( currdeviceid )
                 .arg( currurl )
-                .arg( currid )
-                .arg( currdir );
+                .arg( currid ); 
+        insertline += QString( ", '%1);" ).arg( currdir );
         insert( insertline, NULL );
         if( !statUIDVal.empty() )
         {
@@ -4286,10 +4286,10 @@
     int deviceid = MountPointManager::instance()->getIdForUrl( path );
     QString rpath = MountPointManager::instance()->getRelativePath( deviceid, path );
 
-    albums = query( QString( "SELECT DISTINCT album.name FROM tags_temp, album%1 AS album WHERE tags_temp.dir = '%2' AND tags_temp.deviceid = %3 AND album.id = tags_temp.album AND tags_temp.sampler IS NULL;" )
+    albums = query( QString( "SELECT DISTINCT album.name FROM tags_temp, album%1 AS album WHERE tags_temp.dir = '%3' AND tags_temp.deviceid = %2 AND album.id = tags_temp.album AND tags_temp.sampler IS NULL;" )
               .arg( temporary ? "_temp" : "" )
-              .arg( escapeString( rpath ) )
-              .arg( deviceid ) );
+              .arg( deviceid )
+              .arg( escapeString( rpath ) ) );
 
     for ( uint i = 0; i < albums.count(); i++ )
     {
@@ -5130,15 +5130,23 @@
         if ( needsUpdate )
         {
 
-            KDialog *dialog = new KDialogBase( i18n( "Updating database, please wait" ),
+            KDialog *dialog = new KDialogBase( i18n( "Updating database" ),
                                                0,
                                                KDialogBase::Yes,
                                                KDialogBase::Cancel,
                                                0,
                                                0,
                                                false );
-            //QTimer::singleShot( 0, dialog, SLOT( show() ) );
-            dialog->show();
+            /* TODO: remove the standard window controls from the dialog window, the user should not be able
+                     to close, minimize, maximize the dialog
+                     add additional text, e.g. Amarok is currently updating your database. This may take a while.
+                     Please wait.
+
+                     Consider using a ProgressBarDialog
+            */
+            QTimer::singleShot( 0, dialog, SLOT( show() ) );
+            //process events in the main event loop so that the dialog actually gets shown
+            kapp->processEvents();
             debug() << "Beginning database update" << endl;
 
             updateStatsTables();
@@ -5157,7 +5165,6 @@
             }
             delete dialog;
         }
-
         emit databaseUpdateDone();
     }
 
Comment 6 Martin Burnicki 2006-08-12 09:36:18 UTC
Maximilian,

your latest patches have fixed the problem I had observed. 

There are no more mysql errors, andall the compilations are listed in the collection.

Thanks a bunch,

Martin