Clarify: respondWithStatus #18626

Closed
ktoso opened this Issue Oct 1, 2015 · 11 comments

Comments

Projects
None yet
3 participants
@ktoso
Member

ktoso commented Oct 1, 2015

Hi @sirthias @jrudolph,
I realised we don't have respondWithStatus ported to Akka HTTP.
On this one I'm not sure if it's an omission by design or accident, WDYT?

See:

Note to self ( remove // FIXME https://github.com/akka/akka/issues/18626 from code once resolved )

@ktoso ktoso assigned ktoso and sirthias and unassigned ktoso Oct 1, 2015

@ktoso ktoso added this to the http-1.1 milestone Oct 1, 2015

@ktoso ktoso referenced this issue Oct 1, 2015

Closed

Missing Http Directive Docs #18496

53 of 59 tasks complete
@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 1, 2015

Member

PS: Sorry if I'm too noisy with all these "clarify" tickets – It's mostly so we're able to respond properly once people coming from spray start asking "hey, X is missing!", and I'm noticing those currently since documenting all directives.

I'm rather sure you made the right call when not porting something, just wanted to understand the reasoning behind it where it's not immediately clear to me 😄 Thanks in advance for the help!

Member

ktoso commented Oct 1, 2015

PS: Sorry if I'm too noisy with all these "clarify" tickets – It's mostly so we're able to respond properly once people coming from spray start asking "hey, X is missing!", and I'm noticing those currently since documenting all directives.

I'm rather sure you made the right call when not porting something, just wanted to understand the reasoning behind it where it's not immediately clear to me 😄 Thanks in advance for the help!

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 1, 2015

Member

I didn't remember we once had this directive and I cannot remember a reason for not having ported it or making any decision.

I think the main reason not to include it (despite seeming basic enough) would be that it isn't really useful in most cases. In most cases, you would first figure out the response status code before figuring out any other detail of the response, so the question would be why you would want to override it later on?

Common ways to specify the status code are:

  • use 200 OK ;)
  • use one of the marshallers that allows to specify a status code together with the entity body, e.g. as a Tuple statusCode -> entity
  • build a complete HttpResponsecompletely including the status code
  • reject a request and from a RejectionHandler return an HttpResponse including the (error) status code

I'm not against include responseWithStatus, but if it is included it should mention the more common ways of setting status codes.

Member

jrudolph commented Oct 1, 2015

I didn't remember we once had this directive and I cannot remember a reason for not having ported it or making any decision.

I think the main reason not to include it (despite seeming basic enough) would be that it isn't really useful in most cases. In most cases, you would first figure out the response status code before figuring out any other detail of the response, so the question would be why you would want to override it later on?

Common ways to specify the status code are:

  • use 200 OK ;)
  • use one of the marshallers that allows to specify a status code together with the entity body, e.g. as a Tuple statusCode -> entity
  • build a complete HttpResponsecompletely including the status code
  • reject a request and from a RejectionHandler return an HttpResponse including the (error) status code

I'm not against include responseWithStatus, but if it is included it should mention the more common ways of setting status codes.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 1, 2015

Member

And, no worries about the clarify tickets. They are easier to keep track of than reading through the chat and better for keeping the record for later questions, so 👍 for having them.

Member

jrudolph commented Oct 1, 2015

And, no worries about the clarify tickets. They are easier to keep track of than reading through the chat and better for keeping the record for later questions, so 👍 for having them.

@sirthias

This comment has been minimized.

Show comment
Hide comment
@sirthias

sirthias Oct 2, 2015

Contributor

I agree with @jrudolph, keep the clarify tickets coming, Konrad! :)

As to respondWithStatus we did not port it simply for the reason that Johannes stated:
There is probably no good reason to use it.
Overriding the status code coming back from an inner route appears hacky to me. You can do it, if you must, but we shouldn't "bless" it by providing a nice shortcut.

Contributor

sirthias commented Oct 2, 2015

I agree with @jrudolph, keep the clarify tickets coming, Konrad! :)

As to respondWithStatus we did not port it simply for the reason that Johannes stated:
There is probably no good reason to use it.
Overriding the status code coming back from an inner route appears hacky to me. You can do it, if you must, but we shouldn't "bless" it by providing a nice shortcut.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 8, 2015

