Re: [Tracker] Ready for review - Was: Ontology change support upgrading nrl:maxCardinality 1 to multiple

[Martyn Russell <martyn lanedo com>]

On 26/09/14 16:09, Philip Van Hoof wrote:
> Here you go, that makes reviewing easier :-)
>
> https://git.gnome.org/browse/tracker/log/?h=maxcardinality-change-support-rebased

Thanks Philip,

Comments:

> +                       g_debug("Dealth with allowed nrl:maxCardinality change for %s's %s",
> +                               tracker_class_get_name(class),
> +                               tracker_property_get_name(property));

Is this debug statement a typo?

> @@ -2394,12 +2427,14 @@ db_get_static_data (TrackerDBInterface  *iface,
>
>                         tracker_property_set_is_new_domain_index (property, 
> tracker_ontologies_get_class_by_uri (domain_uri), FALSE);
>                         tracker_property_set_is_new (property, FALSE);
> +                       tracker_property_set_cardinality_changed (property, FALSE);
>                         tracker_property_set_transient (property, transient);
>                         tracker_property_set_uri (property, uri);
>                         tracker_property_set_id (property, id);
>                         tracker_property_set_domain (property, tracker_ontologies_get_class_by_uri 
> (domain_uri));
>                         tracker_property_set_range (property, tracker_ontologies_get_class_by_uri 
> (range_uri));
>                         tracker_property_set_multiple_values (property, multi_valued);
> +                       tracker_property_set_orig_multiple_values (property, multi_valued);
>                         tracker_property_set_indexed (property, indexed);
>                         tracker_property_set_default_value (property, default_value);
>                         tracker_property_set_force_journal (property, force_journal);

Hmm, shouldn't the _set_cardinality_changed() API not be FALSE but be 
based on orig_multiple_values != multiple_values ?

> @@ -645,10 +651,12 @@ tracker_data_ontology_load_statement (const gchar *ontology_path,
>                                         tracker_property_reset_domain_indexes (property);
>                                         tracker_property_reset_super_properties (property);
>                                         tracker_property_set_indexed (property, FALSE);
> +                                       tracker_property_set_cardinality_changed (property, FALSE);
>                                         tracker_property_set_secondary_index (property, NULL);
>                                         tracker_property_set_writeback (property, FALSE);
>                                         tracker_property_set_is_inverse_functional_property (property, FALSE);
>                                         tracker_property_set_default_value (property, NULL);
> +                                       tracker_property_set_multiple_values (property, TRUE);

Just wondering why you set multiple_values to TRUE here... is that the 
default with no cardinality statement? If so, ignore me :)

> @@ -1922,7 +1955,7 @@ tracker_data_ontology_process_statement (const gchar *graph,
>         } else if (g_strcmp0 (predicate, RDFS_SUB_PROPERTY_OF) == 0          ||
>                    g_strcmp0 (predicate, RDFS_DOMAIN) == 0                   ||
>                    g_strcmp0 (predicate, RDFS_RANGE) == 0                    ||
> -                  g_strcmp0 (predicate, NRL_MAX_CARDINALITY) == 0           ||
> +                  /* g_strcmp0 (predicate, NRL_MAX_CARDINALITY) == 0        || */
>                    g_strcmp0 (predicate, TRACKER_PREFIX "indexed") == 0      ||
>                    g_strcmp0 (predicate, TRACKER_PREFIX "transient") == 0    ||
>                    g_strcmp0 (predicate, TRACKER_PREFIX "fulltextIndexed") == 0) {

Just to clarify, it looks like before it was ignoring any cardinality 
changes where by the change an update and now we don't.

> @@ -2576,7 +2612,7 @@ create_decomposed_metadata_property_table (TrackerDBInterface *iface,
>                                          service_name, field_name);
>                         }
>
> -                       if (in_change && !tracker_property_get_is_new (property)) {
> +                       if (in_change && !tracker_property_get_is_new (property) && 
> !tracker_property_get_cardinality_changed (property)) {

Presumably, the check here is because we don't support going from 
multivalued to single valued in this code block and we protect ourselves 
against that...?

I wonder if we should be using the GError for that case, because it 
shouldn't happen normally should it?

****

The rest looks fine, thanks Philip!

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell