Fix descriptor recursion by grouping proto files in reflection builder#60
Fix descriptor recursion by grouping proto files in reflection builder#60zhihuizhang17 wants to merge 3 commits intoelixir-grpc:mainfrom
Conversation
mjheilmann
left a comment
mjheilmann
left a comment
There was a problem hiding this comment.
This looks like a pretty significant amount of changes, I'm not sure I follow all of them and their need
| 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 Enum.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?
2 participants
Add an accumulator (GrpcReflection.Service.Builder.Acc) so the builder gathers all symbols and aliases before emitting a single FileDescriptorProto per actual proto file.