Adding Custom-Time to Firebase Storage

[nathanfallet]

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

• Read the contribution guidelines (CONTRIBUTING.md).
• If this has been discussed in an issue, make sure to link to the issue here.
  If not, go file an issue about this before creating a pull request to discuss.

Fixes #9575

Testing

• Make sure all existing tests in the repository pass after your change.
• If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

• At this time we cannot accept changes that affect the public API. If you'd like to help
  us make Firebase APIs better, please propose your change in a feature request so that we
  can discuss it together.

[nathanfallet]

Linked to https://cloud.google.com/storage/docs/metadata#custom-time

[tonyjhuang]

Thanks for the PR! LGTM, can you add a test

[nathanfallet]

@tonyjhuang It should be okay, tests are passing in Xcode

[morganchen12]

Tony/Sebastian, do we need to go through internal API review for this (also, do we need to add this to web and Android)?

[tonyjhuang]

Yes, filed b/228219437 to track the internal review. AFAICT this would be the first SDK to have support for this field, so there is some work to port it to Android & web.

[nathanfallet]

I thought it was sad that this field was not available in the sdk, and it can be useful (for example my goal is to update the custom time to allow the file to be deleted by a cloud storage rule later, and better than file age that I currently use, which is fixed for all files).
If needed, I'm ready to make a PR for Android. However for the web, I don't have enough knowledge to do it.

[paulb777]

The associated API review is available internally at go/firebase-storage-ios-custom-time

[tonyjhuang]

Thanks Nathan, just to be clear, Android and web are not blockers to getting this PR merged in. We at Firebase have an internal review process any time our public API (aka our SDK surface) is changed, so I've filed one just for the iOS sdk, which we will need to wait for approvals on before merging this PR. The turnaround is typically O(1 week) but can take longer in some cases.

That being said, if you're able to add this to our Android sdk, I'd be more than happy to review that and help get it submitted as well.

[ryanwilson]

Thanks for the PR @nathanfallet! We've done some pretty large refactoring around Storage this release - sorry about that. These files have moved to FirebaseStorageInternal. Do you mind moving these changes there? I'm happy to do so if you'd prefer.

After that, the new API will need to be added to the Swift public API: https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorage/Sources/StorageMetadata.swift

Thanks, let me know if you're able to do so or want me or someone else on the team to tackle this. Cheers!

[nathanfallet]

@ryanwilson Okay I will update my PR to match the refactoring around Storage

[nathanfallet]

@ryanwilson Same changes have been applied to the current master branch.

[ryanwilson]

LGTM! Thanks, will make sure the tests pass and merge afterwards.

[ryanwilson]

Merging now. cc @tonyjhuang

[paulb777]

The PR is breaking the updateMetadata2 test in all the integration tests (which don't run in forked PRs because of needing credentials).

It seems that the updateMetadata API loses the customTime on the round trip in the Metadata Task enqueue. It's encoded here, but is not part of the decoded NSDictionary here.

cc: @ryanwilson @tonyjhuang

[nathanfallet]

So it's a problem coming from the server API? Or it's something I forgot in my changes in the iOS SDK?

[ryanwilson]

Hmm, yeah my guess is it's not being sent by the backend. That method you pointed to @paulb777 is just a convenience method to save a few lines of code:

firebase-ios-sdk/FirebaseStorageInternal/Sources/FIRStorageUtils.m

Lines 162 to 169 in 7402150

	+ (nullable instancetype)frs_dictionaryFromJSONData:(nullable NSData *)data {
	if (!data) {
	return nil;
	}
	return [NSJSONSerialization JSONObjectWithData:data
	options:NSJSONReadingMutableContainers
	error:nil];
	}

[paulb777]

Yep, I understand it's a convenience method - it's just much easier to read dictionary's in the debugger than NSData's.

[paulb777]

Added #9652 to revert until we sort out the integration test issues and backend behavior.