"/" not accepted in searchRepos 's query about github4s HOT 8 CLOSED

hey, did you solve the issue in the end?

from github4s.

I have tested with gh.repos.searchRepos("47degrees/github4s", Nil), it doesn't work also.

The specification of searchRepos don't precise if / is accepted in its argument query. I'm not sure that is a bug, or is a expected behavior. So I have close the issue, and replace gh.repos.searchRepos("sterglee/scalalab", Nil) by gh.repos.searchRepos("scalalab", Nil) in my code.

But I feel it is a bug, params = Map("q" -> s"${encode(query, "utf-8")}+${searchParams.map(_.value).mkString("+")}"), this line in the implementation should allow searchRepo to accept / in query.

I reopen the issue.

from github4s.

curl https://api.github.com/search/repositories?q=sterglee%2Fscalalab does work so I agree there is an issue with the lib

from github4s.

I believe I've identified root cause.

RepositoryInterpreter.scala#L77 encodes the query parameters using a java.net.URLEncoder, which converts any /s into a %2F, which is no problem so far. The issue crops up when a second encoding happens at Http4sSyntax.scala#L62 via an org.http4s.Uri encoder, which then transforms the % in the %2F into a %25, transforming the entire section into %252F. The Github API does not know how to unravel multiple layers of encoding like this, and so it most likely treats the latter 2F as literal and part of the search pattern, which results in a successful search but with zero results.

from github4s.

java.net.URLEncoder appears to be redundant since every URL passed to the HttpClient gets encoded by Http4sSyntax.RequestBuilderOps.toUri anyway.

I think the best way to resolve this, therefore, is by stripping out the usages of java.net.URLEncoder and adding some tests to guard against a bug like this getting reintroduced in the future.

Via the changes in my branch, both of these now work:

scala> val searchRepos = gh.repos.searchRepos("47degrees/github4s", Nil)
val searchRepos: cats.effect.IO[github4s.GHResponse[github4s.domain.SearchReposResult]] = IO(...)

scala> searchRepos.unsafeRunSync()
val res0: github4s.GHResponse[github4s.domain.SearchReposResult] = GHResponse(Right(SearchReposResult(1,false,List(Repository(53343599,github4s,47degrees/github4s,User(60125571,47degrees,https://avatars.githubusercontent.com/u/60125571?v=4,https://github.com/47degrees,None,None,None,None,None,None,Some(https://api.github.com/users/47degrees/followers),Some(https://api.github.com/users/47degrees/following{/other_user}),Organization,None,None,None),false,false,false,RepoUrls(https://api.github.com/repos/47degrees/github4s,https://github.com/47degrees/github4s,git://github.com/47degrees/github4s.git,[email protected]:47degrees/github4s.git,https://github.com/47degrees/github4s.git,https://github.com/47degrees/github4s,HashMap(tags_url -> https://api.github.com/repos/...

scala> val searchRepos = gh.repos.searchRepos("repo:47degrees/github4s", Nil)
val searchRepos: cats.effect.IO[github4s.GHResponse[github4s.domain.SearchReposResult]] = IO(...)

scala> searchRepos.unsafeRunSync()
val res1: github4s.GHResponse[github4s.domain.SearchReposResult] = GHResponse(Right(SearchReposResult(1,false,List(Repository(53343599,github4s,47degrees/github4s,User(60125571,47degrees,https://avatars.githubusercontent.com/u/60125571?v=4,https://github.com/47degrees,None,None,None,None,None,None,Some(https://api.github.com/users/47degrees/followers),Some(https://api.github.com/users/47degrees/following{/other_user}),Organization,None,None,None),false,false,false,RepoUrls(https://api.github.com/repos/47degrees/github4s,https://github.com/47degrees/github4s,git://github.com/47degrees/github4s.git,[email protected]:47degrees/github4s.git,https://github.com/47degrees/github4s.git,https://github.com/47degrees/github4s,HashMap(tags_url -> https://api.github.com/repos/...

from github4s.

I think the best way to resolve this, therefore, is by stripping out the usages of java.net.URLEncoder and adding some tests to guard against a bug like this getting reintroduced in the future.

@josiah14 , Is it possible to use the type system of Scala to guard against a bug like this?
For instance

case class NotEncodedQuery(value : String)
val queryString : NotEncodedQuery = ???
val verbatimQuery = URI(Query.unsafeFromString(Uri.encode(queryString.value)))

from github4s.

@dev590t That's an interesting an worthwhile thought. I'm not sure there's a way to ensure the query string is not encoded though without incurring extra runtime overhead since the query string itself is provided by the library consumer. It would make sense to me to ensure we aren't multi-encoding the string within the library itself, but to assume that just because we haven't encoded it yet that the end user hasn't provided an already encoded string may be a bit presumptuous and thus difficult to really wrap into the type system in a way that seems to indicate we know for sure whether something we've received is encoded or not.

@Daenyth Do you have any input on this? I'm hoping I'm wrong and there's a reasonable way to wrap this sort of validation into the type-system without overhead (or with little-enough overhead that it's justified).

from github4s.

I don't think it's worth making the types more complex to fix this; we can just have a unit test in this case

from github4s.