Re: [Tracker] Anonymous File Nodes branch review

On Wed, 2010-02-03 at 12:08 +0000, Martyn Russell wrote:
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:

1. We should use G_DIR_SEPARATOR_S here instead:

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

(I'm skipping this, change by Carlos)

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

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

Looks right to me

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) " \

(I'm skipping this, change by Carlos)

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

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

(I'm skipping this, change by Carlos)

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\" ...

Don't be confused, this isn't a SPARQL query but a native SQLite query.

There's a dot before "nie:url" because "nie:DataObject" is the table
name and "nie:url" is the column name. Without that dot it would not be
a correct native SQLite query.

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;

No there's no error in that case. It just means that no removing needs
to take place (we can silently ignore it).

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) {

I had the situation where error was set and result wasn't NULL. I think
result is left untouched in case error is set, and if then the stack
variable isn't cleared to NULL, error isn't NULL. So just checking for
result isn't sufficient.

It was a bugfix for something I saw happening.

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

What is hard about this? Note that only the "'%s'" and the
"nie:url ?url" got added tot he original query.

+ 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 

Yes, that's correct SPARQL

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 

There's no inefficiency caused by this. And doing it differently will
break the writeback modules and make things *a lot* harder to port and
support (all writeback modules need a rewrite).

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 

Yes, this is possible and explained here:

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);

Not sure who changed this

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 :)

(I'm skipping this, change by Carlos)

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".

(I'm skipping this, change by Carlos)

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

-       tracker_sparql_builder_subject_iri (metadata, uri);

(I'm skipping this, change by Carlos)

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.

(I'm skipping this, change by Carlos)

15. No need for braces here round preinserts_str:

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

(I'm skipping this, change by Carlos)

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 :)


Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]