Skip to content

Commit

Permalink
Don't infer TypeURL of ads.Resource with utils.GetTypeURL
Browse files Browse the repository at this point in the history
See comment inside `ads.Resource.TypeURL`, this could otherwise cause a panic
when `ads.Resource[proto.Message]` is used instead of a hard type like
`ads.Resource[*ads.Endpoint]`.
  • Loading branch information
PapaCharlie committed Feb 4, 2025
1 parent 3c5cb8c commit 47593a2
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
8 changes: 6 additions & 2 deletions ads/ads.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,12 @@ func (r *Resource[T]) Marshal() (*RawResource, error) {

// TypeURL returns the underlying resource's type URL.
func (r *Resource[T]) TypeURL() string {
var t T
return types.APITypePrefix + string(t.ProtoReflect().Descriptor().FullName())
// A literal `Resource[proto.Message]` works well when the goal is to store the deserialized
// [RawResource]. However, inferring the type URL cannot be done with [utils.GetTypeURL] since it
// uses reflection on the generic type parameter, which in this case is just [proto.Message], which
// causes a panic. Instead, infer the type directly from the deserialized message, avoiding the panic
// if the type parameter is indeed simply proto.Message.
return types.APITypePrefix + string(r.Resource.ProtoReflect().Descriptor().FullName())
}

// UnmarshalRawResource unmarshals the given RawResource and returns a Resource of the corresponding
Expand Down
19 changes: 19 additions & 0 deletions ads/ads_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package ads

import (
"testing"

types "github.com/envoyproxy/go-control-plane/pkg/resource/v3"
"github.com/linkedin/diderot/internal/utils"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
)

func TestTypeURL(t *testing.T) {
require.Panics(t, func() {
utils.GetTypeURL[proto.Message]()
})

r := NewResource[proto.Message]("foo", "0", new(Endpoint))
require.Equal(t, types.EndpointType, r.TypeURL())
}

0 comments on commit 47593a2

Please sign in to comment.