Skip to content

Commit

Permalink
Fixed precedence ordering of proto registry
Browse files Browse the repository at this point in the history
Refer to changelog diff for details.
  • Loading branch information
maoueh committed Jan 22, 2024
1 parent cf53ac7 commit 0bb0a1e
Show file tree
Hide file tree
Showing 11 changed files with 175 additions and 19 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
2 changes: 1 addition & 1 deletion cmd/tools/firehose/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
17 changes: 7 additions & 10 deletions cmd/tools/print/tools_print.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
}
4 changes: 2 additions & 2 deletions jsonencoder/encoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
37 changes: 36 additions & 1 deletion protoregistry/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -19,14 +20,48 @@ 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{},
}
return f
}

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)
Expand Down
98 changes: 98 additions & 0 deletions protoregistry/registry_test.go
Original file line number Diff line number Diff line change
@@ -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]
}
8 changes: 8 additions & 0 deletions protoregistry/testdata/acme/acme.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto3";

package sf.acme.type.v1;

message Block {
string hash = 1;
uint64 num = 2;
}
8 changes: 8 additions & 0 deletions protoregistry/testdata/override_acme/acme.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto3";

package sf.acme.type.v1;

message Block {
string hash_custom = 1;
uint64 num_custom = 2;
}
8 changes: 8 additions & 0 deletions protoregistry/testdata/override_ethereum/ethereum.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto3";

package sf.ethereum.type.v2;

message Block {
bytes hash_custom = 2;
uint64 number_custom = 3;
}
6 changes: 2 additions & 4 deletions protoregistry/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package protoregistry
import (
"fmt"
"os"
"os/user"
"path/filepath"
"strings"

Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion protoregistry/well_known.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 0bb0a1e

Please sign in to comment.