GNOME Bugzilla – Bug 737539
Freebase: topic results are shallower than expected
Last modified: 2014-12-23 10:19:10 UTC
Using the Freebase API in real usecases I found the data tree returned in GDataFreebaseTopicResult objects to be quite shallower than the JSON data received, it turned out that on certain circumstances it wouldn't iterate properly inside "compound" nodes. I'm attaching a couple of patches to fix this.
Created attachment 287298 [details] [review] freebase: reader_fill_object_gvalue() is only meant for "object" types Compound types are only meant to be handled through reader_fill_compound_gvalue(), which will iterate through its "property" array. Otherwise, compound elements with an ID (which is not mandatory, but ok for these to have) would be created as shallow objects instead of containers of further objects/values.
Created attachment 287299 [details] [review] freebase: Fix thinko when creating compound GDataFreebaseTopicObjects The check was actually inverted, receiving a newly created object is obviously the expected condition. This made the parsing of compound objects a lot more shallower than the received data.
Review of attachment 287298 [details] [review]: If you say so. :-) Some unit tests for these changes to the Freebase backend would be useful. Do you have time to write some? Could be as simple as the JSON parsing tests in https://git.gnome.org/browse/libgdata/tree/gdata/tests/tasks.c#n369, but it would be really useful to add those as we close these bugs, so the exact markup which triggered the bug isn’t lost, and can be kept as a regression test.
Review of attachment 287299 [details] [review]: Same comment as above. Tests needed! :-)
Created attachment 287768 [details] [review] tests: Add tests for freebase topic API
Review of attachment 287768 [details] [review]: Great, thanks. A few minor things then it’s ready to commit. ::: gdata/tests/freebase.c @@ +2,3 @@ +/* + * GData Client + * Copyright (C) Philip Withnall 2014 <philip@tecnocode.co.uk> This is wrong. :-) @@ +91,3 @@ + g_main_loop_run (closure.main_loop); + g_assert_no_error (closure.error); + g_assert_nonnull (closure.data); This bumps our GLib dependency to 2.39, which I’m not sure I want to do just yet. Can you use g_assert (closure.data != NULL) instead please? Same for the other uses of the new assertion macros.
I've *finally* gotten to these patches... Removed all traces of glib > 2.30 in tests and pushed everything :) Attachment 287298 [details] pushed as c28a745 - freebase: reader_fill_object_gvalue() is only meant for "object" types Attachment 287299 [details] pushed as 460c79a - freebase: Fix thinko when creating compound GDataFreebaseTopicObjects Attachment 287768 [details] pushed as 5c598dc - tests: Add tests for freebase topic API