Add message filters

[evdokim]

Filters defined on Scalapb GeneratedMessage can be useful to record requests responses in a serialised way (e.g. JSON)

[ccmtaylor]

very cool, thanks!

[ccmtaylor]

this may be easier to express with a foldRight (not sure about where type annotations are necessary):

Suggested change

	val twirpFilter: Filter[Request, Response, Req, Resp] = new TwirpEndpointFilter[Req, Resp]
	val svc = generatedMessageFilters.foldLeft(twirpFilter) { case (accFilter, nextFilter) =>
	accFilter.andThen(nextFilter.toFilter)
	} andThen Service.mk(rpc)
	val init: Service[Req, Resp] = new TwirpEndpointFilter[Req, Resp] andThen Service.mk(rpc)
	val svc = generatedMessageFilters.foldRight(init) { case (messageFilter, accSvc) =>
	messageFilter.toFilter andThen accSvc
	}

[evdokim]

I am not sure that I am following, I think we need to insert message filters between TwirpEndpointFilter and Service.mk(rpc)?

[ccmtaylor]

sorry for the super-late reply! you're right about the positions of the filters. What I was aiming for was building up a Service[Req, Resp] using foldRight instead of building the Filter[Request, Response, Req, Resp]:

      val svc: Service[Req, Resp] = messageFilters.foldRight(Service.mk(rpc)) { case (messageFilter, accSvc) =>
        messageFilter.toFilter[Req, Resp] andThen accSvc
      }
      ProtoRpc(endpointMetadata, new TwirpEndpointFilter[Req, Resp] andThen svc)

That said, if we only keep a single MessageFilter in the builder (and maybe allow combining MessageFilters via andThen in the future), it becomes a moot point :)

[ccmtaylor]

Please note that the Proto* classes are public and thus part of the API of the library. These changes would break backwards compat and force a 2.0 release. It wouldn't be the end of the world, but I'd like to see if we can avoid this so soon after the 1.0 release.

How about, instead of introducing a new ProtoRpcBuilder type, we could keep around the existing ProtoRpc.apply() method, delegating to a new method in object ProtoRpc with three parameters that builds the final ProtoRpc. Does that make sense?

[evdokim]

delegating to a new method in object ProtoRpc with three parameters that builds the final ProtoRpc. Does that make sense?

I tried to do this in the begging, but I couldn't figure our how can we reused the existing method. The problem is that we don't have access to the message filters when we call it here: https://github.com/soundcloud/twinagle/blob/master/codegen/src/main/scala/com/soundcloud/twinagle/codegen/TwinagleServicePrinter.scala#L34

[ccmtaylor]

Shall we remove this change and require using ServerBuilder for users who want to specify MessageFilters? I kinda regret adding the original extension parameter here, but that existed before we publicly exposed ServerBuilder to users. WDYT?

In any case, please move the parameter to the end of the parameter list to keep source compatibility with invocations that don't specify the argument names.

[evdokim]

Makes sense, can you give me an example of a service that uses ServerBuilder, because in all examples that I saw we use .server : https://github.com/soundcloud/tracks/blob/master/src/main/scala/com/soundcloud/tracks/server/App.scala#L18

[evdokim]

@ccmtaylor thank you for polishing up this PR, do you think it is good to go?

[ccmtaylor]

yeah, lgtm :)