After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 794504 - grl-thegamesdb.lua: Error when trying to access GameTitle for some games
grl-thegamesdb.lua: Error when trying to access GameTitle for some games
Status: RESOLVED FIXED
Product: grilo
Classification: Other
Component: plugins
git master
Other Linux
: Normal normal
: ---
Assigned To: grilo-maint
grilo-maint
Depends on:
Blocks:
 
 
Reported: 2018-03-20 00:40 UTC by pirateking
Modified: 2018-04-18 16:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patch (1.08 KB, patch)
2018-03-20 06:56 UTC, Victor Toso
none Details | Review
thegamesdb: Handle situation with only one result (1.30 KB, patch)
2018-04-14 11:56 UTC, Alexander Mikhaylenko
none Details | Review
thegamesdb: Handle situation with only one result (5.16 KB, patch)
2018-04-14 22:46 UTC, Alexander Mikhaylenko
none Details | Review
thegamesdb: Handle situation with only one result (6.04 KB, patch)
2018-04-18 14:50 UTC, Alexander Mikhaylenko
none Details | Review
test: Add a test for Shatterhand game (5.04 KB, patch)
2018-04-18 15:43 UTC, Alexander Mikhaylenko
committed Details | Review
Revert "thegamesdb: Check for empty list" (1.00 KB, patch)
2018-04-18 15:43 UTC, Alexander Mikhaylenko
committed Details | Review
thegamesdb: Handle situation with only one result (1.19 KB, patch)
2018-04-18 15:43 UTC, Alexander Mikhaylenko
none Details | Review
thegamesdb: Handle situation with only one result (1.14 KB, patch)
2018-04-18 15:47 UTC, Alexander Mikhaylenko
committed Details | Review

Description pirateking 2018-03-20 00:40:13 UTC
The following error is received when trying to resolve id for some games

[lua-library] grl-lua-library.c:548: calling source callback function fail: /app/share/grilo-plugins/grl-lua-factory/grl-thegamesdb.lua:94: attempt to index a nil value (field '?')
Comment 1 Victor Toso 2018-03-20 06:56:00 UTC
Created attachment 369891 [details] [review]
proposed patch

Hi, the code on the log is:

> return results_table.Data.Game[1].id.xml

Which very likely means that we are accessing some non existing data in the table instead of invalid data from thegamesdb source.

Can't reproduce the bug so it would be awesome if you could test this change to see if the warnings fade away ;)
Comment 2 pirateking 2018-03-22 20:33:55 UTC
The proposed patch works well, sorry for the late response.
Comment 3 Victor Toso 2018-03-22 21:01:19 UTC
commit 6a06a6fdbafb16d3e4137bfe2522845255eada49
Author: Victor Toso <me@victortoso.com>
Date:   Tue Mar 20 07:49:12 2018 +0100

    thegamesdb: Check for empty list

    Checking if table exists is not enough. If xml field exists,
    grl.lua.xml.string_to_table() will include that into the returning
    table.

    Resolves: https://bugzilla.gnome.org/show_bug.cgi?id=794504
Comment 4 Alexander Mikhaylenko 2018-04-14 11:23:16 UTC
This has caused a regression with results like this:
http://thegamesdb.net/api/GetGamesList.php?name=Akumajou%20Dracula&platform=Sharp%20X68000

#results_table.Data.Game is 0 here, because all keys are non-numeric.

An example of what I mean:

> a = {}
> a.Platform = "test"
> a["id"] = 31754
> a
table: 0x558756851690
> #a
0
> a[1] = 123
> a[2] = 456
> #a
2
Comment 5 Alexander Mikhaylenko 2018-04-14 11:56:13 UTC
Created attachment 370924 [details] [review]
thegamesdb: Handle situation with only one result

In that case, results_table.Data.Game would contain GameTitle, id and
other keys directly and #results_table.Data.Game would return 0.

Fixes regression from bug 794504
Comment 6 Alexander Mikhaylenko 2018-04-14 11:58:41 UTC
I've removed the check from the previous patch, because in the case where there are no results, the xml would be like this:
http://thegamesdb.net/api/GetGamesList.php?name=test&platform=Sharp%20X68000

So "not results_table.Data.Game" would be enough.

pirateking: it would still be useful if you name the games that caused the problem.
Comment 7 Bastien Nocera 2018-04-14 12:09:49 UTC
(In reply to Alexander Mikhaylenko from comment #6)
> pirateking: it would still be useful if you name the games that caused the
> problem.

Yes, and it needs tests as well.
Comment 8 Alexander Mikhaylenko 2018-04-14 22:46:00 UTC
Created attachment 370943 [details] [review]
thegamesdb: Handle situation with only one result

In that case, results_table.Data.Game would contain GameTitle, id and
other keys directly and #results_table.Data.Game would return 0.

Fixes bug 794504
Comment 9 Alexander Mikhaylenko 2018-04-14 22:47:40 UTC
pirateking told me on IRC that it was Shatterhand game for NES.

http://thegamesdb.net/api/GetGamesList.php?name=Shatterhand&platform=Nintendo%20Entertainment%20System%20(NES) has one result as well, so the patch ultimately solves the original issue. :)

I've added a test for this game.
Comment 10 Victor Toso 2018-04-18 07:53:49 UTC
Review of attachment 370943 [details] [review]:

Hi, thanks for the patch.

Please add to the commit log
- that this patch is reverting 6a06a6fdbafb1 as it was the wrong fix for this bug.
- that you are including a test for Shatterhand for NES which reproduces this bug

With that said, you still need to include the code in tests/games/test_games.c for actually testing the Shatterhand case. Maybe you forgot to git add it to this patch?

Cheers,
Comment 11 Alexander Mikhaylenko 2018-04-18 14:50:01 UTC
Created attachment 371095 [details] [review]
thegamesdb: Handle situation with only one result

In that case, results_table.Data.Game would contain GameTitle, id and
other keys directly and #results_table.Data.Game would return 0.

Also add a test for Shatterhand game that reproduces this bug.

Reverts commit 6a06a6fdbafb1.

Fixes bug 794504
Comment 12 Alexander Mikhaylenko 2018-04-18 14:51:48 UTC
Yes, I've forgot to commit it somehow. Second time in a week actually :( Sorry for that.

I've updated it now. The test passes:

TEST: test_games... (pid=22546)
  /thegamesdb/resolve/good-found:                                      OK
  /thegamesdb/resolve/thumbnails-found:                                OK
PASS: test_games

Though I have to disable lua-factory test, because of:

TEST: test_lua_metrolyrics... (pid=22186)
  /lua_factory/sources/metrolyrics:                                    
** (test_lua_metrolyrics:22186): WARNING **: Lyrics of 'ring of fire' from 'johnny cash' changed. Check if metrolyrics.com changed
FAIL
Comment 13 Bastien Nocera 2018-04-18 15:26:57 UTC
Review of attachment 371095 [details] [review]:

Can you please split up the patch?

First revert the old patch, then add the new test, finally add the code to fix it.
Comment 14 Bastien Nocera 2018-04-18 15:28:38 UTC
(In reply to Bastien Nocera from comment #13)
> Review of attachment 371095 [details] [review] [review]:
> 
> Can you please split up the patch?
> 
> First revert the old patch, then add the new test, finally add the code to
> fix it.

This will make it easier to see that the patch fixes the test. The order could also be:
- add new test and show that while it works, it fails with an old test
- add combined patch to make all the tests work
Comment 15 Alexander Mikhaylenko 2018-04-18 15:43:27 UTC
Created attachment 371103 [details] [review]
test: Add a test for Shatterhand game

This is a case of thegamesdb returning only one search result.

This test is expected to fail.
Comment 16 Alexander Mikhaylenko 2018-04-18 15:43:32 UTC
Created attachment 371104 [details] [review]
Revert "thegamesdb: Check for empty list"

This reverts commit 6a06a6fdbafb16d3e4137bfe2522845255eada49.
Comment 17 Alexander Mikhaylenko 2018-04-18 15:43:38 UTC
Created attachment 371105 [details] [review]
thegamesdb: Handle situation with only one result

In that case, results_table.Data.Game would contain GameTitle, id and
other keys directly and #results_table.Data.Game would return 0.

The test introduced in commit 64101a4241de8dac462b442967c4f16c2268f12d
should now pass.
Comment 18 Alexander Mikhaylenko 2018-04-18 15:45:46 UTC
Umm, I guess I shouldn't have referenced the first patch commit hash.
Comment 19 Alexander Mikhaylenko 2018-04-18 15:47:58 UTC
Created attachment 371106 [details] [review]
thegamesdb: Handle situation with only one result

In that case, results_table.Data.Game would contain GameTitle, id and
other keys directly and #results_table.Data.Game would return 0.

All thegamesdb tests should pass now.
Comment 20 Bastien Nocera 2018-04-18 16:15:36 UTC
Attachment 371103 [details] pushed as e5c9aba - test: Add a test for Shatterhand game
Attachment 371104 [details] pushed as 6813192 - Revert "thegamesdb: Check for empty list"
Attachment 371106 [details] pushed as 2d1ec67 - thegamesdb: Handle situation with only one result