From 0bb0a1e7ec6ab681c2f6fe693b64e84e160d2487 Mon Sep 17 00:00:00 2001 From: Matthieu Vachon Date: Sun, 21 Jan 2024 23:58:14 -0500 Subject: [PATCH] Fixed precedence ordering of proto registry Refer to changelog diff for details. --- CHANGELOG.md | 4 + cmd/tools/firehose/client.go | 2 +- cmd/tools/print/tools_print.go | 17 ++-- jsonencoder/encoder.go | 4 +- protoregistry/registry.go | 37 ++++++- protoregistry/registry_test.go | 98 +++++++++++++++++++ protoregistry/testdata/acme/acme.proto | 8 ++ .../testdata/override_acme/acme.proto | 8 ++ .../testdata/override_ethereum/ethereum.proto | 8 ++ protoregistry/utils.go | 6 +- protoregistry/well_known.go | 2 +- 11 files changed, 175 insertions(+), 19 deletions(-) create mode 100644 protoregistry/registry_test.go create mode 100644 protoregistry/testdata/acme/acme.proto create mode 100644 protoregistry/testdata/override_acme/acme.proto create mode 100644 protoregistry/testdata/override_ethereum/ethereum.proto diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b74ef3..9e00e06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ If you were at `firehose-core` version `1.0.0` and are bumping to `1.1.0`, you s ## Unreleased +* Tools printing Firehose `Block` model to JSON now have `--proto-paths` take higher precedence over well-known types and even the chain itself, the order is `--proto-paths` > `chain` > `well-known` (so `well-known` is lookup last). + +* The `tools print one-block` now works correctly on blocks generated by omni-chain `firecore` binary. + * The various health endpoint now sets `Content-Type: application/json` header prior sending back their response to the client. * The `firehose`, `substreams-tier1` and `substream-tier2` health endpoint now respects the `common-system-shutdown-signal-delay` configuration value meaning that the health endpoint will return `false` now if `SIGINT` has been received but we are still in the shutdown unready period defined by the config value. If you use some sort of load balancer, you should make sure they are configured to use the health endpoint and you should `common-system-shutdown-signal-delay` to something like `15s`. diff --git a/cmd/tools/firehose/client.go b/cmd/tools/firehose/client.go index 4e0d4ca..5a4e87f 100644 --- a/cmd/tools/firehose/client.go +++ b/cmd/tools/firehose/client.go @@ -89,7 +89,7 @@ func getFirehoseClientE[B firecore.Block](chain *firecore.Chain[B], rootLog *zap }() } - jencoder, err := print.SetupJsonEncoder(cmd) + jencoder, err := print.SetupJsonEncoder(cmd, chain.BlockFactory().ProtoReflect().Descriptor().ParentFile()) if err != nil { return fmt.Errorf("unable to create json encoder: %w", err) } diff --git a/cmd/tools/print/tools_print.go b/cmd/tools/print/tools_print.go index 37726bc..b5b1a57 100644 --- a/cmd/tools/print/tools_print.go +++ b/cmd/tools/print/tools_print.go @@ -29,6 +29,7 @@ import ( "github.com/streamingfast/firehose-core/jsonencoder" "github.com/streamingfast/firehose-core/protoregistry" "github.com/streamingfast/firehose-core/types" + "google.golang.org/protobuf/reflect/protoreflect" ) func NewToolsPrintCmd[B firecore.Block](chain *firecore.Chain[B]) *cobra.Command { @@ -99,7 +100,7 @@ func createToolsPrintMergedBlocksE[B firecore.Block](chain *firecore.Chain[B]) f return err } - jencoder, err := SetupJsonEncoder(cmd) + jencoder, err := SetupJsonEncoder(cmd, chain.BlockFactory().ProtoReflect().Descriptor().ParentFile()) if err != nil { return fmt.Errorf("unable to create json encoder: %w", err) } @@ -136,7 +137,7 @@ func createToolsPrintOneBlockE[B firecore.Block](chain *firecore.Chain[B]) firec printTransactions := sflags.MustGetBool(cmd, "transactions") - jencoder, err := SetupJsonEncoder(cmd) + jencoder, err := SetupJsonEncoder(cmd, chain.BlockFactory().ProtoReflect().Descriptor().ParentFile()) if err != nil { return fmt.Errorf("unable to create json encoder: %w", err) } @@ -262,15 +263,11 @@ func PrintBStreamBlock(b *pbbstream.Block, printTransactions bool, out io.Writer return nil } -func SetupJsonEncoder(cmd *cobra.Command) (*jsonencoder.Encoder, error) { - pbregistry := protoregistry.New() - protoPaths := sflags.MustGetStringSlice(cmd, "proto-paths") - if len(protoPaths) > 0 { - if err := pbregistry.RegisterFiles(protoPaths); err != nil { - return nil, fmt.Errorf("unable to create dynamic printer: %w", err) - } +func SetupJsonEncoder(cmd *cobra.Command, chainFileDescriptor protoreflect.FileDescriptor) (*jsonencoder.Encoder, error) { + pbregistry, err := protoregistry.New(chainFileDescriptor, sflags.MustGetStringSlice(cmd, "proto-paths")...) + if err != nil { + return nil, fmt.Errorf("new registry: %w", err) } - pbregistry.Extends(protoregistry.WellKnownRegistry) return jsonencoder.New(pbregistry), nil } diff --git a/jsonencoder/encoder.go b/jsonencoder/encoder.go index 4b912e1..d357cf3 100644 --- a/jsonencoder/encoder.go +++ b/jsonencoder/encoder.go @@ -13,9 +13,9 @@ type Encoder struct { protoRegistry *protoregistry.Registry } -func New(files *protoregistry.Registry) *Encoder { +func New(registry *protoregistry.Registry) *Encoder { return &Encoder{ - protoRegistry: files, + protoRegistry: registry, } } diff --git a/protoregistry/registry.go b/protoregistry/registry.go index ae024d0..d01c4e1 100644 --- a/protoregistry/registry.go +++ b/protoregistry/registry.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/types/known/anypb" "github.com/jhump/protoreflect/desc" @@ -19,7 +20,37 @@ type Registry struct { filesDescriptors []*desc.FileDescriptor } -func New() *Registry { +// New creates a new Registry first populated with the well-known types +// and then with the proto files passed as arguments. This means the +// precendence of the proto files is higher than the well-known types. +func New(chainFileDescriptor protoreflect.FileDescriptor, protoPaths ...string) (*Registry, error) { + f := NewEmpty() + + // Proto paths have the highest precedence, so we register them first + if len(protoPaths) > 0 { + if err := f.RegisterFiles(protoPaths); err != nil { + return nil, fmt.Errorf("register proto files: %w", err) + } + } + + // Chain file descriptor has the second highest precedence, it always + // override built-in types if defined. + if chainFileDescriptor != nil { + chainFileDesc, err := desc.WrapFile(chainFileDescriptor) + if err != nil { + return nil, fmt.Errorf("wrap file descriptor: %w", err) + } + + f.filesDescriptors = append(f.filesDescriptors, chainFileDesc) + } + + // Last are well known types, they have the lowest precedence + f.Extends(WellKnownRegistry) + + return f, nil +} + +func NewEmpty() *Registry { f := &Registry{ filesDescriptors: []*desc.FileDescriptor{}, } @@ -27,6 +58,10 @@ func New() *Registry { } func (r *Registry) RegisterFiles(files []string) error { + if len(files) == 0 { + return nil + } + fileDescriptors, err := parseProtoFiles(files) if err != nil { return fmt.Errorf("parsing proto files: %w", err) diff --git a/protoregistry/registry_test.go b/protoregistry/registry_test.go new file mode 100644 index 0000000..355a8dd --- /dev/null +++ b/protoregistry/registry_test.go @@ -0,0 +1,98 @@ +package protoregistry + +import ( + "testing" + + "github.com/jhump/protoreflect/desc" + "github.com/jhump/protoreflect/dynamic" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/types/known/anypb" +) + +func TestUnmarshal(t *testing.T) { + acme := readTestProto(t, "testdata/acme") + + type args struct { + registry *Registry + typeURL string + value []byte + } + tests := []struct { + name string + args args + want func(tt *testing.T, out *dynamic.Message) + assertion require.ErrorAssertionFunc + }{ + { + "chain alone", + args{ + registry: mustRegistry(t, acme.UnwrapFile()), + typeURL: "sf.acme.type.v1.Block", + }, + func(tt *testing.T, out *dynamic.Message) { + assert.Equal(tt, "", out.GetFieldByName("hash")) + assert.Equal(tt, uint64(0), out.GetFieldByName("num")) + }, + require.NoError, + }, + { + "overriding built-in chain with proto path", + args{ + registry: mustRegistry(t, acme.UnwrapFile(), "testdata/override_acme"), + typeURL: "sf.acme.type.v1.Block", + }, + func(tt *testing.T, out *dynamic.Message) { + // If you reach this point following a panic in the Go test, the reason there + // is a panic here is because the override_ethereum.proto file is taking + // precedence over the ethereum.proto file, which is not what we want. + assert.Equal(tt, "", out.GetFieldByName("hash_custom")) + assert.Equal(tt, uint64(0), out.GetFieldByName("num_custom")) + }, + require.NoError, + }, + { + "overridding well-know chain (ethereum) with proto path", + args{ + registry: mustRegistry(t, acme.UnwrapFile(), "testdata/override_ethereum"), + typeURL: "sf.ethereum.type.v2.Block", + value: []byte{0x18, 0x0a}, + }, + func(tt *testing.T, out *dynamic.Message) { + // If you reach this point following a panic in the Go test, the reason there + // is a panic here is because the override_ethereum.proto file is taking + // precedence over the ethereum.proto file, which is not what we want. + assert.Equal(tt, uint64(10), out.GetFieldByName("number_custom")) + }, + require.NoError, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + any := &anypb.Any{TypeUrl: "type.googleapis.com/" + tt.args.typeURL, Value: tt.args.value} + out, err := tt.args.registry.Unmarshal(any) + tt.assertion(t, err) + + tt.want(t, out) + }) + } +} + +func mustRegistry(t *testing.T, chainFileDescriptor protoreflect.FileDescriptor, protoPaths ...string) *Registry { + t.Helper() + + reg, err := New(chainFileDescriptor, protoPaths...) + require.NoError(t, err) + + return reg +} + +func readTestProto(t *testing.T, file string) *desc.FileDescriptor { + t.Helper() + + descs, err := parseProtoFiles([]string{file}) + require.NoError(t, err) + + return descs[0] +} diff --git a/protoregistry/testdata/acme/acme.proto b/protoregistry/testdata/acme/acme.proto new file mode 100644 index 0000000..f547b76 --- /dev/null +++ b/protoregistry/testdata/acme/acme.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package sf.acme.type.v1; + +message Block { + string hash = 1; + uint64 num = 2; +} \ No newline at end of file diff --git a/protoregistry/testdata/override_acme/acme.proto b/protoregistry/testdata/override_acme/acme.proto new file mode 100644 index 0000000..4a34bad --- /dev/null +++ b/protoregistry/testdata/override_acme/acme.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package sf.acme.type.v1; + +message Block { + string hash_custom = 1; + uint64 num_custom = 2; +} \ No newline at end of file diff --git a/protoregistry/testdata/override_ethereum/ethereum.proto b/protoregistry/testdata/override_ethereum/ethereum.proto new file mode 100644 index 0000000..519e894 --- /dev/null +++ b/protoregistry/testdata/override_ethereum/ethereum.proto @@ -0,0 +1,8 @@ +syntax = "proto3"; + +package sf.ethereum.type.v2; + +message Block { + bytes hash_custom = 2; + uint64 number_custom = 3; +} \ No newline at end of file diff --git a/protoregistry/utils.go b/protoregistry/utils.go index 7a1c341..034424e 100644 --- a/protoregistry/utils.go +++ b/protoregistry/utils.go @@ -3,7 +3,6 @@ package protoregistry import ( "fmt" "os" - "os/user" "path/filepath" "strings" @@ -12,11 +11,10 @@ import ( ) func parseProtoFiles(importPaths []string) (fds []*desc.FileDescriptor, err error) { - usr, err := user.Current() + userDir, err := os.UserHomeDir() if err != nil { - return nil, fmt.Errorf("getting current user: %w", err) + return nil, fmt.Errorf("get user home dir: %w", err) } - userDir := usr.HomeDir var ip []string for _, importPath := range importPaths { diff --git a/protoregistry/well_known.go b/protoregistry/well_known.go index 75fcf0d..d2b39c1 100644 --- a/protoregistry/well_known.go +++ b/protoregistry/well_known.go @@ -11,7 +11,7 @@ import ( ) -var WellKnownRegistry = New() +var WellKnownRegistry = NewEmpty() func init() { protoFiles := []string{