Skip to content
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

Closed
mikehardy opened this issue May 24, 2021 · 18 comments
Closed

iOS SDK misses some metadata fields #3398

mikehardy opened this issue May 24, 2021 · 18 comments

Comments

@mikehardy
Copy link

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:

  describe('updateMetadata', function () {
    it('should return the updated metadata for a file', async function () {
      const storageReference = firebase.storage().ref(WRITE_ONLY_NAME);
      const metadata = await storageReference.updateMetadata({
        contentType: 'image/jpeg',
        cacheControl: 'true',
        contentDisposition: 'disposed',
        contentEncoding: 'encoded',
        contentLanguage: 'martian',
        customMetadata: {
          hello: 'world',
        },
      });

      metadata.customMetadata.hello.should.equal('world');
      metadata.generation.should.be.a.String();
      metadata.fullPath.should.equal(WRITE_ONLY_NAME);
      metadata.name.should.equal(WRITE_ONLY_NAME);
      metadata.size.should.be.a.Number();
      should.equal(metadata.size > 0, true);
      metadata.updated.should.be.a.String();
      metadata.timeCreated.should.be.a.String();
      metadata.contentEncoding.should.be.a.String();
      metadata.contentDisposition.should.be.a.String();
      if (device.getPlatform() === 'android') {
        // FIXME on iOS this is 'application/octet-stream'?
        metadata.contentType.should.equal('image/jpeg');
      }
      metadata.bucket.should.equal(`${firebase.app().options.projectId}.appspot.com`);
      metadata.metageneration.should.be.a.String();
      metadata.md5Hash.should.be.a.String();
      if (device.getPlatform() === 'android') {
        // FIXME on iOS this comes back null ?
        should.equal(metadata.cacheControl, 'true');
      }
      if (device.getPlatform() === 'android') {
        // FIXME on iOS this comes back null ?
        should.equal(metadata.contentLanguage, 'martian');
      }
      metadata.customMetadata.should.be.an.Object();
    });

Also this one

  describe('getMetadata', function () {
    it('should return a metadata for a file', async function () {
      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') {
        // FIXME - iOS on emulator this is fullPath not name ?
        metadata.name.should.equal('file1.txt');
      }

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)

@mikehardy
Copy link
Author

@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

@abeisgoat
Copy link
Contributor

It's definitely an issue on the emulator side, no worries, we'll get it sorted :)

@abeisgoat
Copy link
Contributor

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 POST where others PUT some provide names in the query string and URL, some only do it in the URL, etc etc. So there's lots of room, especially in the metadata generation where it's pulled from various sources, to miss bits of data. These values you're seeing are probably just the incorrect defaults hard-coded in the emulator which other SDKs successfully overwrite but in iOS it either a) doesn't supply that info and we're supposed to infer it and b) supplies it via a route we're not currently looking at.

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.

@abeisgoat abeisgoat changed the title firebase-ios-sdk + storage emulator not handling metadata thorougly iOS SDK misses some metadata fields May 25, 2021
@mikehardy
Copy link
Author

mikehardy commented May 25, 2021

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

@mikehardy
Copy link
Author

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 🙏

@Egehanozsoy
Copy link

Any updates on this? would appreciate it.

@abhis3 abhis3 self-assigned this Mar 5, 2022
@abhis3
Copy link
Contributor

abhis3 commented Mar 5, 2022

Hi there @mikehardy and @Egehanozsoy, I started taking a look into this and had a couple of questions.

For the updateMetadata operation:
I used https://github.com/StewartLynch/Storage-Emulator-Issue as a starting point to make a quick and dirty test to root cause the issue, you can find my fork and code here https://github.com/abhis3/Storage-Emulator-Issue/blob/main/Storage%20Emulator%20Issue/StorageManager.swift#L44-L84.

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.

FIRStorageMetadata 0x600003b00640: {
    bucket = "ios-sdk-issues.appspot.com";
    cacheControl = true;
    contentDisposition = disposed;
    contentEncoding = encoded;
    contentLanguage = martian;
    contentType = "image/jpeg";
    generation = 1646439523168;
    md5Hash = "9IjcTb3j0permrvdjPu++w==";
    metadata =     {
        hello = world;
    };
    metageneration = 1;
    name = "Profiles/Jr1a9dSJgOLDrvczNYIRH6vA59Xc.jpg";
    size = 30543;
    timeCreated = "2022-03-04T16:18:43.167Z";
    updated = "2022-03-04T16:18:56.481Z";
}

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:

  1. The metadata.name returning the full path as opposed to the file name
  2. The initial putData (and most likely the putFile) calls don't seem to honor the initial passed in metadata. I tried it here and the metadata wasn't set properly.

For completeness, I ran all of the above on the latest release of the Firebase emulator code and an iPhone13 on the XCode emulator.

@mikehardy
Copy link
Author

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

@abhis3
Copy link
Contributor

abhis3 commented Mar 5, 2022

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

mikehardy added a commit to invertase/react-native-firebase that referenced this issue Mar 5, 2022
firebase-tools 10.2.2 contains an emulator with slightly different behavior
See firebase/firebase-tools#3398
mikehardy added a commit to invertase/react-native-firebase that referenced this issue Mar 5, 2022
firebase-tools 10.2.2 contains an emulator with slightly different behavior
See firebase/firebase-tools#3398
mikehardy added a commit to invertase/react-native-firebase that referenced this issue Mar 5, 2022
firebase-tools 10.2.2 contains an emulator with slightly different behavior
See firebase/firebase-tools#3398
@mikehardy
Copy link
Author

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 🤷

https://github.com/invertase/react-native-firebase/blob/main/packages/storage/e2e/StorageReference.e2e.js

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

@abhis3
Copy link
Contributor

abhis3 commented Mar 10, 2022

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.

@mikehardy
Copy link
Author

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

@tonyjhuang tonyjhuang removed their assignment Mar 14, 2022
@abhis3
Copy link
Contributor

abhis3 commented Mar 24, 2022

Merged #4278

Should be in the next release

@mikehardy
Copy link
Author

I've got fresh results right with storage emulator from release v11.1.0 (current here as of this typing)
I'm down to two thing only -

  1. When you first put an object in storage on the cloud storage with no specific metadata set, StorageMetadata.cacheControl is null as expected (using firebase-android-sdk or firebase-ios-sdk, firebase-js-sdk is untested)

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

  1. When you go to update the 5 StorageMetadata properties that have setters, in order to delete them (so, setting them to nil in firebase-ios-sdk or null in firebase-android-sdk), cloud storage will delete the properties (or in case of contentEncoding set it back to 'identity') but storage emulator will not clear them out

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

@tonyjhuang
Copy link
Contributor

Filed an internal bug to track, b/241813366

@tonyjhuang
Copy link
Contributor

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 firebase-tools, feel free to reopen

mikehardy added a commit to invertase/react-native-firebase that referenced this issue Aug 22, 2022
@mikehardy
Copy link
Author

@tonyjhuang 🏆 champion! Just got back from traveling and tested the release that just came out. They all work now 😄

@mikehardy
Copy link
Author

@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:

  1. When you first put an object in storage on the cloud storage with no specific metadata set, StorageMetadata.cacheControl is null as expected (using firebase-android-sdk or firebase-ios-sdk, firebase-js-sdk is untested)

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:

  1. When you first put an object in storage on the cloud storage with no specific metadata set, StorageMetadata.customMetadata is null - on emulator, it comes back as an existing, but empty object

Should I log a new issue for this?

mikehardy added a commit to invertase/react-native-firebase that referenced this issue Aug 26, 2022
mikehardy added a commit to invertase/react-native-firebase that referenced this issue Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants