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

AWS S3: Expose Content-Type as part of ObjectMetaData #858

Merged
merged 3 commits into from
Mar 23, 2018

Conversation

chetanmeh
Copy link
Contributor

@chetanmeh chetanmeh commented Mar 21, 2018

Fix for #853

To expose the Content-Type I had to implement a CustomContentTypeHeader as akka http by design does not provide a way to construct this header.

Further akka.http.scaladsl.model.ContentType is a sealed trait so its not possible to have an implementation which extends both ContentType and HttpHeader. So the logic in ObjectMetadata also had to be changed to explicitly match header by name. Not sure if there is a better way possible to do this

@lightbend-cla-validator

Hi @chetanmeh,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@ennru ennru changed the title Expose Content-Type as part of ObjectMetaData AWS S3: Expose Content-Type as part of ObjectMetaData Mar 22, 2018
)

//`Content-Type` header is by design not accessible as header. So need to have a custom
//header implementation to expose that
Copy link
Member

Choose a reason for hiding this comment

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

Jup. I can see how it is reasonable to construct your own Content-Type header in this case... we might want to introduce a nicer escape hatch for that at some point - though still making it evident that for 'typical' situations you should not construct it yourself.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM

@ennru ennru merged commit 53ad598 into akka:master Mar 23, 2018
@ennru
Copy link
Member

ennru commented Mar 23, 2018

Thank you for this valuable improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants