GNOME Bugzilla – Bug 761168
[tracker] dash in sparql variable name not allowed, clashes with grilo metadata keys
Last modified: 2016-02-27 13:31:41 UTC
Created attachment 319811 [details] [review] replace underscore with dash if key mapping fails I noticed the current git gnome-music didn't show the release date in it's per-album view anymore. Turns out this is due to updated sparql syntax used in gnome-music + tracker (1.17?) and how this interacts with the grilo tracker plugin. Previously music queried like this : SELECT * AS creation-date This was always incorrect: creation-date is a variable and should always be prefixed by $ or ? and may not contain a dash. For some reason tracker has allowed it in the past and still does for the 'AS' statement (but seemingly not universally anymore). So the current form of the query in gnome-music is like this: YEAR(MAX(nie:contentCreated(?song))) AS ?creation_date The grilo tracker plugin tries to map creation_date to a grilo metadata-key (which are all with dashes) and fails, since the key is underscored now. Possible solutions i could think of at this time: * change (replace?) metadata keys in grilo to allow for underscores. I'm not sure how invasive this is and how much impact it would have on other grilo users. Replacing would not be backwards compatible. On the other hand the 0.3 changes are already an API break afaik. * fix the tracker plugin to match an underscored key name with the dashed metadata key. This has the downside of low discoverability for plugin users. Attached a patch that does the latter.
Created attachment 320691 [details] [review] replace underscore with dash if keymapping fails Noticed I sent a broken patch, replace.
(In reply to Marinus Schraal from comment #0) > Possible solutions i could think of at this time: > * change (replace?) metadata keys in grilo to allow for underscores. I'm not > sure how invasive this is and how much impact it would have on other grilo > users. Replacing would not be backwards compatible. On the other hand the > 0.3 changes are already an API break afaik. Yep, no good! > * fix the tracker plugin to match an underscored key name with the dashed > metadata key. This has the downside of low discoverability for plugin users. > > Attached a patch that does the latter. Yes, better.
Review of attachment 320691 [details] [review]: No commit message... we need that! btw, thanks for the patches! ::: src/tracker/grl-tracker-utils.c @@ +376,3 @@ + + g_free (key_tmp); + return ret; the key parameter is expected to be the sparql-key as I can see from grl-tracker-source-api.c at fill_grilo_media_from_sparql() I think you should check key before doing g_hash_table_lookup with [0] if it fails (GRL_METADATA_KEY_INVALID) you can try to "_" -> "-" and do it again. In case it fails, you should GRL_WARNING and return NULL. See that this fix would allow you to improve [1] as well (removing a check that you are going to do now) [0] https://developer.gnome.org/grilo/unstable/GrlRegistry.html#grl-registry-lookup-metadata-key [1] https://git.gnome.org/browse/grilo-plugins/tree/src/tracker/grl-tracker-source-api.c#n257 Another possibility is mapping both (- and _) in sparql_to_grl_mapping but I don't know if that makes sense with other keys...
Created attachment 321752 [details] [review] tracker replace underscore with dash in mapping This patch goes with the last option, add an extra mapping to the sparql_to_grl_mapping if the key contains dashes. It does make sense to do this for all those keys. Eventually the dashed key could be dropped from the mapping.
Review of attachment 321752 [details] [review]: > Subject: [PATCH] [tracker] Work around stricter sparql 1.1 syntax adherance in > recent tracker release: the dash is disallowed in sparql variables, however > grilo internally depends on dashes in metadata keys. So if a key with > underscores is encountered, replace the underscores with dashes and add it to > the sparql_to_grl_mapping. Please break the commit log. The first line is usually only a small pointer to the fix. Looks good apart from that ::: src/tracker/grl-tracker-utils.c @@ +145,3 @@ (gpointer) GRL_METADATA_KEY_GET_NAME (grl_key), assoc); + if (g_strrstr(canon_name, "_\0")) { I don't think the \0 is necessary here, is it?
Created attachment 322177 [details] [review] tracker: replace dash with underscore in mapping Updated patch, last one was actually completely bogus. Did incorporate suggestions.
Review of attachment 322177 [details] [review]: ::: src/tracker/grl-tracker-utils.c @@ +152,3 @@ + if (g_strrstr (canon_name, "_")) { + g_hash_table_insert (sparql_to_grl_mapping, + (gpointer) g_strdup(canon_name), Actually, this will leak when the hash table is destroyed.
Created attachment 322437 [details] [review] tracker: replace dash with underscore in mapping As discussed on IRC, move the canon name into the struct. It is leaking, but it already was. Moving it into the struct should make it easier to clean up eventually.
Review of attachment 322437 [details] [review]: This code is messy. I was thinking "this will not work, because canon_name holds metadata-key name" so, in the creation-data example, it should hold "creation-date". But then, I double check build_flavored_key(). It changes the variable canon_name doing the "-" to "_". So, your patch works fine. Just small suggestion below. ::: src/tracker/grl-tracker-utils.c @@ +130,3 @@ + assoc->sparql_key_name = build_flavored_key (canon_name, + sparql_key_flavor); + assoc->sparql_key_name_canon = g_strdup (canon_name); you can store canon_name instead of strdup it. @@ +159,1 @@ g_free (canon_name); And not free it here.
Yeah I considered using canon_name, but as you say it isn't quite obvious that it is getting canonicalized in build_flavored_key. I thought this was just more obvious for fresh viewers. Plus in the other case we would have to conditionally free canon_name (since we only map it when it is actually multi word), which is kind of ugly too.
git-bz push failed to updated the status here, probably due the dot in the end of commit log. Patch is pushed, reference is 6027a46574f6b3954d3bec62098555d44e9f6981 https://git.gnome.org/browse/grilo-plugins/commit/?id=6027a46574f6b3954d Thanks