Bug 52231

> Storage plugins should be able to create new accounts into their storage.

That's not what the branch does...

> This
> would be done by calling CreateAccount() with
> Account.Interface.Storage.StorageProvider key in the 'properties' asv.

... what the branch provides is "Telepathy clients should be able to request that an account they create gets stored in a particular storage backend".

+gchar *
+mcp_account_storage_create_account (const McpAccountStorage *storage,
+ const gchar *manager,
+ const gchar *protocol,
+ GHashTable *params)

What do you expect this vfunc to have to do? I'm a bit concerned that none of the in-tree plugins or pseudo-plugins, not even the regression tests, implement it - which makes it rather non-obvious how an implementation would look.

Here are some possible answers:

* Suggest a unique name for the account. In this case, it should be
  called suggest_unique_name and there should be a default implementation
  which calls mcp_account_manager_get_unique_name.

* Confirm whether it considers the manager/protocol/params tuple to be
  something it can deal with, but do not do anything irrevocable yet.
  In this case, it should raise a TpError if it returns NULL, and
  be called check_account_creation or something, and it should be
  specified that after a successful call, the "list" vfunc will *not*
  include that account (unless it already existed, but perhaps that
  should be specified to be an error). Perhaps it should
  take the properties as an additional parameter. The default
  implementation should be to do nothing, successfully.

* Actually create a blank account, including parameter validation and
  perhaps some other stuff. In this case, it should raise a TpError if it
  returns NULL, it should be documented/specified that after a
  successful call, the "list" vfunc will include that account, and
  it should be documented/specified whether a call to "commit"
  is implied. The default implementation should probably do set() for
  a couple of essential keys (CM name, protocol, Enabled=FALSE?),
  and commit() if appropriate, so that it does the right thing for
  existing storage backends.

From its location in the code, I infer that it's actually more or less my second option (check_account_creation) at the moment.

Another possibility is two-phase account creation: suggest a unique name/do basic checks at the location of the current call to this vfunc, then do the real account creation later on, when construct-time properties are set.

The implementation looks reasonable but I'd like a regression test, ideally via a test plugin that does something vaguely realistic. If it's check_account_creation, it should do something like "only store HypotheticalIM accounts whose account key ends with @example.com". If it's full account creation, it should make blocking D-Bus calls to an imaginary account storage daemon for which the test can be a mock implementation.

How MC currently store new account is by creating a unique_name, then telling the storage all the key/value to save for that unique_name. Of course as none of the plugins will know that unique_name they all will return FALSE except for the default storage.

So we need a function before settings the values to tell the storage "hey, here is a new account that you should be dealing with" so that when MC set the keys on the storage, that specific plugin store those keys and return TRUE

It also let the plugin decide of the unique_name to use for that account, so for example storage "Badger" could want to prefix all account names with "badger_"

And finally, the plugin could decide based on the args that it can't deal with such account. In which case I guess raising a GError would be better indeed.

(In reply to comment #2)
> How MC currently store new account is by creating a unique_name, then telling
> the storage all the key/value to save for that unique_name. Of course as none
> of the plugins will know that unique_name they all will return FALSE except for
> the default storage.

Because the storage API is rather weird, this isn't actually enforced or anything.

I think I'd prefer to say that create_account is expected to store the connection manager name and protocol (but nothing else), and replace the calls to mcp_account_manager_get_unique_name, mcd_storage_set_string(MC_ACCOUNTS_KEY_MANAGER) and mcd_storage_set_string(MC_ACCOUNTS_KEY_PROTOCOL) in _mcd_account_manager_create_account with this pseudocode:

if (provider != NULL)
{
    McpAccountStorage *storage = find_storage_for_provider (provider);

    unique_name = mcp_account_storage_create_account (storage, manager,
        protocol, ..., error);
    if (unique_name == NULL)
    {
        raise error;
        return;
    }
}
else
{
    foreach McpAccountStorage storage, highest priority first
    {
        unique_name = mcp_account_storage_create_account (storage, manager,
            protocol, ..., error);
        if (unique_name != NULL)
        {
            if (mcd_storage_get_plugin (priv->plugin_manager) != storage)
            {
                mcp_account_storage_delete_all (storage, unique_name);
                raise NotAvailable, "plugin tried to use a name that already exists";
            }

            /* success */
            break;
        }
        free error;

        if (unique_name == NULL)
            raise NotAvailable, "No backend was willing to create that account";
    }
}

where the default implementation of mcp_account_storage_create_account (inherited by all out-of-tree plugins and by the default plugin) is this (pseudocode):

gboolean create_account (..., error)
{
    unique_name = mcp_account_manager_get_unique_name (...);

    if (unique_name == NULL)
        raise NotAvailable, "Could not construct account name";

    if (!mcp_account_storage_set (self, am, unique_name,
            MC_ACCOUNTS_KEY_MANAGER, manager) ||
        !mcp_account_storage_set (self, am, unique_name,
            MC_ACCOUNTS_KEY_PROTOCOL, protocol)
        raise NotAvailable, "Storage backend could not store new account";

    return TRUE;
}

Then we preserve current functionality for all current plugins, including out-of-tree.

> It also let the plugin decide of the unique_name to use for that account, so
> for example storage "Badger" could want to prefix all account names with
> "badger_"

According to the Tp spec, the account's unique name must be of the form manager/protocol/x for some x. Why would a backend ever want to change the value of x, given that it's opaque?

(One possible reason would be "to set x to badger_123 where 123 is its internal identifier for that account", I suppose.)

Letting the plugin decide the unique name raises the possibility of plugins that generate a unique name that collides with one used by a lower- or higher-priority plugin, if they don't use mcp_account_manager_get_unique_name (which checks for collisions automatically). In the pseudocode above, that's partly handled, but unfortunately it needs a new mcp_account_storage_delete_all method, and it still doesn't handle the case of a plugin stealing a name from a lower-priority one.

Another way to do this would be to provide a mcp_account_manager_account_exists(McpAccountManager, unique_name) method, document that create_account implementations that do not use mcp_account_manager_get_unique_name are expected to call mcp_account_manager_account_exists and not use names for which that returns TRUE, and make failure to do so into a programming error (i.e. g_critical). A badly-written plugin can crash us anyway, so that's not a regression.

(In reply to comment #3)
> (In reply to comment #2)
> > How MC currently store new account is by creating a unique_name, then telling
> > the storage all the key/value to save for that unique_name. Of course as none
> > of the plugins will know that unique_name they all will return FALSE except for
> > the default storage.
> 
> Because the storage API is rather weird, this isn't actually enforced or
> anything.
> 
> I think I'd prefer to say that create_account is expected to store the
> connection manager name and protocol (but nothing else), and replace the calls
> to mcp_account_manager_get_unique_name,
> mcd_storage_set_string(MC_ACCOUNTS_KEY_MANAGER) and
> mcd_storage_set_string(MC_ACCOUNTS_KEY_PROTOCOL) in
> _mcd_account_manager_create_account with this pseudocode:
> 
> if (provider != NULL)
> {
>     McpAccountStorage *storage = find_storage_for_provider (provider);
> 
>     unique_name = mcp_account_storage_create_account (storage, manager,
>         protocol, ..., error);
>     if (unique_name == NULL)
>     {
>         raise error;
>         return;
>     }
> }
> else
> {
>     foreach McpAccountStorage storage, highest priority first
>     {
>         unique_name = mcp_account_storage_create_account (storage, manager,
>             protocol, ..., error);
>         if (unique_name != NULL)
>         {
>             if (mcd_storage_get_plugin (priv->plugin_manager) != storage)
>             {
>                 mcp_account_storage_delete_all (storage, unique_name);
>                 raise NotAvailable, "plugin tried to use a name that already
> exists";
>             }
> 
>             /* success */
>             break;
>         }
>         free error;
> 
>         if (unique_name == NULL)
>             raise NotAvailable, "No backend was willing to create that
> account";
>     }
> }
> 
> where the default implementation of mcp_account_storage_create_account
> (inherited by all out-of-tree plugins and by the default plugin) is this
> (pseudocode):
> 
> gboolean create_account (..., error)
> {
>     unique_name = mcp_account_manager_get_unique_name (...);
> 
>     if (unique_name == NULL)
>         raise NotAvailable, "Could not construct account name";
> 
>     if (!mcp_account_storage_set (self, am, unique_name,
>             MC_ACCOUNTS_KEY_MANAGER, manager) ||
>         !mcp_account_storage_set (self, am, unique_name,
>             MC_ACCOUNTS_KEY_PROTOCOL, protocol)
>         raise NotAvailable, "Storage backend could not store new account";
> 
>     return TRUE;
> }
> 
> Then we preserve current functionality for all current plugins, including
> out-of-tree.

Good idea!

> > It also let the plugin decide of the unique_name to use for that account, so
> > for example storage "Badger" could want to prefix all account names with
> > "badger_"
> 
> According to the Tp spec, the account's unique name must be of the form
> manager/protocol/x for some x. Why would a backend ever want to change the
> value of x, given that it's opaque?
> 
> (One possible reason would be "to set x to badger_123 where 123 is its internal
> identifier for that account", I suppose.)

That's exactly what GOA plugin does. Also having manager/protocol/goa_foo makes debugging easier as you can tell from the object path which are the accounts your plugin is managing :)

> Letting the plugin decide the unique name raises the possibility of plugins
> that generate a unique name that collides with one used by a lower- or
> higher-priority plugin, if they don't use mcp_account_manager_get_unique_name
> (which checks for collisions automatically). In the pseudocode above, that's
> partly handled, but unfortunately it needs a new mcp_account_storage_delete_all
> method, and it still doesn't handle the case of a plugin stealing a name from a
> lower-priority one.

_delete_all() already exists as _delete() the NULL key, no?

> Another way to do this would be to provide a
> mcp_account_manager_account_exists(McpAccountManager, unique_name) method,
> document that create_account implementations that do not use
> mcp_account_manager_get_unique_name are expected to call
> mcp_account_manager_account_exists and not use names for which that returns
> TRUE, and make failure to do so into a programming error (i.e. g_critical). A
> badly-written plugin can crash us anyway, so that's not a regression.

I think it is plugin's responsability to ensure uniqueness, either by using mcp_account_manager_get_unique_name() or by having its own prefix (e.g. "goa_") + an internal identifier. If the unique_name given by a plugin already exists, MC will already warn and discard that account, IIRC that's somewhere in McdAccount.

(In reply to comment #4)
> _delete_all() already exists as _delete() the NULL key, no?

Slightly unintuitive, but documented... good to know.

> I think it is plugin's responsability to ensure uniqueness, either by using
> mcp_account_manager_get_unique_name() or by having its own prefix (e.g. "goa_")
> + an internal identifier. If the unique_name given by a plugin already exists,
> MC will already warn and discard that account, IIRC that's somewhere in
> McdAccount.

If plugins are expected to avoid collisions, please document how they can do so. Identifiers of the form goa_234 are not sufficient, because an account name created by MC could conceivably collide with that (e.g. the fifth account with parameters { "account":  "goa#" } would collide with goa_234, because '#' is 0x23).

Identifiers containing a double-underscore are currently safe, so you could use goa__234. Or, implement mcp_account_manager_account_exists (you can factor it out of get_unique_name) and document that plugins that generate their own IDs are responsible for using that to avoid collisions.

Branch updated.

For the uniqueness enforcing, actually there is nothing new here: When a plugin emits "created" signal, the plugin already decide the name and nothing verify its uniqueness. So I don't think the create() method needs anything special, unless we want to fix both code paths in which case I would suggest a separate bug for handling them both together?

(In reply to comment #5)
> If plugins are expected to avoid collisions, please document how they can do
> so. Identifiers of the form goa_234 are not sufficient, because an account name
> created by MC could conceivably collide with that (e.g. the fifth account with
> parameters { "account":  "goa#" } would collide with goa_234, because '#' is
> 0x23).

Not the reason why GOA have names like "goa_<internal id>" is because we can't save settings on a GOA account. So if 2 accounts have the same manager, protocol and "account" param (that's very likely if you're using the same email address for gtalk, wlm and facebook), mcp_account_manager_get_unique_name() would append a number to uniquify them. But then when MC restart we have no way to know for sure which account had which suffix, so we could mix accounts and break stored logs for example.

libaccounts plugin is different because it can store the generated unique name, so after a restart of MC each account knows what unique name to reuse.

> Identifiers containing a double-underscore are currently safe, so you could use
> goa__234. Or, implement mcp_account_manager_account_exists (you can factor it
> out of get_unique_name) and document that plugins that generate their own IDs
> are responsible for using that to avoid collisions.

I think you're looking too far for the name collision, if the user wants to shoot himself he has easier ways than that :-) 

But indeed I didn't though about it when writing the GOA plugin, and IMO it's not worth changing names and breaking all stored logs.

> + if (iface->create == NULL)
> + {
> + g_set_error (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
> + "This storage does not implement create function");
> + return NULL;
> + }

This can break existing plugins. You can tell by the fact that you had to change the default pseudo-plugin to accommodate it...

In Comment #3 I suggested giving McpAccountStorage a default implementation of ->create (set up in the interface's base_init) which calls mcp_account_manager_get_unique_name, and tries to set the manager and protocol keys. Then you could move responsibility for setting those keys into ->create, removing it from _mcd_account_manager_create_account.

In existing plugins, this will work if and only if the "set" method works, i.e. if and only if the plugin was willing to make the account exist - that seems exactly what we want. In particular, you won't need any changes to the default pseudo-plugin any more.

Conveniently, it also means ->create will never be NULL.

> + * Create an account in long term storage.

Nothing is created in long term storage until you commit(), which the default implementation doesn't. Please document whether implementations are expected to commit changes to long-term storage (by inspection, "no").

The more I read the McpAccountStorage interface, the more I want to replace it with something nicer... but it'll have to do for now.

(In reply to comment #1)
>   it should be documented/specified [whether] after a
>   successful call, the "list" vfunc will include that account

It turns out that the "list" vfunc is documented to only be called once, during startup, so this never becomes significant. (Yay, non-state-recoverable interfaces.)

>   it should be documented/specified whether a call to "commit"
>   is implied

Still needs doing. Also, please document whether the "created" signal is emitted (by inspection: "no").

> I'd like a regression test, ideally via
> a test plugin that does something vaguely realistic.

Still true.

(In reply to comment #7)
> Not the reason why GOA have names like "goa_<internal id>" is because we can't
> save settings on a GOA account.

I think that's a GOA design flaw, but presumably one we're stuck with.

> > Identifiers containing a double-underscore are currently safe, so you could use
> > goa__234. Or, implement mcp_account_manager_account_exists (you can factor it
> > out of get_unique_name) and document that plugins that generate their own IDs
> > are responsible for using that to avoid collisions.
> 
> I think you're looking too far for the name collision, if the user wants to
> shoot himself he has easier ways than that :-) 

I suppose my point is: using prefixes for namespacing is only useful if *everyone* uses prefixes for namespacing. Right now, MC doesn't. I suppose we could make it use a prefix for new accounts in future, like mc_smcv_40example_2ecom0 for an account "smcv@example.com", if necessary...

> But indeed I didn't though about it when writing the GOA plugin, and IMO it's
> not worth changing names and breaking all stored logs.

If the GOA plugin has to remain buggy to keep stored logs valid, that might be a necessary evil, but in that case, please make sure to document a way that new plugins (that don't have the stored-logs problem) can guarantee not to collide, and use it in any new plugins (including the one in the regression test).

A prefix containing double-underscore would be sufficient.

In Telepathy 1.0, perhaps we should relax the definition of an account "unique name" (object-path tail) from "exactly 3 components" to "at least 3 components". Then MC's own storage could use gabble/jabber/smcv_40example_2ecom and the GOA plugin could use gabble/jabber/goa/_123 or something.

(In reply to comment #8)
> > + if (iface->create == NULL)
> > + {
> > + g_set_error (error, TP_ERROR, TP_ERROR_NOT_IMPLEMENTED,
> > + "This storage does not implement create function");
> > + return NULL;
> > + }
> 
> This can break existing plugins. You can tell by the fact that you had to
> change the default pseudo-plugin to accommodate it...

Why would it break existing plugins? They don't implement iface->create, so it returns NULL with an error and it will try the next one in priority order. Since the default plugin always succeed and is build-in we are sure there is at least one plugin that will return non-NULL value.

> In Comment #3 I suggested giving McpAccountStorage a default implementation of
> ->create (set up in the interface's base_init) which calls
> mcp_account_manager_get_unique_name, and tries to set the manager and protocol
> keys. Then you could move responsibility for setting those keys into ->create,
> removing it from _mcd_account_manager_create_account.

All plugins does not support creating accounts. For example GOA doesn't. So it should not have a default implementation, only the default storage pseudo-plugin can always create accounts.

> In existing plugins, this will work if and only if the "set" method works, i.e.
> if and only if the plugin was willing to make the account exist - that seems
> exactly what we want. In particular, you won't need any changes to the default
> pseudo-plugin any more.

I didn't want to set manager/protocol from within ->create() implementation because then why would implementation have to set them and not the params asv? It's a bit more complicated for plugins to handle the asv because MC has code to convert the GValue to a string. So I though it would be better to just tell that the ->create() implementation does not need to set anything but only create a unique_name and ensure that _set() will recognize that unique_name.

> Conveniently, it also means ->create will never be NULL.

Which is what I want to avoid for plugins that does not support creating new accounts like GOA.

> > + * Create an account in long term storage.
> 
> Nothing is created in long term storage until you commit(), which the default
> implementation doesn't. Please document whether implementations are expected to
> commit changes to long-term storage (by inspection, "no").

It is already documented on mcp_account_storage_create(), but mcd_storage_create_account() can be improved indeed.

> The more I read the McpAccountStorage interface, the more I want to replace it
> with something nicer... but it'll have to do for now.

+1

> (In reply to comment #1)
> >   it should be documented/specified [whether] after a
> >   successful call, the "list" vfunc will include that account
> 
> It turns out that the "list" vfunc is documented to only be called once, during
> startup, so this never becomes significant. (Yay, non-state-recoverable
> interfaces.)
> 
> >   it should be documented/specified whether a call to "commit"
> >   is implied
> 
> Still needs doing. Also, please document whether the "created" signal is
> emitted (by inspection: "no").
> 
> > I'd like a regression test, ideally via
> > a test plugin that does something vaguely realistic.
> 
> Still true.

Since it goes through the default storage to create accounts now, it's all tested already, AFAIK.

> (In reply to comment #7)
> In Telepathy 1.0, perhaps we should relax the definition of an account "unique
> name" (object-path tail) from "exactly 3 components" to "at least 3
> components". Then MC's own storage could use gabble/jabber/smcv_40example_2ecom
> and the GOA plugin could use gabble/jabber/goa/_123 or something.

+1

Created attachment 64434 [details] [review]
CreateAccount: make possible to select a desired storage

Created attachment 64435 [details] [review]
McdAccountStorage: fix altered-one signal

Comment on attachment 64435 [details] [review]
McdAccountStorage: fix altered-one signal

Review of attachment 64435 [details] [review]:
-----------------------------------------------------------------

r+ for this patch, please apply to master regardless of the result of the other patch

Comment on attachment 64434 [details] [review]
CreateAccount: make possible to select a desired storage

Review of attachment 64434 [details] [review]:
-----------------------------------------------------------------

r+ with a change to a comment.

::: mission-control-plugins/account-storage.c
@@ +332,5 @@
>    iface->get_restrictions = method;
>  }
>  
> +void
> +mcp_account_storage_iface_implement_create (

Not actually needed, but that's not a blocker.

(One day we should deprecate these in favour of writing directly into the GInterface vtable, since we have to allow that for bindability anyway - not that this API is bindable in any case.)

::: src/mcd-account-manager-default.c
@@ +529,5 @@
> +{
> +  gchar *unique_name;
> +
> +  /* See comment in plugin-account.c::_storage_create_account() for an
> +   * explaination of how this works */

s/for an explaination of how this works/before changing this implementation, it's more subtle than it looks/

(Also, that's not how you spell explanation.)

Thanks, fixed the comment and merged.