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

Add typing for File.metadata #1817

Closed
anantakrishna opened this issue Mar 16, 2022 · 7 comments
Closed

Add typing for File.metadata #1817

anantakrishna opened this issue Mar 16, 2022 · 7 comments
Assignees
Labels
api: storage Issues related to the googleapis/nodejs-storage API. next major: breaking change this is a change that we should wait to bundle into the next major version priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@anantakrishna
Copy link

anantakrishna commented Mar 16, 2022

Environment details

  • OS: Windows
  • Node.js version: 12.20.0
  • npm version: 6.14.8
  • @google-cloud/storage version: 5.18.2

Steps to reproduce

  1. Create instance of the File class.
  2. Access its metadata property in TypeScript.

The issue is that its type is effectivelyany, whereas the reference and example demonstrate the structure of the metadata.

Why not to add this structure to the typing?

@anantakrishna anantakrishna added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 16, 2022
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Mar 16, 2022
@BrennaEpp BrennaEpp added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 16, 2022
@shaffeeullah shaffeeullah added type: question Request for information or clarification. Not an issue. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Mar 16, 2022
@shaffeeullah
Copy link
Contributor

Hi @anantakrishna ,

This is done on purpose. We don't believe it is good practice to validate the data returned by the server. The way this is currently implemented, the data returned by the server gets propagated to you without being processed in any way. This is safer from bugs on our end because if Cloud Storage changes/adds metadata fields, consumers of this library will get those changes without the current version of the library breaking and/or having to upgrade versions.

@BrennaEpp BrennaEpp self-assigned this Mar 16, 2022
@anantakrishna
Copy link
Author

Thank you @shaffeeullah for explanation. I partly agree with this logic, however not fully. Let me explain further.

  • Now metadata contains such properties as md5Hash, cacheControl and other standard metadata keys.
  • Some of them are listed in the metadata docs, and more thoroughly they are listed in the JSON API reference.
  • The fact that they are listed in the reference seems to mean that the Google Cloud Storage product is committed to their naming and presence.
  • This in turn means that the consumer software can rely on them in the source code, which happens de-facto, I believe.
  • If and when Storage changes the metadata fields, be it removing or renaming, it will break the consumer software regardless of whether the nodejs-storage library has typing support for these metadata keys or not.
  • Adding new metadata keys is not a problem, the typing can allow unmentioned properties in the interface.
  • Typing on itself does not do any transformation of the data, therefore the fact that “data returned by the server gets propagated to you without being processed in any way” is not a concern here.
  • The main purpose of adding well-known metadata keys as properties of the Metadata type is to facilitate discoverability and type checking. Currently, when one tries to dig into the File.metadata propery, they face ambiguous any and have to research somewhere else on what are the exact metadata keys there.

I hope now the background of my inquiry is clearer.

@shaffeeullah
Copy link
Contributor

Hi @anantakrishna while I agree that these are strong advantages of including typing for Metadata, I still believe that the disadvantages are more impactful.

  • If we add typing, users must upgrade the library in order to get new metadata fields instead of getting them automatically
  • We don't have a great way of supporting public preview in this library. When GCS metadata features are in public preview, adding those fields to this library potentially leads us to tech debt later (we can't remove fields in this library because that would be a breaking change.
  • This feature request in itself is a breaking change.

@shaffeeullah
Copy link
Contributor

Hi @fmmoret ,

This is a fair suggestion. We will consider implementing this. It isn't a breaking change and will allow users to access new GCS metadata fields without upgrading the library.

@shaffeeullah shaffeeullah reopened this Apr 20, 2022
@shaffeeullah shaffeeullah added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed type: question Request for information or clarification. Not an issue. labels Apr 20, 2022
@danielbankhead danielbankhead added the next major: breaking change this is a change that we should wait to bundle into the next major version label Aug 12, 2022
@ddelgrosso1 ddelgrosso1 added the priority: p3 Desirable enhancement or fix. May not be included in next release. label Oct 27, 2022
@iiian
Copy link

iiian commented Jan 9, 2023

For my part, being able to access the generation number of the file has value, and since generation number is housed under metadata, it would be nice to have a type contract, or to have generation number live elsewhere even.

@danielbankhead
Copy link
Contributor

Fixed via #2234

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. next major: breaking change this is a change that we should wait to bundle into the next major version priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

7 participants