search

GitLiveApp / firebase-kotlin-sdk

A Kotlin-first SDK for Firebase
https://gitliveapp.github.io/firebase-kotlin-sdk/
Apache License 2.0
979 stars 148 forks source link

Add metadata to Firebase Storage #514

Closed deBasMan21 closed 2 weeks ago

nbransby commented 1 month ago

Thanks for this @deBasMan21.

The API design should match the Android Firebase KTX SDK (https://firebase.google.com/docs/storage/android/upload-files?hl=en&authuser=0#add_file_metadata) so code written for android would still compile for KMP, you can read about this here: https://github.com/GitLiveApp/firebase-kotlin-sdk?tab=readme-ov-file#compatibility-with-the-official-firebase-android-sdk

Also could you add some tests?

BasBuijsen commented 1 month ago

@nbransby i added the same syntax so it will compile when you do a drop in replacement.

As for tests, there are currently no tests yet for the whole storage module so i didn't add them yet. If you really require this pr to have tests, can you specify what you want to have tested? It is just an extra object that is passed to the function calls so i don't know what should be tested since there is no logic in there.

BasBuijsen commented 3 weeks ago

@nbransby have you had time to look at my previous comment?

nbransby commented 3 weeks ago

Sorry for the late reply @deBasMan21 I guess the test should just confirm the metadata you set is equal to what you recieve back when you get it

BasBuijsen commented 3 weeks ago

@nbransby Im looking at writing tests for this but in this PR there is no implementation for retrieving metadata from the client side, only for adding it when uploading a file. Ive been looking at a solution for retrieving metadata but its more complex than it seems because it returns a task on android, a completionhandler on ios and a promise on js so it will take some time for me to make it work. Its actually a completely different feature than the functionality proposed in this PR.

Would it be ok if i work on this new functionality and create a seperate PR for it when it is finished, so this PR can be merged and used? I really need this in my current project and i am building a local version of this library to give me access to this functionality now :). Would be much appreciated.

nbransby commented 3 weeks ago

The task / completionhandler / promise thing is easy to handle as we do it everywhere in the sdk but you can just write a test to add metadata and assert it doesn't throw an exception for now

BasBuijsen commented 3 weeks ago

@nbransby i added the tests you requested. I also added the putData method and getMetadata method so the tests deliver a little more value than only validating it won't crash. I think this pr is ready for review now, what do you think?

BasBuijsen commented 2 weeks ago

@nbransby could you take a look at this pr again?

BasBuijsen commented 2 weeks ago

@nbransby The android tests seem to be failing on the auth module while i didn't make any changes there. Locally all tests seem to work so i think it is a problem with running the firebase emulators in ci. Would this be blocking for this pr to merge?

nbransby commented 2 weeks ago

The android tests are flakey so we'll have to ignore them for now