Re: [Tracker] Anonymous File Nodes branch review

[Philip Van Hoof <spam pvanhoof be>]

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

(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 
> remove_tag_for_urns)?
>
> -                                        "  ?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 
> returned).

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

Yes, this is possible and explained here:

http://live.gnome.org/Tracker/Documentation/SparqlFeatures


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

Cheers,


-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be