-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec doesn't mention CamelCasing the URL #244
Comments
Great catch. I think this is a bug in the Go implementation. Fixing the bug by removing the camelcasing poses a compatibility challenge. If the generator is changed, then vendored clients won't request the right URLs for updated servers. I think
but that service was broken anyway. |
Now I am more confused. For the proto package simple
service hello {
rpc world(msg) returns(msg);
} Is the desired behaviour:
|
I think the spec points to 1. Use service and method names as is. I think protoc-gen-twirp is in the wrong. If it switches, though, it might cause compatibility problems; accepting both (again, just in protoc-gen-twirp) would soften the blow. |
To be crystal clear: I no longer work at Twitch, and I'm not in charge around here. I haven't even had a chance to write much Go in 6 months. My opinion is just that, my opinion. I think it's also reasonable to change the spec to match the Go implementation. It's just that that sends a funny signal - it indicates that the spec is just a loose human-readable description of the Go implementation, rather than being the concrete definition that the Go implementation should strive to implement. |
I think you're correct in saying that spec should guide implementation. It is difficult/messy as the mux handler use cases will break for server without external code change as This needs discussion with current decision makers I guess. I can raise a PR which moves go from 3 to 2 and ensures backwards compatibility as a start. |
We have a meeting lined up to discuss this issue next week. I'll report back when I have notes from that conversation. |
I'm back with notes! We agree that the language implementations should follow the spec, because otherwise it would no longer be a spec. Given that there's potential for ambiguity, I intend to make a PR to clarify in the spec that the service and method names should be used literally. We should also document that the Go implementation is not currently compatible with the spec in this regard. We spent the bulk of the conversation talking about different approaches to fixing the incompatibility. Here are some brief descriptions of those approaches (its not a full list):
The general consensus was that we should make it clear to users that the Go implementation does not follow the spec, and that it may not be ideal to change the Go implementation's behavior for compatability purposes if there is an alternative. The last item on the list, warning the user about the spec incompatibility, seems like a reasonable middle-ground: it makes the incompatibility clear to users and tells them how to fix it, yet it does not break the Go implementation's current behavior. Of course, warnings are easy to ignore, so that approach has its limitations. Does it seem like we're on the right track? Does the "warn the user" solution seem like a reasonable way to mitigate this issue? |
Yeah, I think warning is a balanced and pragmatic approach here. I think that switching to literal casing in a new major version should be done too - you could do both of those things, right? |
Thanks for taking the time to talk on this internally. I agree with @spenczar with some additions:
Are you against anonymous statistics reporting from Edit: Re-ordered 2 and 3. Improved wording. |
There's no question that the Go implementation is in violation of the spec, but I'm not yet convinced that the impact of the violation is severe enough to warrant a new major version. Adding a warning to It's definitely gross to be in violation of the spec, but it's important to quantify the impact and weigh it against the cost of fixing it. Releasing a new major version means breaking backwards compatability and, critically, requires that users upgrade in order to take security updates. There is a clear path forward that users can take to prevent the issue from affecting them (use the protobuf style guide's naming conventions). It also seems reasonable to assume that this issue affects a fairly small proportion of Twirp users. |
If the impact is small, we can make the implementation spec compatible and deal with the small amount of hate that comes with it. Correct it when the impact is still small? |
@ofpiyush I think we're thinking of two different types of impact. The impact of making any backwards incompatible change to the Go implementation is significant because it requires a major version bump. There's a cost that users must pay to safely upgrade to a new major version, and not upgrading leaves users at risk of not being able to take updates (such as a security fix). The version bump affects all users of the Go implementation. The impact of the Go implementation's spec violation is relatively small because a user would need to be doing cross-language Twirp communication, and the user would need to use a protobuf schema that doesn't follow the protobuf style guide. As mentioned above, it's still possible for these affected users to workaround the faulty behavior of the Go implementation by using a protobuf schema that generates the same casing across language implementations. I hope this distinction doesn't come across as dismissive; my intention is just to make it clear how I'm thinking about the impact and cost that comes with a major version bump. |
First of all, I think we need to resolve the assumptions we're working with. These are mine:
If something doesn't align with the project's assumptions, I'd be happy to change my POV and re-think. Things I do not know and would really help clarity on:
This cost is what I'm trying to gauge here. If very few people get affected and we can provide tooling to upgrade, then it need not be that big an issue. We're working with information gap so maybe reducing that with anonymous reporting might help?
This violates the 4th and 5th assumption I'm working with. I'm seeing this as having a pseudo-spec without explicitly adding it to the spec. Something lurking to be discovered when people start using multiple languages together. Then they'll anyway need to change their proto, which might have been given out to third party integrators. (One of the use cases @verloop was public APIs over twirp. Thankfully, proto definitions were CamelCased already) That IMHO is worse than either extreme. Lack of this unknown hidden behaviour in some implementations was one of the big selling points of twirp over grpc for me. Solutions
Both paths resolve this one way or the other. |
Thanks for discussing this issue constructively, @ofpiyush.
I think some of these assumptions are a bit too broad for the project in general, but I agree with some of them on this specific issue. For instance, if there was a high impact security issue in Twirp that only affected a small number of users, the impact may outweigh the fact that it only affects a small number of users. I think we've agreed above that the spec should guide implementations, that some users are affected by this, and that clarity is important. For item 5, I agree in principle, but clearly it was unintentional that the Go implementation does not follow the spec. Given the fact that simply fixing the issue directly is a breaking change, assessing alternatives seems prudent.
I disagree with you here. I think it's possible for an implementation to have a documented violation of the spec without compromising the integrity of the spec. It doesn't mean the spec is wrong; it just means the Go implementation is wrong. I think there's a distinction there.
There isn't data on how widely Twirp is used today outside of Twitch, and we will not begin collecting that data. The difficulty of the upgrade isn't as concerning as the fact that all users would need to upgrade to continue receiving updates. Within Twitch alone, there are on the order of hundreds of services that use Twirp, and there's a clear organizational cost that comes with performing a major version upgrade. That's not to say that the cost is never worth paying (again, something like a high impact security issue would be worth paying that cost for, in my opinion), but it's safe to say that its significant for the community at large. I don't think you have acknowledged the approach of documenting the violation and making it clear to users that it exists. Do you have specific concerns with that approach? |
I think my agreement with a major version update might have overshadowed the rest of the points in #244 (comment) Please address them individually. |
Hi @ofpiyush, just jumping in coming from a pure library user. I dont really get that approach:
The very first step would still require a new major version update, right? Because "just moving" packages around brings the same BC break. If so why even keep the legacy package? |
This wouldn't break compatibility, the new package could be made the default new user behaviour by updating readme. Effectively containing the issue to existing users, not letting it grow every day as twirp grows popular.
This wouldn't break compatibility either. It will be annoying yes, that is the whole point of doing it. But it will not be breaking in any way. This should be done before removing. So I'm re-ordering it that way now. Thank you for making me rethink the order!
Existing generated code imports other packages and not Given packages in the ecosystem can move things around, people generally do not keep I'd wager that the typical behaviour is to have the binary on local or already baked into an image ready before building. Updating once in a while when someone has the time and energy to do it. This would also be the perfect time for them to read about the nuances and decide the path for themselves! Nevertheless yes, this will have some impact and would break builds, but not existing server and client code. PS: All these plans, even a new major release (if there will be one) will be spread out over months and will need more thought. Right now we're in disagreement on the general plan of approach. Trade offs between maintenance overhead/ existing user impact / long term project health are the major points of conflict right now. We're all working with imperfect data and trying to present the case for each :) |
It seems like you're suggesting that we make a second generator, direct users to it, and then delete the old generator. I think this is ultimately far more painful than it would be to just fix the issue directly and make a major version upgrade. This kind of change would mean that every user of Twirp needs to change how they generate their code. I don't think having multiple implementations of the Go generator in this repo is something we'd consider.
I don't understand this. Twirp will never intentionally break builds for users.
I don't think it would be good for Twirp to just start writing warnings into |
I am trying to find a way to eventually(over months) completely resolve this. I see warnings as a step along the path and not the final solution. I think @spenczar was also hinting at that. The option to make protobuf style guide mandatory is also open and would also lead to complete resolution. Please reconsider the new user experience. They'll come to this repo and see both go and python implementations with warnings in the Readme. As the project grows, not all users will start from the go implementation. Having said that, it has been over a month and I'm out of options and hope. Now I'll focus on questions specific to twirpy and for other implementations.
|
@ofpiyush I met with some internal folks again today to discuss this issue; I'm hoping this can provide some clarity on how you should proceed with twirpy and on what we're able to do mitigate this on the Go implementation side.
Implementations in other languages should follow the spec as its written. It shouldn't be necessary to add warnings to other languages because the issue should only affect Go clients that talk to non-Go servers (more on this below).
I think it makes the most sense to have this limitation documented in this repo, and for other implementations to link to it if questions arise. At this time, we cannot support a major version upgrade to fix this issue. There may be a time where it makes sense to bundle several backwards-incompatible improvements together in a single major version upgrade, but we can't make any guarantees that it will happen. I can certainly empathize with the experience of both Twirp's users and maintainers of other language implementations as a result of this issue, but we are not able to effectively support a major version upgrade at this time. To mitigate the issue, we would support the following:
|
This will need work from the user as well in some cases. 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.
For go client, we can do something like this: var UseLiteralCasing bool = false If someone is using go client with other language servers, they can set it to Both of these changes increase API surface and have trade offs that we can discuss. Adding compatibility on method names on go server is a safe single step along this path so I am sending a PR for that now. |
Twirp has generally avoided configuration of this type, so it's unlikely something like this would be supported. For now, I think the server compatability change and documentation are the right steps for Twirp to take to mitigate this. The door would remain open to potentially changing the Go client in a major version upgrade down the road. Advising affected users to change their protobuf schema (or ultimately changing the behavior in a major version) seems better than adding this sort of configuration into Twirp. |
Just a different perspective. The spec also does not explicitly mention using the service and method names "as is". Furthermore, from the docs in the HTTP Routes:
With the example:
So if anything, one would assume the desired behaviour were to CamelCase the Method and Service portions of URLs. The protobuf style guide suggests:
So the assumption is majority of users are already writing proto files such as:
Which results in: So, my question is
iirc the officially supported implementations already use CamelCasing, so why swing towards an "as-is" implementation? |
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)) @mfridman I've suggested updating spec multiple times on this thread. Thank you for digging out some official word on the matter! @dpolansky I would strongly suggest getting internal stakeholders to talk directly on the threads that affect them. |
@mfridman Although the section on HTTP Routes is more clear about the casing, the spec implies that the names are used literally. As you point out, many Twirp users are already writing protobuf definitions that follow the style guide. A warning could be used to tell users when their definitions are not following the style guide, which would lead to incompatibility across implementations. I don't think we should do anything drastic to mitigate this issue right now, such as changing the spec or making backwards-incompatible changes to the Go implementation. I think we should move forward with allowing literal casing in the Go server implementation and with providing docs for users about how they can address this issue. Over time, if enough users experience pain with this issue, we can progress towards more significant fixes. |
Twirp fundamentally converts the Service definition into an interface that can be implemented in your programming language of choice. Mapping HTTP routes to interfaces and methods. A more generic version of this problem is that different programming languages have different naming conventions and restrictions. For example, a Ruby method cannot begin with a number, and is common to use underscored names. In Go, an exported method name cannot begin with a lowercase letter. But in JavaScript is common for all method names to start with a lowercased letter. Actually, HTTP routes can contain characters that would not map well to most programming languages (https://stackoverflow.com/questions/4669692/). For example:
So, in a way, it is unrealistic for the Twirp spec to say that routes should be handled with the exact values. The routes could be matched, but converting route characters to valid method names in the given language will end up creating routing collisions. A more realistic approach for the spec could be to explicitly allow names that follow the Protobuf best practices (./), and be undefined for other names. This would help to setup expectations when working on new language implementations. |
We would certainly be having that conversation if twirp were generic and independent of protobuf. The language specification of protobuf doesn't allow for names like the example:
https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#identifiers
All languages that one could use twirp in, can do literal case naming following the protobuf language specification if they wish to do so. There is a distinction to be made between wire protocol and method names. It is required for wire protocol to be as specific as possible so that we can work across languages. There is also a distinction to be made between a style guide and language specification. If the project wishes to make the style guide mandatory, that's a very legitimate option. Specifically,
means other languages should follow protobuf style guide. was that the intent? |
@ofpiyush gotcha. No need to worry about other characters in Twirp then. Scratch that generalization I made.
means that Twirp implementations in other languages must guarantee deterministic routing if the names follow the protobuf style guide. That is achievable and works well with the limitations of the Golang implementation. Keeping other cases as undefined should help clarify the possible issues that may happen when using a different naming style. |
Make this gotcha explicit in the spec document and this issue is resolved. 👍 |
With the latest merge, it is all resolved and done! 😌 I learnt a lot about communication through this issue and related PRs, grew as a person. In a lot of places, I could have done much better, I will in the future 😅 I'm glad and very proud of people at twitch that some of my mistakes in personal communication did not overshadow the actual issue and we were eventually able to work past them. Thank you for bearing with me through the long and arduous back and forth, especially @dpolansky. Closing as resolved. |
Hi friends! I realize that the code changes for this issue were resolved quite a while ago. However, I found the GoDoc for To me, the docs would be much clearer if they started with an introductory paragraph like this:
If you'd like, I can open a PR with similar text. |
The way URLs are defined makes one believe the exact name of service is accepted. https://github.com/twitchtv/twirp/blob/03ac4c/docs/spec_v5.md#urls
In practice however both service and method names are changed. https://github.com/twitchtv/twirp/blob/03ac4c/protoc-gen-twirp/generator.go#L956
Should I add this to the spec for clarity and such that others do not get blind-sided by it while implementing?
@khevse discovered it in twirpy
I believe at least the rust, swagger, TypeScript, Dart and php implementations suffer from this.
Aside: The method itself is marked internal API. It might be easier for implementations if it were public https://github.com/twitchtv/twirp/blob/03ac4c/internal/gen/stringutils/stringutils.go#L71
The text was updated successfully, but these errors were encountered: