Skip to content

Commit

Permalink
[split] com.twitter.finagle.Path: don't support empty path elements
Browse files Browse the repository at this point in the history
These complicate the grammar and makes it difficult to represent the
path "/" accurately. The only place I could these in use was for inet
address like

	/$/inet//8080

this is equivalent to

	/$/inet/0/8080

since Java's InetSocketAddress interpretes an empty host argument as
the Internet address 0.

RB_ID=381909
  • Loading branch information
mariusae authored and CI committed Jun 12, 2014
1 parent 9d4e182 commit 64c5bf7
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 34 deletions.
10 changes: 5 additions & 5 deletions finagle-core/src/main/scala/com/twitter/finagle/NameTree.scala
Original file line number Diff line number Diff line change
Expand Up @@ -323,15 +323,15 @@ private trait NameTreePathParsers extends NameTreeParsers {

val byte: Parser[Byte] = escapedByte | showableChar ^^ (_.toByte)

val label: Parser[Buf] = (
val label: Parser[Buf] =
rep1(byte) ^^ {
case bytes => Buf.ByteArray(bytes:_*)
}
| "" ^^^ Buf.Empty
)

val path: Parser[Path] =
rep1("/" ~> label) ^^ { labels => Path(labels:_*) }
val path: Parser[Path] = (
rep1("/" ~> label) ^^ { labels => Path(labels:_*) }
| "/" ^^^ Path.empty
)

val leaf = path
}
Expand Down
9 changes: 8 additions & 1 deletion finagle-core/src/main/scala/com/twitter/finagle/Path.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import java.util.BitSet
* hierarchically-addressed object.
*/
case class Path(elems: Buf*) {
require(elems.forall(Path.nonemptyBuf))

def startsWith(other: Path) = elems startsWith other.elems

def take(n: Int) = Path((elems take n):_*)
Expand Down Expand Up @@ -48,16 +50,19 @@ case class Path(elems: Buf*) {
}
}

lazy val show = showElems map ("/"+_) mkString ""
lazy val show = "/"+(showElems mkString "/")

override def toString = "Path("+(showElems mkString ",")+")"
}

object Path {
private val nonemptyBuf: Buf => Boolean = !_.isEmpty

implicit val showable: Showable[Path] = new Showable[Path] {
def show(path: Path) = path.show
}

val empty = Path()

private val Utf8Charset = Charset.forName("UTF-8")

Expand Down Expand Up @@ -107,12 +112,14 @@ object Path {
*
* {{{
* /foo/bar/baz
* /
* }}}
*
* parses into the path
*
* {{{
* Path(foo,bar,baz)
* Path()
* }}}
*
* @throws IllegalArgumentException when `s` is not a syntactically valid path.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ class SerialClientDispatcher[Req, Rep](trans: Transport[Req, Rep])
}
}.unit


