[Tracker] Anonymous File Nodes branch review

[Martyn Russell <martyn lanedo com>]

Hi all,

Carlos and Philip recently worked on the anonymous-file-nodes branch to 
fix a regression with the way resources are stored in the database which 
was changed recently. This is the review of that branch:

  http://git.gnome.org/browse/tracker/log/?h=anonymous-file-nodes


1. We should use G_DIR_SEPARATOR_S here instead:

+       slash_uri = g_strconcat (source_uri, "/", NULL);


2. This looks like some left over crack that needs fixing :)

-       tracker_sparql_builder_object_iri (sparql, uri);
+       tracker_sparql_builder_object (sparql, "_:foo");


3. In the search-bar you have these changes, but surely urn should be 
changed for uri in ALL cases here, otherwise we have no ranking? I could 
be wrong though.

+       "  ?uri ?title ?tooltip ?urn fts:rank(?urn) " \


4. Don't we need quotes round ?f (in add_tag_for_urns and again in 
remove_tag_for_urns)?

-                                        "  ?urn nie:isStoredAs ?f ."
+                                        "  ?urn nie:url ?f ."


5. I would improve the this so the query is more readable personally:

stmt = tracker_db_interface_create_statement (iface, "SELECT ID FROM 
\"nie:DataObject\" WHERE \"nie:DataObject\".\"nie:url\" = ?")

It also looks erroneous to me. Why do we have this addition "." before 
\"nie:url\" ?

Shouldn't that be \"nie:DataObject\" ?do . \"nie:url\" ...


6. I think we should add proper error handling here:

+          /* For fallback to the old behaviour, we could do this here:
+           * resource_id = tracker_data_query_resource_id (url); */
+          return;


7. Hmm, not sure why this change was added, usually if result is set and 
> 0 in length then there was no error, checking that here seems 
pointless to me as we would still check the array and its length even if 
there was no error so? This is in 
src/tracker-writeback/tracker-writeback-consumer.c in sparql_query_cb().

-       if (result && result->len > 0) {
+       if (!error && result && result->len > 0) {


8. Again, I would make queries like this easier to read for maintainability:

+ query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
+                          "  <%s> ?predicate ?object ; nie:url ?url ."
+                          "  ?predicate tracker:writeback true "
+                          "}", data->subject, data->subject);

Also, I presume this is valid SPARQL to put '%s' after ?url, looks to me 
like it will just be included as one of the columns in the results - 
isn't that a little inefficient (depending on how many results are 
returned).


9. I didn't know this could be used as an OPTIONAL replacement:

- query = g_strdup_printf ("SELECT ?city ?state ?address ?country "
-                          "WHERE { <%s> mlo:location ?location . "
-                          "OPTIONAL { ?location mlo:address ?address } . "
-                          "OPTIONAL { ?location mlo:city ?city } . "
-                          "OPTIONAL { ?location mlo:country ?country } . "
-                          "OPTIONAL { ?location mlo:state ?state} "
-                          "}", row[0]);
+ query = g_strdup_printf ("SELECT mlo:city (?city) mlo:state (?state) "
+                          " mlo:address (?address) "
+                          " mlo:country (?country) "
+                          "WHERE { <%s> mlo:location ?location }",

If this works then it is a nice reduction and simplification in the 
SPARQL :)


10. Please don't add spaces after conditional statements (in extract_mp3 
and extract_vorbis) :)

                if (md.track_count > 0) {
+
+                       tracker_sparql_builder_insert_close (preupdate);


11. Another case of "_foo" in miner_files_add_to_datasource() and 
process_file_cb(), should use something more meaningful. The "_foo" is 
used to generate the id you know :P and it looks unprofessional :)


12. The documentation here needs a small fix:

+ * through tracker_sparql_builder_new_embedded_insert(), The subject

No need to capitalise "The" there. Unless the comma should be a ".".

+ * function should just provide predicate/object(s) pairs. the data

Here there is no capital "T".


13. Hmm, is this correct in extract_html(), extract_imagemagick(), etc:

-       tracker_sparql_builder_subject_iri (metadata, uri);


14. The get_file_metadata() function in tracker-extract.c needs fixing 
more I think for failure conditions. More specifically, I think the _out 
variables should be set to NULL initially so they don't have to be set 
in every place where we return FALSE. The libstreamanalyser part doesn't 
set them at all, which would break and likely crash things. We also 
don't unref the preupdate variable when we return FALSE on line 267.


15. No need for braces here round preinserts_str:

+  dbus_g_method_return (context,
+                       (preinserts_str) ? preinserts_str : "",
+                        tracker_sparql_builder_get_result (sparql));



Other than that the rest looks good. Let's see if we can get these fixed 
so we can merge to master for tomorrow's release :)

Thanks guys,

--
Regards,
Martyn