tonic-reflection: re-expose v1alpha reflection server builder

[ttkjesper]

While testing the new tonic 0.12 release, we discovered that #1701 upgraded the reflection protocol to v1 from v1alpha.

This breaks compatibility with popular gRPC debugging tools like Postman and Kreya, who only use the v1alpha protocol.

Motivation

Upgrading to 0.12 would potentially introduce a degradation in triaging flows since tools supporting the v1 protocol are limited.

Solution

Following the #1802 PR, this reintroduces the v1alpha protocol in a separate module.

[djc]

I don't think we can semver-compatibly reverse this change as this PR does (nor do I necessarily think that would be the right approach). I'd suggest adding back the v1alpha stuff under a separate name, and adding a changelog entry in the breaking changes section that calls out this change.

[ttkjesper]

That is a fair concern. I've gone through and done the appropriate cleanup to make sure the interface has not changed. The v1alpha module is now published separately, and all relevant traits and structs are now contained within.

I do not believe there should be any breaking change here anymore (the relevant switch from v1alpha -> v1 was already mentioned in the 0.12 breaking changes).

[djc]

The current set of changes makes it too hard for me to review this carefully for semver compatibility. I suggest you make a smaller commit that only moves code around while keeping the public API as is, and a separate commit reintroducing the v1alpha API.

[ttkjesper]

Sure - I've added #1802 to refactor the module layout. When that has been approved and merged we can revisit latter part of the v1alpha support in this one.

[ttkjesper]

I've now updated this PR to contain the appropriate changes.

The diff for the new v1alpha.rs module is as follows:

$ diff -u v1.rs v1alpha.rs
--- v1.rs       2024-07-15 15:53:37.433776000 +0200
+++ v1alpha.rs  2024-07-15 15:54:45.730740900 +0200
@@ -1,8 +1,8 @@
-use crate::pb::v1::server_reflection_server::{ServerReflection, ServerReflectionServer};
+pub use crate::pb::v1alpha::server_reflection_server::{ServerReflection, ServerReflectionServer};

-use crate::pb::v1::server_reflection_request::MessageRequest;
-use crate::pb::v1::server_reflection_response::MessageResponse;
-use crate::pb::v1::{
+use crate::pb::v1alpha::server_reflection_request::MessageRequest;
+use crate::pb::v1alpha::server_reflection_response::MessageResponse;
+use crate::pb::v1alpha::{
     ExtensionNumberResponse, FileDescriptorResponse, ListServiceResponse, ServerReflectionRequest,
     ServerReflectionResponse, ServiceResponse,
 };
@@ -16,7 +16,7 @@

 use crate::server::Error;

-/// A builder used to construct a gRPC Reflection Service (`v1` protocol variant).
+/// A builder used to construct a gRPC Reflection Service (`v1alpha` protocol variant).
 #[derive(Default, Debug)]
 pub struct Builder<'b> {
     file_descriptor_sets: Vec<FileDescriptorSet>,
@@ -75,7 +75,8 @@
     /// Build a gRPC Reflection Service to be served via Tonic.
     pub fn build(mut self) -> Result<ServerReflectionServer<impl ServerReflection>, Error> {
         if self.include_reflection_service {
-            self = self.register_encoded_file_descriptor_set(crate::pb::v1::FILE_DESCRIPTOR_SET);
+            self =
+                self.register_encoded_file_descriptor_set(crate::pb::v1alpha::FILE_DESCRIPTOR_SET);
         }

         let info = crate::server::parser::DescriptorParser::process(

[djc]

These changes still seem unrelated to the idea of adding v1alpha support. I also think they're wrong -- in this case Builder::default() yields a different value than Builder::configure(), which seems potentially surprising.

[ttkjesper]

Ah, you're right! Didn't think of the consequence of adding the derivation - that change was just to reduce some boilerplate with the initialization in the constructor. I've reverted that part of the change.

[djc]

Okay, but there's still a whole bunch of changes with factoring out the parser stuff out of the v1 builder (plus seemingly arbitrary reordering of fields). Again, I'd like to see those in a separate commit or PR. The idea here is to create atomic commits which make it much easier to review the changes (and understand why the changes were made). This is especially when you're both adding a bunch of code (the v1alpha stuff) and moving code around (code that is shared between v1 and v1alpha).

[ttkjesper]

Alright, I've reverted the parser and will be repackaging that into a separate PR so that we can keep the changes self-contained.

[djc]

Sorry for the slow response -- I think I'd prefer to take the extraction of the parser first in a separate PR (or a separate commit within this PR).

[ttkjesper]

No worries! I'll take care of that. :)

[ttkjesper]

The parser has been re-introduced in #1822.

[ttkjesper]

Cleaned up history and rebased towards the lastest master commit.

[oliverjantar]

Hi, is there some timeline when this might be merged?

[djc]

I'm on vacation, will be back next week.

[djc]

I took a deeper look at this and wanted to do a somewhat different approach, resulting in #1888. Please give feedback in that PR!

[flokli]

#1888 has been merged, this can be closed.