protected def write(req: Req) = trans.write(req)
protected def read(permit: Permit) = trans.read() ensure { permit.release() }
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ class DtabTest extends FunSuite {

test("d1 ++ d2") {
val d1 = Dtab.read("/foo => /bar")
val d2 = Dtab.read("/foo=>/biz;/biz=>/$/inet//8080;/bar=>/$/inet//9090")
val d2 = Dtab.read("/foo=>/biz;/biz=>/$/inet/0/8080;/bar=>/$/inet/0/9090")

assert(d1++d2 === Dtab.read("""
/foo=>/bar;
/foo=>/biz;
/biz=>/$/inet//8080;
/bar=>/$/inet//9090
/biz=>/$/inet/0/8080;
/bar=>/$/inet/0/9090
"""))

def assertEval(dtab: Dtab, path: Path, expect: Name*) {
Expand Down
12 changes: 2 additions & 10 deletions finagle-core/src/test/scala/com/twitter/finagle/NameTreeTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class NameTreeTest extends FunSuite {
"aute", "irure", "dolor", "in", "reprehenderit", "in", "voluptate", "velit", "esse", "cillum",
"dolore", "eu", "fugiat", "nulla", "pariatur", "Excepteur", "sint", "occaecat", "cupidatat",
"non", "proident", "sunt", "in", "culpa", "qui", "officia", "deserunt", "mollit", "anim",
"id", "est", "laborum", "")
"id", "est", "laborum")

def pick[T](xs: Seq[T]): T = xs(rng.nextInt(xs.length))

Expand Down Expand Up @@ -48,8 +48,6 @@ class NameTreeTest extends FunSuite {
}

val leaves = Seq[() => NameTree[Path]](
// Note: atoms don't have a syntactic component yet.
// () => NameTree.Atom(new SocketAddress{}),
() => NameTree.Empty,
() => NameTree.Neg,
() => newPath())
Expand All @@ -72,13 +70,7 @@ class NameTreeTest extends FunSuite {

val trees = Seq.fill(100) { newTree(2) }
for (tree <- trees)
try {
assert(splice(NameTree.read(tree.show)) == splice(tree))
} catch {
case NonFatal(exc) =>
fail("Exception %s while parsing %s: %s; spliced: %s".format(
exc, tree.show, tree, splice(tree)))
}
assert(splice(NameTree.read(tree.show)) === splice(tree))
}

test("NameTree.bind: infinite loop") {
Expand Down
10 changes: 5 additions & 5 deletions finagle-core/src/test/scala/com/twitter/finagle/NamerTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ class NamerTest extends FunSuite {
namer("/test/0").notify(Return(NameTree.read("/test/2")))
assert(res.run.sample() === Activity.Pending)

namer("/test/1").notify(Return(NameTree.read("/$/inet//1")))
namer("/test/1").notify(Return(NameTree.read("/$/inet/0/1")))
assert(res.run.sample() === Activity.Pending)

namer("/test/2").notify(Return(NameTree.read("/$/inet//2")))
namer("/test/2").notify(Return(NameTree.read("/$/inet/0/2")))

assertEval(res, ia(1), ia(2))

Expand Down Expand Up @@ -88,16 +88,16 @@ class NamerTest extends FunSuite {

assert(res.sample().eval === Some(Set.empty))

namer("/test/0").notify(Return(NameTree.read("/$/inet//1")))
namer("/test/0").notify(Return(NameTree.read("/$/inet/0/1")))
assertEval(res, ia(1))

namer("/test/0").notify(Return(NameTree.Neg))
assert(res.sample().eval === None)

namer("/test/2").notify(Return(NameTree.read("/$/inet//2")))
namer("/test/2").notify(Return(NameTree.read("/$/inet/0/2")))
assertEval(res, ia(2))

namer("/test/0").notify(Return(NameTree.read("/$/inet//3")))
namer("/test/0").notify(Return(NameTree.read("/$/inet/0/3")))
assertEval(res, ia(3))
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class BindingFactoryTest extends FunSuite with MockitoSugar with BeforeAndAfter
before {
saveBase = Dtab.base
Dtab.base = Dtab.read("""
/test1010=>/$/inet//1010
/test1010=>/$/inet/0/1010
""")
}

Expand Down Expand Up @@ -79,10 +79,10 @@ class BindingFactoryTest extends FunSuite with MockitoSugar with BeforeAndAfter

test("Caches namers") (new Ctx {

val n1 = Dtab.read("/foo/bar=>/$/inet//1")
val n2 = Dtab.read("/foo/bar=>/$/inet//2")
val n3 = Dtab.read("/foo/bar=>/$/inet//3")
val n4 = Dtab.read("/foo/bar=>/$/inet//4")
val n1 = Dtab.read("/foo/bar=>/$/inet/0/1")
val n2 = Dtab.read("/foo/bar=>/$/inet/0/2")
val n3 = Dtab.read("/foo/bar=>/$/inet/0/3")
val n4 = Dtab.read("/foo/bar=>/$/inet/0/4")

assert(news === 0)
Await.result(newWith(n1).close() before newWith(n1).close())
Expand Down Expand Up @@ -113,10 +113,10 @@ class BindingFactoryTest extends FunSuite with MockitoSugar with BeforeAndAfter
})

test("Caches names") (new Ctx {
val n1 = Dtab.read("/foo/bar=>/$/inet//1; /bar/baz=>/$/nil")
val n2 = Dtab.read("/foo/bar=>/$/inet//1")
val n3 = Dtab.read("/foo/bar=>/$/inet//2")
val n4 = Dtab.read("/foo/bar=>/$/inet//3")
val n1 = Dtab.read("/foo/bar=>/$/inet/0/1; /bar/baz=>/$/nil")
val n2 = Dtab.read("/foo/bar=>/$/inet/0/1")
val n3 = Dtab.read("/foo/bar=>/$/inet/0/2")
val n4 = Dtab.read("/foo/bar=>/$/inet/0/3")

assert(news === 0)
Await.result(newWith(n1).close() before newWith(n1).close())
Expand Down

0 comments on commit 64c5bf7

Please sign in to comment.