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

Baseline Transition for Vault Use #2318

Merged

Conversation

Projects
None yet
3 participants
@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Dec 18, 2018

This is a baseline transition for Vault use.

  • Names left the same, do we want to change the name attributes?
  • UnsafeCreated the keys to global singletons. Do we want to control the control flow additionally?
if (parser.getHttpVersion().minor == 1 && parser.isChunked()) {
val trailers = new AtomicReference(Headers.empty)

val attrs = AttributeMap.empty.put[F[Headers]](
val attrs = Vault.empty.insert[F[Headers]](

This comment has been minimized.

Copy link
@ChristopherDavenport

ChristopherDavenport Dec 18, 2018

Author Member

Here we insert a value as an arbitrary F defined by the server, but could be read as a different effect type elsewhere. This is a bit worrisome.

This comment has been minimized.

Copy link
@rossabaker

rossabaker Dec 19, 2018

Member

Instead of exposing Message.Keys.TrailerHeaders[F], can we hide it away as a method on Message so that getting and setting can only be in the Request's F?


package object websocket {
private[this] object Keys {
val WebSocket: AttributeKey[Any] = AttributeKey[Any]
val WebSocket: Key[Any] = Key.newKey[IO, Any].unsafeRunSync

This comment has been minimized.

Copy link
@ChristopherDavenport

ChristopherDavenport Dec 18, 2018

Author Member

This is where we can get in trouble with effect type mismatches.


private[this] object Keys {
val PushResponses: AttributeKey[Any] = AttributeKey[Any]
val PushResponses: Key[Any] = Key.newKey[IO, Any].unsafeRunSync

This comment has been minimized.

Copy link
@ChristopherDavenport

ChristopherDavenport Dec 18, 2018

Author Member

Another problematic spot.

@ChristopherDavenport

This comment has been minimized.

Copy link
Member Author

ChristopherDavenport commented Dec 18, 2018

I'm just ecstatic that its such a small diff and it compiled and tested clean.

@rossabaker
Copy link
Member

rossabaker left a comment

So, there's the issue with the trailer headers, which seems independent of Vault vs. AttributeMap. That needs to be cleaned up no matter how we proceed.

A second comment is about Vault itself: much of this diff is changes of method names from those inspired by scala.Map to those that match Data.Vault.Lazy. Data.Vault.Lazy itself uses function names that look like Data.Map. I wonder whether the Scala port would be better using scala.Map names for operations instead of a literal Haskell translation. Make it an idiomatic port rather than a literal port. It would certainly shrink this diff.

Finally, to this PR itself: the keys are always unsafeRunSync()ed in the Vault implementation, and we just changed the names of all the methods. I like Vault, and probably would have happily copied that instead of AttributeMap had it existed during The Bootstrapping. But what do we gain from switching now? The only thing I can think is referential transparency when custom keys are handled in an effect, but we uncovered no internal use cases of that. Is Vault safer or better in any other way?

if (parser.getHttpVersion().minor == 1 && parser.isChunked()) {
val trailers = new AtomicReference(Headers.empty)

val attrs = AttributeMap.empty.put[F[Headers]](
val attrs = Vault.empty.insert[F[Headers]](

This comment has been minimized.

Copy link
@rossabaker

rossabaker Dec 19, 2018

Member

Instead of exposing Message.Keys.TrailerHeaders[F], can we hide it away as a method on Message so that getting and setting can only be in the Request's F?

@@ -29,6 +29,7 @@ lazy val core = libraryProject("core")
fs2Io,
log4s,
parboiled,
vault,

This comment has been minimized.

Copy link
@rossabaker

rossabaker Dec 19, 2018

Member

Inlining this dependency is still the big elephant in the room.


val Destination = AttributeKey[String]
val Destination = Key.newKey[IO, String].unsafeRunSync

This comment has been minimized.

Copy link
@rossabaker

rossabaker Dec 19, 2018

Member

Can we make this private if we're doing the unsafe thing?

@rossabaker

This comment has been minimized.

Copy link
Member

rossabaker commented Dec 20, 2018

I think i give Vault an edge over AttributeMap because, even if we do .unsafeRunSync(), it makes us sign the waiver. It's a slight improvement, but it is an improvement.

@ChristopherDavenport

This comment has been minimized.

Copy link
Member Author

ChristopherDavenport commented Jan 11, 2019

I don't see a need for vault to change, but could make java/scala style methods as well which could reduce additional noise.

I don't mind doing the work here after the other PR's are all in with changes to attribute maps.

As far as the fact that it currently only does it this way, that was intentional. I wanted to sign the waiver to start off with, but later we could make middlewares return tuples with the key to extract the values generated by that middleware etc, which I believe would often be a better pattern. However I thought that the minimal piece was ideal.

As per the library dependency. Utilizing a shared tool means that this can and will translate with other peoples vault use. Where as an internal one means that conversions would be required to work with the library.

I think it has value, but do understand the reticence.

@aeons

aeons approved these changes Feb 12, 2019

@ChristopherDavenport ChristopherDavenport merged commit d01114f into http4s:master Feb 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ChristopherDavenport ChristopherDavenport deleted the ChristopherDavenport:vaultReplacement branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.