-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Storage: Blob.compose doesn't use default content_type #5834
Comments
@frankyn I'm not sure how the GCS team would like to shape this feature. |
Acking. Adding to GCS client library weekly. |
@frankyn -- any updates? |
Apologies on the wait. content-type is optional and defaults to Additionally, I'm looking at @tseaver do you have objections to surfacing content_type as an optional parameter for compose()? |
Great! content-type as an optional parameter (but falling back to the default through Could this lead to unexpected behavior? For example, composing blobs of some non-default type but not specifying that type in compose leads them to be composed as |
@rasmi that sounds like a fair condition. if content_type is not None:
# use this
if self._get_content_type is not `None`
# use this
else:
# it's okay to not provide content_type as it will default to application/octet-stream. I think inferring the content-type might be not be as helpful as simply surfacing a parameter for the content type. I think it would feel magical which is not a bad thing, but it may or may not be the desired result for everyone. |
Okay, agreed. I can submit a PR. However, I have an implementation question. Content type is not explicitly used in Additionally,
at the top of |
@frankyn as long as the sources = [bucket.blob(name) for name in ['source1.txt', 'source2.txt']]
destination = bucket.blob('destination.txt)
destination.content_type = 'text/plain'
# set other destination properties here
destination.compose(sources) I don't really see the value in adding Another Way to Do It(TM). This issue is a request that, if the destination blob does not have def _deduce_content_type_for_compose(self, sources):
if self.content_type is None:
source_types = set(
src.content_type for src in sources if src.content_type is not None)
if len(source_types) == 1:
return source_types[0]
return self.content_type |
Thanks @tseaver, your comment helps. How about not failing the compose if a content_type is not found? |
That would be fine. It has been a while, but I wonder if I added that check because the |
Ah good question. I do recall this came up for Node.js and the GCS API has since changed this requirement. The code used to be correct but now the restriction has been removed. |
PR #6031 makes that change, including a system test. Note that the server does not set |
Blob.compose
does not use theself._get_content_type
method, which means Blob.compose raises an error if thecontent_type
is not explicitly specified. This is documented behavior, but is it intended behavior? Should compose useself._get_content_type
to use the default type, or perhaps infer the type from the blobs being composed?The text was updated successfully, but these errors were encountered: