Document what to do instead, if using the (anti pattern) respondWithMediaType from Spray - Explain the benefits of marshalling #190

Open
ktoso opened this Issue Sep 8, 2016 · 22 comments

Comments

Projects
None yet
2 participants
@ktoso
Member

ktoso commented Sep 8, 2016

Issue by ktoso
Thursday Oct 01, 2015 at 12:35 GMT
Originally opened as akka/akka#18625


Hi @sirthias @jrudolph,
I realised we don't have respondWithMediaType ported to Akka HTTP and am expecting that this is because we want people to rely on using the content type negotiation that HTTP does instead of baking their own using this directive...?

I'd like to confirm my thinking about it so I can sleep peacefully at night 😉

Refs:

@ktoso ktoso added this to the http-backlog milestone Sep 8, 2016

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Thursday Oct 01, 2015 at 18:56 GMT


Yes, it is an anti-pattern we didn't manage to get rid of.

Unfortunately, (as we found out too late) it was part of one of the introductory examples, which needed it to serve scala-xml literals properly as text/html. IIRC we changed that behavior with the current xml node marshaller, so there are little reasons to use responseWithMediaType instead of using the marshaller infrastructure proper.

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Thursday Oct 01, 2015 at 18:56 GMT


Yes, it is an anti-pattern we didn't manage to get rid of.

Unfortunately, (as we found out too late) it was part of one of the introductory examples, which needed it to serve scala-xml literals properly as text/html. IIRC we changed that behavior with the current xml node marshaller, so there are little reasons to use responseWithMediaType instead of using the marshaller infrastructure proper.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by sirthias
Friday Oct 02, 2015 at 09:41 GMT


Yes, +1 to "respondWithMediaType is definitely an anti-pattern"!

Member

ktoso commented Sep 8, 2016

Comment by sirthias
Friday Oct 02, 2015 at 09:41 GMT


Yes, +1 to "respondWithMediaType is definitely an anti-pattern"!

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by ktoso
Friday Oct 02, 2015 at 10:32 GMT


Thanks for confirming guys!
Sounds like this is best solved by adding a small note in the migration guide akka/akka#17972 - in came someone used it in spray, now they'll be forced to abandon that anti-pattern :-)

Member

ktoso commented Sep 8, 2016

Comment by ktoso
Friday Oct 02, 2015 at 10:32 GMT


Thanks for confirming guys!
Sounds like this is best solved by adding a small note in the migration guide akka/akka#17972 - in came someone used it in spray, now they'll be forced to abandon that anti-pattern :-)

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by ktoso
Friday Oct 09, 2015 at 12:53 GMT


Added a note in migration-from-spray.rst, should be filled in a bit more soon.

Member

ktoso commented Sep 8, 2016

Comment by ktoso
Friday Oct 09, 2015 at 12:53 GMT


Added a note in migration-from-spray.rst, should be filled in a bit more soon.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by ktoso
Saturday Nov 21, 2015 at 18:06 GMT


Reopening, it was mentioned in http://akiradeveloper.hatenadiary.com/entry/2015/11/02/222054 as lack of this directive caused him to abandon akka-http (and another thing). We should re-visit the directive and explicitly explain what to do instead if we really don't want it.

// cc @rkuhn @jrudolph @sirthias

Member

ktoso commented Sep 8, 2016

Comment by ktoso
Saturday Nov 21, 2015 at 18:06 GMT


Reopening, it was mentioned in http://akiradeveloper.hatenadiary.com/entry/2015/11/02/222054 as lack of this directive caused him to abandon akka-http (and another thing). We should re-visit the directive and explicitly explain what to do instead if we really don't want it.

// cc @rkuhn @jrudolph @sirthias

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by sirthias
Thursday Nov 26, 2015 at 07:58 GMT


I seriously doubt that the lack of a single directive can be the single cause why anyone would not use akka-http. Especially since you can easily write this directive yourself, if you so wish, with only a few lines of code.

respondWithMediaType is an anti-pattern because it essentially circumvents the content negotiation logic that the marshalling infrastructure was carefully designed to enable.

Member

ktoso commented Sep 8, 2016

Comment by sirthias
Thursday Nov 26, 2015 at 07:58 GMT


I seriously doubt that the lack of a single directive can be the single cause why anyone would not use akka-http. Especially since you can easily write this directive yourself, if you so wish, with only a few lines of code.

respondWithMediaType is an anti-pattern because it essentially circumvents the content negotiation logic that the marshalling infrastructure was carefully designed to enable.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Thursday Nov 26, 2015 at 09:02 GMT


