Can we allow null as well as "" for contentType args

[csells]

UploadObject fails if there's a null contentType but succeeds if the contentType is "". Can we allow null and translate to "" for the user?

[jskeet]

We can if we think that's an appropriate default. We could look at making it an optional parameter in that case... I've never been sure whether "" is a good content type to specify...

[csells]

I don't think there is a good default for content type. However, in this case "" means "no content specified", so null should be allowed for that as well.

[jskeet]

I've been thinking about this a bit - I'm still somewhat uncomfortable about it, particularly as it would end up being returned as "" (I'd hope, anyway).

Given that this is a required parameter, and it's just as easy to write "" as null, I'm not sure why we'd want to conflate them. Basically, we want users to think about the right content type, and this feels like it's moving in the wrong direction.

Happy to be convinced though :)

[Capstan]

I made contentType an optional parameter for Google Cloud Storage in Q1. If you are still seeing otherwise, it's a service bug you should report.

[jskeet]

Aha. In that case we should make it optional as well, definitely. Thanks Capstan!

[jskeet]

@csells: Let's work out what we want the shape of this to look like. A few options:

1. Leave the parameter where it is, and just allow it to be null. (User still has to specify it - it's required from a C# perspective.)
2. Remove the parameter, putting it into UploadObjectOptions - one downside is that it's not relevant when uploading an existing object.
3. Switch the source and contentType parameters, and make it default to null.

Any particular preference?

[csells]

Option 1 is my preference.

[jskeet]

Well that's handy, as it should be a trivial change. Will send you a PR shortly...

[jskeet]

(Note that this PR doesn't translate null to "" as the content type as per the original comment - it just doesn't set it, which sounds like it should be fine.)

[jskeet]

Reopening this as it looks like there's other code that does require a content type.

[Capstan]

Pointers?

[jskeet]

@Capstan: I need to look into it tomorrow.

[jskeet]

Okay, found it. It's in the shared resumable upload code, which is what we use for all uploads in the wrapper API. As per https://cloud.google.com/storage/docs/json_api/v1/how-tos/resumable-upload, the X-Upload-Content-Type is required - so it's validated in ResumableUpload.cs.

The simplest fix here will be to just coalesce null to "", as per @csells original comment. Will send that out as a separate PR.

[Capstan]

Aha. That doc is incorrect and was missed when we relaxed that requirement.

[jskeet]

Ah, so it's reasonable to not specify the X-Upload-Content-Type header in general for resumable upload? If that's the case, I'll file a bug against the other github repo :)

[Capstan]

I would not go that far. There exist other uploader service customers besides GCS who may still require it. But it should not be required for all services, no.

[jskeet]

Okay - in that case, we shouldn't prohibit it in the uploader; if it is required and a client doesn't specify it, the server can fail the request - we shouldn't do so too conservatively. Have filed googleapis/google-api-dotnet-client#743

When that's fixed, we can revert some of the previous PR...

[jskeet]

Finally fixed by #176.