mcp_account_storage_get: replace with get_attribute, get_parameter

The old API in which plugins poked new values into the McdStorage
was non-obvious, and also fundamentally incompatible with the idea
that each account is owned by at most one plugin: if an account
in a high-priority plugin is masked by one in a low-priority plugin,
the McdStorage can't prevent the low-priority plugin from changing
its effective attribute and parameter values.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=27727

3 files changed, 83 insertions, 312 deletions

diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
index 24e2232b..bac90ad3 100644
--- a/src/mcd-account-manager-default.c
+++ b/src/mcd-account-manager-default.c
@@ -52,6 +52,12 @@ typedef struct {
     gboolean absent;
 } McdDefaultStoredAccount;
 
+static GVariant *
+variant_ref0 (GVariant *v)
+{
+  return (v == NULL ? NULL : g_variant_ref (v));
+}
+
 static McdDefaultStoredAccount *
 lookup_stored_account (McdAccountManagerDefault *self,
     const gchar *account)
@@ -267,116 +273,59 @@ set_attribute (McpAccountStorage *self,
   return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED;
 }
 
-static gboolean
-get_parameter (const McpAccountStorage *self,
-    const McpAccountManager *am,
+static GVariant *
+get_attribute (McpAccountStorage *self,
+    McpAccountManager *am,
     const gchar *account,
-    const gchar *prefixed,
-    const gchar *parameter)
+    const gchar *attribute,
+    const GVariantType *type,
+    McpAttributeFlags *flags)
 {
   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
   McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
 
-  if (parameter != NULL)
-    {
-      gchar *v = NULL;
-      GVariant *variant = NULL;
-
-      if (sa == NULL || sa->absent)
-        return FALSE;
-
-      variant = g_hash_table_lookup (sa->parameters, parameter);
-
-      if (variant != NULL)
-        {
-          mcp_account_manager_set_parameter (am, account, parameter,
-              variant, MCP_PARAMETER_FLAG_NONE);
-          return TRUE;
-        }
+  if (flags != NULL)
+    *flags = 0;
 
-      v = g_hash_table_lookup (sa->untyped_parameters, parameter);
-
-      if (v == NULL)
-        return FALSE;
-
-      mcp_account_manager_set_value (am, account, prefixed, v);
-    }
-  else
-    {
-      g_assert_not_reached ();
-    }
+  if (sa == NULL || sa->absent)
+    return FALSE;
 
-  return TRUE;
+  /* ignore @type, we store every attribute with its type anyway; MC will
+   * coerce values to an appropriate type if needed */
+  return variant_ref0 (g_hash_table_lookup (sa->attributes, attribute));
 }
 
-static gboolean
-_get (const McpAccountStorage *self,
-    const McpAccountManager *am,
+static GVariant *
+get_parameter (McpAccountStorage *self,
+    McpAccountManager *am,
     const gchar *account,
-    const gchar *key)
+    const gchar *parameter,
+    const GVariantType *type,
+    McpParameterFlags *flags)
 {
   McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self);
   McdDefaultStoredAccount *sa = lookup_stored_account (amd, account);
+  GVariant *variant;
+  gchar *str;
+
+  if (flags != NULL)
+    *flags = 0;
 
   if (sa == NULL || sa->absent)
     return FALSE;
 
-  if (key != NULL)
-    {
-      GVariant *v = NULL;
+  variant = g_hash_table_lookup (sa->parameters, parameter);
 
-      if (g_str_has_prefix (key, "param-"))
-        {
-          return get_parameter (self, am, account, key, key + 6);
-        }
+  if (variant != NULL)
+    return g_variant_ref (variant);
 
-      v = g_hash_table_lookup (sa->attributes, key);
-
-      if (v == NULL)
-        return FALSE;
-
-      mcp_account_manager_set_attribute (am, account, key, v,
-          MCP_ATTRIBUTE_FLAG_NONE);
-    }
-  else
-    {
-      GHashTableIter iter;
-      gpointer k, v;
-
-      g_hash_table_iter_init (&iter, sa->attributes);
-
-      while (g_hash_table_iter_next (&iter, &k, &v))
-        {
-          if (v != NULL)
-            mcp_account_manager_set_attribute (am, account, k,
-                v, MCP_ATTRIBUTE_FLAG_NONE);
-        }
+  str = g_hash_table_lookup (sa->untyped_parameters, parameter);
 
-      g_hash_table_iter_init (&iter, sa->parameters);
-
-      while (g_hash_table_iter_next (&iter, &k, &v))
-        {
-          if (v != NULL)
-            mcp_account_manager_set_parameter (am, account, k, v,
-                MCP_PARAMETER_FLAG_NONE);
-        }
-
-      g_hash_table_iter_init (&iter, sa->untyped_parameters);
-
-      while (g_hash_table_iter_next (&iter, &k, &v))
-        {
-          if (v != NULL)
-            {
-              gchar *prefixed = g_strdup_printf ("param-%s",
-                  (const gchar *) k);
-
-              mcp_account_manager_set_value (am, account, prefixed, v);
-              g_free (prefixed);
-            }
-        }
-    }
+  if (str == NULL)
+    return NULL;
 
-  return TRUE;
+  return mcp_account_manager_unescape_variant_from_keyfile (am,
+      str, type, NULL);
 }
 
 static gchar *
@@ -1026,7 +975,8 @@ account_storage_iface_init (McpAccountStorageIface *iface,
   iface->desc = PLUGIN_DESCRIPTION;
   iface->priority = PLUGIN_PRIORITY;
 
-  iface->get = _get;
+  iface->get_attribute = get_attribute;
+  iface->get_parameter = get_parameter;
   iface->set_attribute = set_attribute;
   iface->set_parameter = set_parameter;
   iface->create = _create;
diff --git a/src/mcd-storage.c b/src/mcd-storage.c
index af63dfb8..dffc498f 100644
--- a/src/mcd-storage.c
+++ b/src/mcd-storage.c
@@ -61,38 +61,11 @@ G_DEFINE_TYPE_WITH_CODE (McdStorage, mcd_storage,
     G_TYPE_OBJECT,
     G_IMPLEMENT_INTERFACE (MCP_TYPE_ACCOUNT_MANAGER, plugin_iface_init))
 
-typedef struct {
-    /* owned string => GVariant
-     * e.g. { 'DisplayName': <'Frederick Bloggs'> } */
-    GHashTable *attributes;
-    /* owned string => owned GVariant
-     * e.g. { 'account': <'fred@example.com'>, 'password': <'foo'> } */
-    GHashTable *parameters;
-    /* owned string => owned string escaped as if for a keyfile
-     * e.g. { 'account': 'fred@example.com', 'password': 'foo' }
-     * keys of @parameters and @escaped_parameters are disjoint */
-    GHashTable *escaped_parameters;
-    /* reffed */
-    McpAccountStorage *plugin;
-} McdStorageAccount;
-
-static void
-mcd_storage_account_free (gpointer p)
-{
-  McdStorageAccount *sa = p;
-
-  g_hash_table_unref (sa->attributes);
-  g_hash_table_unref (sa->parameters);
-  g_hash_table_unref (sa->escaped_parameters);
-  g_object_unref (sa->plugin);
-  g_slice_free (McdStorageAccount, sa);
-}
-
 static void
 mcd_storage_init (McdStorage *self)
 {
   self->accounts = g_hash_table_new_full (g_str_hash, g_str_equal,
-      g_free, mcd_storage_account_free);
+      g_free, g_object_unref);
 }
 
 static void
@@ -199,31 +172,6 @@ mcd_keyfile_escape_variant (GVariant *variant)
   return ret;
 }
 
-static McdStorageAccount *
-lookup_account (McdStorage *self,
-    const gchar *account)
-{
-  return g_hash_table_lookup (self->accounts, account);
-}
-
-static McdStorageAccount *
-mcd_storage_account_new (McpAccountStorage *plugin,
-    const gchar *account)
-{
-  McdStorageAccount *sa;
-
-  sa = g_slice_new (McdStorageAccount);
-  sa->attributes = g_hash_table_new_full (g_str_hash, g_str_equal,
-      g_free, (GDestroyNotify) g_variant_unref);
-  sa->parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
-      g_free, (GDestroyNotify) g_variant_unref);
-  sa->escaped_parameters = g_hash_table_new_full (g_str_hash, g_str_equal,
-      g_free, g_free);
-  sa->plugin = g_object_ref (plugin);
-
-  return sa;
-}
-
 static struct {
     const gchar *type;
     const gchar *name;
@@ -335,104 +283,6 @@ mcd_storage_init_value_for_attribute (GValue *value,
   return FALSE;
 }
 
-static void
-mcpa_set_attribute (const McpAccountManager *ma,
-    const gchar *account,
-    const gchar *attribute,
-    GVariant *value,
-    McpAttributeFlags flags)
-{
-  McdStorage *self = MCD_STORAGE (ma);
-  McdStorageAccount *sa = lookup_account (self, account);
-
-  g_return_if_fail (sa != NULL);
-
-  if (value != NULL)
-    {
-      g_hash_table_insert (sa->attributes, g_strdup (attribute),
-          g_variant_ref_sink (value));
-    }
-  else
-    {
-      g_hash_table_remove (sa->attributes, attribute);
-    }
-}
-
-static void
-mcpa_set_parameter (const McpAccountManager *ma,
-    const gchar *account,
-    const gchar *parameter,
-    GVariant *value,
-    McpParameterFlags flags)
-{
-  McdStorage *self = MCD_STORAGE (ma);
-  McdStorageAccount *sa = lookup_account (self, account);
-
-  g_return_if_fail (sa != NULL);
-
-  g_hash_table_remove (sa->parameters, parameter);
-  g_hash_table_remove (sa->escaped_parameters, parameter);
-
-  if (value != NULL)
-    g_hash_table_insert (sa->parameters, g_strdup (parameter),
-        g_variant_ref_sink (value));
-}
-
-static void
-set_value (const McpAccountManager *ma,
-    const gchar *account,
-    const gchar *key,
-    const gchar *value)
-{
-  McdStorage *self = MCD_STORAGE (ma);
-  McdStorageAccount *sa = lookup_account (self, account);
-
-  g_return_if_fail (sa != NULL);
-
-  if (g_str_has_prefix (key, "param-"))
-    {
-      g_hash_table_remove (sa->parameters, key + 6);
-      g_hash_table_remove (sa->escaped_parameters, key + 6);
-
-      if (value != NULL)
-        g_hash_table_insert (sa->escaped_parameters, g_strdup (key + 6),
-            g_strdup (value));
-    }
-  else
-    {
-      if (value != NULL)
-        {
-          GValue tmp = G_VALUE_INIT;
-          GError *error = NULL;
-
-          if (!mcd_storage_init_value_for_attribute (&tmp, key, NULL))
-            {
-              g_warning ("Not sure what the type of '%s' is, assuming string",
-                  key);
-              g_value_init (&tmp, G_TYPE_STRING);
-            }
-
-          if (mcd_keyfile_unescape_value (value, &tmp, &error))
-            {
-              g_hash_table_insert (sa->attributes, g_strdup (key),
-                  g_variant_ref_sink (dbus_g_value_build_g_variant (&tmp)));
-              g_value_unset (&tmp);
-            }
-          else
-            {
-              g_warning ("Could not decode attribute '%s':'%s' from plugin: %s",
-                  key, value, error->message);
-              g_error_free (error);
-              g_hash_table_remove (sa->attributes, key);
-            }
-        }
-      else
-        {
-          g_hash_table_remove (sa->attributes, key);
-        }
-    }
-}
-
 static gchar *
 unique_name (const McpAccountManager *ma,
     const gchar *manager,
@@ -698,16 +548,13 @@ mcd_storage_dup_accounts (McdStorage *self,
 {
   GPtrArray *ret = g_ptr_array_new ();
   GHashTableIter iter;
-  gpointer k, v;
+  gpointer k;
 
   g_hash_table_iter_init (&iter, self->accounts);
 
-  while (g_hash_table_iter_next (&iter, &k, &v))
+  while (g_hash_table_iter_next (&iter, &k, NULL))
     {
-      McdStorageAccount *sa = v;
-
-      if (g_hash_table_size (sa->attributes) > 0)
-        g_ptr_array_add (ret, g_strdup (k));
+      g_ptr_array_add (ret, g_strdup (k));
     }
 
   g_ptr_array_add (ret, NULL);
@@ -731,15 +578,15 @@ McpAccountStorage *
 mcd_storage_get_plugin (McdStorage *self,
     const gchar *account)
 {
-  McdStorageAccount *sa;
+  McpAccountStorage *plugin;
 
   g_return_val_if_fail (MCD_IS_STORAGE (self), NULL);
   g_return_val_if_fail (account != NULL, NULL);
 
-  sa = lookup_account (self, account);
-  g_return_val_if_fail (account != NULL, NULL);
+  plugin = g_hash_table_lookup (self->accounts, account);
+  g_return_val_if_fail (plugin != NULL, NULL);
 
-  return sa->plugin;
+  return plugin;
 }
 
 /*
@@ -818,33 +665,38 @@ mcd_storage_get_attribute (McdStorage *self,
     GValue *value,
     GError **error)
 {
-  McdStorageAccount *sa;
+  McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
+  McpAccountStorage *plugin;
   GVariant *variant;
+  gboolean ret;
 
   g_return_val_if_fail (MCD_IS_STORAGE (self), FALSE);
   g_return_val_if_fail (account != NULL, FALSE);
   g_return_val_if_fail (attribute != NULL, FALSE);
   g_return_val_if_fail (!g_str_has_prefix (attribute, "param-"), FALSE);
 
-  sa = lookup_account (self, account);
+  plugin = g_hash_table_lookup (self->accounts, account);
 
-  if (sa == NULL)
+  if (plugin == NULL)
     {
       g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
           "Account %s does not exist", account);
       return FALSE;
     }
 
-  variant = g_hash_table_lookup (sa->attributes, attribute);
+  variant = mcp_account_storage_get_attribute (plugin, ma, account,
+      attribute, type, NULL);
 
   if (variant == NULL)
     {
       g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
-          "Setting '%s' not stored by account %s", attribute, account);
+          "Account %s has no attribute '%s'", account, attribute);
       return FALSE;
     }
 
-  return mcd_storage_coerce_variant_to_value (variant, value, error);
+  ret = mcd_storage_coerce_variant_to_value (variant, value, error);
+  g_variant_unref (variant);
+  return ret;
 }
 
 /*
@@ -863,40 +715,38 @@ mcd_storage_get_parameter (McdStorage *self,
     GValue *value,
     GError **error)
 {
-  McdStorageAccount *sa;
-  const gchar *escaped;
+  McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
+  McpAccountStorage *plugin;
   GVariant *variant;
+  gboolean ret;
 
   g_return_val_if_fail (MCD_IS_STORAGE (self), FALSE);
   g_return_val_if_fail (account != NULL, FALSE);
   g_return_val_if_fail (parameter != NULL, FALSE);
+  g_return_val_if_fail (!g_str_has_prefix (parameter, "param-"), FALSE);
 
-  sa = lookup_account (self, account);
+  plugin = g_hash_table_lookup (self->accounts, account);
 
-  if (sa == NULL)
+  if (plugin == NULL)
     {
       g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
           "Account %s does not exist", account);
       return FALSE;
     }
 
-  variant = g_hash_table_lookup (sa->parameters, parameter);
+  variant = mcp_account_storage_get_parameter (plugin, ma, account,
+      parameter, type, NULL);
 
-  if (variant != NULL)
-    return mcd_storage_coerce_variant_to_value (variant, value, error);
-
-  /* OK, we don't have it as a variant. How about the keyfile-escaped
-   * version? */
-  escaped = g_hash_table_lookup (sa->escaped_parameters, parameter);
-
-  if (escaped == NULL)
+  if (variant == NULL)
     {
       g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
-          "Parameter '%s' not stored by account %s", parameter, account);
+          "Account %s has no parameter '%s'", account, parameter);
       return FALSE;
     }
 
-  return mcd_keyfile_unescape_value (escaped, value, error);
+  ret = mcd_storage_coerce_variant_to_value (variant, value, error);
+  g_variant_unref (variant);
+  return ret;
 }
 
 /*
@@ -1508,29 +1358,23 @@ mcd_storage_set_attribute (McdStorage *self,
     const gchar *attribute,
     const GValue *value)
 {
-  McdStorageAccount *sa;
   GVariant *new_v;
   gboolean updated = FALSE;
+  McpAccountStorage *plugin;
 
   g_return_val_if_fail (MCD_IS_STORAGE (self), FALSE);
   g_return_val_if_fail (account != NULL, FALSE);
   g_return_val_if_fail (attribute != NULL, FALSE);
   g_return_val_if_fail (!g_str_has_prefix (attribute, "param-"), FALSE);
 
-  sa = lookup_account (self, account);
-  g_return_val_if_fail (sa != NULL, FALSE);
+  plugin = g_hash_table_lookup (self->accounts, account);
+  g_return_val_if_fail (plugin != NULL, FALSE);
 
   if (value != NULL)
     new_v = g_variant_ref_sink (dbus_g_value_build_g_variant (value));
   else
     new_v = NULL;
 
-  if (new_v != NULL)
-    g_hash_table_insert (sa->attributes, g_strdup (attribute),
-        g_variant_ref (new_v));
-  else
-    g_hash_table_remove (sa->attributes, attribute);
-
   updated = update_storage (self, account, FALSE, attribute, new_v);
 
   tp_clear_pointer (&new_v, g_variant_unref);
@@ -1560,29 +1404,21 @@ mcd_storage_set_parameter (McdStorage *self,
     const GValue *value)
 {
   GVariant *new_v = NULL;
-  McdStorageAccount *sa;
   gboolean updated = FALSE;
+  McpAccountStorage *plugin;
 
   g_return_val_if_fail (MCD_IS_STORAGE (self), FALSE);
   g_return_val_if_fail (account != NULL, FALSE);
   g_return_val_if_fail (parameter != NULL, FALSE);
 
-  sa = lookup_account (self, account);
-  g_return_val_if_fail (sa != NULL, FALSE);
+  plugin = g_hash_table_lookup (self->accounts, account);
+  g_return_val_if_fail (plugin != NULL, FALSE);
 
   if (value != NULL)
     {
       new_v = g_variant_ref_sink (dbus_g_value_build_g_variant (value));
     }
 
-  g_hash_table_remove (sa->escaped_parameters, parameter);
-
-  if (new_v != NULL)
-    g_hash_table_insert (sa->parameters, g_strdup (parameter),
-        g_variant_ref (new_v));
-  else
-    g_hash_table_remove (sa->parameters, parameter);
-
   updated = update_storage (self, account, TRUE, parameter, new_v);
 
   tp_clear_pointer (&new_v, g_variant_unref);
@@ -2118,9 +1954,6 @@ plugin_iface_init (McpAccountManagerIface *iface,
 {
   DEBUG ();
 
-  iface->set_value = set_value;
-  iface->set_attribute = mcpa_set_attribute;
-  iface->set_parameter = mcpa_set_parameter;
   iface->unique_name = unique_name;
   iface->identify_account_async = identify_account_async;
   iface->identify_account_finish = identify_account_finish;
@@ -2134,32 +1967,20 @@ mcd_storage_add_account_from_plugin (McdStorage *self,
     const gchar *account,
     GError **error)
 {
-  McdStorageAccount *sa = lookup_account (self, account);
+  McpAccountStorage *other = g_hash_table_lookup (self->accounts, account);
 
-  if (sa != NULL)
+  if (other != NULL)
     {
       g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
           "account %s already exists in plugin '%s', cannot create "
           "for plugin '%s'",
           account,
-          mcp_account_storage_name (sa->plugin),
+          mcp_account_storage_name (other),
           mcp_account_storage_name (plugin));
       return FALSE;
     }
 
   g_hash_table_insert (self->accounts, g_strdup (account),
-      mcd_storage_account_new (plugin, account));
-
-  if (!mcp_account_storage_get (plugin, MCP_ACCOUNT_MANAGER (self),
-      account, NULL))
-    {
-      g_set_error (error, TP_ERROR, TP_ERROR_NOT_AVAILABLE,
-          "plugin '%s' denied any knowledge of account %s",
-          mcp_account_storage_name (plugin),
-          account);
-      g_hash_table_remove (self->accounts, account);
-      return FALSE;
-    }
-
+      g_object_ref (plugin));
   return TRUE;
 }
diff --git a/src/mcd-storage.h b/src/mcd-storage.h
index be0d0aa4..d78b595ba 100644
--- a/src/mcd-storage.h
+++ b/src/mcd-storage.h
@@ -30,7 +30,7 @@ G_BEGIN_DECLS
 typedef struct {
   GObject parent;
   TpDBusDaemon *dbusd;
-  /* owned string => owned McdStorageAccount */
+  /* owned string => owned McpAccountStorage */
   GHashTable *accounts;
 } McdStorage;