Explore going the route of using Clients ?

@dhermes

After our last talk, there have been quite a few different ideas tossed around to make it clear and obvious which credentials and project IDs are in use during a particular API call, some of those have been....

Changing the default values globally
Making "Connections" a context manager (with connection: # do something)
Creating the concept of a client

We do (1), are talking about doing (2), while the others tend to do (3) -- and comparing the code, I think (3) is the nicest.

gcloud-node:

var gcloud = require('gcloud');
var project1_storage = gcloud.storage({projectId: 'project1', keyFilename: '/path/to/key'});
var project2_storage_auto = gcloud.storage();

# Do things with the two "clients"...
bucket1 = project1_storage.get_bucket('bucket-1')
bucket2 = project2_storage_auto.get_bucket('bucket2')

gcloud-ruby

require 'gcloud/storage'
project1_storage = Gcloud.storage "project-id-1" "/path/to/key"
project2_storage_auto = Gcloud.storage  # Magically figure out the project ID and credentials

# Do things with the two "clients"...
bucket1 = project1_storage.find_bucket "bucket-1"
bucket2 = project2_storage_auto.find_bucket "bucket-2"

gcloud-python

from gcloud import storage
from gcloud.credentials import get_for_service_account_json

# Create two different credentials.
credentials1 = get_for_service_account_json('key1.json')
credentials2 = get_for_service_account_json('key2.json')

# Create two different connections.
connection1 = storage.Connection(credentials=credentials1)
connection2 = storage.Connection(credentials=credentials2)

# Get two different buckets
bucket1 = storage.get_bucket('bucket-1', project='project1', connection=connection1)
bucket2 = storage.get_bucket('bucket-2', project='project2', connection=connection2)

gcloud-python if we followed the client pattern:

from gcloud import storage

project1_storage = storage.Client('project1', '/path/to/key')
project2_storage_auto = storage.Client()

# Do things with the two "clients"...
bucket1 = project1_storage.get_bucket('bucket-1')
bucket2 = project2_storage_auto.get_bucket('bucket-2')

Another option for gcloud-python using the client pattern:

import gcloud

project1_storage = gcloud.storage('project1', '/path/to/key')
project2_storage_auto = gcloud.storage()

# Do things with the two "clients"
bucket1 = project1_storage.get_bucket('bucket-1')
bucket2 = project2_storage_auto.get_bucket('bucket-2')

/cc @dhermes @tseaver

I honestly don't see the benefit to bundling project together with connection: it only gets passed to a couple of constructors (storage.bucket.Bucket, pubsub.topic.Topic). "Burying" it in a client object doesn't feel like a win for such limited use. The datastore.dataset.Dataset case is different, because a non-default dataset_id has to be passed in lots of places.

If we don't bundle the project, then we could just focus on adopting a common pattern for connection passing / lookup everywhere (#825 and related PRs).

Note that the work toward #825 makes #2 above (using a connection as a context manager) actually feasible.

It's probably worth it to create a list of all config that could be a candidate for a global (or membership in a long-lived object like a Dataset).

Right now we have

Datastore
Dataset ID
Connection -> Credentials
Storage
Project (only used in 2 of 34 methods, methods only required for "startup")
Bucket (not used anywhere, but could be)
Connection -> Credentials
Pubsub
Project (used in all API requests in the path, but rarely used directly in code)
Connection -> Credentials

While the back-end pubsub API has project in all requests, our wrappers only require passing the project ID when constructing a Topic and when listing topics / subscriptions for a project: the other cases all get their project ID from the relevant Topic instance.

@tseaver

@tseaver Added this to my list

Issues about the "always use a Client" notion we discussed in Monday's call:

It interferes with the "stackable" pattern we use to allow batches / transactions to proxy for their underlying connections.
In every place we just visited in the Add optional 'connection' parameter for any method that hits the wire #825 PRs, we would probably end up replacing the connection=None argument and the _require_connection() inside the method with client=None and _require_client(), all FBO developers who a) don't want to use the default project ID, and b) don't want to pass it to the Bucket / Topic constructors or the list_buckets / list_topics / list_subscriptions queries.
This line of development could logically be pushed to a point where we move all the API methods onto the Client, dropping all the intermediate classes in favor of pure imperative code.

@tseaver

@tseaver, hate to make more work for you, but is it crazy to toss in short examplar code snippets illustrating the problem ?

Some questions I'm having trouble answering based on our conversations...

Is it bad form to have objects know about their "parent" (Blob knowing about it's Bucket, which knows about it's Client; or Entity + Key knowing about their Dataset which knows about its Client) ? If so, why?

Is it bad form to make things involving the parent happen on that parent rather than the child? For example...

# Swap
topic = Topic('topic_name')
topic.create()
# for
client.create_topic('topic_name')

and then

subscription = topic.subscribe('subscription name')
# or
subscription = topic.get_subscription('subscription_name')

Is it terrible if batches only permit a single client at a time? That is...

clientA = datastore.Client()  # With automatic credentails...
clientB = datastore.Client(project_id='other-project')  # With other credentials...

entityA = ... # Get an entity from clientA
entityB = ... # Get an entity from clientB

entityA.age = 5
entityB.age = 5

with clientA.batch() as batch:
batch.put(entityA)
batch.put(entityB)  # Fails?

You mentioned that if we go the client route, the "operate on the module" idea is not a good one. I don't have a huge preference, but I'm curious why it'd be horrible to offer an import-time constructed client if they want to "just use the default everything". For example...
```
from gcloud.datastore import default_client as datastore
key = datastore.Key('Product', 123)
datastore.get([key])
```
(I have no problems with us skipping this, but I wanted to ask....)

I get that this is an enormous number of questions, so if it's going to make GitHub explode and make everything unreadable, let's pick one of them for now and go with that...

Is it bad form to have objects know about their "parent" (Blob knowing about it's Bucket, which knows about it's Client; or Entity + Key knowing about their Dataset which knows about its Client) ? If so, why?

I think there are different cases:

A Bucket instance is tied to a project (server-side), but not necessarily to a connection: e.g., one might have a connection whose credentials / scopes provided only read access to a given bucket, and another connection whose credentials / scopes had owner access. Also note that the project is only needed when instantiating a Bucket which does not yet exist server-side: existing buckets know their project, but the library user won't have to deal with it.
A Blob instance is necessarily tied to a bucket due to the server-side API. There is no conceivable access pattern which allows using a blob with a foreign bucket. In addition, nearly all the Blob methods are implemented using self.bucket.
A Key instance knows its dataset_id, but needs nothing else from a "parent" object. Again, one might be using a key across different connections in order to take advantage of different credentials / scopes. An Entity instance holds its Key, but needs nothing beyond it (no connection, for instance).
Like a bucket, a Topic instance is tied to a project at creation time. No other methods require / allow the project, however. Also like a bucket, a topic could be used across multiple connections.
As a blob is tied to its bucket, a Subscription instance is intrinsically tied to its topic, and uses it to implement its methods (mostly to compute its path).

[Can you] toss in short examplar code snippets illustrating the problem?

["Always use a client"] interferes with the "stackable" pattern we use to allow batches / transactions to proxy for their underlying connections.

At the moment, one can use a batch or a transaction as a context manager an not pass it everywhere, or call methods through it. E.g.:

    from gcloud import datastore
    with datastore.Transaction():
        keys = [entity.key for entity in some_list]
        datastore.delete(keys)

This works because the _require_connection code is willing to look up the current batch / transaction from a thread-local stack, falling back to use the global default. Calling API functions through the notional Client instance will defeat that stacking. Note that a object methods will be also lose that stacking.

In every place we just visited in the #825 PRs, we would probably end up replacing the connection=None argument and the _require_connection() inside the method with client=None and _require_client(), all FBO developers who a) don't want to use the default project ID, and b) don't want to pass it to the Bucket / Topic constructors or the list_buckets / list_topics / list_subscriptions queries.

Per #825, all of the methods / functions that trigger remote API calls now take an optional connection argument (modulo #858 which isn't yet merged). They all fall back to using either the "stack" (for batch/transaction support) or the global default. If we implement "always use a client", then we would have to undo all that work (which cleaned up the code a lot, and go back to using self.connection attributes / properties.

This line of development could logically be pushed to a point where we move all the API methods onto the Client, dropping all the intermediate classes in favor of pure imperative code.

It is a bit of a strawman, but having "always use a client" as the access pattern reduces the desirability of the various client-side objects: they become basically thin wrappers over the client.

At the moment, one can use a batch or a transaction as a context manager an not pass it everywhere, or call methods through it. E.g.:
from gcloud import datastore
with datastore.Transaction():
    keys = [entity.key for entity in some_list]
    datastore.delete(keys)
This works because the _require_connection code is willing to look up the current batch / transaction from a thread-local stack, falling back to use the global default. Calling API functions through the notional Client instance will defeat that stacking. Note that a object methods will be also lose that stacking.

I'm proposing that we make people be a bit more explicit for batches and transactions:

from gcloud import datastore
client = datastore.Client() #  This guesses the credentials and project ID magically
# or
client = datastore.Client(project='project-id', key_path='/path/to/key.json')

keys = [ .... ]

with client.Transaction() as t:
  t.delete(keys)

# or

with client.Transaction():
  client.delete(keys)

Wouldn't this make the "require connection" stuff just go away all together? We never guess, or look to the global scope, or anything like that...

Like Danny, I've run out of steam to argue.

The reason that I suggested starting a DX document is that I think it will either make the concerns I have obvious, or else make it clear that they can be addressed cleanly.

At the moment, the only way I can see to cleanly implement clients without re-welding them in to every class we just cleaned up, is to have the clients work at a layer "above" the classes we have now, with proxies for the "real" objects that pass in the connection from the client to every call that takes one. Maybe we can come up with a cleaner implementation than that, if we have a specific outline of how the client objects get used.