From 6d7d275ba65b662cd7392749c6bfd3ca5d5a3db3 Mon Sep 17 00:00:00 2001 From: Michael Heilmann Date: Fri, 1 May 2026 13:36:00 -0400 Subject: [PATCH 1/2] improve resolution for deeply nested messages --- lib/grpc_reflection/service/builder.ex | 56 ++++++++++++++++++++------ test/case/nested_messages_test.exs | 6 --- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/lib/grpc_reflection/service/builder.ex b/lib/grpc_reflection/service/builder.ex index 07ace26..c173209 100644 --- a/lib/grpc_reflection/service/builder.ex +++ b/lib/grpc_reflection/service/builder.ex @@ -14,7 +14,10 @@ defmodule GrpcReflection.Service.Builder do new_state = process_service(service) State.merge(state, new_state) end) + # shrink_cycles must run first so the symbol table reflects final merged filenames + # before resolve_dependencies rewrites dep strings through it |> State.shrink_cycles() + |> resolve_dependencies() {:ok, tree} end @@ -91,22 +94,30 @@ defmodule GrpcReflection.Service.Builder do symbol: Enum.find(fields, fn f -> f.name == name end).type_name } end) - |> Enum.reject(fn %{symbol: s} -> is_nil(s) or State.has_symbol?(state, s) end) + |> Enum.reject(fn %{symbol: s} -> is_nil(s) end) |> Enum.reduce(state, fn %{mod: mod, symbol: symbol}, state -> symbol = Util.trim_symbol(symbol) - response = - if symbol in nested_types do - build_response(parent_symbol, module) - else - build_response(symbol, mod) - end + if State.has_symbol?(state, symbol) do + state + else + {file, filename} = + if symbol in nested_types do + # nested types belong in the same file as their ancestor — look it up rather + # than building a new synthetic file (which would have wrong FQDNs) + parent_filename = state.symbols[parent_symbol] + {state.files[parent_filename], parent_filename} + else + response = build_response(symbol, mod) + {response, response.name} + end - state - |> Extensions.add_extensions(symbol, mod) - |> State.add_file(response) - |> State.add_symbol(symbol, response.name) - |> trace_message_refs(symbol, mod) + state + |> Extensions.add_extensions(symbol, mod) + |> State.add_file(file) + |> State.add_symbol(symbol, filename) + |> trace_message_refs(symbol, mod) + end end) end @@ -140,6 +151,27 @@ defmodule GrpcReflection.Service.Builder do end end + # Rewrite each file's dependency list to use actual filenames from the symbol table. + # build_response names deps as `type_symbol <> ".proto"`, but nested types share a file + # with their ancestor, so that filename may not exist. Must run after shrink_cycles so + # merged cycle filenames are already reflected in the symbol table. + defp resolve_dependencies(%State{files: files, symbols: symbols} = state) do + resolved_files = + Map.new(files, fn {filename, descriptor} -> + resolved_deps = + descriptor.dependency + |> Enum.map(fn dep -> + type_symbol = String.trim_trailing(dep, ".proto") + symbols[type_symbol] || dep + end) + |> Enum.uniq() + + {filename, %{descriptor | dependency: resolved_deps}} + end) + + %{state | files: resolved_files} + end + # protoc with the elixir generator and protobuf.generate slightly differ for how they # generate descriptors. Use this to potentially unwrap the service proto when dealing # with descriptors that could come from a service module. diff --git a/test/case/nested_messages_test.exs b/test/case/nested_messages_test.exs index 1c0494d..16a7bfc 100644 --- a/test/case/nested_messages_test.exs +++ b/test/case/nested_messages_test.exs @@ -3,12 +3,6 @@ defmodule GrpcReflection.Case.NestedMessagesTest do use GrpcCase, service: Nested.NestedService.Service - # nested_messages.proto defines two services sharing the same deeply nested types. - # The builder raises a symbol conflict when processing the second traversal of - # OuterMessage.MiddleMessage.InnerMessage via AnotherNestedService. - # Tagged skip until the library supports multi-service files with shared nested types. - @moduletag :skip - versions = ["v1", "v1alpha"] for version <- versions do From b7633fffbbad93131a94494c0c494884a5a1b406 Mon Sep 17 00:00:00 2001 From: Michael Heilmann Date: Fri, 1 May 2026 13:42:22 -0400 Subject: [PATCH 2/2] linting --- lib/grpc_reflection/service/builder.ex | 58 +++++++++++++------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/lib/grpc_reflection/service/builder.ex b/lib/grpc_reflection/service/builder.ex index c173209..1e02232 100644 --- a/lib/grpc_reflection/service/builder.ex +++ b/lib/grpc_reflection/service/builder.ex @@ -79,46 +79,46 @@ defmodule GrpcReflection.Service.Builder do end defp trace_message_fields(state, parent_symbol, module, fields) do - # nested types arent a "separate file", they return their parents' response nested_types = Util.get_nested_types(parent_symbol, module.descriptor()) module.__message_props__().field_props |> Map.values() |> Enum.map(fn %{name: name, type: type} -> - %{ - mod: - case type do - {_, mod} -> mod - mod -> mod - end, - symbol: Enum.find(fields, fn f -> f.name == name end).type_name - } + mod = + case type do + {_, mod} -> mod + mod -> mod + end + + %{mod: mod, symbol: Enum.find(fields, fn f -> f.name == name end).type_name} end) |> Enum.reject(fn %{symbol: s} -> is_nil(s) end) |> Enum.reduce(state, fn %{mod: mod, symbol: symbol}, state -> - symbol = Util.trim_symbol(symbol) + trace_field_ref(state, parent_symbol, Util.trim_symbol(symbol), mod, nested_types) + end) + end - if State.has_symbol?(state, symbol) do - state + defp trace_field_ref(state, _parent_symbol, symbol, _mod, _nested_types) + when is_map_key(state.symbols, symbol), + do: state + + defp trace_field_ref(state, parent_symbol, symbol, mod, nested_types) do + {file, filename} = + if symbol in nested_types do + # nested types belong in the same file as their ancestor — look it up rather + # than building a new synthetic file (which would have wrong FQDNs) + parent_filename = state.symbols[parent_symbol] + {state.files[parent_filename], parent_filename} else - {file, filename} = - if symbol in nested_types do - # nested types belong in the same file as their ancestor — look it up rather - # than building a new synthetic file (which would have wrong FQDNs) - parent_filename = state.symbols[parent_symbol] - {state.files[parent_filename], parent_filename} - else - response = build_response(symbol, mod) - {response, response.name} - end - - state - |> Extensions.add_extensions(symbol, mod) - |> State.add_file(file) - |> State.add_symbol(symbol, filename) - |> trace_message_refs(symbol, mod) + response = build_response(symbol, mod) + {response, response.name} end - end) + + state + |> Extensions.add_extensions(symbol, mod) + |> State.add_file(file) + |> State.add_symbol(symbol, filename) + |> trace_message_refs(symbol, mod) end defp build_response(symbol, module) do