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

RequestReader .as support for Cookies #348

Closed
trane opened this Issue Jul 13, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@trane
Contributor

trane commented Jul 13, 2015

DecodeRequest is helpful for String, but String is not the only included RequestReader in Finch. It would be nice to not have to unpack a Cookie value before having access to the implicit String => A conversions.

We can add value classes in the PRequestReader object accounting for Cookie and Option[Cookie] fairly easily.

The DecodeRequest has an apply method with the signature of def apply[A](f: String => Try[A]): DecodeRequest[A]. We can do two different things:

  • Make this more generic (which seems like overkill) def apply[A,B](f: A => Try[B]): DecodeRequest[B]
  • Add another apply with Cookie => Try[A] ... def apply[A](f: Cookie => Try[A]): DecodeRequest[A]

I have a use case for this now, but I'm wondering how useful it would be for Finch in general.

@vkostyukov vkostyukov added the question label Jul 13, 2015

@vkostyukov vkostyukov added this to the Finch 0.8.0 milestone Jul 13, 2015

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Jul 13, 2015

Member

As I understand it all of the cookie's properties except for value are metadata that may affect the selection of the cookie (through the success or failure of the RequestReader), but value is generally the only part we need to "decode". So for example we might have cookie("whatever").should("cookie must be version 1")(_.version == 1) or whatever, but we won't usually need the version during decoding.

What would be a case where you need to look at non-value properties during decoding?

Member

travisbrown commented Jul 13, 2015

As I understand it all of the cookie's properties except for value are metadata that may affect the selection of the cookie (through the success or failure of the RequestReader), but value is generally the only part we need to "decode". So for example we might have cookie("whatever").should("cookie must be version 1")(_.version == 1) or whatever, but we won't usually need the version during decoding.

What would be a case where you need to look at non-value properties during decoding?

@trane

This comment has been minimized.

Show comment
Hide comment
@trane

trane Jul 13, 2015

Contributor

I can't think of a particular use case other than the value. The motivation for this issue was to enable .as directly on the return value of the RequestReader[Cookie], e.g. val foo: RequestReader[Foo] = cookie("foo_cookie").as[Foo].

This makes the code relatively simple in the PRequestReader object, since it is the same as the String ops..

  implicit class CookieReaderOps[R](val rr: PRequestReader[R, Cookie]) extends AnyVal {
    def as[A](implicit decoder: DecodeRequest[A], tag: ClassTag[A]): PRequestReader[R, A] = rr.embedFlatMap { cookie =>
      Future.const(decoder(cookie.value).rescue(notParsed[A](rr, tag)))
    }
  }

  implicit class CookieOptionReaderOps[R](val rr: PRequestReader[R, Option[Cookie]]) extends AnyVal {
    def as[A](implicit decoder: DecodeRequest[A], tag: ClassTag[A]): PRequestReader[R, Option[A]] = rr.embedFlatMap {
      case Some(cookie) => Future.const(decoder(cookie.value).rescue(notParsed[A](rr, tag)) map (Some(_)))
      case None => Future.None
    }
  }

Alternatively, we could treat the Cookie as a special case of String, and just pass cookieInstance.value to the String decoders.

Contributor

trane commented Jul 13, 2015

I can't think of a particular use case other than the value. The motivation for this issue was to enable .as directly on the return value of the RequestReader[Cookie], e.g. val foo: RequestReader[Foo] = cookie("foo_cookie").as[Foo].

This makes the code relatively simple in the PRequestReader object, since it is the same as the String ops..

  implicit class CookieReaderOps[R](val rr: PRequestReader[R, Cookie]) extends AnyVal {
    def as[A](implicit decoder: DecodeRequest[A], tag: ClassTag[A]): PRequestReader[R, A] = rr.embedFlatMap { cookie =>
      Future.const(decoder(cookie.value).rescue(notParsed[A](rr, tag)))
    }
  }

  implicit class CookieOptionReaderOps[R](val rr: PRequestReader[R, Option[Cookie]]) extends AnyVal {
    def as[A](implicit decoder: DecodeRequest[A], tag: ClassTag[A]): PRequestReader[R, Option[A]] = rr.embedFlatMap {
      case Some(cookie) => Future.const(decoder(cookie.value).rescue(notParsed[A](rr, tag)) map (Some(_)))
      case None => Future.None
    }
  }

Alternatively, we could treat the Cookie as a special case of String, and just pass cookieInstance.value to the String decoders.

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Jul 13, 2015

Member

cookie("foo_cookie").map(_.value).as[Cookie] isn't too bad, though.

If we did want a more concise version, I think I'd prefer a new cookieValue request reader over changes to the decoder.

Member

travisbrown commented Jul 13, 2015

cookie("foo_cookie").map(_.value).as[Cookie] isn't too bad, though.

If we did want a more concise version, I think I'd prefer a new cookieValue request reader over changes to the decoder.

@vkostyukov

This comment has been minimized.

Show comment
Hide comment
@vkostyukov

vkostyukov Jul 13, 2015

Member

I would agree with Travis on this. map(_.value).as[Whatever] sounds super reasonable to me.

Member

vkostyukov commented Jul 13, 2015

I would agree with Travis on this. map(_.value).as[Whatever] sounds super reasonable to me.

@trane

This comment has been minimized.

Show comment
Hide comment
@trane

trane Jul 13, 2015

Contributor

Yes, I agree that map isn't too bad. I also think that changing the decoder is overkill, especially since we should be able to treat the main use case of a more terse .as simply as a special case of String.

I suppose that it seems like a helpful shorthand to me, especially since the metadata in a cookie seems more useful as validation than for decoding.

I imagine a helpful api to be something like this...

cookie("foo_cookie").shouldNot(beExpired).as[Foo]

Which is really just shorthand for

cookie("foo_cookie").should("not be expired")(_.maxAge > Duration(???)).map(_.value).as[Foo]
Contributor

trane commented Jul 13, 2015

Yes, I agree that map isn't too bad. I also think that changing the decoder is overkill, especially since we should be able to treat the main use case of a more terse .as simply as a special case of String.

I suppose that it seems like a helpful shorthand to me, especially since the metadata in a cookie seems more useful as validation than for decoding.

I imagine a helpful api to be something like this...

cookie("foo_cookie").shouldNot(beExpired).as[Foo]

Which is really just shorthand for

cookie("foo_cookie").should("not be expired")(_.maxAge > Duration(???)).map(_.value).as[Foo]

@vkostyukov vkostyukov modified the milestones: Finch 0.9.0, Finch 0.8.0 Jul 30, 2015

@vkostyukov vkostyukov added the easy label Mar 14, 2016

@vkostyukov vkostyukov closed this Oct 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment