Bug 302837

Summary: Some columns in MySQL db are marked non-null when they should be null when updating from very old database (e.g. Amarok 2.0)
Product: [Applications] amarok Reporter: Tom Chiverton <bugs.kde.org>
Component: Collection BrowserAssignee: Amarok Developers <amarok-bugs-dist>
Status: RESOLVED FIXED    
Severity: critical CC: matej, mitchell, ralf-engels
Priority: NOR Keywords: release_blocker
Version: 2.6.0   
Target Milestone: 2.7   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In: 2.7
Sentry Crash Report:
Attachments: Result of "Amarok --deamarok --debug > /tmp/startup.txt 2>&1 " with network unmounted, then selecting update collection
Second attempt to attach result of : amarok --debug > /tmp/startup.txt 2>&1
startup output from version with second rev of patch
My DB structure

Description Tom Chiverton 2012-07-01 14:58:11 UTC
My MP3 collection is a mounted CIFS share. If Amarok is used when this share isn't mounted i.e. after a reboot outside my LAN it removes all tracks from the collection. The ratings do not reappear when I remount and restart Amarok.

Reproducible: Always

Steps to Reproduce:
1. Mount CIFS share with MP3 on it.
2. Rate the file in Amarok
3. Confirm shows up in search in Amarok
4. Close Amarok.
5. unmount the CIFS share.
start amarok
update collection
confirm file doesn't show up in search
close amarok
mount CIFS
start amarok
update collection
file appears, but rating lost
Actual Results:  
all meta data lost

Expected Results:  
meta data kept

This is a really common setup, and a very easy way to loose years worth of meta data. I happen to have a backup. Lucky me.

No, I don't want to write my metadata to the MP3, other people use them
Comment 1 Myriam Schweingruber 2012-07-07 23:26:51 UTC
Normally the AFT should prevent that, or am I mistaken?
Comment 2 Tom Chiverton 2012-07-08 09:57:41 UTC
What's AFT ? How do I check it's attempt to prevent this ?
Comment 3 Myriam Schweingruber 2012-07-10 11:11:27 UTC
It is the Amarok File Tracking, see here for more details: http://amarok.kde.org/wiki/Amarok_File_Tracking#How_do_I_turn_it_on.3F where also advanced features are described.
Comment 4 Tom Chiverton 2012-07-10 17:49:05 UTC
Oh, well, no, obviously it doesn't prevent it. I've never run amarok_afttagger as far as I know.

My guess would be Amarok starts up, starts the collection scanner, which sees the mount point is there but is now empty, and removes all the files in that path.... otherwise Amarok's file database would grow without limit ? Which is an obvious tension with what AFT does. I don't think AFT covers this use case ?

Amarok should recognise network filesystems and treat them correctly. There's a 'SMB' plugin but I've no idea what it does, and it's about page is no help.
Comment 5 Tom Chiverton 2012-08-19 10:47:02 UTC
Even the latest Amarok 2.6 still removes all track metadata if you turn on Amarok when on a different network .

Why aren't network shares with music on correctly supported, still ? Isn't total data loss a priority ?
Comment 6 Myriam Schweingruber 2012-08-19 14:04:39 UTC
Thank you for the feedback. Ralf, please have a look at this.
Comment 7 Matěj Laitl 2012-08-19 14:52:58 UTC
Hmm, this shouldn't happen IF you have Dynamic Collection turned on (hidden config, enabled by default) and relevant plugin enabled under Local Collection Backend in config. (Note: AFT is irrevelant here)

Amarok should detect that those tracks come from a mount that is not connected and refuse to delete them (probably unless you do a full scan though).

> Why aren't network shares with music on correctly supported, still ? Isn't total data loss a priority?

They are supported, this is a bug.

Tom, are you able to to look at your database contents? Look at the urls table, what "device" do entries for your remote tracks have? What is the corresponding device row in the devices table?

