panic: reflect: call of reflect.Value.Elem on struct Value

[klokare]

I had a playlist with more than 100 tracks in it so I needed to go to the next page in the returned object. When calling client.NextPage(plist.Tracks), where plist is a *spotify.FullPlaylist, I received the following panic:

panic: reflect: call of reflect.Value.Elem on struct Value

goroutine 1 [running]:
reflect.Value.Elem(0x159f900, 0xc00009c0c0, 0x99, 0xc0000c6138, 0x60, 0xc0004c6000)
        /usr/local/go/src/reflect/value.go:843 +0x1a5
github.com/zmb3/spotify.(*Client).NextPage(0xc000591c80, 0x16a6800, 0xc00009c0c0, 0xc00009c0c0, 0x82)
        /Users/hummerb/.go/pkg/mod/github.com/zmb3/spotify@v0.0.0-20201231194903-e2d01d9b8bd2/page.go:105 +0xbe
jbox/data.EnsurePlaylist(0x16b12a0, 0xc00006cbc0, 0x16b4ba0, 0xc00016cea0, 0xc00009a480, 0x1604125, 0x1b, 0x1, 0x0, 0x0, ...)

The code in NextPage is expecting a reference. I changed the definition of FullPlaylist from

// FullPlaylist provides extra playlist data in addition to the data provided by SimplePlaylist.
type FullPlaylist struct {
    SimplePlaylist
    // The playlist description.  Only returned for modified, verified playlists.
    Description string `json:"description"`
    // Information about the followers of this playlist.
    Followers Followers          `json:"followers"`
    Tracks    PlaylistTrackPage `json:"tracks"`
}

to

// FullPlaylist provides extra playlist data in addition to the data provided by SimplePlaylist.
type FullPlaylist struct {
    SimplePlaylist
    // The playlist description.  Only returned for modified, verified playlists.
    Description string `json:"description"`
    // Information about the followers of this playlist.
    Followers Followers          `json:"followers"`
    Tracks    *PlaylistTrackPage `json:"tracks"`
}

This corrected the problem for me.

[strideynet]

Thanks for raising this. I'll take a look tonight :)

[strideynet]

@klokare Can you please provide an in-context example of you invoking NextPage(), so that I can attempt to reproduce the issue ?

[strideynet]

Unfortunately, I've not been able to reproduce your issue locally when paginating over tracks in a playlist. Have you looked at the example here https://github.com/zmb3/spotify/blob/master/examples/paging/page.go ?

[klokare]

Your example calls client.GetPlaylistTracks which returns a PlaylistTrackPage. I was calling client.GetPlaylist which returns a FullPlaylist which as the following definition

// FullPlaylist provides extra playlist data in addition to the data provided by SimplePlaylist.
type FullPlaylist struct {
    SimplePlaylist
    // The playlist description.  Only returned for modified, verified playlists.
    Description string `json:"description"`
    // Information about the followers of this playlist.
    Followers Followers         `json:"followers"`
    Tracks    PlaylistTrackPage `json:"tracks"`
}

where Tracks does not point to a reference of PlaylistTrackPage.

When I try to next page on the FullPlaylist.Tracks page is when I get the panic. Here is test that demonstrates:

package spotify

import (
    "testing"
    "time"

    "golang.org/x/oauth2"
)

func TestNextPage(t *testing.T) {

    // Set variables
    playlistID := ID("1J5OttmTD3T7HuyXJMYcTl") // Chill Complete public playlist, 137 tracks
    authToken := "YOUR-VALID-AUTH-TOKEN"

    // Create client
    var client Client
    client = getClientFromAuthToken(authToken)

    // Run test with baseline. Should panic due to no reference
    t.Run("baseline-panics", func(t *testing.T) {

        // Catch panic
        defer func() {
            msg := recover()
            if msg != nil {
                t.Error(msg)
            }
        }()

        // Retrieve the playlist
        playlist, err := client.GetPlaylist(playlistID)
        if err != nil {
            t.Error(err)
            return
        }

        // Show the number of records
        t.Logf("playlist has %d tracks", playlist.Tracks.Total)

        // Go to the next page
        if err = client.NextPage(playlist.Tracks); err != nil {
            t.Error(err)
        }
    })

    // Run test with modified stuct. Should not panic
    t.Run("modified-with-reference", func(t *testing.T) {

        // Catch panic
        defer func() {
            msg := recover()
            if msg != nil {
                t.Error(msg)
            }
        }()

        // Retrieve the playlist
        playlist, err := client.GetPlaylist(playlistID)
        if err != nil {
            t.Error(err)
            return
        }

        // Switch to the Playlist with a reference
        altPlay := &FullPlaylistWithReference{
            SimplePlaylist: playlist.SimplePlaylist,
            Description:    playlist.Description,
            Followers:      playlist.Followers,
            Tracks:         &playlist.Tracks,
        }

        // Show the number of records
        t.Logf("playlist has %d tracks", altPlay.Tracks.Total)

        // Go to the next page
        if err = client.NextPage(altPlay.Tracks); err != nil {
            t.Error(err)
        }

    })

}

type FullPlaylistWithReference struct {
    SimplePlaylist
    // The playlist description.  Only returned for modified, verified playlists.
    Description string `json:"description"`
    // Information about the followers of this playlist.
    Followers Followers          `json:"followers"`
    Tracks    *PlaylistTrackPage `json:"tracks"`
}

func getClientFromAuthToken(authToken string) Client {
    scopes := []string{}
    auth := NewAuthenticator("", scopes...)
    auth.SetAuthInfo(authToken, "")
    token := &oauth2.Token{
        AccessToken:  authToken,
        RefreshToken: "",
        Expiry:       time.Now().Add(time.Hour),
    }

    client := auth.NewClient(token)
    client.AutoRetry = true
    return client
}

The referenced playlist in the test has over 100 tracks which is why I needed to go to the next page. It should be a public playlist but, really, any with 101+ tracks will do.

[strideynet]

Thank you for the additional detail. I'll take another look now.

[strideynet]

Interesting, I've been able to reproduce this now.

Whilst your solution works, I am cautious of making a change to a structure that is already in the wild by changing this to a pointer, and breaking any assumptions currently being made by developers and also wary of pointers potentially introducing some nulliness issues here. We already have a v2 in the works, and I'd quite like to rework the pagination as part of that (as the current system using reflection can probably be swapped out for something a bit simpler).

The following (passing in a pointer to the Tracks field) does work:

if err = client.NextPage(&playlist.Tracks); err != nil {
    t.Error(err)
}

So I think adding a note to the v1 godoc with a link to this issue, or a mention of needing to pass in a pointer here is probably the best way forward until a better pagination system is introduced in v2. Does that seem agreeable to you?

[klokare]

Short answer:

Thanks for thinking this through so throughly, @strideynet. Your solution (e.g., client.NextPage(&playlist.Tracks)) works for me. Calling client.GetPlaylistTracks(playlistID) which returns the *PlaylistTrackPage reference, would work, too, though I prefer not to make two calls to the Spotify API when one will do.

I am happy to change my code to utilize the API as it is by passing &playlist.Tracks to client.NextPage. Adding a note to the godoc will certainly be helpful to the rare individual who does what I did.

Longer answer:

I whole heartedly agree that changing the API or its current behaviour is risky and should not be done lightly. Perhaps, though, my intial report is focusing on the wrong portion of the code. The client.NextPage method requires the pageable parameter to be a pointer so it can perform a Set operation.

I believe the issue is really that basePage implements canPage as

// pageable is an internal interface for types that support paging
// by embedding basePage.
type pageable interface{ canPage() }

func (b basePage) canPage() {}

instead of

func (b *basePage) canPage() {}

In other words, pageable.canPage requires a modifable receiver for the client.NextPage(pageable) to succeed (well, not panic).

By changing it to a pointer receiver, my code would not even compile because PlaylistTracks.canPage does not implement func (b *basePage) canPage() {} whereas &playlist.Tracks would.

So all of that to say, it might be a good direction for v2. Thanks for the amazing library. Really saved me a ton of time integrating my app with Spotify.

[strideynet]

Indeed, I was just having a play locally as your comment came through and came to the same conclusion on changing the value receiver to a pointer receiver. This would not, in my mind, actually constitute a breaking change, since nobody could have been relying on passing a non-pointer value in since it would have panicked.

I'm going to open a PR and pass this by the original maintainer. Many thanks for being a very helpful bug reporter!

[klokare]

great minds think alike! your solution works in my code so let's call this issue closed.

[zmb3]

Thanks @klokare for the well written issue and repro, and thanks @strideynet for putting together the fix! Appreciate you both 👍