Arvados

This is caused by "omitempty" in arvados.Collection's json struct tag. After deleting the last file, manifest_text is empty, so manifest_text is omitted from the update request body.

        ManifestText              string     `json:"manifest_text,omitempty"`

14236-delete-last-file @ f0af3b1225e5617b0c2635ff8466d6d8b89e50ca https://ci.curoverse.com/view/Developer/job/developer-run-tests/899/

Tom Clegg wrote:

14236-delete-last-file @ f0af3b1225e5617b0c2635ff8466d6d8b89e50ca https://ci.curoverse.com/view/Developer/job/developer-run-tests/899/

Instead of removing omitempty, should we convert this into a pointer? I could easily see someone writing code intended to update another field (like name or properties) and accidentally clobbering manifest_text.

It sucks but the least bad way to interoperate Go structs with json seems to be to make everything pointers (that's how the Azure Go SDK works, for example).

IMO that pointer approach is an awkward way to coerce the omitempty tag into implementing selective updates: it adds repetitive new() and nil checks, and is susceptible to different kinds of bugs, like copying a struct and then accidentally modifying both instead of just the copy. It also doesn't go far enough to address all cases (how do I update to NULL?). I agree it is desirable to offer a selective-update mechanism that prevents callers from accidentally overwriting non-zero values with default zero values, but I'm not convinced pointers will be the answer, and I don't think we should block this bugfix while we figure it out.

(As an aside, the Azure SDK doesn't necessarily do everything wrong, but I wouldn't use it as evidence that something is a good idea.)

FWIW I did look through the code and API test logs for examples of Go-default-zero values being passed in updates but didn't find any.

Tom Clegg wrote:

IMO that pointer approach is an awkward way to coerce the omitempty tag into implementing selective updates: it adds repetitive new() and nil checks, and is susceptible to different kinds of bugs, like copying a struct and then accidentally modifying both instead of just the copy. It also doesn't go far enough to address all cases (how do I update to NULL?). I agree it is desirable to offer a selective-update mechanism that prevents callers from accidentally overwriting non-zero values with default zero values, but I'm not convinced pointers will be the answer, and I don't think we should block this bugfix while we figure it out.

I did a quick audit of the code base. Mostly we use the Go SDK for reading, not writing. I did find one place in fs_collection.go where we do a partial update, but that operation includes ManifestText, so it wouldn't be broken by this change. So although I think this has a high potential for future mistakes, I don't think it breaks any existing code right now.

(As an aside, the Azure SDK doesn't necessarily do everything wrong, but I wouldn't use it as evidence that something is a good idea.)

I use it as evidence that other people have thought about the problem and didn't come up with a more elegant solution.

FWIW I did look through the code and API test logs for examples of Go-default-zero values being passed in updates but didn't find any.

That's what "omitempty" means, right? Don't send the value if it has Go-default-zero. Since we use that everywhere, that's what we'd expect... except for those fields where the default-zero value is meaningful. Another case that comes to mind is Container Request priority, which has the same ambiguity priority to 0.

So I guess you should merge the quick fix on the expectation we're going to revisit it, we need to decide on a pattern because this is a recurring problem across the whole Go SDK.