Also please re-reproduce this bug, but start amarok as `amarok  --debug` for the time it deletes the metadata (the second run) and post its complete output. Thanks and sorry for the data-loss.
Comment 8 Tom Chiverton 2012-08-19 15:46:49 UTC
"refuse to delete them (probably unless you do a full scan though)."
Not my reproduce steps above do do this, but then Amarok 2.5 certainly did them at irregular intervals all by itself.

Here is the table output :
select distinct deviceid from urls;
+----------+
| deviceid |
+----------+
|        6 |
+----------+

 select * from devices where id =6;
+----+------+-------+----------------+--------------------------------------+------------+-----------+
| id | type | label | lastmountpoint | uuid                                 | servername | sharename |
+----+------+-------+----------------+--------------------------------------+------------+-----------+
|  6 | uuid |       | /              | 01aa6a10-1c91-4b24-9632-a45de910f911 |            |           |

The above makes no sense to me. Shouldn't the mount point be the file system path (/media/foo-baz) where the collection is mounted ? None of the lastmountpoint entries seem to match my network location.
Comment 9 Tom Chiverton 2012-08-19 15:53:08 UTC
Created attachment 73301 [details]
Result of "Amarok --deamarok --debug > /tmp/startup.txt 2>&1 " with network unmounted, then selecting update collection
Comment 10 Tom Chiverton 2012-08-19 15:54:13 UTC
By which I mean results of "amarok --debug > /tmp/startup.txt 2>&1" of course. FireFox and my keyboard had a falling out.
Comment 11 Matěj Laitl 2012-08-19 16:00:35 UTC
(In reply to comment #9)
> Created attachment 73301 [details]
> Result of "Amarok --deamarok --debug > /tmp/startup.txt 2>&1 " with network
> unmounted, then selecting update collection

Please submit it again in plain text (and pick it in Bugzilla), I'm unable to open this one. Please submit at plain-text each time so the file can be  viewed directly in bugzilla.
Comment 12 Matěj Laitl 2012-08-19 16:05:29 UTC
(In reply to comment #8)
> "refuse to delete them (probably unless you do a full scan though)."
> Not my reproduce steps above do do this, but then Amarok 2.5 certainly did
> them at irregular intervals all by itself.

There are 2 distinct actions:
 a) "Update Collection", which triggers at refular intervals (if enabled) and is accessible from the tools menu. This action shouldn't remove meta-data for inaccessible files.
 b) "Fully Rescan Collection", which drops the entire db first and then start over. Only accessible form the Config Dialog.

> Here is the table output :
> select distinct deviceid from urls;
> +----------+
> | deviceid |
> +----------+
> |        6 |
> +----------+
> 
>  select * from devices where id =6;
> +----+------+-------+----------------+--------------------------------------
> +------------+-----------+
> | id | type | label | lastmountpoint | uuid                                
> | servername | sharename |
> +----+------+-------+----------------+--------------------------------------
> +------------+-----------+
> |  6 | uuid |       | /              | 01aa6a10-1c91-4b24-9632-a45de910f911
> |            |           |
> 
> The above makes no sense to me. Shouldn't the mount point be the file system
> path (/media/foo-baz) where the collection is mounted ? None of the
> lastmountpoint entries seem to match my network location.

Yes, this seems to be the culprit. Amarok somehow doesn't detect the share and goes to the root filesystem. What is the output of `solid-hardware list details` with the share mounted and unmounted? (post directly here, strip irrevelant entries, the revelant ones should have an udi like /org/kde/fstab/something) What kdelibs version do you have?
Comment 13 Tom Chiverton 2012-08-19 16:08:03 UTC
Created attachment 73303 [details]
Second attempt to attach result of : amarok --debug > /tmp/startup.txt 2>&1

It's too big to attach without bzip'ing it : 
falken@wopr:~/.kde/share/apps/amarok$ tar tjvf /tmp/startup.txt.bz2 
-rw-rw-r-- falken/falken 1505589 2012-08-19 17:06 tmp/startup.txt

