Add gRPC reflection support by anuraaga · Pull Request #292 · connectrpc/connect-py

anuraaga · 2026-07-06T07:41:44Z

Fixes #206

This adds implementations of gRPC reflection, following the practice of other connect implementations, and even grpc-python actually of accepting the services to return rather than introspecting the running server (which could be a starlette router, etc).

Because protobuf-py allows vendored codegen, I took the approach of just generating the official protos via BSR, and did the same for status.proto too which we previously vendored.

As a quite small file in the end, it feels like overkill to publish a separate pypi package, but it is definitely independent functionality. Open to thoughts.

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

stefanvanburen · 2026-07-06T13:41:11Z

nit: can we mark the _gen/ directory here as linguist-generated in the .gitattributes?

still should do this I think 😄

Sorry forgot git add

stefanvanburen · 2026-07-06T13:54:01Z

+            *descs: The descriptors to make available for reflection.
+
+        Returns:
+            A new instance of [ServerReflectionService], for use with [ServerReflectionASGIApplication].


This says it's for use with ServerReflectionASGIApplication but it's actually used in the 4 subclasses below, maybe could be reworded?

separately, we may need to enable something else in the docs build to linkify these?:

Ah good catch - I think linking to the app is important so took the user-focused, maintainer-challenging approach of just duplicating the four docstrings.

And while I thought the [] form was working fine for us before, I noticed that currently on protobufpy.com it's not working there either. Maybe a zensical change - I guess we'll need to stick to absolute links for robustness. For now did it for this PR and will do a sweep later.

On the bright side, this reminded me to add the protobuf inventory link so we link back to protobufpy.com!

stefanvanburen · 2026-07-06T13:55:46Z

+        else:
+            fds.extend(fd for fd in _file_descriptor_with_dependencies(desc.file, seen))
+            service_names.add(desc.type_name)
+    return Registry(*fds), list(service_names)


should we sort the list of service_names for deterministic results?

stefanvanburen · 2026-07-06T13:56:36Z

+        case Oneof("file_containing_symbol", symbol):
+            desc: DescFile | None = None
+            for d in registry:
+                if isinstance(d, (DescFile, DescExtension)):


why do we skip extensions?

Not sure why I thought to do that, fixed

stefanvanburen · 2026-07-06T13:59:53Z

