hyperledger / fabric-admin-sdk

Fabric SDK for Admin Capability services
Apache License 2.0
31 stars 19 forks source link

Use non-deprecated protocol buffer APIs in Go implementation #45

Closed bestbeforetoday closed 1 year ago

bestbeforetoday commented 1 year ago

In 2020, a new protocol buffer API was released for Go, and the old protocol buffer API was deprecated:

https://go.dev/blog/protobuf-apiv2

The Fabric protocol buffer bindings published at github.com/hyperledger/fabric-protos-go are based on the deprecated protobuf APIv1. The bindings published at github.com/hyperledger/fabric-protos-go-apiv2 are based on the new protobuf APIv2 and are the ones currently recommended for use by client applications:

https://hyperledger.github.io/fabric-protos/

Since the newer bindings are also used by the Fabric Gateway client API, use of the deprecated bindings could cause compatibility issues for applications that want to make use of both fabric-gateway and fabric-admin-sdk.

bestbeforetoday commented 1 year ago

@SamYuan1990 This task is made extremely difficult due to the use of github.com/hyperledger-twgc/tape/pkg/infra/basic in the end-to-end tests. That API exposes fabric-protos-go in its public API in the following functions (amongst others):

The implementations of these function all involve a call to DialConnection() followed by a trivial call to the gRPC client stub constructor function, so they provide very little value.

Is it practical to remove those helper functions from the hyperledger-twgc/tape API, or to move hyperledger-twgc/tape to use the newer protobuf bindings? Alternatively, it might be necessary to avoid using that package in order to allow adoption of the newer protobuf bindings.

SamYuan1990 commented 1 year ago

@bestbeforetoday , I found that Message define at github.com/golang/protobuf/proto

type Message = protoiface.MessageV1

is different with google.golang.org/protobuf/proto FYI

// Message is the top-level interface that all messages must implement.
// It provides access to a reflective view of a message.
// Any implementation of this interface may be used with all functions in the
// protobuf module that accept a Message, except where otherwise specified.
//
// This is the v2 interface definition for protobuf messages.
// The v1 interface definition is "github.com/golang/protobuf/proto".Message.
//
// To convert a v1 message to a v2 message,
// use "github.com/golang/protobuf/proto".MessageV2.
// To convert a v2 message to a v1 message,
// use "github.com/golang/protobuf/proto".MessageV1.
type Message = protoreflect.ProtoMessage
SamYuan1990 commented 1 year ago

but

Error: internal/configtxlator/update/update.go:13:5: SA1019: "github.com/golang/protobuf/proto" is deprecated: Use the "google.golang.org/protobuf/proto" package instead. (staticcheck)
level=info msg="Memory: 520 samples, avg is 143.8MB, max is 497.2MB"
level=info msg="Execution took 51.901306683s"
    pt "github.com/golang/protobuf/proto"
       ^

it still won't work as the package been marked as deprecated

SamYuan1990 commented 1 year ago

for details https://github.com/Hyperledger-TWGC/fabric-admin-sdk/actions/runs/3498309962/jobs/5858502717

bestbeforetoday commented 1 year ago

The API v2 protocol buffer message interface is "google.golang.org/protobuf/proto".Message. This is different from the API v1 Message interface in "github.com/golang/protobuf/proto".Message. To move to API v2, ideally the code all needs to be updated to use "google.golang.org/protobuf/proto".Message and stop using "github.com/golang/protobuf/proto".Message.

If code really needs to be able to tolerate callers passing both V1 or a V2 message, an option might be to accept the "github.com/golang/protobuf/proto".GeneratedMessage interface and then call "github.com/golang/protobuf/proto".MessageV2() to explicitly convert whatever is passed in to a V2 message for use. These methods will get flagged as using deprecated APIs but if that was kept to a small subset of code (or very specific packages) then they could be marked in ignore rules. I did something similar in fabric-gateway before it migrated entirely to using protobuf APIv2:

https://github.com/hyperledger/fabric-gateway/tree/49b3141587ffeda4c0ea188107e584728f8014f4/pkg/internal/util

SamYuan1990 commented 1 year ago

https://github.com/hyperledger/fabric-gateway/tree/49b3141587ffeda4c0ea188107e584728f8014f4/pkg/internal/util

wait a min. for admin sdk, should we use v2 message directly? did fabric move to v2 message?

bestbeforetoday commented 1 year ago

Fabric did not (yet) move to using fabric-protos-go-apiv2. However, at the wire level, the messages are completely compatible. The client can use fabric-protos-go-apiv2 (and google.golang.org/protobuf) while the server uses the older API. This already happens today with fabric-gateway, which uses the newer API.

The admin SDK should not be using Fabric internal code directly so there should be no coupling between the protobuf API version used by Fabric and the admin SDK.