Fix descriptor recursion by grouping proto files in reflection builder #60
Conversation
| defmodule GrpcReflection.Service.Builder.Acc do | ||
| @moduledoc false | ||
|
|
||
| defstruct services: [], |
There was a problem hiding this comment.
This looks like State, except it externalizes the update/merge operations
| end | ||
|
|
||
| cond do | ||
| String.contains?(base, "/priv/protos/") -> |
There was a problem hiding this comment.
/priv/protos/ and /test/support/protos are magic strings that might only apply to this project and the examples directories.
It is not appropriate to risk leaking users file directory structure if they don't align with one of these. This might need some configuration to identify source proto paths to properly and safely trim from dynamic locations
| "testserviceV2.TestService.proto", | ||
| "testserviceV2.TestService.proto" | ||
| ] | ||
| assert Enum.sort(Enum.uniq(names)) == |
There was a problem hiding this comment.
Is Enu.uniq covering for something?
| alias GrpcReflection.Service.State | ||
|
|
||
| @spec build_reflection_tree(any()) :: | ||
| {:error, <<_::216>>} | {:ok, GrpcReflection.Service.State.t()} |
There was a problem hiding this comment.
{:error, <<_::216>>} is an odd and specific typespec, where is this coming from?
| def trim_symbol("." <> symbol), do: symbol | ||
| def trim_symbol(symbol), do: symbol | ||
|
|
||
| def proto_filename(module) do |
There was a problem hiding this comment.
I don't think this is reliable when there are protobuf options like https://github.com/elixir-protobuf/protobuf?tab=readme-ov-file#one-file-per-module that can be triggered by users. We would need to restrict them from using that option or find an alternative
There was a problem hiding this comment.
You really raised a good point. I will do more investigation about this.
| module.__info__(:compile) | ||
| |> Keyword.get(:source) |
There was a problem hiding this comment.
Is this information normally available in a production build?
Add an accumulator (GrpcReflection.Service.Builder.Acc) so the builder gathers all symbols and aliases before emitting a single FileDescriptorProto per actual proto file.