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

Implement a matrix path variable parser #3844

Merged
merged 6 commits into from Nov 15, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 69 additions & 0 deletions dsl/src/main/scala/org/http4s/dsl/impl/Path.scala
Expand Up @@ -14,6 +14,7 @@ import cats.data.Validated._
import cats.implicits._
import org.http4s._
import scala.util.Try
import cats.Traverse

/** Base class for path extractors. */
trait Path {
Expand Down Expand Up @@ -218,6 +219,74 @@ object LongVar extends PathVar(str => Try(str.toLong))
*/
object UUIDVar extends PathVar(str => Try(java.util.UUID.fromString(str)))

/** Matrix path variable extractor
* For an example see [[https://www.w3.org/DesignIssues/MatrixURIs.html MatrixURIs]]
* This is useful for representing a resource that may be addressed in multiple dimensions where order is unimportant
*
* {{{
*
* object BoardVar extends MatrixVar("square", List("x", "y"), str => Try(str.toInt).toOption)
* Path("/board/square;x=5;y=3") match {
* case Root / "board" / BoardVar(x, y) => ...
* }
* }}}
*/
abstract class MatrixVar[F[_]: Traverse, A](
name: String,
domain: F[String],
range: String => Option[A]) {
Copy link
Member

Choose a reason for hiding this comment

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

In common use, are all parameters in the matrix of the same type?

/address;city=Indianapolis;zipCode=46268

Do I have to represent zipCode as a String? If we stripped range and returned Strings, could we nest extractors?

case Root / AddressVar(city, Zip(zipCode)) =>

Copy link
Contributor Author

@rzeigler rzeigler Nov 12, 2020

Choose a reason for hiding this comment

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

I'm not really sure there is a common use. The problem I was specifically looking for was a means to solve awkward urls like /board/x/0/y/1 which implies a hierarchical structure of resources where none exists. Beyond that the goal is something that more people than just me is an ergonomic API. range is basically a mini-extractor so I will make this change.


private val domainSize = domain.size

def unapplySeq(str: String): Option[Seq[A]] =
if (str.nonEmpty) {
val firstSemi = str.indexOf(';')
if (firstSemi < 0 && (domain.nonEmpty || name != str)) None
else if (firstSemi < 0 && name == str) Some(Seq.empty[A])
// Matrix segment didn't match the expected name
else if (str.substring(0, firstSemi) != name) None
else {
val assocListOpt =
if (firstSemi >= 0) toAssocList(str, firstSemi + 1, List.empty, 0L)
else Some(List.empty[(String, String)])
assocListOpt.flatMap { assocList =>
val located = domain.toList
.traverse(dom => assocList.find(_._1 == dom).map(_._2).flatMap(range))
located <* None.whenA(assocList.length != domain.size)
}
}
} else None

private def toAssocList(
str: String,
position: Int,
accumulated: List[(String, String)],
accumulatedCt: Long): Option[List[(String, String)]] =
if (accumulatedCt >= domainSize) {
None
} else {
val nextSplit = str.indexOf(';', position)
// Empty axis so fail
if (nextSplit == position) None
// We are accumulating the remainder
else if (nextSplit <= 0)
toAssocListElem(str, position, str.length).map(_ :: accumulated)
else
toAssocListElem(str, position, nextSplit)
.flatMap(elem => toAssocList(str, nextSplit + 1, elem :: accumulated, accumulatedCt + 1L))
}

private def toAssocListElem(str: String, position: Int, end: Int): Option[(String, String)] = {
val delimSplit = str.indexOf('=', position)
val nextDelimSplit = str.indexOf('=', delimSplit + 1)
// if the segment does not contain an = inside then it is invalid
if (delimSplit < 0 || delimSplit === position || delimSplit >= end) None
// if the segment contains multiple = then it is invalid
else if (nextDelimSplit < end && nextDelimSplit >= 0) None
else Some(str.substring(position, delimSplit) -> str.substring(delimSplit + 1, end))
}
}

/** Multiple param extractor:
* {{{
* object A extends QueryParamDecoderMatcher[String]("a")
Expand Down
90 changes: 90 additions & 0 deletions dsl/src/test/scala/org/http4s/dsl/PathSpec.scala
Expand Up @@ -15,6 +15,7 @@ import org.http4s.Uri.uri
import org.http4s.dsl.io._
import org.scalacheck.Arbitrary
import org.scalacheck.Arbitrary.arbitrary
import scala.util.Try

class PathSpec extends Http4sSpec {
implicit val arbitraryPath: Arbitrary[Path] =
Expand Down Expand Up @@ -234,6 +235,95 @@ class PathSpec extends Http4sSpec {
}
}

"Matrix extractor" >> {
object BoardExtractor
extends impl.MatrixVar("square", List("x", "y"), s => Try(s.toInt).toOption)

object EmptyExtractor extends impl.MatrixVar[List, String]("square", List.empty, s => Some(s))

"valid" >> {
"a matrix var" in {
(Path("/board/square;x=42;y=0") match {
case Root / "board" / BoardExtractor(x, y) if x == 42 && y == 0 => true
case _ => false
}) must beTrue
}

"a matrix var mid path" in {
(Path("/board/square;x=42;y=0/piece") match {
case Root / "board" / BoardExtractor(x, y) / "piece" if x == 42 && y == 0 => true
case _ => false
}) must beTrue
}

"an empty matrix var but why?" in {

(Path("/board/square") match {
case Root / "board" / EmptyExtractor() => true
case _ => false
}) must beTrue
}
}

"invalid" >> {
"empty with semi" in {
(Path("/board/square;") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"empty without semi" in {
(Path("/board/square") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"empty with mismatched name" in {
(Path("/board/other") match {
case Root / "board" / EmptyExtractor() => true
case _ => false
}) must beFalse
}

"empty axis" in {
(Path("/board/square;;y=0") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"empty too many = in axis" in {
(Path("/board/square;x=42=0;y=9") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"unparseable" in {
(Path("/board/square;x=42;y=soda") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"not enough axes" in {
(Path("/board/square;x=42") match {
case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"to many axes" in {
(Path("/board/square;x=42y=0;z=39") match {
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be x=42;y=0? And why is this invalid? I might have expected extra axes to be ignored.

Copy link
Contributor Author

@rzeigler rzeigler Nov 12, 2020

Choose a reason for hiding this comment

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

The similarity to query parameters probably means that they should be. I just need to make the toAssocList stack safe since its currently capped at the number of parameters that are expected to be extracted.

case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}
}
}

"consistent apply / toList" in prop { (p: Path) =>
Path(p.toList) must_== p
}
Expand Down