googleapis / nodejs-storage

Node.js client for Google Cloud Storage: unified object storage for developers and enterprises, from live data serving to data analytics/ML to data archiving.
https://cloud.google.com/storage/
Apache License 2.0
891 stars 367 forks source link

Apparently wrong type of `CopyOptions.metadata` #2389

Closed anantakrishna closed 2 months ago

anantakrishna commented 6 months ago

It seems that the type of CopyOptions.metadata is wrong.

Prior to https://github.com/googleapis/nodejs-storage/pull/2234 (landed in f48dcd2d00081aea8990f35b68a91248f3862abe), CopyOptions.metadata was of type Metadata=any: https://github.com/googleapis/nodejs-storage/pull/2234/files#diff-bc9705d0f7a567399044dfc66ccc82d4d9aa1cff116842a0094d54e463c9ecbcL313, https://github.com/googleapis/nodejs-storage/pull/2234/files#diff-dd08f09f3839e8051415914bfa81750cc0f28ae2d9b1d6b95eb87bb0efff2577L48. This fact indicates that this property refers to the custom metadata. Moreover, cacheControl, contentType and other known metadata properties are declared directly on the CopyOptions interface.

After that PR was merged, CopyOptions.metadata got type FileMetadata, which itself contains cacheControl, contentType, as well as nested metadata, which is essentially a Record<string, string> and means “custom metadata”.

It seems to me that CopyOptions.metadata should be of type Record<string, string> directly. This is confirmed by the code below.

Steps to reproduce

file.copy(to,
  {
    contentType: 'outer', // A property from CopyOptions
    metadata: { // Typed as FileMetadata
      contentType: 'internal',
      custom1: 'outer', // Not a part of FileMetadata, but allowed by TypeScript
      metadata: { // Intended to hold custom metadata, but ignored
        custom2: 'internal'
      }
    },
  }
);

The copied file gets the following metadata:

Whereas custom2 is ignored.

image


Environment details

ddelgrosso1 commented 6 months ago

Thanks @anantakrishna we will take a look and see about getting it fixed up.

anantakrishna commented 6 months ago

Thanks. Just in case, here are related changes: https://github.com/googleapis/nodejs-storage/pull/1406, https://github.com/googleapis/nodejs-storage/pull/1426.

vishwarajanand commented 3 months ago

There are a couple of issues here:

  1. I am not sure whether CopyOptions.metadata tyope should be reverted, I raised a PR but will await @ddelgrosso1 to reassert. I do think it should be changed though for neatness.

  2. For the repro provided, I can validate that the nested metadata is indeed sent on request stream, but the backend seems to be ignoring (because it's nested?). There are a few approaches for end users here, but the library should not be doing either of these:

    • Json encode the nested metadata into first level of custom metadata.
    • Flatten all nested metadata down to first level
ddelgrosso1 commented 3 months ago
  1. We discussed offline but putting here for visibility as well. Yes, CopyOptions.metadata should be reverted. It should not be of type FileMetadata. CopyOptions was meant to represent editable metadata and I missed this during my previous changes.

  2. This point should become moot if we change it back to the correct typing.