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

Create a 'strictEntity' directive #26

Closed
akka-ci opened this issue Sep 8, 2016 · 13 comments
Closed

Create a 'strictEntity' directive #26

akka-ci opened this issue Sep 8, 2016 · 13 comments
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted t:routing Issues related to the routing DSL
Milestone

Comments

@akka-ci
Copy link

akka-ci commented Sep 8, 2016

Issue by Joe-Edwards
Tuesday Jul 05, 2016 at 09:45 GMT
Originally opened as akka/akka#20881


From #20850

A Directive1[HttpEntity.Strict] which calls toStrict on the HTTP Entity safely with a specified timeout (to avoid the deadlock in the other ticket). N.B. should also replace the entity in the request context.

@akka-ci akka-ci added this to the 2.4.x milestone Sep 8, 2016
@akka-ci akka-ci added help wanted Identifies issues that the core team will likely not have time to work on t:http t:routing Issues related to the routing DSL labels Sep 8, 2016
@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Tuesday Jul 05, 2016 at 10:59 GMT


Thanks for the ticket, I agree it's better to provide a directive whcih does this "safely" than assuming people will do the right thing (and get 20 "almost correct" impls ;-))

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Tuesday Jul 05, 2016 at 11:00 GMT


PRs very welcome to this one, should be rather simple :)

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by Hawstein
Tuesday Jul 12, 2016 at 17:32 GMT


@Joe-Edwards @ktoso I will work on this if you don't mind. And AFAICS, just adding a extractStrictRequestEntity method to BasicDirectives solves this issue. Correct me if I miss something.

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by Joe-Edwards
Tuesday Jul 12, 2016 at 18:36 GMT


Go for it. If it's any help, what I'd come up with so far was:

  def extractStrictEntity(timeout: FiniteDuration): Directive1[HttpEntity.Strict] = {
    Directive { inner => ctx =>
      ctx.request.entity match {
        case strict: HttpEntity.Strict =>
          inner(Tuple1(strict))(ctx)
        case nonStrict =>
          import ctx.{executionContext, materializer}
          nonStrict.toStrict(timeout).flatMap(strict => inner(Tuple1(strict))(ctx.mapRequest(_.copy(entity = strict))))
      }
    }
  }

I'm not sure if the optimization in the strict case is really necessary (perhaps using FastFuture will make it redundant anyway?)

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Tuesday Jul 12, 2016 at 21:08 GMT


The optimisation is not needed, toStrict does the right thing (returns "this") if the entity is Strict already.

I was more thinking of making a directive that works via mapRequest, i.e. it transforms the entity transparently.

strictEntity { // toStricts, but we can do various things inside
  extractDataBytes { // works since strict already
    // .. 
  } ~ get { 
    extractRequest { r => r.entity.dataBytes .... } 
  }
}

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Tuesday Jul 12, 2016 at 21:09 GMT


For inspiration look at mapRequest

val route =
      mapRequest(transformToPostRequest) {
        extractRequest { req =>
          complete(s"The request method was ${req.method.name}")
        }
      }

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by Hawstein
Wednesday Jul 13, 2016 at 20:00 GMT


@ktoso just make a PR for this issue.

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by Joe-Edwards
Thursday Jul 14, 2016 at 09:42 GMT


The benefit of extracting the entity is that you get don't need to match/cast it. Usually you want a strict entity so you can get the data explicitly:

strictEntity(...) {
  extractRequest { request =>
    val entity: HttpEntity = request.entity // don't know it's strict yet :(
    entity match {
      case strict: HttpEntity.Strict => strict.data // now we do, but have an non-exhaustive match :(
    }
  }
}

vs

extractStrictEntity(...) { strict =>
  strict.data // immediately available in a type safe way
}

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Thursday Jul 14, 2016 at 09:59 GMT


Sure, however it does not compose as nicely.
My proposal composes more nicely though, and is beneficial for other directives as well (e.g. formFields which people keep getting wrong), instead of just this single case.

I stand by the need of doing a directive like I proposed, and optionally adding a extractStrictEntity.

@akka-ci
Copy link
Author

akka-ci commented Sep 8, 2016

Comment by ktoso
Friday Jul 22, 2016 at 09:33 GMT


Thanks a lot @Hawstein for implementing this feature :-)

@jrudolph jrudolph added the 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted label Sep 8, 2016
@ktoso ktoso removed the t:http label Sep 12, 2016
@ktoso ktoso removed this from the 2.4.x milestone Sep 12, 2016
@catalin-ursachi
Copy link
Contributor

Was this not already implemented by akka/akka#20953?

@ktoso
Copy link
Member

ktoso commented Jan 2, 2017

Correct, thanks a lot for noticing @catalin-ursachi !

@ktoso ktoso closed this as completed Jan 2, 2017
@ktoso ktoso added this to the duplicate milestone Jan 2, 2017
@ktoso ktoso removed the help wanted Identifies issues that the core team will likely not have time to work on label Jan 2, 2017
@ktoso
Copy link
Member

ktoso commented Jan 2, 2017

Duplicate of akka/akka#20881 (already solved)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted t:routing Issues related to the routing DSL
Projects
None yet
Development

No branches or pull requests

4 participants