It seems the bigger problem of the blog poster was to get the entity types to work with the S3 API uploads. I guess it could be a confusion about when to use CloseDelimited vs Default but Google Translate didn't preserve enough semantics for me to understand what the real problem was.

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Thursday Nov 26, 2015 at 09:02 GMT


It seems the bigger problem of the blog poster was to get the entity types to work with the S3 API uploads. I guess it could be a confusion about when to use CloseDelimited vs Default but Google Translate didn't preserve enough semantics for me to understand what the real problem was.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by ktoso
Sunday Nov 29, 2015 at 19:44 GMT


We tried to reach out to him (in Japanese, thanks @eed3si9n!) for more details.
I guess what we should do here though is to look how he used it (how it was being used), and explain how to do it properly in the docs. Keeping the ticket open until we have that.

Member

ktoso commented Sep 8, 2016

Comment by ktoso
Sunday Nov 29, 2015 at 19:44 GMT


We tried to reach out to him (in Japanese, thanks @eed3si9n!) for more details.
I guess what we should do here though is to look how he used it (how it was being used), and explain how to do it properly in the docs. Keeping the ticket open until we have that.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by eed3si9n
Sunday Nov 29, 2015 at 21:58 GMT


@jrudolph is right about the main complaint being CloseDelimited. Should I open a new issue for this?
The motivation for using CloseDelimited with Content-Length > 0 was that some clients are not able to handle the Chunked response.

Member

ktoso commented Sep 8, 2016

Comment by eed3si9n
Sunday Nov 29, 2015 at 21:58 GMT


@jrudolph is right about the main complaint being CloseDelimited. Should I open a new issue for this?
The motivation for using CloseDelimited with Content-Length > 0 was that some clients are not able to handle the Chunked response.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by rkuhn
Wednesday Dec 02, 2015 at 08:16 GMT


Hmm, so I take it that those less capable clients are relevant and we should support them? If so, then we’ll need to make it possible and document it. Otherwise we just need to document it.

Member

ktoso commented Sep 8, 2016

Comment by rkuhn
Wednesday Dec 02, 2015 at 08:16 GMT


Hmm, so I take it that those less capable clients are relevant and we should support them? If so, then we’ll need to make it possible and document it. Otherwise we just need to document it.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Wednesday Dec 02, 2015 at 08:45 GMT


As I understood it the problem was in writing a client for S3 uploads. Using chunked uploads with S3 seems to be possible but is quite complex (due to having to use an extra framing protocol layer in addition to the HTTP chunking to provide signatures). CloseDelimited, however, can not be used for requests (= uploads) because (half-)closing the connection to signal end of stream is not allowed for requests. This leaves a Default entity as the only streaming way to upload data. However, for a Default entity the content length must be known in advance.

I guess one of those things was the problem.

However, none of these problems are problems of akka-http but these are all limits inherent in HTTP. We tried to model everything that is available in HTTP by the HttpEntity ADT which is very explicit in what it needs and provides. AFAIK we support all the modes that are possible with HTTP. We already have some documentation about the entity types. To be able to improve the documentation we would need to know more clearly what exactly the problem was.

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Wednesday Dec 02, 2015 at 08:45 GMT


As I understood it the problem was in writing a client for S3 uploads. Using chunked uploads with S3 seems to be possible but is quite complex (due to having to use an extra framing protocol layer in addition to the HTTP chunking to provide signatures). CloseDelimited, however, can not be used for requests (= uploads) because (half-)closing the connection to signal end of stream is not allowed for requests. This leaves a Default entity as the only streaming way to upload data. However, for a Default entity the content length must be known in advance.

I guess one of those things was the problem.

However, none of these problems are problems of akka-http but these are all limits inherent in HTTP. We tried to model everything that is available in HTTP by the HttpEntity ADT which is very explicit in what it needs and provides. AFAIK we support all the modes that are possible with HTTP. We already have some documentation about the entity types. To be able to improve the documentation we would need to know more clearly what exactly the problem was.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Wednesday Dec 02, 2015 at 08:57 GMT


BTW, I just guessed what the problem was from the keywords. It would be nice if you could confirm that this is at least partly correct, @eed3si9n :)

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Wednesday Dec 02, 2015 at 08:57 GMT


BTW, I just guessed what the problem was from the keywords. It would be nice if you could confirm that this is at least partly correct, @eed3si9n :)

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by eed3si9n
Wednesday Dec 02, 2015 at 09:51 GMT


