Opportunistically migrate accounts from untyped to typed Parameters

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=71093
Reviewed-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
[also depend on new tp-glib for
tp_connection_manager_param_dup_variant_type -smcv]

6 files changed, 186 insertions, 19 deletions

diff --git a/configure.ac b/configure.ac
index a428ee62..5be3c80c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -156,7 +156,7 @@ PKG_CHECK_MODULES(DBUS, [dbus-1 >= 0.95, dbus-glib-1 >= 0.82])
 AC_SUBST(DBUS_CFLAGS)
 AC_SUBST(DBUS_LIBS)
 
-PKG_CHECK_MODULES([TELEPATHY], [telepathy-glib >= 0.23.0])
+PKG_CHECK_MODULES([TELEPATHY], [telepathy-glib >= 0.23.1])
 AC_DEFINE([TP_VERSION_MIN_REQUIRED], [TP_VERSION_0_24],
   [Ignore post-0.24 deprecations])
 AC_DEFINE([TP_VERSION_MAX_ALLOWED], [TP_VERSION_0_24],
diff --git a/src/mcd-account.c b/src/mcd-account.c
index 9374751f..0cfcdabe 100644
--- a/src/mcd-account.c
+++ b/src/mcd-account.c
@@ -194,6 +194,9 @@ _mcd_account_connection_context_free (McdAccountConnectionContext *c)
 static guint _mcd_account_signals[LAST_SIGNAL] = { 0 };
 static GQuark account_ready_quark = 0;
 
+static void mcd_account_changed_property (McdAccount *account,
+    const gchar *key, const GValue *value);
+
 GQuark
 mcd_account_error_quark (void)
 {
@@ -466,6 +469,7 @@ static void on_manager_ready (McdManager *manager, const GError *error,
 {
     McdAccount *account = MCD_ACCOUNT (user_data);
     GError *invalid_reason = NULL;
+    TpProtocol *protocol;
 
     if (error)
     {
@@ -481,6 +485,38 @@ static void on_manager_ready (McdManager *manager, const GError *error,
         g_clear_error (&account->priv->invalid_reason);
     }
 
+    protocol = _mcd_manager_dup_protocol (account->priv->manager,
+            account->priv->protocol_name);
+
+    if (protocol != NULL)
+    {
+        if (mcd_storage_maybe_migrate_parameters (
+                account->priv->storage,
+                account->priv->unique_name,
+                protocol))
+        {
+            GHashTable *params = _mcd_account_dup_parameters (account);
+
+            if (params != NULL)
+            {
+                GValue value = G_VALUE_INIT;
+
+                g_value_init (&value, TP_HASH_TYPE_STRING_VARIANT_MAP);
+                g_value_take_boxed (&value, params);
+                mcd_account_changed_property (account, "Parameters", &value);
+                g_value_unset (&value);
+            }
+            else
+            {
+                WARNING ("somehow managed to migrate parameters without "
+                    "being able to emit change notification");
+            }
+        }
+
+
+        g_object_unref (protocol);
+    }
+
     mcd_account_loaded (account);
 }
 
@@ -655,9 +691,6 @@ on_connection_abort (McdConnection *connection, McdAccount *account)
     _mcd_account_set_connection (account, NULL);
 }
 
-static void mcd_account_changed_property (McdAccount *account,
-    const gchar *key, const GValue *value);
-
 static void
 mcd_account_request_presence_int (McdAccount *account,
                                   TpConnectionPresenceType type,
diff --git a/src/mcd-storage.c b/src/mcd-storage.c
index 3bd21de9..68ccd431 100644
--- a/src/mcd-storage.c
+++ b/src/mcd-storage.c
@@ -2060,3 +2060,124 @@ mcd_storage_dup_typed_parameters (McdStorage *self,
 
   return params;
 }
+
+/* See whether we can migrate the parameters from being stored without
+ * their types, to being stored with their types.
+ * Commit changes and return TRUE if anything happened. */
+gboolean
+mcd_storage_maybe_migrate_parameters (McdStorage *self,
+    const gchar *account_name,
+    TpProtocol *protocol)
+{
+  McpAccountManager *api = MCP_ACCOUNT_MANAGER (self);
+  McpAccountStorage *plugin;
+  gchar **untyped_parameters = NULL;
+  gsize i;
+  gboolean ret = FALSE;
+
+  plugin = g_hash_table_lookup (self->accounts, account_name);
+  g_return_val_if_fail (plugin != NULL, FALSE);
+
+  /* If the storage backend can't store typed parameters, there's no point. */
+  if (!mcp_account_storage_has_any_flag (plugin, account_name,
+        MCP_ACCOUNT_STORAGE_FLAG_STORES_TYPES))
+    goto finally;
+
+  untyped_parameters = mcp_account_storage_list_untyped_parameters (
+      plugin, api, account_name);
+
+  /* If there's nothing to migrate, there's also no point. */
+  if (untyped_parameters == NULL || untyped_parameters[0] == NULL)
+    goto finally;
+
+  DEBUG ("trying to migrate %s", account_name);
+
+  for (i = 0; untyped_parameters[i] != NULL; i++)
+    {
+      const gchar *param_name = untyped_parameters[i];
+      const TpConnectionManagerParam *param = tp_protocol_get_param (protocol,
+          param_name);
+      GError *error = NULL;
+      GVariantType *type = NULL;
+      GVariant *value;
+      McpAccountStorageSetResult res;
+
+      if (param == NULL)
+        {
+          DEBUG ("cannot migrate parameter '%s': not supported by %s/%s",
+              param_name, tp_protocol_get_cm_name (protocol),
+              tp_protocol_get_name (protocol));
+          goto next_param;
+        }
+
+      type = tp_connection_manager_param_dup_variant_type (param);
+
+      DEBUG ("Migrating parameter '%s' of type '%.*s'",
+          param_name,
+          (gint) g_variant_type_get_string_length (type),
+          g_variant_type_peek_string (type));
+
+      value = mcp_account_storage_get_parameter (plugin, api,
+          account_name, param_name, type, NULL);
+
+      if (value == NULL)
+        {
+          DEBUG ("cannot migrate parameter '%s': %s #%d: %s",
+              param_name, g_quark_to_string (error->domain), error->code,
+              error->message);
+          g_error_free (error);
+          goto next_param;
+        }
+
+      if (!g_variant_is_of_type (value, type))
+        {
+          DEBUG ("trying to convert parameter from type '%s'",
+              g_variant_get_type_string (value));
+
+          /* consumes parameter */
+          value = tp_variant_convert (value, type);
+
+          if (value == NULL)
+            {
+              DEBUG ("could not convert parameter to desired type");
+              goto next_param;
+            }
+        }
+
+      res = mcp_account_storage_set_parameter (plugin, api,
+          account_name, param_name, value, MCP_PARAMETER_FLAG_NONE);
+
+      switch (res)
+        {
+          case MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED:
+            /* it really ought to be CHANGED, surely? */
+            DEBUG ("Tried to upgrade parameter %s but the "
+                "storage backend claims not to have changed it? "
+                "Not sure I really believe that", param_name);
+            /* fall through to the CHANGED case */
+
+          case MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED:
+            ret = TRUE;
+            break;
+
+          case MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED:
+            WARNING ("Failed to set parameter %s", param_name);
+            break;
+
+          default:
+            WARNING ("set_parameter returned invalid result code %d "
+                "for parameter %s", res, param_name);
+        }
+
+next_param:
+      if (type != NULL)
+        g_variant_type_free (type);
+    }
+
+  if (ret)
+    mcp_account_storage_commit (plugin, api, account_name);
+
+finally:
+  g_strfreev (untyped_parameters);
+  return ret;
+}
diff --git a/src/mcd-storage.h b/src/mcd-storage.h
index 16f112bd..56732f11 100644
--- a/src/mcd-storage.h
+++ b/src/mcd-storage.h
@@ -166,6 +166,10 @@ gboolean mcd_storage_init_value_for_attribute (GValue *value,
 GHashTable *mcd_storage_dup_typed_parameters (McdStorage *self,
     const gchar *account);
 
+gboolean mcd_storage_maybe_migrate_parameters (McdStorage *self,
+    const gchar *account_name,
+    TpProtocol *protocol);
+
 G_END_DECLS
 
 #endif /* MCD_STORAGE_H */
diff --git a/tests/twisted/account-storage/load-keyfiles.py b/tests/twisted/account-storage/load-keyfiles.py
index dfba53b4..b6c9d2ef 100644
--- a/tests/twisted/account-storage/load-keyfiles.py
+++ b/tests/twisted/account-storage/load-keyfiles.py
@@ -240,8 +240,22 @@ def test(q, bus, mc):
     # The masked account is still masked
     assert open(variant_file_names['masked'], 'r').read() == ''
 
-    # Teach the one that knows its CM that the 'password' is a string.
-    # This results in the higher-priority file being written out.
+    # Because the CM exists, we can work out the correct types
+    # for the 'migration' account's parameters. This triggers a commit
+    # even though nothing has conceptually changed, so we have the type
+    # for later. The file is copied, not moved, because XDG_DATA_DIRS are,
+    # conceptually, read-only.
+    assert not os.path.exists(old_key_file_name)
+    assert not os.path.exists(newer_key_file_name)
+    assert os.path.exists(low_prio_variant_file_names['migration'])
+    assert os.path.exists(variant_file_names['migration'])
+    assertEquals("'password_in_variant_file'",
+        account_store('get', 'variant-file', 'param-password',
+            account=tails['migration']))
+    assertEquals("uint32 42", account_store('get', 'variant-file',
+        'param-snakes', account=tails['migration']))
+
+    # Setting the password still does the right thing.
     account_ifaces['migration'].UpdateParameters({'password': 'hello'}, [])
     q.expect('dbus-signal',
             path=account_paths['migration'],
@@ -250,21 +264,16 @@ def test(q, bus, mc):
             predicate=(lambda e:
                 'Parameters' in e.args[0]),
             )
-    # Check the account has copied (not moved! XDG_DATA_DIRS are,
-    # conceptually, read-only) 'migration' from the old to the new name
     assert not os.path.exists(old_key_file_name)
     assert not os.path.exists(newer_key_file_name)
     assert os.path.exists(low_prio_variant_file_names['migration'])
     assert os.path.exists(variant_file_names['migration'])
-    assertEquals("'hello'", account_store('get', 'variant-file',
-        'param-password', account=tails['migration']))
-    # Parameters whose types are still unknown are copied too, but their
-    # types are still unknown
-    assertEquals("keyfile-escaped '42'", account_store('get', 'variant-file',
-        'param-snakes', account=tails['migration']))
+    assertEquals("'hello'",
+        account_store('get', 'variant-file', 'param-password',
+            account=tails['migration']))
 
     # 'absentcm' is still only in the low-priority location: we can't
-    # known the types of its parameters
+    # known the types of its parameters, so it doesn't get migrated.
     assert not os.path.exists(old_key_file_name)
     assert not os.path.exists(newer_key_file_name)
     assert os.path.exists(low_prio_variant_file_names['absentcm'])
diff --git a/tests/twisted/account-storage/storage_helper.py b/tests/twisted/account-storage/storage_helper.py
index ebbe78e6..0f67df92 100644
--- a/tests/twisted/account-storage/storage_helper.py
+++ b/tests/twisted/account-storage/storage_helper.py
@@ -141,12 +141,12 @@ AutomaticPresence=2;available;;
     assertEquals("'Second to none'",
             account_store('get', 'variant-file', 'DisplayName',
                 account=a2_tail))
-    # MC doesn't currently ensure that parameters are stored with their
-    # proper types.
-    assertEquals("keyfile-escaped 'dontdivert1@example.com'",
+    # Because the CM is installed, we can work out the right types
+    # for the parameters, too.
+    assertEquals("'dontdivert1@example.com'",
             account_store('get', 'variant-file', 'param-account',
                 account=a1_tail))
-    assertEquals("keyfile-escaped 'dontdivert2@example.com'",
+    assertEquals("'dontdivert2@example.com'",
             account_store('get', 'variant-file', 'param-account',
                 account=a2_tail))