Re: [Tracker] Anonymous File Nodes branch review

[Carlos Garnacho <carlos lanedo com>]

Hi!

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

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 
> remove_tag_for_urns)?
>
> -                                        "  ?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 
> 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);

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,