Yes. The implied "client software" that doesn't handle chunked response seems to be Amazon S3. Maybe he meant to say "peer." In any case, I think he's trying to call Complete Multipart Upload, which seems to require Content-Length. To handle large files, he wants to return Stream[T] from it.
Most likely this is the code - https://github.com/akiradeveloper/akka-s3/blob/9d1288acf5db5d6725b96b6ff3b78233a7d7cf91/src/main/scala/akka/s3/api/CompleteMultipartUpload.scala

Member

ktoso commented Sep 8, 2016

Comment by eed3si9n
Wednesday Dec 02, 2015 at 09:51 GMT


Yes. The implied "client software" that doesn't handle chunked response seems to be Amazon S3. Maybe he meant to say "peer." In any case, I think he's trying to call Complete Multipart Upload, which seems to require Content-Length. To handle large files, he wants to return Stream[T] from it.
Most likely this is the code - https://github.com/akiradeveloper/akka-s3/blob/9d1288acf5db5d6725b96b6ff3b78233a7d7cf91/src/main/scala/akka/s3/api/CompleteMultipartUpload.scala

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Wednesday Dec 02, 2015 at 10:03 GMT


So, this is a restriction from S3 not from akka-http, right? If S3 requires a Content-Length you need to compute it somehow and use an HttpEntity.Default regardless from where the actual data comes.

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Wednesday Dec 02, 2015 at 10:03 GMT


So, this is a restriction from S3 not from akka-http, right? If S3 requires a Content-Length you need to compute it somehow and use an HttpEntity.Default regardless from where the actual data comes.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by eed3si9n
Wednesday Dec 02, 2015 at 10:08 GMT


So, this is a restriction from S3 not from akka-http, right?

It's a combination of both since Spray was able to handle it by returning Content-Length > 0 with CloseDelimited, and now he's looking into other web frameworks.

Member

ktoso commented Sep 8, 2016

Comment by eed3si9n
Wednesday Dec 02, 2015 at 10:08 GMT


So, this is a restriction from S3 not from akka-http, right?

It's a combination of both since Spray was able to handle it by returning Content-Length > 0 with CloseDelimited, and now he's looking into other web frameworks.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by jrudolph
Wednesday Dec 02, 2015 at 10:47 GMT


No, there was no CloseDelimited in spray. If you want to supply a Content-Length, HttpEntity.Default is the right choice.

Member

ktoso commented Sep 8, 2016

Comment by jrudolph
Wednesday Dec 02, 2015 at 10:47 GMT


No, there was no CloseDelimited in spray. If you want to supply a Content-Length, HttpEntity.Default is the right choice.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by betandr
Monday Dec 07, 2015 at 17:33 GMT


FWIW we use respondWithMediaType currently with Spray but I'm really happy to stop using it...

Member

ktoso commented Sep 8, 2016

Comment by betandr
Monday Dec 07, 2015 at 17:33 GMT


FWIW we use respondWithMediaType currently with Spray but I'm really happy to stop using it...

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by omervk
Sunday Mar 20, 2016 at 19:35 GMT


Hi all,

I've gone over the conversation in this thread and the linked threads and would like to pose a question. I have an API that creates an object, but only knows how to answer when the client Accepts application/json. A user of my API sent a request using text/plain and received 406 Not Acceptable with the error Resource representation is only available with these types: application/json, but the object was still created. This is an unexpected side-effect.

Based on the answers to this question on StackOverflow, I created the following to wrap around my route:

extract(_.request.getHeader(classOf[Accept])) { accept =>
    if (!accept.isPresent || accept.get().acceptsAll() || accept.get().mediaRanges.exists(_.matches(MediaTypes.`application/json`))) {
        // My routes here...
    } else {
        reject(UnacceptedResponseContentTypeRejection(Set(ContentNegotiator.Alternative(MediaTypes.`application/json`))))
    }
}

This works, but I have two questions:

  1. Does it make sense to have this? I would have assumed there was a built-in mechanism for this.
  2. Issue #18425 linked in OP notes that we should use Marshaller.oneOf but I'm not sure how one would go about doing this.
Member

ktoso commented Sep 8, 2016

Comment by omervk
Sunday Mar 20, 2016 at 19:35 GMT


Hi all,

I've gone over the conversation in this thread and the linked threads and would like to pose a question. I have an API that creates an object, but only knows how to answer when the client Accepts application/json. A user of my API sent a request using text/plain and received 406 Not Acceptable with the error Resource representation is only available with these types: application/json, but the object was still created. This is an unexpected side-effect.

