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

optionMarshaller - is None returning a 200 what we really want? #19740

Closed
ktoso opened this issue Feb 10, 2016 · 3 comments
Closed

optionMarshaller - is None returning a 200 what we really want? #19740

ktoso opened this issue Feb 10, 2016 · 3 comments
Assignees
Labels
discuss Tickets that need some discussion before proceeding. Not decided if it's a good idea. obsolete – reopen if necessary Ticket closed as currently obsolete, reopen if discussion still relevant t:http:server:dsl t:http
Milestone

Comments

@ktoso
Copy link
Member

ktoso commented Feb 10, 2016

Hi @sirthias @jrudolph,
I was reading this blogpost : http://danielasfregola.com/2016/02/07/how-to-build-a-rest-api-with-akka-http/

And realised that when we have an Option[String] for example, and we complete with a None the default EmptyValue[] mechanism kicks in and the response will be a 200 response. Is there some deeper reason behind that? I would have thought a 404 would be more appropriate to model the None case.

Code in question:

  implicit def optionMarshaller[A, B](implicit m: Marshaller[A, B], empty: EmptyValue[B]): Marshaller[Option[A], B] =
    Marshaller { implicit ec 
      {
        case Some(value)  m(value)
        case None         FastFuture.successful(Marshalling.Opaque(()  empty.emptyValue) :: Nil)
      }
    }

Let me know if there's a story behind the 200 response there or if we simply should fix it, thanks a lot in advance!

// Yup I'm back from vacation, was great (now a bit sick though), so back to hacking full speed now :-)

@ktoso ktoso added t:http t:http:server:dsl 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted discuss Tickets that need some discussion before proceeding. Not decided if it's a good idea. 3 - in progress Someone is working on this ticket and removed 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted labels Feb 10, 2016
@ktoso ktoso added this to the 2.4.x milestone Feb 10, 2016
@ktoso ktoso self-assigned this Feb 10, 2016
@sirthias
Copy link
Contributor

I don't think there is generally "correct" semantic for marshalling None.
Depending on the application None could rendered to a 200 Ok, 204 No Content, 404 Not Found or any other kind of response.

Note that in the case you want a 404 response you can use the rejectEmptyResponse directive which was inherited from spray (http://spray.io/documentation/1.2.2/spray-routing/misc-directives/rejectEmptyResponse/) and is part of the MiscDirectives.

@ktoso
Copy link
Member Author

ktoso commented Feb 10, 2016

That's true as well that the right answer is "it depends" here hm...
Instead of changing behaviour I'll document how to change the none case by providing an EmptyValue implicitly and mention rejectEmptyResponse then.

@ktoso
Copy link
Member Author

ktoso commented May 18, 2016

Closing for now as obsolete

@ktoso ktoso closed this as completed May 18, 2016
@ktoso ktoso added obsolete – reopen if necessary Ticket closed as currently obsolete, reopen if discussion still relevant and removed 3 - in progress Someone is working on this ticket labels May 18, 2016
@johanandren johanandren modified the milestones: invalid, 2.4.x Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Tickets that need some discussion before proceeding. Not decided if it's a good idea. obsolete – reopen if necessary Ticket closed as currently obsolete, reopen if discussion still relevant t:http:server:dsl t:http
Projects
None yet
Development

No branches or pull requests

3 participants