[digikam] [Bug 363697] New: Album visible even after deletion

[bugzilla_noreply]

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

            Bug ID: 363697
           Summary: Album visible even after deletion
           Product: digikam
           Version: 5.0.0
          Platform: Kubuntu Packages
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: NOR
         Component: Database-Mysql
          Assignee: [hidden email]
          Reporter: [hidden email]

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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
                   |                            |e

--- Comment #1 from [hidden email] ---
*** Bug 364614 has been marked as a duplicate of this bug. ***

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #2 from [hidden email] ---
*** Bug 364614 has been marked as a duplicate of this bug. ***

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #3 from Johannes Hirte <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #4 from [hidden email] ---
Yes. I agree with you.
DB doesn't work well in case of deletion/moving.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #5 from Johannes Hirte <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #6 from Johannes Hirte <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #7 from Johannes Hirte <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #8 from Johannes Hirte <[hidden email]> ---
(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;"

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #9 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #10 from [hidden email] ---
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...

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #11 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #12 from Maik Qualmann <[hidden email]> ---
Created attachment 99669
  --> https://bugs.kde.org/attachment.cgi?id=99669&action=edit
foreignKeyChecks.patch

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

Maik

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

Maik Qualmann <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Album visible even after    |Album visible even after
                   |deletion                    |deletion [patch]

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #13 from Johannes Hirte <[hidden email]> ---
Patch works for me with MySQL/MariaDB.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

Richard Mortimer <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #14 from Richard Mortimer <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #15 from [hidden email] ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #16 from Johannes Hirte <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

--- Comment #17 from [hidden email] ---
Richard,

Could you please provide your suggestions here?

Thank you.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[bugzilla_noreply]

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

Maik Qualmann <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #99669|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel