Review Request: SqlScanResultProcessor: fix data-loss bug; squashed commits, recent on top

[Matěj Laitl]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105488/
-----------------------------------------------------------

(Updated July 9, 2012, 12:38 a.m.)


Review request for Amarok and Ralf Engels.


Changes
-------

fix title, sorry for noise.


Summary (updated)
-----------------

SqlScanResultProcessor: fix data-loss bug; squashed commits, recent on top


Description
-------

SqlScanResultProcessor: debugging; not intended to be merged to master


SqlScanResultProcessor: don't accidentally delete tracks, defensive rewrite

This fixes data-loss that I can trigger every time by toggling "Local
Files & USB Mass Storage Backend" in Config -> Plugins, restarting
Amarok and triggering collection update / rescan.

In theory, more things such as cloning changing disk could trigger this
problem, from SqlScanResultProcessor::deleteDeletedDirectories() comment:

We need to match directories by their (absolute) path, otherwise following
scenario triggers statistics loss (perhaps bugs 298275, 292245):

1. user relocates collection to different filesystem, but clones path structure
   or toggles MassStorageDeviceHandler enabled in Config -> plugins.
2. collectionscanner knows nothings about directory ids, so it doesn't detect
   any track changes and emits a bunch of skipped (unchanged) dirs with no
   tracks.
3. SqlRegistry::getDirectory() called there from returns different directory id
   then in past.
4. deleteDeletedDirectories() is called, and if it operates on directory ids,
   it happily removes _all_ directories, taking tracks with it.
5. Tracks disappear from the UI until full rescan, stats, lyrics, labels are
   lost forever.

Also add a handful of asserts, ScanResultProcessor is very complicated
and small error or corner-case in logic may result in horrible data
losses.

Reporters of linked bugs, please try to reproduce your data-loss with
this patch applied and report back in both cases. In negative case,
please reopen and attach full updated amarok --debug log.

After this patch, only (statistics, lyrics and labels)-loss operations
should be:
 a) moving track out of mounted collection folders [by design]
 b) changing both metadata and url from outside of a track not tagged
    by amarok_afttagger [we can do nothing about this]
 c) changing the meta-data within Amarok followed by moving the track
    (within or outside of Amarok) not tagged by amarok_afttagger
    [bug 292245, I think I know the cause now]

BUG: 298275
CCBUG: 292245
FIXED-IN: 2.6
REVIEW: 105488

SqlMeta::Track: remove unused methods deviceId() and rpath()

There will be made misleading by the next commit, so better not to have
them at all. id() and urlId() are unused, too, but these are at least
in theory useful.

SqlScanResultProcessor: remove dead code, sanitize includes


SqlRegistry, DatabaseUpdater: delete stats and everything in removeTrack()

SqlRegistry::removeTrack() had code to remove the entry from the tracks
table and to preserve the entry in the statistics table. This doesn't
work, because SqlRegistry construction calls
DatabaseUpdater::deleteAllRedundant( url ) on next start-up that
removes the url entry, plus we don't know how long we should keep the
entry, so we just delete everything. ScanResultProcessor should be
witty enough not to delete tracks that have been moved to another
directory and/or device, even if it is currently unavailable.

Additionally, we clean up the statistics, urls_labels, lyrics tables
on start-up to avoid stale entries (pointing to deleted url) in a way
similar to what it is done with other tables.

SqlScanResultProcessor: rename attributes in preparation for a fix

...because I was really confused by the old names.


This addresses bugs 292245 and 298275.
    https://bugs.kde.org/show_bug.cgi?id=292245
    https://bugs.kde.org/show_bug.cgi?id=298275


Diffs
-----

  src/core-impl/collections/db/sql/DatabaseUpdater.h 57af65de0e3cbea5d7b7693f2d586eb1fe823870 
  src/core-impl/collections/db/sql/DatabaseUpdater.cpp 59e071d2f4fc69d79196e22e9119ff02805f28ca 
  src/core-impl/collections/db/sql/SqlMeta.h 8ee1014eb160547c4cbc0ea5571c12a895a79c64 
  src/core-impl/collections/db/sql/SqlMeta.cpp a9b678aa4e3e55a2394da6f6581c159580fe6fc5 
  src/core-impl/collections/db/sql/SqlRegistry.h 8d801178cc070c7772363d28d86ca63467996ebe 
  src/core-impl/collections/db/sql/SqlRegistry.cpp 1b9efe5490bebc849b6c52c0b0b3fd51a5565418 
  src/core-impl/collections/db/sql/SqlScanResultProcessor.h 1bd96db03363cf8f760811fb231fd9facf521e8e 
  src/core-impl/collections/db/sql/SqlScanResultProcessor.cpp 6699b982e124193d068e2bd093bf0d15d6e34a9c 

Diff: http://git.reviewboard.kde.org/r/105488/diff/


Testing
-------


Thanks,

Matěj Laitl

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120709/1570bf6a/attachment-0001.html>