Should Blobs and Buckets be stateless or keep a reference to a StorageService ?

[jgeewax]

Right now, it looks like our Blob and Bucket objects are stateless, in that they don't keep a reference to a StorageService object. What this means is, given we have the following:

StorageService storage = ... // Get a storage service (irrelevant how here)
Bucket bucket = storage.get("bucket-name");
Blob blob = storage.get("bucket-name", "blobname");

The only way to interact with these objects is by keeping a reference to the StorageService:

BlobReadChannel channel = storage.reader("bucket", "blob");
// or 
BlobReadChannel channel = storage.reader(blob); // I think?

There's no way to do:

BlobReadChannel channel = blob.getReadChannel(); // Or anything like this.

The purpose of this issue is to discuss whether or not these objects should hold a reference to the StorageService which would make things like blob.getReadChannel() or blob.delete() possible rather than always calling storage.getReadChannel(blob) and storage.delete(blob).

The benefits of this are friendlier-looking code (IMO at least....). The downsides are (I think) serialization becomes a bit more confusing, and what does it mean to send one "StorageService-aware" Blob into another StorageService method, ie:

StorageService storageA = ... // Authenticated with read permissions only.
StorageService storageB = ... // Authenticated with read *and* write permissions.
Blob blobA = storageA.get("bucket-name", "blob");
blobA.delete(); // This should fail: blobA is tied to storageA, which has read-only permissions.
storageB.delete(blobA); // Should this "override" the `StorageService`?

I have no idea what the right answer is, but wanted to open the floor for discussion.

/cc @aozarov @jboynes

[aozarov]

I think that this is nice but consider it in the level of a syntactic sugar. I don't think an extra reference to Storage service would be a problem.

Two more things to consider are:

1. consistency with the Datastore API
2. This would either make the blobs/bucket mutable or operation would have to return a new copy

[mziccard]

I am taking over on this.
It would be nice for functional Bucket to return functional Blob objects (in methods like get or list) rather than BucketInfo objects (so that users do not need to do further conversions as new Blob(bucket.get("someBlob"))).

This is perfectly doable for get methods. For list method instead we use the underlying storage.list wich returns a ListResult<BlobInfo>. ListResult is an iterable that handles pagination and is also serializable. ListResult therefore takes only seriablizable types. Because of that we can not instantiate ListResult<Blob>. I see two solutions here given that we want to preserve the fact that storage works with serializable types (and I think we should):

1. Define a BlobList class which wraps ListResult and is an Iterable<Blob>
2. Let bucket methods return BlobInfo rather than functional Blob

I would personally prefer solution 1.
@jgeewax @aozarov @ajkannan thoughs?

[aozarov]

I also think that Bucket should return (functional) Blob.

Another option would be remove the constrain that T needs to be Serializable but
document that the results param must be Serializable. and then pass results<Blob>
that keeps a transient reference to storageService and convert BlobInfo to Blob on the fly.

Now the question is how do we create storageService upon de-serialization of that implementation
of results. For that I think we can do the following (probably should be done as a prior PR):

I think it is reasonable to ask XXXFactory to be serialize.
with bothXXXOptions and XXXFactory serializable we can always construct the implementation
for XXX service.
We could pass both factory and option to the service impl (maybe even have part of the service contract) getFactory as we have now getOptions.

If we do so, we could technically even make the functional Blob and Bucket Serializable and the
keep the signature of ListResult as is...

What do you think?

[mziccard]

I am not sure I would want functional classes as Blob, Bucket or XXX Service to be serializable. I think having serializable BlobInfo and BucketInfo is good as the classes are semantically containers or metadata. But for XXX Service, Blob and Bucker I struggle to find reasons for making them serializable.

[aozarov]

Yes, we don't have to make or declare them as serializable, the other approach was to transform them on the fly and remove the requirement of having T serializable. Any opinion on that?

[mziccard]

(Sorry, closed by mistake)
The other approach would still require making XXXFactory serializable, right? Removing the requirement of having T serializable but keeping a serializable ListResult seems a bit... unusual :) to me.

[aozarov]

Yes, it would still need to make XXXFactory serializable. I think that is reasonable as the factory usually
embeds the logic of how to construct the service (or fetch it from some place).

It is actually very common to have a serialize container but not to force its elements compile time
signature to be serialize. Lots of examples from the JDK, e.g. ArrayList.

[mziccard]

Oh I see!
I have already seen serializable factories so that's ok. The only thing left that might be considered "strange" is having a getFactory method in the XXX Service implementation (or contract). Do you think it's ok?

[aozarov]

I don't see a problem with that, but I don't think it is a must in this case.
We can explicitly pass factory to Bucket and Blob if you prefer.

[mziccard]

No it's ok then. I prefer having a getFactory method in XXX Service rather then passing the factory to Blob and Bucket. I'll work on a PR that does this before moving back Blob and Bucket.

[mziccard]

Coming back to this after some thinking.
To avoid having to deal with a lot of Serializable but still reuse ListResult we could split ListResult into its interface (ListResult) and implementation (BaseListResult). We will still have a serializable BaseListResult class for BucketInfo and BlobInfo used by StorageImpl. At the same time we can define a new class BlobListResult, implementing ListResult on the Blob type, and used internally by Bucket. By doing this we have that both Bucket and Storage return ListResult, BaseListResult is the original ListResult class almost unchanged and we are free to make BlobListResult serializable if need be. Thoughts?

PS: sorry for being a bit annoying on this but I feel like we should close this issue for good :)

[aozarov]

@mziccard I think that is a fine plan. We may later not need BlobListResult but you should not be blocked by it.

[aozarov]

Fixed by #171