If this doesn't work, email tom at-sign falkensweb full-stop com and we'll work something else out.
Comment 14 Tom Chiverton 2012-08-19 16:12:58 UTC
This seems the right sold-hardware block.

(mounted)
udi = '/org/kde/fstab///bookcase.house/mp3'
  parent = '/org/kde/fstab'  (string)
  vendor = 'mp3'  (string)
  product = 'bookcase.house'  (string)
  description = 'mp3 on bookcase.house'  (string)
  StorageAccess.accessible = true  (bool)
  StorageAccess.filePath = '/media/bookcase-mp3'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://bookcase.house/mp3'  (string)

(not mounted)

udi = '/org/kde/fstab///bookcase.house/mp3'
  parent = '/org/kde/fstab'  (string)
  vendor = 'mp3'  (string)
  product = 'bookcase.house'  (string)
  description = 'mp3 on bookcase.house'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = '/media/bookcase-mp3'  (string)
  StorageAccess.ignored = false  (bool)
  NetworkShare.type = 'Cifs'  (0x2)  (enum)
  NetworkShare.url = 'smb://bookcase.house/mp3'  (string)

dpkg claims
kdelibs-bin                                                                  4:4.8.5-0ubuntu0.1~ppa3                  
kdelibs5                                                                     4:4.8.5-0ubuntu0.1~ppa3  