+            "error_response",
+            ErrorResponse(
+                error_code=_CODE_NOT_FOUND,
+                error_message=f"symbol {req.message_request} not found",


this will leak the Oneof representation of the underlying message_request, should we use req.message_request.value instead?

Just simplified it, the original request is returned anyways

stefanvanburen · 2026-07-06T14:01:06Z

+        # HACK: The v1alpha request type is binary compatible with v1, but will fail to serialize as a submessage
+        # of an unrelated type, so to otherwise allow the rest of the implementation to be shared, we do this
+        # conversion here.
+        original_req = ServerReflectionRequest.from_binary(req.to_binary())


I can't really say I understand this code - is it still valid? I don't see a test that's hitting this branch but maybe I'm missing it. I think this is perhaps already handled in callers?

Indeed, this was from a hacky cast approach which I abandoned since the proper type-safe one wasn't too bad, deleted

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

stefanvanburen

getting closer, just found a couple more things.

re:

it feels like overkill to publish a separate pypi package

I guess it does make me a tad nervous to include it in the base package as breakages could really bite us, and I imagine we'd like to get to connect-py 1.0 somewhat soon after protobuf-py makes it there. doesn't seem like a huge lift to split it out? could be worth considering.

sorry for the late thoughts; appreciate putting up with the rounds of reviews here.

stefanvanburen · 2026-07-07T17:57:00Z

still should do this I think 😄

stefanvanburen · 2026-07-07T18:00:09Z

+    "ServerReflectionAlphaService",
+    "ServerReflectionAlphaServiceSync",
+    "ServerReflectionAlphaWSGIApplication",
+    "ServerReflectionService",


any plan to export ServerReflectionClient et al for client impls (perhaps as a follow-up)? seems like we ought to provide a client w/ similar version negotiation logic to grpcreflect-go

Didn't even think of providing a client since it's always been grpcurl or something. Let's keep this in backlog, I suspect users aren't needing the client

sounds good - I suspect eventually we might want to provide something similar to grpcreflect-go's client, which is a little complex — mostly around the fallback behavior. I'm sure users could just use the generated stuff for one or the other, but might be nice to wrap it ourselves (eventually).

stefanvanburen · 2026-07-07T18:00:39Z

+                if d.type_name == symbol:
+                    desc = d.file


nit: should break after we've found the type

Suggested change

if d.type_name == symbol:

desc = d.file

if d.type_name == symbol:

desc = d.file

break

stefanvanburen · 2026-07-07T18:01:42Z

+    service_names: set[str] = set()
+    for desc in descs:
+        if isinstance(desc, DescFile):
+            fds.extend(fd for fd in _file_descriptor_with_dependencies(desc, seen))


Suggested change

fds.extend(fd for fd in _file_descriptor_with_dependencies(desc, seen))

fds.extend(_file_descriptor_with_dependencies(desc, seen))

stefanvanburen · 2026-07-07T18:01:53Z

+            fds.extend(fd for fd in _file_descriptor_with_dependencies(desc, seen))
+            service_names.update(d.type_name for d in desc.services)
+        else:
+            fds.extend(fd for fd in _file_descriptor_with_dependencies(desc.file, seen))


Suggested change

fds.extend(fd for fd in _file_descriptor_with_dependencies(desc.file, seen))

fds.extend(_file_descriptor_with_dependencies(desc.file, seen))

stefanvanburen · 2026-07-07T18:07:09Z

+        if file.name not in seen:
+            seen.add(file.name)
+            yield file


nit: we've already continue'd above, can dedent here instead

Suggested change

if file.name not in seen:

seen.add(file.name)

yield file

seen.add(file.name)

yield file

stefanvanburen · 2026-07-07T18:07:37Z

+        for dep in file.dependencies:
+            queue.append(dep)


Suggested change

for dep in file.dependencies:

queue.append(dep)

queue.extend(file.dependencies)

stefanvanburen · 2026-07-07T18:13:46Z

+            res.message_response = _create_file_response(desc, seen)
+        case Oneof("file_containing_symbol", symbol):
+            desc: DescFile | None = None
+            for d in registry:


I think this is missing methods, fields, and enumeration values which IIUC should be a part of the grpcreflect "spec". I think we should have test cases for e.g. connectrpc.example.Hat.name and connectrpc.example.Haberdasher.MakeHat, etc.

Something like the following might work?:

def _find_file_for_symbol(registry: Registry, symbol: str) -> DescFile | None: desc = ( registry.message(symbol) or registry.enum(symbol) or registry.service(symbol) or registry.extension(symbol) ) if desc: return desc.file # The symbol may instead name a member of a type: a service method # (pkg.Service.Method), a message field or oneof (pkg.Message.field), or # an enum value, which per proto scoping rules lives in the enum's parent # scope (pkg.VALUE for a top-level enum in package pkg). parent, _, member = symbol.rpartition(".") if not member: return None if svc := registry.service(parent): if any(m.name == member for m in svc.methods): return svc.file return None if (msg := registry.message(parent)) and ( any(f.name == member for f in msg.fields) or any(o.name == member for o in msg.oneofs) ): return msg.file # An enum value of a message-nested enum also has a message as its parent # scope, so always finish with the enum value scan. for d in registry: if ( isinstance(d, DescEnum) and d.type_name.rpartition(".")[0] == parent and any(v.name == member for v in d.values) ): return d.file return None

Thanks for noticing this - I was relying on the Golang doc for FindDescriptorByName, which are no docs 😅 So I looked at the implementation and also the reflection.proto, it seems to indeed include method names and enum values, but not field names so went with that

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga

Thanks - wasn't too worried on stability since the unique API, accepting descriptors, seemed pretty simple. But still, it's probably worth the split, and matches what grpcio does anyways

anuraaga · 2026-07-08T03:26:55Z

Sorry forgot git add

anuraaga · 2026-07-08T03:28:15Z

+    "ServerReflectionAlphaService",
+    "ServerReflectionAlphaServiceSync",
+    "ServerReflectionAlphaWSGIApplication",
+    "ServerReflectionService",


Didn't even think of providing a client since it's always been grpcurl or something. Let's keep this in backlog, I suspect users aren't needing the client

anuraaga · 2026-07-08T03:43:38Z

+            res.message_response = _create_file_response(desc, seen)
+        case Oneof("file_containing_symbol", symbol):
+            desc: DescFile | None = None
+            for d in registry:


Thanks for noticing this - I was relying on the Golang doc for FindDescriptorByName, which are no docs 😅 So I looked at the implementation and also the reflection.proto, it seems to indeed include method names and enum values, but not field names so went with that

anuraaga · 2026-07-08T05:24:34Z

  - local: protoc-gen-connectrpc
    out: .
+  # TODO: Consider extracting a testing package to share haberdasher among packages.
+  - local: protoc-gen-py


This seemed simplest for now

anuraaga · 2026-07-08T05:24:47Z


 [dependency-groups]
 dev = [
+  # Install all packages to be available in docs building, etc.


Found this is probably a good idea

anuraaga · 2026-07-08T05:25:39Z

 ::: connectrpc.compat
+
+
+::: connectrpc_grpcreflect


Probably a good time to split pages but will do that later

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

stefanvanburen

lgtm

stefanvanburen · 2026-07-08T13:06:34Z

@@ -0,0 +1,36 @@
+# connectrpc-grpcreflect
+
+gRPC reflection services to support tools such as [grpcurl](https://github.com/fullstorydev/grpcurl)


nit: I know grpcurl is the O.G., but we may want to at least mention buf curl or other GUI/terminal clients eventually.

stefanvanburen · 2026-07-08T13:07:45Z

+* text=auto eol=lf
+
+src/connectrpc/_gen linguist-generated=true
+conformance/test/gen linguist-generated=true


Suggested change

conformance/test/gen linguist-generated=true

conformance/test/gen linguist-generated=true

connectrpc-grpcreflect/connectrpc_grpcreflect/_gen linguist-generated=true

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga requested a review from a team July 6, 2026 07:41

Add gRPC reflection support

32cdfed

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga force-pushed the grpc-reflection branch from 133c2e1 to 32cdfed Compare July 6, 2026 07:42

stefanvanburen reviewed Jul 6, 2026

View reviewed changes

Cleanup

2e0a4a9

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

stefanvanburen reviewed Jul 7, 2026

View reviewed changes

Separate package

e4bb23f

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga force-pushed the grpc-reflection branch from 659cff1 to e4bb23f Compare July 8, 2026 04:58

fix docs build (maybe)

7164a88

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga commented Jul 8, 2026

View reviewed changes

readme

c682cf8

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga force-pushed the grpc-reflection branch from e9768d5 to c682cf8 Compare July 8, 2026 05:48

version

b84ec4c

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

stefanvanburen approved these changes Jul 8, 2026

View reviewed changes

cleanup

0468696

Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>

anuraaga merged commit 02fe314 into connectrpc:main Jul 9, 2026
23 checks passed

	fds.extend(fd for fd in _file_descriptor_with_dependencies(desc, seen))
	fds.extend(_file_descriptor_with_dependencies(desc, seen))

	for dep in file.dependencies:
	queue.append(dep)
	queue.extend(file.dependencies)

		@@ -0,0 +1,36 @@
		# connectrpc-grpcreflect

		gRPC reflection services to support tools such as [grpcurl](https://github.com/fullstorydev/grpcurl)

	conformance/test/gen linguist-generated=true
	conformance/test/gen linguist-generated=true
	connectrpc-grpcreflect/connectrpc_grpcreflect/_gen linguist-generated=true

Uh oh!

Conversation

anuraaga commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanvanburen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanvanburen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees