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

added a FormDataDecoder #3641

Merged
merged 8 commits into from Oct 15, 2020
Merged

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Aug 11, 2020

A simple middleware that uses QueryParamDecoder to decode values in [[org.http4s.UrlForm]]. Can be composed to decode nested case class as well as list of elements. More features can be found in the test.

scala> import cats.implicits._
scala> import cats.data._
scala> import org.http4s.server.middleware.FormDataDecoder._
scala> import org.http4s.ParseFailure
scala> case class Foo(a: String, b: Boolean)
scala> case class Bar(fs: List[Foo], f: Foo, d: Boolean)
scala>
scala> implicit val fooMapper = (
     |   field[String]("a"),
     |   field[Boolean]("b")
     | ).mapN(Foo.apply)
scala>
scala> val barMapper = (
     |   list[Foo]("fs"),
     |   nested[Foo]("f"),
     |   field[Boolean]("d")
     | ).mapN(Bar.apply)
scala>
scala> barMapper(
     |   Map(
     |    "fs[].a" -> Chain("a1", "a2"),
     |    "fs[].b" -> Chain("true", "false"),
     |    "f.a" -> Chain("fa"),
     |    "f.b" -> Chain("false"),
     |    "d" -> Chain("true"))
     | )
res1: ValidatedNel[ParseFailure, Bar] = Valid(Bar(List(Foo(a1,true), Foo(a2,false)),Foo(fa,false),true))

@kailuowang
Copy link
Contributor Author

kailuowang commented Aug 11, 2020

build broken on a seemingly unrelated test

[error]   x Insert a User-Agent header (130 ms)
[error]    List() does not contain User-Agent: http4s-blaze/1.0.0-M4+7-c90fc5ed-SNAPSHOT (Http1ClientStageSpec.scala:202)
[error] org.http4s.client.blaze.Http1ClientStageSpec.$anonfun$new$31(Http1ClientStageSpec.scala:202)
[error] Actual:   
[error] Expected: User-Agent: http4s-blaze/1.0.0-M4+7-c90fc5ed-SNAPSHOT

@rossabaker rossabaker added enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch labels Aug 12, 2020
/**
* A simple middle ware that uses [[QueryParamDecoder]] to decode values in [[org.http4s.UrlForm]]
*
* @example
Copy link
Member

@rossabaker rossabaker Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

}

object FormDataDecoder {
type FormData = Map[String, Chain[String]]
Copy link
Member

@rossabaker rossabaker Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're following precedent, but I'm not sure we got this Chain right. It's great for a combine operation, but how often are we combining forms?

Copy link
Contributor Author

@kailuowang kailuowang Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose these simply because UrlForm uses this data type, and the purpose of this class is to be chained after UrlForm so that users can easily write:

    HttpRoutes
     .of[F] {
       case req @ POST -> Root =>
         req.as[MyEntity].flatMap { entity =>
           Ok()
         }
     }

Copy link
Contributor Author

@kailuowang kailuowang Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear. I am open to change it. I can also change it in both UrlForm and here. Let me know.

Copy link
Member

@rossabaker rossabaker Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't change UrlForm in 0.21 because of bincompat, but we should consider it for 1.0.

The question then is whether we should make this like UrlForm in 0.21, or make it look like the future. Curious what other contributors think on this.

Copy link
Contributor

@Daenyth Daenyth Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think consistency within the release is preferable; if users of UrlForm need to change for 1.0, then changing code using this in the same manner shouldn't be a burden


package org.http4s
package server
package middleware
Copy link
Member

@rossabaker rossabaker Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a middleware, which are Http => Http or Client => Client. Would core be a more appropriate place for this, I wonder?

Copy link
Contributor Author

@kailuowang kailuowang Aug 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to core.

Copy link
Member

@rossabaker rossabaker left a comment

This is shaping up nicely. There's a test failure now related to this.

.get(key)
.flatMap(_.headOption)
.toRight(s"$key is missing")
.toRight(s"$key is missing" + data.toString)
Copy link
Member

@rossabaker rossabaker Aug 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this mixes interpolation and +.

Copy link
Contributor Author

@kailuowang kailuowang Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry that was a debugging thing that I forgot to remove.

@kailuowang
Copy link
Contributor Author

kailuowang commented Aug 18, 2020

@rossabaker just made some minor doc improvement. I think I addressed all feedbacks, Is there anything else I can improve?

Copy link
Member

@rossabaker rossabaker left a comment

This looks good to me!

@rossabaker rossabaker added this to the 0.21.8 milestone Aug 23, 2020
@kailuowang
Copy link
Contributor Author

kailuowang commented Sep 2, 2020

@rossabaker do I need to chase another maintainer review for this PR before it can be merged?
BTW, if you have concern over the use of Chain, I will be happy to maintain this feature going forward. To me, this is the missing piece for using http4s for a traditional server-side-rendering website, which I believe is a solid alternative to the current web development trend in many use cases.

@rossabaker
Copy link
Member

rossabaker commented Oct 15, 2020

I apologize profusely for the slow merge. We'll get this into the 0.21.8 release.

@rossabaker rossabaker merged commit 281efe8 into http4s:master Oct 15, 2020
@rossabaker
Copy link
Member

rossabaker commented Oct 15, 2020

Ugh, I meant to cherry-pick this one.

rossabaker pushed a commit that referenced this pull request Oct 15, 2020
@kailuowang kailuowang deleted the form-data-mapper branch Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements retarget Cherry-pick or reopen on another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants