New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iOS SDK misses some metadata fields #3398
Comments
@abeisgoat here's the new issue as requested - I wouldn't be surprised if it's true home is firebase-ios-sdk but I can't be sure of course - sorry |
It's definitely an issue on the emulator side, no worries, we'll get it sorted :) |
To add a bit of color onto this, the reason these issues are surfacing with iOS and not other platforms is because each implementation of the Storage SDK is quite different. I don't think there was great coordination in the implementation (probably done off a shared collection of overlapping legacy docs) so different SDKs provide different defaults and even go as far as to hit completely different paths, some Regardless, we'll have to do some more digging, we know iOS has had the least eyes on it, so I appreciate the detailed report. |
My pleasure - I figure I was blazing trail as an integrator as we (Invertase) were wrapping it for FlutterFire and react-native-firebase so we could release the goodies in coordination with you all for Google IO. And for that it was a smashing success :-). Lots of happy users. I noticed in our own code we have some old+busted stuff as well owing to how old the storage module is itself. One of the originals I think. These tests are part of our automated e2e harness so I can point them at anything and re-run if you want me to re-test when you think you've got rough edges smoothed |
We just had another issue logged where the difference may be iOS simulator vs iOS real device, though in my experience it was iOS Simulator using Storage emulator vs iOS Simulator using Storage cloud backend. Would love to see any attention on this though, we've had those assertions in our test harness commented out for this case for months and it causes us triage difficulties when issues pop up in the area 🙏 |
Any updates on this? would appreciate it. |
Hi there @mikehardy and @Egehanozsoy, I started taking a look into this and had a couple of questions. For the updateMetadata operation: When I ran the above with the firebase emulator I saw that the metadata is correct on an update operation and subsequent gets, I'll paste a copy of what the metadata is from the code linked above after an metadata update operation.
All of the comparisons in the subsequent lines also return true, so as of now I'm having a tough time reproducing the issue via XCode/emulator to root cause this. Are you experiencing this issue still and if so is it possible to grab some more details or an MCVE? For the other issues: However, I did manage to replicate 2 issues that you mentioned that I'll start ironing out:
For completeness, I ran all of the above on the latest release of the Firebase emulator code and an iPhone13 on the XCode emulator. |
Huh. Nice results, I haven't checked this in quite a while but I'll throw it on the queue for retest and report here. If you're keen to look before I do, our readme in our tests directory in react-native-firebase should step through execution if the tests but if course a react-native development environment setup might be a step to far for most, understably |
Thanks for the info! I'll keep a note of that and see if I get some time here soon to dig into it, in the meanwhile I'll start working on the fixes for the 2 issues I outlined above |
firebase-tools 10.2.2 contains an emulator with slightly different behavior See firebase/firebase-tools#3398
firebase-tools 10.2.2 contains an emulator with slightly different behavior See firebase/firebase-tools#3398
firebase-tools 10.2.2 contains an emulator with slightly different behavior See firebase/firebase-tools#3398
All of the FIXMEs in this file are still needed for the reasons described in a comment just above each FIXME, as far as I can tell each is still a problem, and there was actually a regression on android 🤷 Every FIXME in that file relates to a failure on one platform (or both) for data from storage emulator to coincide with expectations, mostly on iOS, but 2 on android now Tested just now and I made sure I was using firebase-tools 10.2.2 (current as of now) - which is good because some behavior did change from 10.2.0 to 10.2.2 |
Thanks for all of the info Mike, I've put out a PR that should hopefully go in soon and from then on out you'll be able to see it in the newer releases. I'll update this issue when that happens. TLDR there were in fact some issues with the way we set the metadata on file uploads. Regarding your other points, I took a look through the FIXMEs and wasn't able to replicate them unfortunately in my local iOS emulator setup. I also cross checked the behaviors for the FIXMEs you linked, and we're seeing the emulator is similar to how our prod services work so it's been tough to replicate exactly. See https://github.com/abhis3/Storage-Emulator-Issue/blob/main/Storage%20Emulator%20Issue/StorageManager.swift#L105 for a sample of what I was working on to sanity check. Do you think you could provide a small sample app on Android or iOS MCVE (feel free to use one of our quickstart repos such as https://github.com/firebase/quickstart-android, https://github.com/firebase/quickstart-ios, or even the repo I linked above) that reproduces the behavior you're experiencing in your current test harness. That would enable us to root out these issues that you're seeing. |
Huh - I'm no stranger to difficult to reproduce corner cases. How about we wait until the next firebase-tools release with the current set of fixes, then I'll re-test with my rig, then try to isolate one of them with one of the quickstarts? That way we're not chasing ghosts (the current fixes will be in), but we'll be focused enough on one of them that hopefully we can make some incremental progress. It is of course possible that our react-native-firebase API wrappers have some issue and it will be nice uncover it, so I'll keep pushing on it bit by bit |
Merged #4278 Should be in the next release |
I've got fresh results right with storage emulator from release v11.1.0 (current here as of this typing)
When you do the same thing against the storage emulator, the returned metadata from the upload (or if you fetch metadata) will have cacheControl as 'public, max-age=3600' - so that's a difference between cloud and emulator
So I have this in my test right now while using emulator and can only verify them when I'm using cloud storage: // Now let's make sure removing metadata works
metadata = await storageReference.updateMetadata({
contentType: null,
cacheControl: null,
contentDisposition: null,
contentEncoding: null,
contentLanguage: null,
customMetadata: null,
});
// Things that we may set (or remove)
// FIXME for storage emulator values are not cleared. Works against cloud
// TODO log issue with firebase-tools repository
// should.equal(metadata.cacheControl, undefined); // fails on android against storage emulator
// should.equal(metadata.contentDisposition, undefined); // fails on android against storage emulator
// should.equal(metadata.contentEncoding, 'identity'); // fails on android against storage emulator
// should.equal(metadata.contentLanguage, undefined); // fails on android against storage emulator
// should.equal(metadata.contentType, undefined); // fails on android against storage emulator
// should.equal(metadata.customMetadata, undefined); // fails on android against storage emulator |
Filed an internal bug to track, b/241813366 |
This should be fixed by #4889 and #4869 @mikehardy I'm going to close this issue for now. If you're still running into issues after upgrading to the latest version of |
@tonyjhuang 🏆 champion! Just got back from traveling and tested the release that just came out. They all work now 😄 |
@tonyjhuang actually - hate to be a pain on this one but there are two tiny things left that are deviations from cloud behavior, in the emulator This one from above still happens:
When you do the same thing against the storage emulator, the returned metadata from the upload (or if you fetch metadata) will have cacheControl as 'public, max-age=3600' - so that's a difference between cloud and emulator And a new one with the latest release:
Should I log a new issue for this? |
I have a little batch of ios-specific metadata issues using the storage emulator and firebase-ios-sdk (firebase-android-sdk works in these cases)
https://github.com/invertase/react-native-firebase/blob/fe3d22f54d0d706529eb5367795b568d9e783a4e/packages/storage/e2e/StorageReference.e2e.js#L373-L387
The FIXMEs here:
Also this one
If the emulator was at fault I would expect cross-platform behavior but these ones are platform-specific.
Not sure if they need to go here or in the SDK repo for ios
Originally posted by @mikehardy in #3385 (comment)
The text was updated successfully, but these errors were encountered: