setMetadata() method merges existing with new data?

[jgeewax]

Looks like if I call setMetadata on an object, it merges the data with what's in production...

var bucket = ...; // Get a bucket somehow.
var file = bucket.file('my-file');

file.setMetadata({
  metadata: {
    first: 'first'
  }
});

file.setMetadata({
  metadata: {
    second: 'second'
  }
}, function(err, metadata) {
  // metadata here is {first: 'first', second: 'second'}
  // Expected it to be just {second: 'second'} ....
});

I suspect that we can't .. fix this all that much... but maybe we can at least document it ?

---

The options we have so far:

• Make metadata immutable: remove setMetadata
• Operate on a single attribute at a time: getMetadata(key) and setMetadata(key, value)
• Rename setMetadata to patchMetadata or mergeMetadata
• Add some documentation about the behavior of setMetadata

/cc @Capstan @aozarov

[ryanseys]

Remember #459? Yeah we switched to PATCH because using PUT destroyed ACLs with BigQuery, making our datasets inaccessible. I'm not sure if this is also the case with Storage but I feel it would be appropriate to remain consistent across our APIs in this library.

[ryanseys]

We can still document it for our users. If they want to delete an existing value, I believe the correct way is to set it to null.

[aozarov]

Yes, but I doubt if doing it (delete and set) should be done by the library as it can fail in the middle and leave it in an undesired state.

BTW, nulls may be a separate issue which I mentioned to @jgeewax.
In Java nulls are considered as "ignore" in patch operation and instead a special "null" value should be used (e.g. http://javadoc.google-http-java-client.googlecode.com/hg/latest/com/google/api/client/util/Data.html#NULL_BIG_DECIMAL) [which clearly should be done transparently by the library]. Not sure if the same problem applies to node.js apiary client.

[ryanseys]

I doubt if doing it (delete and set) should be done by the library as it can fail in the middle and leave it in an undesired state.

The API call operation is atomic so either the whole thing succeeds or fails.

[ryanseys]

I think I see what you were saying. I meant that using PATCH you can delete existing values by setting them to null. We never make 2 calls for setting metadata. That requires you to know about the ones are set already, which is a getMetadata call away. We leave this as an exercise for the user if they want to do that.

[aozarov]

What I was saying is that we should probably not do the following:

func setMetadata(blob, metadata) {
  rpc.patch(blob, {"metadata": null})
  rpc.patch(blob, {"metadata": metadata})
}

[ryanseys]

Oh we'd never do that. The end result is the same as just rpc.patch(blob, {"metadata": metadata})

I meant if you wanted to set completely new metadata, including destroying the ones currently set, you need to know the existing ones and set them to null so as to achieve the same result as using PUT.

[ryanseys]

Like this:

// metadata is currently { a: '1', b: '2', c: '3' };
rpc.patch(blob, { metadata: { a: 'x', b: null, c: null } }) // need to know b and c exist so you can set them to null
// metadata is now { a: 'x' };

This is the same as:

// metadata is currently { a: '1', b: '2', c: '3' };
rpc.put(blob, { metadata: { a: 'x' } })
// metadata is now { a: 'x' };

[aozarov]

Is it? Is rpc.put doing an update (https://cloud.google.com/storage/docs/json_api/v1/objects/update)?
If so, wouldn't it clear out the other Blob's user-given properties (e.g. contentLanguage)?

[jgeewax]

So back to the main issue:

My concern here is that set typically means "replace the value up there with the value I'm sending from down here". set to us means... "merge this value from down here into whatever data exists up there."

Can we rename anything ? or are we just OK with adding some docs that explain this? Or do we try to get Google to fix something ? What makes the most sense as a user of this library?

[aozarov]

I doubt that a function that can only append is that useful.
One options (until fixed by the backed) would be to make it a read-only property which can only be set when creating a new file. Once apiary fix the issue property can become again read-write.

[jgeewax]

As a side comment: Metadata (and other properties) on S3 Objects are immutable. To change them you must copy the object.

Another option is to make Metadata not an "object" in Javascript, but instead a method that has keys and values. So:

file.setMetadata('key', 'value', function(metadata) {});

The idea being, you can always clear out the metadata for any key you know of... (.setMetadata('key', null)) and since we don't let you do an entire dictionary, it's never a concern that it operates this way....

If we left the existing setMetadata method, it could be patchMetadata or mergeMetadata instead to clarify what exactly is happening.

Thoughts?

[ryanseys]

I'm against patchMetadata and mergeMetadata because they sound technical in nature and scary. If we change to update semantics then the user must always specify their acls and contentType in the request according to https://cloud.google.com/storage/docs/json_api/v1/objects/update

I doubt that a function that can only append is that useful.

The function is not append only, if you know the key you can set it to null and it will be deleted.

I wish that acls and contentType weren't overwritten when you set the metadata or that the raw key/value pair of custom metadata was completely separate from the "properties" of the object, but this is an issue I have with the API itself, not our library. Not sure how much easier we could make it without making the user jump through more hoops.

[jgeewax]

because they sound technical in nature and scary

Because they are -- I'm proposing we make setMetadata an option on a single key, so the catalog here looks like:

// Set a single key of metadata:
file.setMetadata('client', 1234);

// Remove a single key of metadata:
file.setMetadata('client', null);
// or
file.clearMetadata('client');
// or
file.removeMetadata('client');

// Merge in (or patch) a whole bunch of metadata:
file.patchMetadata({  // or mergeMetadata
  important: 'true',
  client: '1234'
});

// Clear all metadata
file.clearMetadata();  // I don't think we have a way to make this atomic...

The idea would be that we're discouraging people from sending in a big dictionary, because it really isn't what you expect. For all other set methods, the action is "replace". If we leave setMetadata as is, it's only "replace" if operating on a single key. It's "merge" or "patch" if operating on the whole thing.

The function is not append only, if you know the key you can set it to null and it will be deleted.

Exactly -- if you know the keys... which may be a long list...

[ryanseys]

The idea would be that we're discouraging people from sending in a big dictionary

So we're encouraging them to make many tiny requests to set all their metadata fields? That's just silly.

I'm more for calling it patchMetadata than I am for dumbing the whole thing down into setting and deleting a single key at a time. The whole idea of the library was to be more idiomatic so we're taking a step backward here by renaming it to patch. Not everyone knows PATCH semantics and we have assumed our developers don't wanna destroy their acls every time (or set them every time either). This was the best trade-off.

If anything we should rename the method and force people to update their code.

file.clearMetadata(); // I don't think we have a way to make this atomic...

Yeah we could use UPDATE semantics for that and just not set anything but I don't know why someone would ever want to do this because their acls will be destroyed, effectively locking them out of updating their file ever again.

[jgeewax]

The whole idea of the library was to be more idiomatic so we're taking a step backward here by renaming it to patch.

There is a balance we have to find. We can use words people know, and have them be lies, or we can use words they don't know and have them be the truth. Right now, setMetadata doesn't set data the way someone would expect set to work:

thing.setMetadata({first: 1});
assert(thing.getMetadata() == {first: 1});

thing.setMetadata({second: 2});
assert(thing.getMetadata() == {second: 2}); // This *fails*.

If we make it operate on single values, then it acts like I'd expect set to act:

thing.setMetadata('first', 1);
assert(thing.getMetadata('first') == 1);

thing.setMetadata('second', 2);
assert(thing.getMetadata('second') == 2); // This *passes*.

---

Regarding clearMetadata: I think that's a mistake too.

Yeah we could use UPDATE semantics for that

We couldn't though, right? If we did that it'd blast out more than just the metadata -- it'd remove everything else (ACLs, etc).

file.makePublic();
assert(/* file is public */);

file.setMetadata({first: 1});
assert(file.getMetadata() == {first: 1});

file.clearMetadata();
assert(file.getMetadata() == null /* or {} */); // This *passes*. 
assert(/* file is public */); // This *fails*, right?

[stephenplusplus]

setThing(value) definitely means "set thing to value, overwriting if necessary".

But I see setMetadata as a complex version of that, given the concept of metadata being an object of "things". So, I see setMetadata() as setMetadata({ thing: this }).

It would be like if we went the road of setMetadata('thing', 'value') and then made a batch method that grouped them together for developer convenience, we would still call it setMetadata, but it would accept an object of things... which would leave us where we are now.

If we're just trying to not confuse developers by our terminology as opposed to adding new functionality, can we each ask a non-Google dev what they think and take a vote? :)

[jgeewax]

I'm down with that. How do we find impartial people ? I'm asking around inside Google UX (who do studies and whatnot...) but I suspect you guys might have some people we could poll...

[stephenplusplus]

Sounds good, the more thoughts the better. I'm asking a couple JS friends.

[aozarov]

Did you consider my original suggestion (which apparently is consistent with S3). Only accept metadata in create request and make it a read-only attribute (until the underlying issue is fixed) or do you see it as a problem (conceptually or implementation wise)?

[jgeewax]

To be clear, that's a further option, removing setMetadata all together -- and effectively making Metadata immutable from our API.

I suspect people would be a bit miffed at that...

[jgeewax]

@stephenplusplus : Any progress on asking around? I'm still seeing how we would do broader user polling on our side.

[stephenplusplus]

I'll follow up with some folks and report back.

I was reminded that React uses setState in a similar way (3rd example from https://facebook.github.io/react/):

You can see in getInitialState, the "state" object is set to its default value with 2 properties, items and text. Later, in onChange, just the text property is updated, where in handleSubmit, both are updated.

[jgeewax]

To be more explicit: setState ... "Merges nextState with the current state."

https://facebook.github.io/react/docs/component-api.html#setstate

[jgeewax]

Sounds like the resolution to this issue, then, is to update setMetadata's documentation to say "Merge nextMetadata with the current metadata" or similar.

[ryanseys]

I like that idea :) Just clearer documentation!

[jgeewax]

It's also nice that React does it too, so we aren't just being different for no good reason....

[ryanseys]

What if React does it because someone else did it for no good reason :)

[jgeewax]

Then we're just blind followers jumping off cliffs because our friends do :(

That said, I think it's more like jumping into a puddle than off a cliff -- at worst it's slightly surprising for users. Let's make sure we have a good example of what happens when you just "set" new metadata... (that the old data doesn't go away unless you specifically tell it to).