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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate ContentType field on create/modify ops #1141

Merged
merged 3 commits into from May 2, 2023

Conversation

alorlea
Copy link
Contributor

@alorlea alorlea commented Apr 27, 2023

This PR tries to address 2 bugs that where found when using some of the create/modify operations GCS supports in our integration tests that used the following interactions:

  • Rely on a streamable upload operations of GCS to upload of a File.
  • Submit an update on the blob to override the metadata content-type information without modifying the bytes of the blob.

To crosscheck, we ran the same tests against the real GCS API to verify the behaviour to expect:

  1. Streamable upload, by default if no content-type is provided; GCS sets it up to 'application/octet-stream' or uses the one provided by the user in the BlobInfo (This is the expected behaviour for this operation in GCS).
  2. For an update, when we merge metadata information regarding the content-type, it should override and reflect the change on the update response.

Also this behaviour is referenced on the API spec, that the content-type should be set to application/octet-stream on object creation if it is not defined:

  1. GCS Insert Request Body
  2. GCS Update Request Body

Also should hopefully address the following issue #1098 馃檪

Fixes #1098.

This commit tries to address 2 bugs that where found when using
some of the create/modify operations GCS supports like:
* using the streamable upload operations of GCS to upload of a File.
* update the blob metadata content-type without modifying the bytes of
  the blob.

For the streamable upload, by default if no content-type is provided;
GCS sets it up to 'application/octet-stream' or uses the one provided by
the user in the BlobInfo (This is the expected behaviour for this operation in GCS).

For the update, when we merge metadata information regarding the
content-type, it should override it the update response should propagate
this change.
@mishako
Copy link

mishako commented May 2, 2023

@fsouza Hey! 馃憢 Please review when you have time! 馃檹

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Thank you for contributing and sorry for the delayed review. Change looks good, do you think you could add a test that failed previously but passes with this change? I promise a quicker turnaround after the test is added :)

This adds a unit test to check that when calling
the updateObject method and a content type is provided,
we override the previous value stored.
Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Thank you!

fakestorage/object_test.go Outdated Show resolved Hide resolved
@fsouza fsouza merged commit e2d2de1 into fsouza:main May 2, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content-Type being set incorrectly for resumable uploads.
3 participants