Allow more special characters in Path

[DieBauer]

I'm using the Methodbuilder and use the passed Name with a custom Namer to do a lookup.

Http.client.methodBuilder(name)

When the path contains characters that are not allowed by Path.showableChars https://github.com/twitter/finagle/blob/develop/finagle-core/src/main/scala/com/twitter/finagle/Path.scala#L59 (for example some OData endpointstyle /endpoint/Method(':param:')) this method crashes with an exception.

Caused by: java.lang.IllegalArgumentException: end of input expected but '(' found at '/endpoint/Method[(]':param:')'
    at com.twitter.finagle.NameTreeParsers.illegal(NameTreeParsers.scala:29)
    at com.twitter.finagle.NameTreeParsers.illegal(NameTreeParsers.scala:36)
    at com.twitter.finagle.NameTreeParsers.ensureEnd(NameTreeParsers.scala:72)
    at com.twitter.finagle.NameTreeParsers.parseAllPath(NameTreeParsers.scala:278)
    at com.twitter.finagle.NameTreeParsers$.parsePath(NameTreeParsers.scala:7)
    at com.twitter.finagle.Path$.read(Path.scala:136)
    at com.twitter.finagle.Name$.apply(Name.scala:198)
    at com.twitter.finagle.BaseResolver.eval(Resolver.scala:169)
    at com.twitter.finagle.http.MethodBuilder$.from(MethodBuilder.scala:32)
    at com.twitter.finagle.Http$Client.methodBuilder(Http.scala:287)

This means that my endpoint cannot be used by the methodBuilder.

I'd like the Path parser to be opened up to more 'showableChars', (for specifically OData endpoints, the parentheses, and the single quote).

[tigerlily-he]

Hi @DieBauer, I'm a Finagle maintainer.

Would you want the list of showable characters to be this?

val showableChars: Seq[Char] =
    ('0' to '9') ++ ('A' to 'Z') ++ ('a' to 'z') ++ "_:.#$%-'()".toSeq

We don't use the OData protocol at Twitter, so we haven't encountered this kind of endpoint name style. That said, we could support additional characters if there aren't any security risks and incompatibility with internal code.

Would you like to make a PR to add the additional characters to try it out?

[DieBauer]

Thanks for the reply @tigerlily-he , I've added the 3 characters in the showableChars variable. Then, some tests start to fail.

This one now goes to green :)

    assert(NameTreeParsers.parsePath("/endpoint/OData('action')") == Path.Utf8("endpoint", "OData('action')"))

However, the manual parser of all NameTrees is too simple to fit right away. Especially, this part is where I think the Parser fails with the new showable characters.

The parsePath keeps on 'eating' the ) while in some cases that's part of the NameTree, and sometimes part of the Path.

a test like this

    assert(
      NameTreeParsers.parseNameTree("1 * /foo & 2 * (2 * /bar)") ==
          NameTree.Union(
            NameTree.Weighted(1d, NameTree.Leaf(Path.Utf8("foo"))),
            NameTree.Weighted(4d, NameTree.Leaf(Path.Utf8("bar")))
          )
    )

now fails with java.lang.IllegalArgumentException: ')' expected but end of input found at '1 * /foo & 2 * (2 * /bar)[]'

while in this case the ) should not be considered part of the Path.

How would you recommend to continue, it seems the NameTreeParser is an optimized parser, and carrying around state might not be the desired approach here.

[hamdiallam]

Depending on the where the path is parsed, perhaps we could make this a stack param so that it's customizable?

[jyanJing]

Hi @DieBauer ,

I think the reason that the test failed after adding () to the showable chars is, () are treated as a special character in the NameTree, and not in Path, but NameTree could include many paths. So if we add the same characters to Path, it will break the logic in NameTree. Unfortunately I couldn't think of an easy fix, maybe as @hamdiallam suggests to add a stack param, but it might also involve going through the logic of NameTree in terms of its relationship with Path. But I could be wrong.

Maybe as a workaround for now, do you think you could massage the path from your end, like having some mapping of OData style path to Finagle supported path, in your example, instead of passing in /endpoint/OData('action'), maybe use /endpoint/OData/action?

[DieBauer]

Thanks for looking into this. I'll try to see if I can put a workaround in place!

[jyanJing]

Thanks @DieBauer , resolving this issue for now, please feel free to reply to the thread or re-open the issue if you have any questions!