These are standard Kubuntu packages.
Comment 15 Matěj Laitl 2012-08-19 16:26:53 UTC
(In reply to comment #14)
> This seems the right sold-hardware block.

Hmm, this looks correct. From your logs (the second one worked alhough you named in incorrectly, it was tar packed with bzip2, so it shoul've been named somehhing.tar.bz2 - I thought you bzipped the .txt directly; the first one was incomplete upload) I see the following culprit:

amarok:             BEGIN: virtual bool SmbDeviceHandlerFactory::canHandle(const Solid::Device&) const 
amarok:             END__: virtual bool SmbDeviceHandlerFactory::canHandle(const Solid::Device&) const [Took: 0s] 
amarok:             [MountPointManager] Factory can't handle device  "/org/kde/fstab///bookcase.house/mp3"

Let me look at the code.
Comment 16 Matěj Laitl 2012-08-19 22:55:42 UTC
Tom,
the proposed fix is now available at https://git.reviewboard.kde.org/r/106094/ - please test it. It is created atop of current master, but there's a change it will apply to 2.6, too.

When testing it, be sure to hit "Update Collection" with all your shares mounted, then check that url device ids and devices are correnct in the database, then check that meta-data are not lost. Thanks! If you need help patching or building Amarok, please ask on #amarok.
Comment 17 Tom Chiverton 2012-08-23 17:55:04 UTC
Is there anyway to get a prebuilt nightly or something ? Last time I tried compiling a KDE project from source it turned into a bit of a nightmare !

Is something like http://blogs.fsfe.org/myriam/2009/09/26/compiling-amarok-from-git-locally-full-summary/ still the easiest way to do ?
Comment 18 Tom Chiverton 2012-08-23 19:15:59 UTC
OK, built a patched Amarok.

Started Amarok with share mounted (shows meta data for files still), pressed Tools, Update collection. Wait a bit, then close and exit Amarok.

Start the DB stand alone again and :

mysql> select distinct deviceid from urls;
+----------+
| deviceid |
+----------+
|        6 |
+----------+
1 row in set (0.00 sec)

mysql> select * from devices where id=6;
+----+------+-------+----------------+--------------------------------------+------------+-----------+
| id | type | label | lastmountpoint | uuid                                 | servername | sharename |
+----+------+-------+----------------+--------------------------------------+------------+-----------+
|  6 | uuid |       | /              | 01aa6a10-1c91-4b24-9632-a45de910f911 |            |           |
+----+------+-------+----------------+--------------------------------------+------------+-----------+
1 row in set (0.01 sec)

so no change ?
It might be that Amarok is moaning about collection scanner being the wrong version, even though the newly built one is first in PATH ?
Anyway, I have a backup of the DB, so I did a full rescan from the settings pages.

No change to the DB values.
Comment 19 Matěj Laitl 2012-08-24 10:26:40 UTC
(In reply to comment #17)
> Is there anyway to get a prebuilt nightly or something ? Last time I tried
> compiling a KDE project from source it turned into a bit of a nightmare!

You don't need to build KDE at all.

> Is something like
> http://blogs.fsfe.org/myriam/2009/09/26/compiling-amarok-from-git-locally-full-summary/
> still the easiest way to do?

Yes.

(In reply to comment #18)
> OK, built a patched Amarok.
> 
> Started Amarok with share mounted (shows meta data for files still), pressed
> Tools, Update collection. Wait a bit, then close and exit Amarok.
> 
> mysql> select * from devices where id=6;
> +----+------+-------+----------------+--------------------------------------
> +------------+-----------+
> | id | type | label | lastmountpoint | uuid                                
> | servername | sharename |
> +----+------+-------+----------------+--------------------------------------
> +------------+-----------+
> |  6 | uuid |       | /              | 01aa6a10-1c91-4b24-9632-a45de910f911
> |            |           |
> +----+------+-------+----------------+--------------------------------------
> +------------+-----------+
> 1 row in set (0.01 sec)
> 
> so no change ?
> It might be that Amarok is moaning about collection scanner being the wrong
> version, even though the newly built one is first in PATH ?
> Anyway, I have a backup of the DB, so I did a full rescan from the settings
> pages.
> 
> No change to the DB values.

Hmm, strange. Please post ouput of the `amarok -d --nofork 2>&1 | grep -iC2 'devicehandler|mountpointmana'` of the patched Amarok (the review request has been updated, please apply the latest patch) and also:
mysql> select * from devices where type != 'uuid'
Comment 20 Tom Chiverton 2012-08-25 09:10:41 UTC
With that latest patch:

mysql> select * from devices where type != 'uuid';
+----+------+-------+-----------------------------------+------+----------------+-----------+
| id | type | label | lastmountpoint                    | uuid | servername     | sharename |
+----+------+-------+-----------------------------------+------+----------------+-----------+
|  7 | smb  |       | /home/falken/smb4k/bookcase-video |      | bookcase.house | video     |
+----+------+-------+-----------------------------------+------+----------------+-----------+
1 row in set (0.04 sec)

Note that's a different share, no MP3 on it.

I think this may be the issue, but I'll attach the whole log file in a tick :
amarok:             [MountPointManager] found handler for  "/org/kde/fstab///bookcase.house/falken" 
amarok:             BEGIN: virtual DeviceHandler* SmbDeviceHandlerFactory::createHandler(const Solid::Device&, const QString&, SqlStorage*) const 
amarok:               [ERROR__] [MySqlStorage] "GREPME MySQLe query failed! (1062) Duplicate entry '' for key 'devices_uuid' on INSERT INTO devices( type, servername, sharename, lastmountpoint ) VALUES ( 'smb', 'bookcase.house', 'falken', '/home/falken/smb4k/bookcase-falken' );" 
amarok:               [WARNING] [SmbDeviceHandler] Inserting into devices failed for type=smb, server= "bookcase.house" , share= "falken" 
amarok:             END__: virtual DeviceHandler* SmbDeviceHandlerFactory::createHandler(const Solid::Device&, const QString&, SqlStorage*) const [Took: 0s] 
amarok:             [MountPointManager] Factory  "smb" could not create device handler 
amarok:           END__: void MountPointManager::createHandlerFromDevice(const Solid::Dev
Comment 21 Tom Chiverton 2012-08-25 09:12:51 UTC
Created attachment 73453 [details]
startup output from version with second rev of patch
Comment 22 Tom Chiverton 2012-08-25 09:17:33 UTC
Figure this may be useful too :

mysql> select type, servername, sharename, lastmountpoint,uuid from devices;                                                                                  
| uuid |                |           | /media/cdrom0                     | 2C45-2480                            |
| uuid |                |           | /media/Media                      | 0E24-E54A                            |
| uuid |                |           | /media/dell-rescue-partition      | 2c45-2480                            |
| uuid |                |           | /media/Nokia N900                 | 4acd-9f4d                            |
| uuid |                |           | /home                             | 1d8b8b90-94f5-4e40-b502-648945f40763 |
| uuid |                |           | /                                 | 01aa6a10-1c91-4b24-9632-a45de910f911 |
| smb  | bookcase.house | video     | /home/falken/smb4k/bookcase-video |                                      | 
| uuid |                |           | /media/FC30-3DA9                  | fc30-3da9                            |
| uuid |                |           | /media/KUBUNTU-LIV                | 4e53-461a                            |
| uuid |                |           | /media/DATA                       | e870-2b82                            |
| uuid |                |           | /media/326D-EF7F                  | 326d-ef7f                            |
| uuid |                |           | /media/6266-3132                  | 6266-3132                            |
Comment 23 Matěj Laitl 2012-08-25 09:45:58 UTC
(In reply to comment #20)
> amarok:               [ERROR__] [MySqlStorage] "GREPME MySQLe query failed!
> (1062) Duplicate entry '' for key 'devices_uuid' on INSERT INTO devices(
> type, servername, sharename, lastmountpoint ) VALUES ( 'smb',
> 'bookcase.house', 'falken', '/home/falken/smb4k/bookcase-falken' );" 
> amarok:               [WARNING] [SmbDeviceHandler] Inserting into devices
> failed for type=smb, server= "bookcase.house" , share= "falken" 

This is the root of the problem! The error is strange, because normally the devices_uuid column should permit NULL values, and duplicit NULL values are okay in UNIQUE keys.

However, if your Amarok database is rather old (you use the same one from early Amarok 2 versions), is may be our db update scripts not working correctly. Could you please attach (plain text) your db *structure*? Please exclude unimportant amazon_*, jamendo_*, magnatune_*, opmldirectory_* tables.
Comment 24 Tom Chiverton 2012-08-25 14:56:22 UTC
Yes, this has been my music collection since Amarok 1.x so it's been through many updates :-)

I took this with mysqldump --host=127.0.0.1 --no-data --skip-add-drop-table --skip-set-charset --skip-comments amarok admin albums artists bookmark_groups bookmarks composers devices directories genres images labels lyrics playlist_groups playlist_tracks playlists podcastchannels podcastepisodes statistics statistics_permanent statistics_tag tracks urls urls_labels years > /tmp/db-struct.txt
Comment 25 Tom Chiverton 2012-08-25 14:56:45 UTC
Created attachment 73464 [details]
My DB structure
Comment 26 Matěj Laitl 2012-08-25 15:03:34 UTC
(In reply to comment #25)
> Created attachment 73464 [details]
> My DB structure

Oh yes, this confirms my suspiction - the uuid column is NOT NULL in your db. (and perhaps many other columns that should be null instead) We'll have to roll out new db update so set appropriate columns as NULL. Unfornunately this won't happen in a week or 2, because I'm occipied by real life now.

Until then you can work-aroung it by manually making the devices.uuid column NULL, but please keep your current database as a back-up to test fixes that will appear.
Comment 27 Matěj Laitl 2012-09-28 21:24:51 UTC
Git commit 81737be27a33351d57aae8d9604e7d0c4f3dec32 by Matěj Laitl.
Committed on 20/08/2012 at 00:45.
Pushed by laitl into branch 'master'.

{Smb,Nfs}DeviceHandler: use Solid instead of KMountPoint

KMountPoint seems to be unreliable and not working at least for smb
shares. Use Solid's NetworkShare for the same task, which is much more
*cough* ... solid. :-)

Server name & share name extraction changed a bit, but I've tested it
to give the same names as the old implementation. (not for some corner
cases, but old behaviour could be considered bogus there)

v2: use QUrl::host(), ::path() instead of uglier QString::section(),
reorder some calls for more meaningful debug messages.

This does not yet solve bug 302837 fully, just the first part of it.
FIXED-IN: 2.7
REVIEW: 106094

M  +38   -37   src/core-impl/collections/db/sql/device/nfs/NfsDeviceHandler.cpp
M  +38   -37   src/core-impl/collections/db/sql/device/smb/SmbDeviceHandler.cpp

http://commits.kde.org/amarok/81737be27a33351d57aae8d9604e7d0c4f3dec32
Comment 28 Tom Chiverton 2012-10-10 18:55:22 UTC
No dice. I tried updating the column def. (varchar(255) COLLATE utf8_bin null) and now Amarok won't believe anything is in my collection at all. 

Rolling back the DB change makes stuff appear again.

This was with the 2.6.0 release Amarok.

Let me know if I can be further help.
Comment 29 Matěj Laitl 2012-10-10 21:45:47 UTC
(In reply to comment #28)
> No dice. I tried updating the column def. (varchar(255) COLLATE utf8_bin
> null) and now Amarok won't believe anything is in my collection at all. 
> 
> Rolling back the DB change makes stuff appear again.

Thinking about it more, you'd need to update more columns (set all columns of the devices table except id as NULL)  and perhaps empty the devices table. (first try without empying it) Then after starting Amarok, perform full collection rescan. This still shoudn't delete your statistics as far as the tracks can be identified by unique id. (e.g. not changed in the mean time)

I still want to make a proper database-updating patch to do this automatically, only depends on how many of free time I'll be able to get.
Comment 30 Tom Chiverton 2012-11-11 15:44:35 UTC
I don't suppose there is a clean DB schema anywhere I could you to check mine against ?
Comment 31 Matěj Laitl 2012-11-11 17:26:21 UTC
(In reply to comment #30)
> I don't suppose there is a clean DB schema anywhere I could you to check
> mine against ?

There are a few options:
 * Amarok source/HACKING/mysql_database_schema.txt (could get outdated in future)
 * Logging in as a new user and looking at the db Amarok creates
 * DatabaseUpdater::createTables() method in src/core-impl/collections/db/sql/DatabaseUpdater.cpp

Also sorry for being rather silent lately, I still want to fix this for all users (by a database update) for Amarok 2.7, hopefully I'll find time.
Comment 32 Matěj Laitl 2012-12-09 21:57:46 UTC
Git commit 3080bf539c36ecbc7377f714794bae3b49e6575b by Matěj Laitl.
Committed on 09/12/2012 at 22:41.
Pushed by laitl into branch 'master'.

DATABASE UPDATE: fix some columns being NOT NULL while they should be NULL

This MySQL database schema update solves bug 302837. In short, updates
4 -> 5, 5 -> 6, 6 -> 7 and 9 -> 10 ignored NULL status of some columns
and replaced them with NOT NULL columns, causing various consequences,
one of them is Dynamic Collection not working. Fix it back.

A list of columns to fix was obtained by comparing a database created
by Amarok 2.1.1 and then upgraded to current versin with a db freshly
created by Amarok 2.6-git.

Tom, please test that this fixes your problem if you (temporarily)
restore your old database where you were able to reproduce your bug.
FIXED-IN: 2.7

M  +2    -0    ChangeLog
M  +109  -2    src/core-impl/collections/db/sql/DatabaseUpdater.cpp
M  +1    -0    src/core-impl/collections/db/sql/DatabaseUpdater.h

http://commits.kde.org/amarok/3080bf539c36ecbc7377f714794bae3b49e6575b