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.

Sign up for GitHub

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

Jump to bottom

feat(storage): make it possible to disable Content-Type sniffing #9431

Merged
merged 3 commits into from Feb 23, 2024
Merged

feat(storage): make it possible to disable Content-Type sniffing #9431

merged 3 commits into from Feb 23, 2024
+65 −34

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Feb 16, 2024 •
edited

As described in google/go-cloud#3298 (comment), we want to disable automatic Content-Type detection when inserting an object to Google Cloud Storage (GCS).

Previously it wasn't possible to disable this auto-detection, even though googleapi.MediaOptions provides a ForceEmptyContentType option (https://github.com/googleapis/google-api-go-client/blob/v0.165.0/googleapi/googleapi.go#L283).

We enable this by adding a Writer option to set this value.

Closes #9430

@stanhu stanhu requested review from a team as code owners February 16, 2024 00:04
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: storage Issues related to the Cloud Storage API. labels Feb 16, 2024
@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch 2 times, most recently from dfba087 to 066692e Compare February 16, 2024 01:27
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 16, 2024
@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch from 066692e to ed35cc7 Compare February 16, 2024 01:31
@stanhu
Copy link
Contributor Author

stanhu commented Feb 16, 2024

@quartzmo Would you mind reviewing this? Thank you!

@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch 2 times, most recently from af8ffa5 to d0d0cdf Compare February 20, 2024 18:09
@noahdietz
Copy link
Contributor

@quartzmo Would you mind reviewing this? Thank you!

Actually this client is own by the @googleapis/cloud-storage-dpe team. They should be the ones to review it, but they should probably respond to the bug you opened first (thanks for doing that).

@stanhu
Copy link
Contributor Author

stanhu commented Feb 21, 2024

@noahdietz Thanks, would appreciate any help getting this pull request and #9430 reviewed. We've been blocked for weeks on this.

@frankyn frankyn self-assigned this Feb 21, 2024
@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 21, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 21, 2024
frankyn
frankyn requested changes Feb 21, 2024
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR; added two requests.

storage/integration_test.go Outdated Show resolved Hide resolved
storage/http_client.go Show resolved Hide resolved
storage/integration_test.go Show resolved Hide resolved
@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch 2 times, most recently from 6b2356a to 2c26c72 Compare February 21, 2024 18:48
@stanhu stanhu requested a review from frankyn February 21, 2024 20:33
@frankyn
Copy link
Member

frankyn commented Feb 21, 2024

Thanks @stanhu, I'm waiting on another review from @tritone.

@frankyn frankyn requested a review from tritone February 21, 2024 23:59
tritone
tritone reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a fix to the gRPC implementation of the Writer. Otherwise LGTM; thanks for the contribution!

storage/http_client.go Show resolved Hide resolved
@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch from 2c26c72 to 3e671a3 Compare February 22, 2024 19:24
tritone
tritone reviewed Feb 22, 2024
View reviewed changes
storage/grpc_client.go Outdated Show resolved Hide resolved
@stanhu
feat(storage): make it possible to disable Content-Type sniffing
932bab0 
As described in
google/go-cloud#3298 (comment),
we want to disable automatic `Content-Type` detection when
inserting an object to Google Cloud Storage (GCS).

Previously it wasn't possible to disable this auto-detection, even
though `googleapi.MediaOptions` provides a `ForceEmptyContentType`
option (https://github.com/googleapis/google-api-go-client/blob/v0.165.0/googleapi/googleapi.go#L283).

We enable this by adding a `Writer` option to set this value.

Closes googleapis#9430
@stanhu stanhu force-pushed the sh-storage-allow-empty-content-type branch from 3e671a3 to 932bab0 Compare February 22, 2024 19:50
@stanhu stanhu requested a review from tritone February 22, 2024 20:13
@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 22, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 22, 2024
frankyn
frankyn approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @tritone and @stanhu!

@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2024
@frankyn frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. automerge Merge the pull request once unit tests and other checks pass. labels Feb 23, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 23, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit 0676670 into googleapis:main Feb 23, 2024
9 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 23, 2024
@release-please release-please bot mentioned this pull request Feb 23, 2024
Merged
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 23, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
e7a8cdf 
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to leave
`Content-Type` blank to use the `Response-Content-Type` override in a
signed URL. This commit adds the `DisableContentTypeDetection` option
to the the driver and passes along the value to make it possible to
keep Content-Type blank in the GCS object metadata.

This commit needs
googleapis/google-cloud-go#9431 to work.
`go.mod` has been updated to use pseudo-version until
cloud.google.com/go/storage v1.39 is available
(googleapis/google-cloud-go#9457).
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 23, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
643f86b 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs
googleapis/google-cloud-go#9431 to work.
`go.mod` has been updated to use pseudo-version until
cloud.google.com/go/storage v1.39 is available
(googleapis/google-cloud-go#9457).
@stanhu stanhu mentioned this pull request Feb 23, 2024
Merged
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 23, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
8066870 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs
googleapis/google-cloud-go#9431 to work.
`go.mod` has been updated to use pseudo-version until
cloud.google.com/go/storage v1.39 is available
(googleapis/google-cloud-go#9457).
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 23, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
53a0705 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs
googleapis/google-cloud-go#9431 to work.
`go.mod` has been updated to use pseudo-version until
cloud.google.com/go/storage v1.39 is available
(googleapis/google-cloud-go#9457).
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 23, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
74b5319 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs
googleapis/google-cloud-go#9431 to work.
`go.mod` has been updated to use pseudo-version until
cloud.google.com/go/storage v1.39 is available
(googleapis/google-cloud-go#9457).
stanhu added a commit to stanhu/go-cloud that referenced this pull request Feb 29, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
2132176 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs cloud.google.com/go/storage v1.39
(googleapis/google-cloud-go#9431).
stanhu added a commit to stanhu/go-cloud that referenced this pull request Mar 1, 2024
@stanhu
blob/gcsblob: Ensure driver sets Content-Type auto-detection properly
71bf492 
google#3371 made it possible to
disable Go Cloud's Content-Type auto-detection via the
`DisableContentTypeDetection` option. However, the Google Cloud
Storage (GCS) driver still performed auto-detection even if this
option were enabled, so it was previously not possible to keep
`Content-Type` blank. This commit adds the
`DisableContentTypeDetection` option to the the driver and passes
along the value to make it possible to keep Content-Type blank in the
GCS object metadata. This enables the use of the
`Response-Content-Type` override in a signed URL.

This commit needs cloud.google.com/go/storage v1.39
(googleapis/google-cloud-go#9431).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankyn frankyn frankyn approved these changes

@tritone tritone tritone approved these changes

Assignees

@frankyn frankyn

Labels
api: storage Issues related to the Cloud Storage API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

storage: make it possible to disable Content-Type sniffing
5 participants
@stanhu @noahdietz @frankyn @tritone @kokoro-team