Add support for spec compatible paths in generated server code

[ofpiyush]

Issue #, if available:
#244
Description of changes:

• Adds support for spec compatible paths in generated server code.
• Updates protoc to 3.12.4

No backwards incompatible changes have been made. Specifically:

• Go generated clients haven't been changed and will continue to send requests to the same URL as before.
• Go generated server will continue to serve on the CamelCased URLs it did before.
• <ServiceName>PathPrefix constant is unchanged as of now.
• svc.PathPrefix() still returns the CamelCased value.

Caveat: If someone is using a mux, they'll need to add the literal cased URL for handling manually as the intent was to not add anything to the API surface. For context: start reading from here

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ofpiyush]

protobuf@3.11.0 is not available on brew anymore.

protobuf@3.12.4 didn't seem to break anything so I've updated that in this PR as well.

stdin stdout code in pycompat broke with python 3.8 on my local so I've updated it.

Let me know if I should force install 3.11.0 or change pycompat in any way.

[dpolansky]

The changes to the protoc version and pycompat seem fine. I had some minor feedback about testing, should be a quick change. Thanks for your help with this!

[ofpiyush]

I've moved test to it's own package. Waiting for word on changes for service name and client hook.

I'll copy paste relevant parts from the comments to have context here.

Server

This will need work from the user as well in cases where they're using a mux. We can manage method names without any changes from the user, but service name will need changes from the user.

One possible path which will contain this to 1 user change only, is to add a new method:

example.RegisterHaberdasherServer(mux, handler)

Then we can keep changing code inside that function over releases and eventually into the new major version(if that ever happens) while making sure backwards compatibility is maintained.

Client

The next best option I have is to generate a compatibility client hook and use a map to override URL path like I did on the test here.

User experience could look something like:

client := example.NewHaberdasherProtobufClient(url, client, twirp.WithClientHooks(example.CompatibilityHook))

[dpolansky]

Thanks for the refresher @ofpiyush! I read those comments, but didn't quite understand what you meant with regard to the server-side aspect until now. I think we should proceed with just addressing the method names for now, as addressing the service names will be complex and expands the API surface-area, which we try to be cautious about. Does that sound reasonable to you?

With regard to the compatibility hook, I would be hesitant to recommend it as a general solution to this problem because although it gets the job done, the hooks aren't intended for this purpose and it could lead to confusion. Hooks are generally not intended to mutate requests or responses that significantly. For now, I'd feel better about telling users to change their protobuf definitions if this issue affects them than tell them to use hooks to solve the problem. My opinion is that if this issue is painful enough for many Twirp users, we would then release a new major version with either changes to the spec or changes to the Go implementation.

---

Thanks for moving the test case to a separate package. Once we publish the docs PR, could we just update that comment to direct users there as we discussed previously? Thanks again for addressing the feedback.

[ofpiyush]

I think we should proceed with just addressing the method names for now

We could do slightly better and add code to handle it in non-mux cases and we could drop a line of gotcha of documentation for people who'd use a mux.

The next best option to a new method is to add a new PathPrefix constant.

Mux user experience would look something like:

mux.Handle(example.HaberdasherPathPrefix, twirpHandler)
mux.Handle(example.HaberdasherSomeNamingConventionPathPrefix, twirpHandler)

I see the RegisterHaberDasherServer as a better alternative because that is a one time change for the user. It would open up path for other changes as well like #263 and eventually a smoother transition to v6.

because although it gets the job done, the hooks aren't intended for this purpose and it could lead to confusion

Then I should change my test as well. What alternative would you suggest for the test?

The next best option I have is to do it as a http client wrapper. User experience would look something like:

client := example.NewHaberdasherProtobufClient(url, example.HaberdasherCompatibilityClient(client))

---

For now, I'd feel better about telling users to change their protobuf definitions

I think the fundamental assumption you're working with is that these poorly named services (even in go itself) are not in production or given out to third party.

I come from a world where some APIs are already public, our python servers were live with other python clients before go clients were introduced. We weren't even explicitly following protobuf style guide. We just happened to be fortunate that the random convention I followed isn't incompatible.

I think making that requirement part of the spec would be a much better alternative if adding small amounts of code to the API surface is not acceptable.

[dpolansky]

@ofpiyush I'd like to focus on just making this incremental improvement to handle the method names for now.

Then I should change my test as well. What alternative would you suggest for the test?

The next best option I have is to do it as a http client wrapper. User experience would look something like:

client := example.NewHaberdasherProtobufClient(url, example.HaberdasherCompatibilityClient(client))

I think this is preferable to modifying the request through hooks, so the test could use this method. I'd like to avoid adding additional scope to this PR beyond that though, such as providing the HTTP client implementation in the generated code as you demonstrated here.

[ofpiyush]

I realised that if I don't make urls with literal casing for service names as well then even non-mux cases will not work.

Changes:

• Go generated server now works with complete literal cased routes and go client compatible routes.
• Updated the comment to better reflect the current stand and linked to the documentation.
• Updated tests to use a wrapper on http.Client and now we check for literal cased URLs.

[dpolansky]

@ofpiyush could you squash your commits into a single commit please? the squash-and-merge through github messes with the commit author data unfortunately

@marioizquierdo could you take a look as well?

[marioizquierdo]

On first glance everything looks good. Although it may be useful to think of better variable names and comments to make this non-trivial logic easier to grasp by future contributors. I'll give it a second look tonight, test it locally to make sure everything is good, and think of ways to document this behavior in code.

[ofpiyush]

@dpolansky done.

@marioizquierdo I agree! I tried to improve naming and added a couple of comments. Feel free to send in suggestions. 🙌

[marioizquierdo]

After reviewing more carefully, I understand better what is going on here:

• Go services will handle literal route paths (according to spec) in addition to CamelCased paths (to support the mistake made by early Twirp versions).
• Go clients will not change (for now), they still send requests to CamelCased paths.
• The generated method svc.PathPrefix() will not change, it will still return the CamelCased path.
• The generated constant <Service>PathPrefix will not change, it will still return the CamelCased path.

With this, we want to make a first step for having Twirp handling routes that have Service and Method names that are not CamelCased, what we are calling "literal casing" here. And while we can't just change Go clients to do that in one step, we can start making the transition easier by handling both literal (spec compliant) and CamelCased (Go-clients compliant) in the server 👍

I added some suggestions to make this case more explicit in the code.

[marioizquierdo]

Good job. Thank you for your contribution!

Two minor comments, but it is ready to merge when you are ready. Remember to squash your commits into a single commit please if you can, so Github better keeps your author data when merging into the main branch.

[marioizquierdo]

This is the same as using servName, but it's fine, being explicit may be more readable.

[ofpiyush]

Yes I've been trying to keep strings and go variables separate so that later one can change these easier if necessary.

[marioizquierdo]

This PR will be part of the upcoming v7 release.
@ofpiyush are you happy with all the changes? I can merge this in the release branch and get it ready with all other changes.

[ofpiyush]

Yes. Feel free to merge it.

V7?

[marioizquierdo]

Yes, we are going to skip v6 because there was a previous version draft that was never fully implemented.

[ofpiyush]

This PR can safely be added to V5. Can we add client support in V7 as well?

[marioizquierdo]

Yes, this can safely be added to v5 because it is backwards compatible. And the client support needs to be backwards compatible as well (through a client option, with default value that works like before). I'm including this on next release because it is easier to manage; I'll spend some time this week managing the v7 release and this seems a change worth of mentioning together with the other improvements. If you want, please submit another PR with the client side, so we can bundle it together with the v7 release 🙏 😺 . If you don't have time right now, it's fine, we can handle that part later as well.