feat(storage): Add `getDownloadUrl` method to the Storage API #2036

maneesht · 2023-01-06T21:52:51Z

tonyjhuang · 2023-01-09T18:58:55Z

src/storage/storage.ts

+    if (!metadata?.metadata?.firebaseStorageDownloadTokens) {
+      throw new FirebaseError({
+        code: 'storage/no-download-token',
+        message: 'No download token available. Please create one in the Firebase Console.',


we actually do have a token API that the console uses. We would have to implement it here as well but perhaps we could point users to that method instead of the console

lepatryk

Thanks Maneesh for putting it together. I really think our customers would appreciate it.

However our download url design is a hot mess right now and I'm hoping we can clean it up in 2023. This means getDownloadUrl() in the current form will either be deprecated or at least discouraged.

Does it make sense to add it here if we are planning to deprecate it a few months later?

tonyjhuang · 2023-04-13T14:20:37Z

src/storage/cloud-extensions.ts

+  // Gets metadata from firebase backend instead of GCS
+  getFirebaseMetadata(): Promise<FirebaseMetadata> {
+    // We need to talk to the firebase storage endpoints instead of the google cloud bucket endpoint
+    const endpoint = "https://firebasestorage.googleapis.com/v0";


How can configure this for testing against our internal testing endpoints or emulator?

The expectation is that you call getStorage(app, 'myendpoint') or getStorage(undefined, 'myEndpoint')

src/storage/cloud-extensions.ts

tonyjhuang · 2023-04-14T13:48:17Z

src/storage/cloud-extensions.ts

+  constructor(
+    bucket: FirebaseStorageBucket,
+    name: string,
+    private endpoint: string,


I'm curious how other SDKs handle env overrides. Could you confirm this is commonly how this is implemented?

Firestore just uses the google cloud package and does something like this:

const settings: Settings = { host: "localhost:8080", ssl: false }; // Initialize Firestore const firestore = new Firestore(settings);

For RTDB, we ask them to override the databaseUrl:

admin.initializeApp({ credential: admin.credential.cert(serviceAccount), databaseURL: "https://movie-picker-729bb-default-rtdb.firebaseio.com", });

~~So I don't think there's a best practices way to get this done. My recommendation is to do something like:~~

getStorage({ host: 'myhosturl' });

~~So that we can allow for more options to be customized down the road. WDYT?~~

EDIT: Introducing a new ENV var seems cleaner than piping through a bunch of classes. WDYT?

Ack, I don't have a strong sense of what design is right here, env var sounds fine.

Eventually these patterns should be consolidated imo, but that can happen async.

tonyjhuang · 2023-04-14T13:49:53Z

src/storage/cloud-extensions.ts

+   * Gets metadata from firebase backend instead of GCS
+   * @returns {FirebaseMetadata}
+   */
+  getFirebaseMetadata(): Promise<FirebaseMetadata> {


should this be private?

tonyjhuang · 2023-04-14T13:51:18Z

src/storage/cloud-extensions.ts

+      });
+    }
+    const [token] = downloadTokens.split(",");
+    return `${this.endpoint}/v0/b/${this.bucket.name}/o/${encodeURIComponent(


seems like there's a bug here, getFirebaseMetadata above assumes this.endpoint has the /v0 url part.

tonyjhuang · 2023-04-14T14:00:27Z

src/storage/cloud-extensions.ts

+    this.endpoint = endpoint;
+  }
+  /**
+   * 


Did you want me to add more information about this?

tonyjhuang · 2023-04-14T14:00:36Z

src/storage/cloud-extensions.ts

+    super(bucket, name, options);
+  }
+  /**
+   * Gets metadata from firebase backend instead of GCS


typo: Firebase

tonyjhuang · 2023-04-14T14:01:42Z

src/storage/storage.ts

@@ -57,10 +60,12 @@ export class Storage {

      process.env.STORAGE_EMULATOR_HOST = `http://${process.env.FIREBASE_STORAGE_EMULATOR_HOST}`;
    }
+    this.endpoint = (userEndpoint || process.env.STORAGE_EMULATOR_HOST || 'https://firebasestorage.googleapis.com') + '/v0';


Since we already have one for the emulator host, does it make sense to use another env var for the endpoint override?

tonyjhuang

Small nits but overall lgtm

src/storage/cloud-extensions.ts

tonyjhuang · 2023-04-18T20:26:19Z

src/storage/cloud-extensions.ts

+          uri,
+        },
+        (err, body) => {
+          console.log(body);


tonyjhuang · 2023-04-18T20:27:14Z

src/storage/cloud-extensions.ts

+   * @returns {FirebaseMetadata}
+   */
+  private getFirebaseMetadata(): Promise<FirebaseMetadata> {
+    // Build any custom headers based on the defined interceptors on the parent


is this comment still accurate?

tonyjhuang · 2023-04-18T20:27:44Z

src/storage/cloud-extensions.ts

+  }
+
+  /**
+   * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available.


https://tsdoc.org/pages/tags/throws/

tonyjhuang · 2023-04-18T20:27:57Z

src/storage/cloud-extensions.ts

+
+  /**
+   * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available.
+   * @returns {Promise<string>}


Please update with a description of the result or remove as this is redundant with the method signature

tonyjhuang · 2023-04-18T20:28:46Z

src/storage/cloud-extensions.ts

+export class FirebaseStorageClient extends StorageClient {
+  /**
+   * 
+   * @param bucketName 


tonyjhuang · 2023-04-18T20:28:50Z

src/storage/cloud-extensions.ts

+ */
+export class FirebaseStorageBucket extends Bucket {
+  /**
+   * @param name


tonyjhuang

overall direction lgtm, small nits, can you go through the whole PR to make sure its up to date?

tonyjhuang · 2023-05-02T21:57:29Z

src/storage/index.ts

+}
+/**
+ * Gets metadata from Firebase backend instead of GCS
+ * @returns {FirebaseMetadata}


small nit: jsdoc clauses unnecessary unless they add context that cant be inferred from the method signature

go/java-practices/javadoc#param

tonyjhuang · 2023-05-02T22:20:43Z

src/storage/index.ts

+
+/**
+ * Gets the download URL for a given file. Will throw a `FirebaseError` if there are no download tokens available.
+ * @returns {Promise<string>}


same comment

tonyjhuang · 2023-05-02T22:21:58Z

src/storage/index.ts

+  endpoint: string,
+  file: File
+): Promise<FirebaseMetadata> {
+  // Build any custom headers based on the defined interceptors on the parent


is this comment still up to date?

tonyjhuang · 2023-05-02T22:24:48Z

etc/firebase-admin.storage.api.md

@@ -17,7 +19,8 @@ export function getStorage(app?: App): Storage;
 // @public
 export class Storage {
    get app(): App;
-    bucket(name?: string): Bucket;
+    // Warning: (ae-forgotten-export) The symbol "FirebaseStorageBucket" needs to be exported by the entry point index.d.ts


tonyjhuang · 2023-05-02T22:25:46Z

test/integration/storage.spec.ts

@@ -38,6 +37,12 @@ describe('admin.storage', () => {
    return verifyBucket(bucket, 'storage().bucket(string)')
      .should.eventually.be.fulfilled;
  });
+  it('bucket(string) creates a download token', async () => {


update tests

…d-link-storage

lahirumaramba · 2023-06-07T18:00:52Z

Added the staging label to trigger integration tests. Thanks!

okcoker · 2023-06-28T16:40:18Z

@tonyjhuang (or anyone else) Any idea when the next release will be?

MorenoMdz · 2023-07-04T15:28:13Z

@tonyjhuang (or anyone else) Any idea when the next release will be?

Bumping this. cc @maneesht

Edit:

In the meantime, I got it to work by using the code from this PR directly in my service, thanks again @maneesht !!

utils:

export interface FirebaseMetadata {
  name: string
  bucket: string
  generation: string
  metageneration: string
  contentType: string
  timeCreated: string
  updated: string
  storageClass: string
  size: string
  md5Hash: string
  contentEncoding: string
  contentDisposition: string
  crc32c: string
  etag: string
  downloadTokens?: string
}

export function getFirebaseMetadata(
  endpoint: string,
  file: File,
): Promise<FirebaseMetadata> {
  const uri = `${endpoint}/b/${file.bucket.name}/o/${encodeURIComponent(
    file.name,
  )}`

  return new Promise((resolve, reject) => {
    file.storage.makeAuthenticatedRequest(
      {
        method: 'GET',
        uri,
      },
      (err, body) => {
        if (err) {
          reject(err)
        } else {
          resolve(body)
        }
      },
    )
  })
}

export async function getDownloadUrl(file: File): Promise<string> {
  const endpoint =
    (process.env.STORAGE_EMULATOR_HOST ||
      'https://firebasestorage.googleapis.com') + '/v0'
  const { downloadTokens } = await getFirebaseMetadata(endpoint, file)
  if (!downloadTokens) {
    throw new Error(
      'No download token available. Please create one in the Firebase Console.',
    )
  }
  const [token] = downloadTokens.split(',')
  return `${endpoint}/b/${file.bucket.name}/o/${encodeURIComponent(
    file.name,
  )}?alt=media&token=${token}`
}

service

  generateDownloadUrl(filePath: string) {
    const bucket = firebaseAdmin
      .storage()
      .bucket(this.configService.get('FIREBASE_STORAGE_BUCKET'))
    const file = bucket.file(filePath)

    return getDownloadUrl(file)
  }

tonyjhuang · 2023-07-11T18:50:05Z

Hi, we expect this change to rollout this week. You'll be able to follow along with the release at https://firebase.google.com/support/release-notes/admin/node

Includes: - #2216 - #2209 - #2036 - #2197

tonyjhuang · 2023-07-12T20:18:31Z

This feature is now available in https://github.com/firebase/firebase-admin-node/releases/tag/v11.10.0 🥳

okcoker · 2023-07-18T20:37:24Z

Congrats and thanks, @maneesht!

maneesht added 2 commits January 6, 2023 13:25

Added getDownloadUrl to storage spec

3cc7ea6

Removed unnecessary var assignment

f617907

maneesht requested a review from tonyjhuang January 6, 2023 21:53

Updated api

0188a88

tonyjhuang reviewed Jan 9, 2023

View reviewed changes

lahirumaramba self-assigned this Jan 9, 2023

lahirumaramba self-requested a review January 9, 2023 19:00

lepatryk reviewed Jan 12, 2023

View reviewed changes

Created download tokens if not available

44cbde7

tonyjhuang suggested changes Apr 13, 2023

View reviewed changes

maneesht added 4 commits April 13, 2023 15:31

Addressed comments

a93c8d3

Updated to fix endpoint

925433f

Removed file

07d3fc2

Made storage modifiable

55362d5

tonyjhuang suggested changes Apr 14, 2023

View reviewed changes

willbattel mentioned this pull request Apr 16, 2023

[FR] Admin 'getDownloadURL' for Storage #1352

Closed

maneesht added 3 commits April 18, 2023 11:37

Updated to stop passing endpoint info around

5ff2ec0

Removed only

d2cd1c0

Removed it only

8581f91

tonyjhuang reviewed Apr 18, 2023

View reviewed changes

Modified to use top-level instead of extending gcs API

784aec4

tonyjhuang reviewed May 2, 2023

View reviewed changes

maneesht added 6 commits May 3, 2023 10:15

Addressed comments

2ede36b

Updated docs

8226d52

Merge remote-tracking branch 'origin/master' into mtewani/add-downloa…

bcf7118

…d-link-storage

Updated lockfile

d32c44d

Updated storage integration test

be58f54

Updated md

a13b3e2

maneesht marked this pull request as ready for review May 8, 2023 15:22

Merge branch 'master' into mtewani/add-download-link-storage

712d95c

maneesht added 9 commits June 1, 2023 10:03

Reverted from origin

116df89

Merge remote-tracking branch 'origin/master' into mtewani/add-downloa…

21e9f04

…d-link-storage

Replaced in with of

d718edf

Updated env setting

89d1176

Fixed linting errors

8bfb21d

Fixed test

3b7e2ff

Undid fix

5e89622

Made sure tests worked

d64713b

Removed extra env var

730c6f0

lahirumaramba approved these changes Jun 7, 2023

View reviewed changes

lahirumaramba added the release:stage Stage a release candidate label Jun 7, 2023

maneesht and others added 4 commits June 7, 2023 11:02

Addressed comments

4443bf5

Deleted file ref

54f130e

Fixed integration tests

66f41b5

Merge branch 'master' into mtewani/add-download-link-storage

a883e1b

maneesht merged commit aed9148 into master Jun 7, 2023
5 checks passed

maneesht deleted the mtewani/add-download-link-storage branch June 7, 2023 23:44

lahirumaramba added a commit that referenced this pull request Jul 12, 2023

[chore] Release 11.10.0

222607e

Includes: - #2216 - #2209 - #2036 - #2197

lahirumaramba mentioned this pull request Jul 12, 2023

[chore] Release 11.10.0 #2246

Merged

lahirumaramba added a commit that referenced this pull request Jul 12, 2023

[chore] Release 11.10.0 (#2246)

d18d623

Includes: - #2216 - #2209 - #2036 - #2197

roblo1801 mentioned this pull request May 17, 2024

[Snyk] Upgrade firebase-admin from 11.6.0 to 11.11.1 roblo1801/webstoreback#2

Open

X-oss-byte mentioned this pull request Jun 11, 2024

[Snyk] Upgrade firebase-admin from 11.1.0 to 11.11.1 X-oss-byte/next.js#1224

Open

aravindvnair99 mentioned this pull request Jun 12, 2024

[Snyk] Upgrade firebase-admin from 11.3.0 to 12.1.1 aravindvnair99/Spot-the-Hole#550

Open

aravindvnair99 mentioned this pull request Jun 24, 2024

[Snyk] Upgrade firebase-admin from 11.3.0 to 12.1.1 aravindvnair99/Spot-the-Hole#556

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): Add `getDownloadUrl` method to the Storage API #2036

feat(storage): Add `getDownloadUrl` method to the Storage API #2036

feat(storage): Add getDownloadUrl method to the Storage API #2036

feat(storage): Add getDownloadUrl method to the Storage API #2036

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(storage): Add `getDownloadUrl` method to the Storage API #2036

feat(storage): Add `getDownloadUrl` method to the Storage API #2036