Re: [Tracker] Anonymous File Nodes branch review


On miÃ, 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);

Hmm, we're building an uri here, I don't think we want uris with '\' if
tracker is ported (again?) to Windows :)

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

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

I guess _:foo can be renamed to something more descriptive, but that's
precisely the way to create autogenerated urns.

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 think it only changed for queries that return files (as of now at
least), I guess all queries could change to provide both urn and uri

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

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

not AFAIK, if we used a literal there such as \"%s\" we'd need these.

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

hmm? has that changed in this branch?

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;

hmm, I don't remember this changing in the branch. There should be only
changes to extractors, tracker-miner-fs and libtracker-common.

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 

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 

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

Yes it is, metadata here is now an embedded TrackerSparqlBuilder, and
the subject must be set by tracker-extract.c so it's the same than the
one used in tracker-miner-fs, so extractors now must not specify
subjects for the embedded metadata.

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.

Yeah, the LSA part could need some double checking, I don't think it's
going to provide us the sparql we actually expect.

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,

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