Based on the answers to this question on StackOverflow, I created the following to wrap around my route:

extract(_.request.getHeader(classOf[Accept])) { accept =>
    if (!accept.isPresent || accept.get().acceptsAll() || accept.get().mediaRanges.exists(_.matches(MediaTypes.`application/json`))) {
        // My routes here...
    } else {
        reject(UnacceptedResponseContentTypeRejection(Set(ContentNegotiator.Alternative(MediaTypes.`application/json`))))
    }
}

This works, but I have two questions:

  1. Does it make sense to have this? I would have assumed there was a built-in mechanism for this.
  2. Issue #18425 linked in OP notes that we should use Marshaller.oneOf but I'm not sure how one would go about doing this.
@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by ktoso
Monday Mar 21, 2016 at 10:18 GMT


Hi @omervk, yeah that's the current behaviour indeed (see also akka/akka#16974 ) and it's something we'd like to improve (make the marshallers more lazy).

Please track that other ticket too to get status updates on it (or help out if you can!)

Member

ktoso commented Sep 8, 2016

Comment by ktoso
Monday Mar 21, 2016 at 10:18 GMT


Hi @omervk, yeah that's the current behaviour indeed (see also akka/akka#16974 ) and it's something we'd like to improve (make the marshallers more lazy).

Please track that other ticket too to get status updates on it (or help out if you can!)

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Sep 8, 2016

Member

Comment by omervk
Monday Mar 21, 2016 at 11:14 GMT


Thanks! 👍

Member

ktoso commented Sep 8, 2016

Comment by omervk
Monday Mar 21, 2016 at 11:14 GMT


Thanks! 👍

@ktoso ktoso added the 1 - triaged label Sep 8, 2016

@jrudolph jrudolph changed the title from Document what to do instead, if using the (anti pattern) respondWithMediaType from Spray to Document what to do instead, if using the (anti pattern) respondWithMediaType from Spray - Explain the benefits of marshalling Sep 6, 2017

@ykycxzsv

This comment has been minimized.

Show comment
Hide comment
@ykycxzsv

ykycxzsv Nov 10, 2017

Especially since you can easily write this directive yourself, if you so wish, with only a few lines of code.

can someone please show how an implementation of respondWithMediaType for akka-http would look? copying the one from spray doesn't work, as the data types have changed, and i'm not familiar enough with akka-http to know how to rewrite it. our code base unfortunately has tons of respondWithMediaType, so we really need this in order to upgrade, as it's not realistical to rewrite everything at once

here's the spray implementation for reference

  def respondWithMediaType(mediaType: MediaType): Directive0 = {
    val rejection = UnacceptedResponseContentTypeRejection(ContentType(mediaType) :: Nil)
    extract(_.request.isMediaTypeAccepted(mediaType)).flatMap[HNil] { if (_) pass else reject(rejection) } &
      mapRequestContext(_.withContentNegotiationDisabled) &
      mapHttpResponseEntity(_.flatMap { case HttpEntity.NonEmpty(ct, buf) ⇒ HttpEntity(ct.withMediaType(mediaType), buf) }) &
      MiscDirectives.cancelRejection(rejection)
}

Especially since you can easily write this directive yourself, if you so wish, with only a few lines of code.

can someone please show how an implementation of respondWithMediaType for akka-http would look? copying the one from spray doesn't work, as the data types have changed, and i'm not familiar enough with akka-http to know how to rewrite it. our code base unfortunately has tons of respondWithMediaType, so we really need this in order to upgrade, as it's not realistical to rewrite everything at once

here's the spray implementation for reference

  def respondWithMediaType(mediaType: MediaType): Directive0 = {
    val rejection = UnacceptedResponseContentTypeRejection(ContentType(mediaType) :: Nil)
    extract(_.request.isMediaTypeAccepted(mediaType)).flatMap[HNil] { if (_) pass else reject(rejection) } &
      mapRequestContext(_.withContentNegotiationDisabled) &
      mapHttpResponseEntity(_.flatMap { case HttpEntity.NonEmpty(ct, buf) ⇒ HttpEntity(ct.withMediaType(mediaType), buf) }) &
      MiscDirectives.cancelRejection(rejection)
}
@ykycxzsv

This comment has been minimized.

Show comment
Hide comment
@ykycxzsv

ykycxzsv Jan 19, 2018

@ktoso could you please demonstrate how to implement this directive in akka-http as you suggested earlier in the thread? it would help migration a lot

@ktoso could you please demonstrate how to implement this directive in akka-http as you suggested earlier in the thread? it would help migration a lot

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