Pluggable JSON Libraries

[rpless]

A potential solution to #45 (it also increases test coverage to ~81% which benefits #76).
Basically, I removed most of the old json wrapper. Instead Finch users must implement either implicits or filters to convert their preferred Json representation to something that Finch can send in an HTTP response.

As examples, the implicit for the scala 2.10 library is included in the updated docs.
The implicit conversion for argonaut is similar. It basically just involves calling asJson instead of toString inside the implicit (after you have setup the implicit conversions for argonaut, of course).
Jackson is a bit more complicated, but basically involves invoking an object mapper inside the implicit.
Because the nature of the conversion can be very dependent on the nature of the data (depending on library choice), I've left the conversion itself to Finch users to implement.

I would like any thoughts on this PR. I'm not sure its exactly what Finch needs for pluggable json libraries, but I think its a step in the right direction.

[vkostyukov]

Is it possible to get rid of this JsonResponseConverver but having an implicit converter imported? It guess scala compiler will figure out that it has to use an implicit converter and return not JSONObject but Json. Could you please check it?

[vkostyukov]

Thanks @rpless! That is definitely what I had in mind. The only concern from my side is an additional transformation layer. We have to get rid of it and it would be just perfect.

[rpless]

Excellent catch! I changed the return type of the turn function inside step 4 to return a JsonResponse and imported the JsonHelpers object and it works. It completely removes that additional step.

[vkostyukov]

Can we also get rid of this? Just use Json type directly.

[vkostyukov]

Great then! That how I saw it: user imports required implicit converters for his JSON library and lets the magic happen.

[coveralls]

Coverage increased (+23.87%) when pulling 653fe29 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[coveralls]

Coverage increased (+23.87%) when pulling 653fe29 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[coveralls]

Coverage increased (+23.87%) when pulling 653fe29 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[vkostyukov]

There is still a JsonResponse.

[rpless]

Ugh, I though I had gotten them all. Give me a minute to fix.

[coveralls]

Coverage increased (+23.87%) when pulling 8fd762f on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[coveralls]

Coverage increased (+23.87%) when pulling e4e6559 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[vkostyukov]

Cool, thanks! The code looks good to me. The only question left is how we provide those to-Json converters? Do you think we should release them as part of Finch or as a separated projects like finch-jackson, etc. I don't really like the idea heaving a plenty of single-file projects like finch-argonaut but I neither like the idea of depending Finch on 3-5 json libraries.

@travisbrown, could you please review this? I would love to hear your opinion on this idea. I personally like the approach of Json trait and implicit converters (that what I had in mind). But it's not clear for me where to put those converters. We can't force people to write them every time. We at least should provide a support of popular JSON packages.

[coveralls]

Coverage increased (+23.87%) when pulling e4e6559 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[vkostyukov]

@rodrigopr, could you also take a look?

[rodrigopr]

We should keep this endpoint example here (also the User endpoint above).

[vkostyukov]

Nice catch! I missed that. @rpless could you please keep this Ticket endpoint?

[rodrigopr]

Beside the changes in the documentation, it looks good to me too. 👍

@vkostyukov, @rpless, about the json conversion, one thing that i'm testing here is a RequestReader that converts a json (from the request body) to an object (using argonaut).
This RequestReader takes an implicit DecodeJson[A] (argonaut trait) to do the actual decoding.

We could use the same approach on the serialization:

class TurnModelIntoJson[A](implicit encoder: EncodeJson[A]) extends Service[A, Json] {
  def apply(obj: A) = {
    val json = encoder(obj)
    turn(req).toFuture
  }
}

But using new Encode/Decode traits, to don't depend on argonaut project.
Other sub-modules (i.e.: finch-argonaut) could have transformations from the external lib to the finch internal traits.

Is that what you guys are thinking?

[coveralls]

Coverage increased (+23.87%) when pulling 4588272 on rpless:feature/json-abstraction into fd3d193 on finagle:master.

[vkostyukov]

Using a pair of DecodeJson/EncodeJson we can get rid of Json trait itself. I don't see why do we need to keep it. It's just an additional abstraction layer.

let's say EncodeJson looks like:

trait EncodeJson[A] {
  def apply[A](json: A): String
}

Then TurnJsonIntoHttp looks like:

class TurnJsonIntoHttp[A](implicit encode: EncodeJson) extends Service[A, HttpResponse] {
  def apply(req: A) = {
    val json = encode(req) // string
    // wire string
  }
}

I like this approach by the way. The things are getting symmetric. We can user encode/decode for responses/requests. I like it.

What do you think @rpless, @rodrigopr?

[rpless]

Ok, fixed the docs. It looks like I overwrite the wrong example.

But I agree with encode/decode implicit idea. Should we close this PR and start a new one or just keep adding things here?

Also we'll have the same issue with implicit encode/decode that we did with this PR. I don't think Finch should provide them (because of the library dependencies). I think substantial sized converters should be separate repos and small ones (~5 lines) should just be written by the user. (Users of libraries like argonaut have to set up implicits anyway and jackson users have to write domain specific object mappers each time).

[rodrigopr]

I agree @vkostyukov, the json may become unnecessary.
On the examples TurnJsonIntoHttp is applied to the final endpoint. For this case we would have to apply it to each Service (inside the endpoint route), to capture the proper type in the implicit encoder.
Other than that, it should work fine.

Also agree with @rpless, we should close this PR and start a new one.

[vkostyukov]

Why don't you want to keep everything in a one thread?

I agree with Ryan @rpless. The encode/decode idea doesn't answer the question where to put concrete encode/decode implementations. I'm not sure where to put them. It would be great to have everything working out-of-the-box but having a 3-5 json dependencies doesn't sound like a good idea.

[rpless]

If we keep this PR that means we'll have all of the commits from above that we don't want in the repository's history. (Unless we're ok with rebasing this PR before merging it to master, then we can get rid of those commits)

[vkostyukov]

Alright. Let's start a new thread. Thanks guys @rpless, @rodrigopr for all your support. Special kudos for Ryan @rpless who did all the hard work! Ryan are you on Twitter?

[rpless]

@vkostyukov I do now. https://twitter.com/ryan_plessner

[rodrigopr]

@rpless, @vkostyukov, by close I meant merge it if it's good to go. :-)
Think the PR is not a complete solution for #45, but is a great improvement on its own, we can continue the work from it. And use #45 for further discussions.

[vkostyukov]

Agreed. Let's merge it and start from here.

[coveralls]

Coverage increased (+23.87%) when pulling 4588272 on rpless:feature/json-abstraction into fd3d193 on finagle:master.