Member

For reference, we still have overrideStatusCode(201) which seems to do the same thing.

So we have it in that sense.
Strong enough opinions to remove it or keep as is, WDYT?

// cc @jrudolph @sirthias @rkuhn

Member

ktoso commented Oct 8, 2015

For reference, we still have overrideStatusCode(201) which seems to do the same thing.

So we have it in that sense.
Strong enough opinions to remove it or keep as is, WDYT?

// cc @jrudolph @sirthias @rkuhn

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 8, 2015

Member

At the very least made it not recommended:

  /**
   * Not recommended directive, try to solve your problem by providing a custom [[RejectionHandler]] for example.
   * 
   * Overrides the response status code with the given one.
   */
  def overrideStatusCode(responseStatus: StatusCode): Directive0 =
    mapResponse(_.copy(status = responseStatus))
Member

ktoso commented Oct 8, 2015

At the very least made it not recommended:

  /**
   * Not recommended directive, try to solve your problem by providing a custom [[RejectionHandler]] for example.
   * 
   * Overrides the response status code with the given one.
   */
  def overrideStatusCode(responseStatus: StatusCode): Directive0 =
    mapResponse(_.copy(status = responseStatus))
@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 8, 2015

Member

Ah those elusive little directives, I guess we probably just renamed it during the migration and no one could remember...

Regarding the ScalaDoc maybe switch the lines around so that the summary is still at the top. That said, I wonder if we should keep it if we have only harsh words for it. Seems inconsequential and unfair to the little one. If we cannot find a positive description that would offer at least one however far-fetched use case for it, we should set it free.

Member

jrudolph commented Oct 8, 2015

Ah those elusive little directives, I guess we probably just renamed it during the migration and no one could remember...

Regarding the ScalaDoc maybe switch the lines around so that the summary is still at the top. That said, I wonder if we should keep it if we have only harsh words for it. Seems inconsequential and unfair to the little one. If we cannot find a positive description that would offer at least one however far-fetched use case for it, we should set it free.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 8, 2015

Member

Trying to be inventive I imagined this use case:

          authorize(quotaOk) {
            doThings()
          } ~
          overrideStatusCode(StatusCodes.EnhanceYourCalm) { // 420, e.g. used by Twitter
            doThings()
          } 

Not sure if that's realistic at all, rather not... 😉

Member

ktoso commented Oct 8, 2015

Trying to be inventive I imagined this use case:

          authorize(quotaOk) {
            doThings()
          } ~
          overrideStatusCode(StatusCodes.EnhanceYourCalm) { // 420, e.g. used by Twitter
            doThings()
          } 

Not sure if that's realistic at all, rather not... 😉

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Oct 8, 2015

Member

It's still weird that you first build a full response just to get rid of it later on.

Member

jrudolph commented Oct 8, 2015

It's still weird that you first build a full response just to get rid of it later on.

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 8, 2015

Member

Ok, since we can't find a good use case, I'll remove it.

Member

ktoso commented Oct 8, 2015

Ok, since we can't find a good use case, I'll remove it.

@ktoso ktoso assigned ktoso and unassigned sirthias Oct 8, 2015

@ktoso ktoso closed this Oct 8, 2015

@ktoso ktoso removed the 3 - in progress label Oct 8, 2015

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Oct 8, 2015

Member

Removed, closing.

It's free now :-)

Member

ktoso commented Oct 8, 2015

Removed, closing.

It's free now :-)

ktoso added a commit to ktoso/akka that referenced this issue Oct 8, 2015

-htp #18626 remove overrideStatusCode
we don't recommend using it.

ktoso added a commit to ktoso/akka that referenced this issue Oct 9, 2015

-htp #18626 remove overrideStatusCode
we don't recommend using it.

ktoso added a commit to ktoso/akka that referenced this issue Oct 12, 2015

-htp #18626 remove overrideStatusCode
we don't recommend using it.

ktoso added a commit to ktoso/akka that referenced this issue Jan 11, 2016

-htp #18626 remove overrideStatusCode
we don't recommend using it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment