GNOME Bugzilla – Bug 794504
grl-thegamesdb.lua: Error when trying to access GameTitle for some games
Last modified: 2018-04-18 16:15:56 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 '?')
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 ;)
The proposed patch works well, sorry for the late response.
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
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
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
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.
(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.
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
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.
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,
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
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
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.
(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
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.
Created attachment 371104 [details] [review] Revert "thegamesdb: Check for empty list" This reverts commit 6a06a6fdbafb16d3e4137bfe2522845255eada49.
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.
Umm, I guess I shouldn't have referenced the first patch commit hash.
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.
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