Closed mikehardy closed 2 years ago
@mikehardy Sorry about the trouble and thanks for the detailed report.
I'll investigate tomorrow if no one beats me to it. :)
Thanks! Honestly, I'm curious (on the theme of objective-c / swift skills) to see how this works out, I'm sure to learn something
I'm able to reproduce the 9.x behavior change with the following test:
My initial assessment is that even though this is an inadvertent API breakage, 9.x is actually fixing a bug. customMetadata is documented and implemented to be a Dictionary with String keys and String values. It's not valid to store an NSNull as a value and 8.x was only allowing it because of Objective-C's sloppy type handling.
customMetadata
should be updated just like any other Swift or Objective-C Dictionary that specifies its key and value types. Is it feasible to update the react-native bridge accordingly?
cc: @tonyjhuang for any additional thoughts about this assessment or the Android/web comments in the OP.
I can do anything reasonable in react-native-firebase, so my direct question then is:
what exactly do I need to do in order to remove a key from customMetadata? I don't have the Objective-C to turn "customMetadata should be updated just like any other Swift or Objective-C Dictionary that specifies its key and value types" into a mental algorithm and then code. Do I need treat it as an "all at once" update, and if there is a null key, fetch current metadata so I have full set, remove keys with null values, then set the whole thing? Or ? :thinking:
(with time available / in absence of implementation direction I'll play around with the idea / hypothesis of "whole update without key removes the key" by using NSMutableDictionary's remove methods, or the filter methods etc + test what happens when I send in a customMetadata object with key missing to see what it looks like when it is refetched)
Yep, make a mutableCopy of the customMetadata and use removeObjectForKey:
and then update it.
I'm more thoroughly verifying all metadata behavior now, and unfortunately it appears we were never verifying just removing regular metadata.
Setting it to null on android against storage emulator does not clear it, and I have yet to test ios behavior either v8 or v9. We have had metadata issues against the storage emulator that did not manifest in cloud storage before so I need to isolate whether it happens against cloud storage or just emulator but there's a wrinkle, besides the platform diparity where for settable metadata it's either set to null to clear (web, android), or set to empty string (ios, but what if you want the metadata to exist and just be empty? perhaps that is not valid for any of the settable ones)
I can attest that android right now does not purge entries from customMetadata that are not mentioned when you call updateMetadata, so if you have 2 keys and then send one in with null, android will return customMetadata with just the one you did not send. Stated differently: you send null to clear a key, and unsent keys are retained on android
So at minimum here for customMetadata on ios, correcting a bug with regard to sloppy nullness or not, we now have a platform difference between android and ios with regard to customMetadata key clearing, and I fear I may have to do a metadata fetch / local process / update metadata in order to coalesce the platforms.
I do understand that the use case of clearing keys on customMetadata was apparently never specified, I understand it was always either a null object or string/string map (in whatever language typing that is), but clearing via null did used to work :shrug: and I'm not sure how to clear a key idiomatically otherwise except in platform-specific ways now since android retains and we hypothesis for ios I need to clobber the whole map
I've confirmed the behavior in #9858 that customMetadata
updates to the server as a full replacement dictionary.
@vkryachko or @tonyjhuang - Any ideas about why Android is different?
I haven't probed the JS or admin SDKs, not sure which side they fall on for customMetadata (null key removes key vs have to do full update), for what it's worth. And note that iOS v8 was on the null key removes key side ;-)
Some interesting (?) results here from firebase-ios-sdk. I have focused my testing today on iOS + cloud storage (to avoid the storage emulator permutations, and not clutter it with android chatter).
Here's what I've got - a total failure to update metadata, from what I can tell. I checked against v8 here and v9, same results.
Create a text file, with no metadata set, and you get this as a result of the creation:
2022-05-30 07:46:55.517 Df testing[32055:72ea6] STORAGE returned dictionary {
bucket = "react-native-firebase-testing.appspot.com";
contentDisposition = "inline; filename*=utf-8''file1.txt";
contentEncoding = identity;
contentType = "text/plain";
generation = 1653951632228772;
md5Hash = "LwOwNje/Fik3eT91bw8Vgw==";
metageneration = 1;
name = "playground/1653914800319/list/file1.txt";
size = 6;
timeCreated = "2022-05-30T23:00:32.293Z";
updated = "2022-05-30T23:00:32.293Z";
}
Attempt to update that text file, sending in metadata that looks like this:
2022-05-30 07:47:06.151 Df testing[32055:72f09] STORAGE pre-update metadata {
cacheControl = "no-store";
contentDisposition = "Attachment; filename=example.html";
contentEncoding = gzip;
contentLanguage = es;
contentType = "image/jpeg";
customMetadata = {
hello = world;
};
}
And the update call will succeed, giving you...this ?
022-05-30 07:47:07.070 Df testing[32055:72ea6] STORAGE post-update metadata FIRIMPLStorageMetadata 0x60000165bb60: {
bucket = "react-native-firebase-testing.appspot.com";
contentDisposition = "inline; filename*=utf-8''file1.txt";
contentEncoding = identity;
contentType = "text/plain";
generation = 1653951632228772;
md5Hash = "LwOwNje/Fik3eT91bw8Vgw==";
metadata = {
hello = world;
};
metageneration = 2;
name = "playground/1653914800319/list/file1.txt";
size = 6;
timeCreated = "2022-05-30T23:00:32.293Z";
updated = "2022-05-30T23:00:44.094Z";
}
Attempt to remove the settable keys, per the android (not iOS!) docs, by sending in null (iOS says to send in '' / empty string):
2022-05-30 07:47:07.784 Df testing[32055:72f08] STORAGE pre-update metadata {
cacheControl = "<null>";
contentDisposition = "<null>";
contentEncoding = "<null>";
contentLanguage = "<null>";
contentType = "<null>";
}
and get
2022-05-30 07:47:09.042 Df testing[32055:72ea6] STORAGE post-update metadata FIRIMPLStorageMetadata 0x600001674780: {
bucket = "react-native-firebase-testing.appspot.com";
contentEncoding = identity;
generation = 1653951632228772;
md5Hash = "LwOwNje/Fik3eT91bw8Vgw==";
metadata = {
hello = world;
};
metageneration = 3;
name = "playground/1653914800319/list/file1.txt";
size = 6;
timeCreated = "2022-05-30T23:00:32.293Z";
updated = "2022-05-30T23:00:46.064Z";
}
I'm out of time for the day, but I think what this is really showing is one of:
1- my test harness is utterly broken and my API usage is incorrect / based on misunderstandings 2- the documentation is wrong and updateMetadata can't actually set metadata, even the ones that say they are settable 3- the ever-present option representing a surprising form of error I have not thought of yet
I'm not sure I can move the area of "storage.updateMetadata" forward more pushing from the react-native side with information, I think the internal test harness here could use an expansion where all of the updatable metadata bits are tested to see if you all reproduce.
I can see your swift integration test here and it looks like it should be probing these things: https://github.com/firebase/firebase-ios-sdk/blob/585b4c83dbeca3e25895ae2f0d2ed7056b3cac7b/FirebaseStorage/Tests/Integration/StorageIntegration.swift#L442
The objective c integration test does not seem to be probing things, but if it's derived from swift now, I'm not sure if that means anything.
You can see where I added the logging I included above, and how we are calling the API, in these lines in my commit here https://github.com/invertase/react-native-firebase/pull/6274/commits/8528aa382de7e696fcac28df876567fcf9b74f24#diff-a1ebeb5a270518df94ba9c1257fda0fce39ff3fb494b57224dff3399917b6f94R148-R156
@mikehardy Thanks for sharing your investigation and navigating through some confusing APIs.
We are still running the Objective C integration tests with the Swift implementation and the test to clear metadata is at https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorageInternal/Tests/Integration/FIRStorageIntegrationTests.m#L657. This test passes with both v8 and v9. It's using nil
to do the clearing. Where do you see docs that say to send in an empty string?
I'll examine those tests and see if I can push this forward today, thanks
To your question: https://firebase.google.com/docs/storage/ios/file-metadata#update_file_metadata
You can delete writable metadata properties by passing the empty string:
Thanks. My testing shows that the empty string only works for a subset of the writable metadata properties.
The tests use "nil" instead and that seems to work. I've created the internal b/234471744 to update the docs.
Excellent! We were sending NSNull
in there - as context that's just what the react-native type conversion maps javascript-layer 'null' into - when I took the results of initWithDictionary
from that and manually compared/converted NSNull
to nil
I was able to delete all the writable items except I was unable to clear customMetadata entirely.
I examine your test and note that instead of setting it to nil
, you set it to an empty dictionary:
Even if documentation is updated to indicate clearing a value is done with nil
vs empty string, custom metadata will not fit that documentation
I'm still having a lot of difficulty updating - not removing - and I'm still trying to figure out why, perhaps it is more subtle type stuff (like nil vs NSNull) - but I'm working under the assumption it is something we are doing incorrectly in react-native-firebase since the tests seem thorough at your layer
Alright, I am just stumped. I have tried setting storageMetadata.customMetadata to NSNull, nil (as documented, or to be documented), @{}
and [NSDictionary dictionary]
(both analogs to what you do in the test fixture) and I just cannot get the thing to go to null as asserted here in the test here
I just cannot see the problem. One of those moments where simply hearing that "yes indeed, I've run this test and I know those lines have executed and I know that setting customMetadata to [NSDictionary dictionary]
actually results in nil coming back" would be great so I know if I should continue trying anything/everything here or not.
NSLog of the object right before updateMetadata
API call when it is set like in your test harness:
2022-05-31 16:13:34.122 Df testing[45797:5196d] STORAGE pre-update outgoing storageMetadata FIRIMPLStorageMetadata 0x6000024f9360: {
metadata = {
};
}
And right after in completion handler:
2022-05-31 16:13:35.047 Df testing[45797:5171d] STORAGE post-update metadata FIRIMPLStorageMetadata 0x6000024f90e0: {
bucket = "react-native-firebase-testing.appspot.com";
contentEncoding = identity;
generation = 1654031604618449;
md5Hash = "LwOwNje/Fik3eT91bw8Vgw==";
metadata = {
hello = world;
};
metageneration = 3;
name = "playground/1654031558982/list/file1.txt";
size = 6;
timeCreated = "2022-05-31T21:13:24.686Z";
updated = "2022-05-31T21:13:35.002Z";
}
:weary:
The snippet in the docs is fixed in https://github.com/firebase/snippets-ios/pull/282, but looks like this issue should still remain open to address the custom metadata issue.
Confirmed locally and in #9870. For me, both
metadata.customMetadata = [NSDictionary dictionary];
metadata.customMetadata = nil;
clear the customMetadata on the server.
Thanks for that @paulb777 - no eureka moment but for me anyway, knowing someone else has seen it work gives me the will to carry on when otherwise stumped. I'll find the issue, and I bet it will be connected with why I can clear (most) data now but my updates aren't taking effect.
I think the remaining items are
the update all at once policy is a little uncomfortable for an implementer because we do not always have all of the metadata so now we need to fetch / process / remove keys as a facade for the ios platform, but it's technically possible at least
Hmm, based on testUpdateMetadata
passing with both v8 and v9, I'm not sure anything on iOS has changed other than the NSNull issue discussed above.
The docs fix should be on the way.
The docs fix was submitted yesterday (cl/452158983, for googlers).
I'm going to close, since not clear there's anything else for us to do here.
@mikehardy Thanks for the report and let us know if there's anything else.
Sorry this has hung out there - I'm still actively working on the task but you are right there may not be anything actionable for firebase-ios-sdk as it's conforming with published types now. Just firebase-android-sdk apparent behavior difference and react-native-firebase implementation shortcomings is what it looks like to me as well
It has something to do with using FIRStorageMetadata initWithDictionary
or not
If I do what you do in the integration tests and just alloc/init then carry my data in "manually" in code the metadata values are updated correctly, but if I initWithDictionary it does not. Specifically it appears the contentEncoding and contentLanguage are not set to the correct type so the whole metadata update silently fails (an issue, I think? you can try to set your update-able metadata values into an NSDictionary, do an initWithDictionary on the StorageMetadata object using that dict and see it I think)
Took a while to isolate what the difference was here! Still not sure exactly why, but that's why react-native-firebase was not successfully updating metadata and your integration test is/was.
Still working through this (finally got another time slice for it...) but that's a result worth reporting.
Thanks @mikehardy. I'll reopen to investigate.
apologies ;-)
A confounding factor: now I have a method for setting/updating metadata which is nice, but since I can't use initWithDictionary (for whatever reason) I have to manually set each element.
Subtle issue with that: StorageMetadata.md5Hash is settable on initial upload, but not on updates. So my metadata utility function needs to copy it in if it is there (in case it is the initial upload case) but it's readonly, so the compiler won't let me:
It works in initWithDictionary because you're in class and can assign to _md5Hash
Exercising an API 100% is...illuminating :-)
Another fun trick: trying to clear out metadata now that I've got update understood better. If you alloc/init a blank FIRStorageMetadata then set the 5 update-possible properties to nil, then call updateMetadata on the ref with that metadata, it won't clear them out.
What's different between your integration test and my updateMetadata implementation? You're chaining off create/update calls before nil'ing them - it's not a clean / otherwise empty metadata object.
Try doing an alloc/init of a fresh FIRStorageMetadata here instead of using the response from previous update:
I'm about done for the day but I'm going to attempt to implement the opposite - for update metadata I'm going to try calling metadataWithCompletion then do my updates in the completion handler using the returned metadata as my base instead of the empty/pure alloc/init
That was it! If on my updateMetadata API I first call fetchWithCompletion and then manually copy the 5 props in the completion handler into the obtained metadata object then pass that to update metdata you can update it and nil it out, everything works. So in order for nil-ing out those 5 properties to work it has to be based off an existing metadata for some reason, it cannot be a fresh alloc/init metadata.
✔ should return the updated metadata for a file (7119ms)
I think there is something worth investigating here as I would expect these experimentally observed things to not happen:
But it can be made to work and I've done so (at least on iOS against cloud storage), so I am not blocked on this. Just need to have the same success now against storage emulator and/or file issues there then make it work the same in android. No problem...
Thanks for the help here and elsewhere as ever @paulb777
@mikehardy Glad you got it working. 👍
I'm not sure I follow the point about initWithDictionary
? See the added test in #9926
Well :thinking: - forgot about that test, sorry. My experimental observations against v8 were definitely showing something, and it was that initWithDictionary was not working for me. I wasn't setting md5hash https://github.com/firebase/firebase-ios-sdk/pull/9926/files#diff-4e116b2f1110087a3fd42b6088bff0b29f7afe50ad5685019bec66eee6ee1626R686 - maybe that's it ?
Not supposed to send md5hash in on an update I think, so I'm not
Also, this will fail if you go against the storage emulator in my experience, for some reason, while application/octet-stream will work - do you execute these against emulator or cloud? https://github.com/firebase/firebase-ios-sdk/pull/9926/files#diff-4e116b2f1110087a3fd42b6088bff0b29f7afe50ad5685019bec66eee6ee1626R683
Yep, needed to set md5hash
for the assertMetadata validation. The contentEncoding
updates are also kind of weird, but consistent between v8 and v9. These tests run against a real project in the cloud.
One last item here before I move on with current set of workarounds for whatever is going on here - it appears that StorageMetadata name / path are conflated in firebase-ios-sdk whereas firebase-js-sdk and firebase-android-sdk
Here name == path, despite documentation indicating it will be last path component - JS and Android are correctly returning just last path component as name
So in our test harness right now I've got this cutout for the platform difference
const storageReference = firebase.storage().ref(`${PATH}/list/file1.txt`);
const metadata = await storageReference.getMetadata();
metadata.generation.should.be.a.String();
metadata.fullPath.should.equal(`${PATH}/list/file1.txt`);
if (device.getPlatform() === 'android') {
metadata.name.should.equal('file1.txt');
} else {
// FIXME on ios file comes through as fully-qualified
// TODO log issue with firebase-ios-sdk repository
metadata.name.should.equal(`${PATH}/list/file1.txt`);
}
shows up against cloud storage and storage emulator so I think it's got to be in here?
Apart from bug-spam - some gratitude! I just got react-native-firebase v15 out the door with firebase-ios-sdk v9 https://github.com/invertase/react-native-firebase/blob/main/CHANGELOG.md#1500-2022-06-20 - thanks for all the help
@mikehardy Congrats on the release of react-native-firebase v15! 🎆
I'm not able to reproduce the metadata name problem you describe. This new integration test works on a real Firebase project: https://github.com/firebase/firebase-ios-sdk/pull/9926/commits/018cc6b930f96b9b6254cf96db5d599c38accd09
Going to close now since no identified next steps, but feel free to continue the conversation here.
Step 0: Are you in the right place?
firebase
tag.[REQUIRED] Step 1: Describe your environment
CocoaPods
iOS
[REQUIRED] Step 2: Describe the problem
I'm forward-porting react-native-firebase to the v9+ SDK release here, and I've run into a problem with Storage.
Specifically, we attempt to mirror the firebase-js-sdk API surface area to react-native library consumers (for interoperability with web code) but we call down to underlying firebase-ios-sdk and firebase-android-sdk
firebase-js-sdk documents that to remove a customMetadata key/value pair on a StorageReference, you send in a null value to the updateMetadata call.
This worked fine with firebase-ios-sdk v8 and lower, but now it appears there are some Swift-y types / casts happening, and nulls will crash an app
I will attempt to highlight the important frames 7-16 from the boilerplate with a lot of newlines:
Steps to reproduce:
1- create a StorageReference with customMetadata =
{ removeMe: 'please'
} 2- using that reference callupdateMetadata({ removeMe: null })
3- now the reference should have customMetadata withremoveMe
== undefined (that is, it should be been removed)Relevant Code:
Here is our test that used to run okay on iOS (it had a failure on android we're still pursuing, but iOS was working)
https://github.com/invertase/react-native-firebase/blob/52715959a375e8babd129baec38356b78ce80ab6/packages/storage/e2e/StorageReference.e2e.js#L403-L428
Note that the documentation for writable metadata properties indicates you may delete them passing the empty string: https://firebase.google.com/docs/storage/ios/file-metadata#update_file_metadata
On Android (and web) writable metadata is removed by passing null https://firebase.google.com/docs/storage/android/file-metadata#update_file_metadata https://firebase.google.com/docs/storage/web/file-metadata#update_file_metadata
No mention is made on how to remove customMetadata keys.
If you attempt sending an empty string in for a key with customMetadata, that just results in the value being the empty string, the value is not removed, so there may be something to think about for API consistency.
But in the past at least, sending in a null for a customMetadata key removed it fine.
Here's the iOS code that does the update:
https://github.com/invertase/react-native-firebase/blob/52715959a375e8babd129baec38356b78ce80ab6/packages/storage/ios/RNFBStorage/RNFBStorageModule.m#L145-L156
...and the
buildMetadataFromMap
method ishttps://github.com/invertase/react-native-firebase/blob/52715959a375e8babd129baec38356b78ce80ab6/packages/storage/ios/RNFBStorage/RNFBStorageCommon.m#L395-L399
That
storageMetadata.customMetadata =
line is the crash, when theFIRStorageMetadata
is initialized with an NSDictionary that contains nullsI understand that nil NSNull null etc can be bothersome in Objective-C and for interop, and who knows what the react-native bridge is translating 'null' from javascript in to at the objective-c layer, right? so I chucked this line in to the code:
And it spits out this, which seems to indicate it is an NSNull if I understand correctly.
I tried changing the swfit StorageMetadata.swift file from this repo a bit, playing with type signatures etc but if my objective-c skills are lacking (they are) my Swift skills are almost non-existent, and making interoperable optional/nullable types is beyond me unfortunately
Help?