Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track.find should handle an invalid Spotify id #67

Closed
joelpresence opened this issue Nov 6, 2015 · 1 comment
Closed

Track.find should handle an invalid Spotify id #67

joelpresence opened this issue Nov 6, 2015 · 1 comment

Comments

@joelpresence
Copy link

Thanks for sharing this cool gem!

t = RSpotify::Track.find %w(2UzMpPKPhbcC8RbsmuURAZ 7Jzsc04YpkRwB1zeyM39wE 44)
NoMethodError: undefined method `[]' for nil:NilClass
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/track.rb:53:in `initialize'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:46:in `new'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:46:in `block in find_many'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:46:in `map'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:46:in `find_many'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:33:in `find'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/track.rb:31:in `find'
    from (irb):21
    from /Users/joel/.rvm/rubies/ruby-2.2.2/bin/irb:11:in `<main>'

RSpotify should not raise a NoMethodError here. Instead, it should handle the case where an invalid id, '44', returns no corresponding track.

Note that the behavior is different here than doing a search on that single bad id 44. For example:

RSpotify::Track.find '4'
RestClient::BadRequest: 400 Bad Request
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/abstract_response.rb:74:in `return!'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/request.rb:495:in `process_result'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/request.rb:421:in `block in transmit'
    from /Users/joel/.rvm/rubies/ruby-2.2.2/lib/ruby/2.2.0/net/http.rb:853:in `start'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/request.rb:413:in `transmit'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/request.rb:176:in `execute'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient/request.rb:41:in `execute'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rest-client-1.8.0/lib/restclient.rb:65:in `get'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/connection.rb:62:in `send_request'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/connection.rb:36:in `block (2 levels) in singleton class'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:54:in `find_one'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/base.rb:36:in `find'
    from /Users/joel/.rvm/gems/ruby-2.2.2/gems/rspotify-1.15.4/lib/rspotify/track.rb:31:in `find'
    from (irb):20
    from /Users/joel/.rvm/rubies/ruby-2.2.2/bin/irb:11:in `<main>'

I would expect RSpotify::Track.find to return nil here instead of raising an exception. However, it looks like the implementation that takes an array of ids doesn't handle the case where some of the ids return no results and it falsely assumes that all results will be valid.

Why would this happen? Suppose we're doing analytics on user track plays. It could happen that we recorded a user playback of track id X and then before we try to query the Spotify API, they delete that track. Or the track id somehow becomes corrupt.

Either way, find should return nil here, not raise an exception.

Thanks. :-)

@guilhermesad
Copy link
Owner

Good catch, thanks for flagging this. There are two different Spotify API endpoints: one for finding a track, one for finding several tracks. When a string is passed to find() the former is used; when an array is passed, the latter is.

If there are invalid ids the first endpoint raises a 400, whereas the second one returns null for each invalid instance. In order to be faithful to the API, I made a change to reflect this accordingly:

If there are invalid ids when passing an array, return nil for each instance. But if you're searching for a single instance, you'll still get the correspondent API response (400).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants