Add request handler for play websocket requests

[easel]

PlayAdapter now includes def handleWebSocketRequestHeader: RequestHeader => ZIO[R, Result, Unit]
and always passes websocket requests through webSocketOrResult to allow for authentication. Play auth example
now authenticates websocket requests.

[ghostdogpr]

Hi! What's the jitpack file doing? It seems to cause the CI to fail and I am not sure what it is for.

[easel]

Sorry, the jitpack stuff was so I could actually build and test in my app. I'll scrub it out.

All that being said, while this patch works, it's only 1/2 useful because there's no way to do the equivalent of the httpRequestWrapper for webSockets. I'm playing with a solution to do this using the zio context but haven't quite gotten all the type gymnastics worked out yet.

[easel]

@ghostdogpr I've mostly run aground attempting to solve the problem of passing the authentication token into the subscription provider. The root problem is that the pattern in use for play and akka http relies on a fiber ref, and as things stand, passing that fiber ref into the stream doesn't work.

I'm not familiar enough with the guts of zio streams to know if there's a way to force them to somehow instantiate and run on the same the same fiber as they are instantiated on, but the few naive things I've tried have run ground.

The brute force approach is to just allow a A => IO[Auth, ZStream[Auth, E, T]] as member of the Subscription. I've partially traced through the schema generation and execution code to make this work but haven't quite made it yet.

All that being said, I think this PR as it stands is an improvement on the current state (no more misspelled and unused methods and at least you can enforce security on subscriptions) so perhaps it's best to merge it and deal with passing context into subscriptions elsewhere. I'm happy either way.

[ghostdogpr]

Definitely 👍 on fixing the typo!
@paulpdaniels can you have a look at this one, as the play expert? 😄

[easel]

Ok, I think I've got this working. The root issue here is that the web socket request is a two stage process, where we first respond to the http request, and then hand off a flow to play. That flow now receives the play RequestHeaders and re-runs the handleWebSocketRequest to ensure thread locals are set before starting the subscription stream.

There's also now a Subscriptions pattern for ARG => RIO[R, ZStream[R, E, T]]. Now that I've got it figured out, ZStream.fromEffect(authMethod).flatMap(streamMethod) is working but I think it's still a reasonable thing to allow for completeness so I left it in.

There's a fairly detailed test in the ExecutionSpec that tests passing authentication context via a wrapper similiar to the play and http4s wrappers. (Un)fortunately it doesn't demonstrate the failure since it's specific to the Task-Future boundary , but I think it's a useful addition to the test suite since it demonstrates deriving a custom schema for a custom environment and tests the underlying mechanisms use by the play and akka http authentication examples.

In any case, I think this PR is ready for review.

[paulpdaniels]

@ghostdogpr sure I’ll take a look tomorrow!

[ghostdogpr]

@easel I would prefer to keep it simple and only allow ZStream in subscriptions. As you mentioned, a ZIO can easily be turned into a ZStream and I prefer if all uses use a single way to reduce the chance for edge cases.

It looks like some of the new tests are failing with Scala 3.

[easel]

Ok, I've dropped the ARGS => RIO[ZStream]. Should we consider deprecating the RIO[ZStream] subscription schema as well?

I've also dropped the new test. That said, the test exercises generating custom schemas and works on everything but scala3. I worry it exposes some limitations in the schema derivation under scala 3. Is it worth splitting it out into a new PR to chase down?

[ghostdogpr]

Yeah let's drop RIO[ZStream] as well.

Okay for adding a new PR with the Scala 3 failing test, I can take a look at it. Thanks!

[ghostdogpr]

Minor: can you clean and sort imports?

[easel]

Imports should be cleaned up now.

[ghostdogpr]

Got the OK from @paulpdaniels.

While reviewing, I was thinking, maybe we should be consistent with RequestWrapper? handleWebSocketRequestHeader exposes more or less the same thing as RequestWrapper.apply.

We could either add WebSocketRequestWrapper that wraps the current function just to be consistent with RequestWrapper, or even potentially unifying them under a common sealed trait to keep a single parameter (and add the ability to combine them). What do you think?

[easel]

@ghostdogpr I considered trying to merge them as well. I can say that for my use case the implementations are almost identical. I guess the challenge is that the semantics aren't quite the same -- for websockets failures are on the error channel, while request wrapper just provides an opportunity to side effect the environment and execute the effect or not.

That said, if we changed, perhaps RequestWrapper would be better off with an explicit error channel?

I can experiment with some options -- I think it would be unfortunate to change the signature of RequestWrapper such that it broke everybodies code.

If we can introduce a RequestWrapperOrError under RequestWrapper with a wrapRequestOrFail(requestHeader: RequestHeader): ZIO[R, Result, Result] and provide a default implementation of the new wrapRequestOrFail method that lifts the result to the error channel if status code doesn't begin with 200 or something, legacy code would work on websockets "for free".

At some point we could also deprecate the one-parameter RequestWrapper, and then flip the makePost, makeGet etc methods that use it over to the new semantics where the graphql code doesn't get run if the wrapper errors out, bringing them in line with the websocket semantics.

Is that what you had in mind?

[ghostdogpr]

I like your last idea. Deprecate the existing method and expose one with the unified semantics (with a different name for RequestWrapper so that both the old way and the new way can work at the same time?). And if the old way implementation could actually use the new way under the hood, the code stays simple.

[easel]

Ok, I've implemented more or less what I described above. Let me know what you think.

I still need to wire it into my production app and see how it behaves. There really aren't tests exercising this stuff atm so it's possible I've screwed up the type gymnastics someplace.

[ghostdogpr]

LGTM! Do you want to do some testing on your project before we merge it?

[easel]

So I've got this wired and have been using it for a while, seemingly successfully. I'm happy to merge whenever you are.

[easel]

Thanks for the merge @ghostdogpr

[ghostdogpr]

@easel I'd love your feedback on this ongoing PR: #1125
I'm re-implementing the adapters based on Tapir so that they all share the same code and functionalities. I implemented something called "request interceptors" which can do what ContextWrapper and RequestWrapper used to do in Play and Akka adapters. If you could have a look at this, it would be awesome.