Bug 363697 - Album visible even after deletion [patch]
Summary: Album visible even after deletion [patch]
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Unclassified
Component: Database-Mysql (show other bugs)
Version: 5.0.0
Platform: Kubuntu Packages Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-30 09:29 UTC by swatilodha27
Modified: 2016-07-02 14:38 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.0.0


Attachments
foreignKeyChecks.patch (1.88 KB, patch)
2016-06-23 18:52 UTC, Maik Qualmann
Details
albumRoot.patch (4.18 KB, patch)
2016-06-28 16:51 UTC, Maik Qualmann
Details
removeStaleAlbum.patch (4.71 KB, patch)
2016-06-29 17:53 UTC, Maik Qualmann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description swatilodha27 2016-05-30 09:29:47 UTC
I use MySQL Internal and the album are still even after deletion.

Reproducible: Always

Steps to Reproduce:
1.Open digiKam with MySQL Internal support. 
2.Add a new album and some pictures.
3.Delete it. 

Actual Results:  
The album is still visible in digiKam and when I try to delete it, it says "it doesn't exist". 

The alums though doesn't exist in my Pictures folder.

Expected Results:  
The album deleted shouldn't be visible.
Comment 1 swatilodha27 2016-06-21 22:02:15 UTC
*** Bug 364614 has been marked as a duplicate of this bug. ***
Comment 2 swatilodha27 2016-06-21 22:04:53 UTC
*** Bug 364614 has been marked as a duplicate of this bug. ***
Comment 3 Johannes Hirte 2016-06-21 22:18:03 UTC
As my report is marked as a duplicate of this, please see my description there: https://bugs.kde.org/show_bug.cgi?id=364614

It't not only the album shouldn't be visible anymore after deletion, the whole db management seems to be broken in case of moving/deleting items.
Comment 4 swatilodha27 2016-06-21 22:20:58 UTC
Yes. I agree with you.
DB doesn't work well in case of deletion/moving.
Comment 5 Johannes Hirte 2016-06-22 18:02:04 UTC
After some digging into the code, I've seen that deleted images are finally removed from the db. It's just done asynchronously, deferred for some days:

bool CollectionScanner::checkDeleteRemoved()
{
...
    return (daysPast > 7  && completeScans > 2) ||
           (daysPast > 30 && completeScans > 0) ||
           (completeScans > 30);
}

Don't know, why this is delayed so much, but I can confirm that pictures marked as deleted are purged from the db when setting completeScans > 30 in the db.
Comment 6 Johannes Hirte 2016-06-22 22:19:03 UTC
some more debugging:

void CoreDB::makeStaleAlbum(int albumID) does not work. After this function, albumRoot of the deleted album has to be 0 and relativePath has to be prefixed with "1-", where 1 is the id of the rootalbum. This does not happen. When setting albumRoot to 0 by hand, digikam does the cleanup on next start and purges the entry from the db. Maybe it's a problem with foreign key checks, that stop digikam from updating the table.
Comment 7 Johannes Hirte 2016-06-22 22:51:51 UTC
Of course, foreign key checks is part of the problem. Disabling this with:

    d->db->execSql(QString("SET FOREIGN_KEY_CHECKS = 0;"));
    d->db->execSql(QString("UPDATE Albums SET albumRoot=0, relativePath=? WHERE id=?;"),
                   newRelativePath, albumID);

    // for now, we make no distinction to deleteAlbums wrt to changeset
    d->db->recordChangeset(AlbumChangeset(albumID, AlbumChangeset::Deleted));
    d->db->execSql(QString("SET FOREIGN_KEY_CHECKS = 1;"));

will make deletion of albums work. But I'm not sure, if this is really the right way to fix this.
Comment 8 Johannes Hirte 2016-06-23 11:23:47 UTC
(In reply to Johannes Hirte from comment #7)
> Of course, foreign key checks is part of the problem. Disabling this with:
> 
>     d->db->execSql(QString("SET FOREIGN_KEY_CHECKS = 0;"));
>     d->db->execSql(QString("UPDATE Albums SET albumRoot=0, relativePath=?
> WHERE id=?;"),
>                    newRelativePath, albumID);
> 
>     // for now, we make no distinction to deleteAlbums wrt to changeset
>     d->db->recordChangeset(AlbumChangeset(albumID, AlbumChangeset::Deleted));
>     d->db->execSql(QString("SET FOREIGN_KEY_CHECKS = 1;"));
> 
> will make deletion of albums work. But I'm not sure, if this is really the
> right way to fix this.

Okay, this won't work as mysql and sqlite have different ways for disabling/enabling foreign key checks.

The real problem is, that Albums.albumRoot has to be set to a value that doesn't exists in AlbumRoots. The two options I see for solving this:

1) working with a fake album in AlbumRoots, so all albums that should be deleted can be set to this fake album

2) disable foreign key checks for this operation. This has to be done in a generic way disableForeignKeyCheck()/enableForeignKeyCheck() that implements the specific part for every DBMS
for MySQL: "SET FOREIGN_KEY_CHECKS = 0/1;"
for sqlite: " PRAGMA foreign_keys = OFF/ON;"
Comment 9 caulier.gilles 2016-06-23 12:45:20 UTC
Johannes,

Thanks to investiguate. Take a look to this entry :

https://bugs.kde.org/show_bug.cgi?id=237878

Which go to the opposite way about foreign keys support.

Note : i prefer solution 2/ instead fake album way.

Gilles Caulier
Comment 10 swatilodha27 2016-06-23 13:36:05 UTC
Johannes,

Your second solution seems to fit here. Temporary disabling referential constraints might work 'cause we've parent-child tables. 

In the function makeStaleAlbum(), there's an update query which doesn't work. (Reason why Albums table is not updated)
We could use:
//
SET FOREIGN_KEY_CHECKS = 0;
UPDATE.....
SET FOREIGN_KEY_CHECKS = 1;
//
I think this should work...
Comment 11 caulier.gilles 2016-06-23 13:49:05 UTC
Swati,

A patch will be welcome here to be able to test this solution, if Johannes is favorable to use this way of course...

Also, what's about the bug #237878 ?

Gilles Caulier
Comment 12 Maik Qualmann 2016-06-23 18:52:49 UTC
Created attachment 99669 [details]
foreignKeyChecks.patch

I think a temporary disabling is fine here. Please test the patch.

Maik
Comment 13 Johannes Hirte 2016-06-23 21:40:55 UTC
Patch works for me with MySQL/MariaDB.
Comment 14 Richard Mortimer 2016-06-24 08:21:36 UTC
Disabling foreign key checks is pretty much the same as not using referential integrity. It allows the database to get into "illegal" states. It is also highly non-portable between database engines.

I just tried a quick test with MySQL 5.5 and simulated a crash halfway through deleting an album. That left the database with an albumRoot set to a non-existant value in the database.

mysql> set foreign_key_checks = 0;
Query OK, 0 rows affected (0.00 sec)

mysql> update Albums set albumRoot = 0 where id = 1;
Query OK, 1 row affected (0.19 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql> set foreign_key_checks = 1;
Query OK, 0 rows affected (0.00 sec)

mysql> select * from Albums;
+----+-----------+--------------+------------+---------+------------+------+
| id | albumRoot | relativePath | date       | caption | collection | icon |
+----+-----------+--------------+------------+---------+------------+------+
|  1 |         0 | /            | 2015-11-25 | NULL    | NULL       | NULL |
|  2 |         1 | /Capture     | 2015-11-25 | NULL    | NULL       | NULL |
|  3 |         1 | /Test        | 2015-11-25 | NULL    | NULL       | NULL |
|  4 |         1 | /Webcam      | 2015-11-03 | NULL    | NULL       | NULL |
+----+-----------+--------------+------------+---------+------------+------+
4 rows in set (0.00 sec)

I really do think that the fake, placeholder, album route is the correct way to go. It is similar to dropping foreign key checks but it does not leave the database in an inconsistent state.
Comment 15 swatilodha27 2016-06-25 06:41:09 UTC
Is it possible to already have a "Fake Album" row when the DB is initially created? 
In the DB schema file, adding a query:
//
INSERT INTO AlbumRoots values(0, "Fake Album Root", 0, 0, NULL, NULL);
//

This way the current UPDATE query in the makeStaleAlbum() would execute without foreign key constraint failure.

Also, when deleteStaleAlbum() would be called, all those Albums would be deleted from the DB.
It just need to be take care to include this function in a way that it gets executed each time when digiKam is started.
Comment 16 Johannes Hirte 2016-06-27 20:46:09 UTC
(In reply to swatilodha27 from comment #15)
> Is it possible to already have a "Fake Album" row when the DB is initially
> created? 
> In the DB schema file, adding a query:
> //
> INSERT INTO AlbumRoots values(0, "Fake Album Root", 0, 0, NULL, NULL);
> //
> 
> This way the current UPDATE query in the makeStaleAlbum() would execute
> without foreign key constraint failure.

Did this manually and it seems to work. But I don't know if other parts of the code may get confused about this fake album.

> Also, when deleteStaleAlbum() would be called, all those Albums would be
> deleted from the DB.
> It just need to be take care to include this function in a way that it gets
> executed each time when digiKam is started.

I don't really understand the mechanism of hints for now. Just calling deleteStaleAlbums() unconditionally will do, what you're suggesting. Maybe this is overkill and we can hook this in another place better.
Comment 17 swatilodha27 2016-06-28 14:12:29 UTC
Richard,

Could you please provide your suggestions here?

Thank you.
Comment 18 Maik Qualmann 2016-06-28 16:51:41 UTC
Created attachment 99742 [details]
albumRoot.patch

A first new patch, it works. The question is, is correct and can solved this problem?

Maik
Comment 19 swatilodha27 2016-06-28 19:57:52 UTC
(In reply to Maik Qualmann from comment #18)
> Created attachment 99742 [details]
> albumRoot.patch
> 
> A first new patch, it works. The question is, is correct and can solved this
> problem?

Well, it did work for me. After deletion of album, it was neither visible, nor present in DB.
And this patch solves the issue in right way I think.
Comment 20 Richard Mortimer 2016-06-29 09:44:06 UTC
I like the look of Maik's patch. It does seem to be working in a portable way that does not rely on any magic database primary key values. A deleted AlbumRoot is easy to identify too.

A few comments on the patch:

the magic relativePath value "*deleted*" should really be a constant that is used throughout the application. That way it is easier to ensure consistency and find places where the value is used.

I haven't looked but does addAlbumRoot cope gracefully with situations where there is already an existing albumRoot for deleted? If not then the code needs to check for presence of a deleted album root before trying to add one.
Comment 21 Maik Qualmann 2016-06-29 10:43:01 UTC
I wonder just what need the stale album function. Perhaps Marcel can give an indication. I have in the CollectionScanner this function replaced with CoreDB::deleteAlbum(). It seems to work without problems. I create tonight a patch.

Maik
Comment 22 Maik Qualmann 2016-06-29 17:53:13 UTC
Created attachment 99765 [details]
removeStaleAlbum.patch

This patch removes stale album function. Swati, you can test it please.

Maik
Comment 23 swatilodha27 2016-06-29 18:55:57 UTC
(In reply to Maik Qualmann from comment #22)
> Created attachment 99765 [details]
> removeStaleAlbum.patch
> 
> This patch removes stale album function. Swati, you can test it please.

The deleted album is no more visible in DK, but entry still exists in DB.
Is it the same for you?
Comment 24 Maik Qualmann 2016-06-29 19:56:35 UTC
No, the entry does not exist here in the DB. Strange.

Maik
Comment 25 Maik Qualmann 2016-06-29 20:46:46 UTC
Git commit adc7a5868a10f8f21ac3b36edecdd9f53f1efec4 by Maik Qualmann.
Committed on 29/06/2016 at 20:45.
Pushed by mqualmann into branch 'master'.

disable temporary Foreign_Key_Checks
FIXED-IN: 5.0.0

M  +2    -2    NEWS
M  +2    -0    libs/database/coredb/coredb.cpp
M  +17   -0    libs/database/engine/dbenginebackend.cpp
M  +6    -0    libs/database/engine/dbenginebackend.h

http://commits.kde.org/digikam/adc7a5868a10f8f21ac3b36edecdd9f53f1efec4
Comment 26 Maik Qualmann 2016-06-29 20:51:22 UTC
I think that this solution is better at the moment. I have set this problem on my todo list, to find a clean solution for the DB.

Maik
Comment 27 swatilodha27 2016-07-02 13:51:53 UTC
Maik,

Could your last patch be included before 5.0.0 ?
Comment 28 Maik Qualmann 2016-07-02 14:38:09 UTC
Yes, the FOREIGN_KEY_CHECKS patch is included into git/master.

Maik