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

Malformed model object stuctures breaks stuff, including MPD server #865

Closed
kingosticks opened this Issue Sep 23, 2014 · 7 comments

Comments

4 participants
@kingosticks
Member

kingosticks commented Sep 23, 2014

If you add add a track to the tracklist that has an an empty string for the album name, the Track instance within Mopidy has no artist.name field. But the MPD translator's track_to_mpd_format function will try to access this non-existent field and raise an exception. This breaks the MPD playlist command and prevents my MPD client (gmpc) from connecting to Mopidy.

Steps to reproduce:

  • Add to the tracklist:
curl -X POST -H Content-Type:application/json -d '{
  "method": "core.tracklist.add",
  "jsonrpc": "2.0",
  "params": [[
{"__model__":"Track","name":"Foo","uri":"local:some:uri","album":{"uri":"x","name":""}}]]
  ,
  "id": 1
}' http://localhost:6680/mopidy/rpc
  • Send MPD command and get a timeout: mpc playlist

Traceback reports "AttributeError: 'dict' object has no attribute 'name'".
Full traceback at http://dpaste.com/0DHKT27

@jodal

This comment has been minimized.

Member

jodal commented Sep 24, 2014

What is happening is that the album dict is missing the __model__ field which tells our JSON deserializer that this is a Mopidy model, so the dict is passed straight through, and we end up with Track(name='Foo', uri='local:some:uri', album={'uri': 'x', 'name': ''}) in the tracklist.

The question is how to guard against this... We probably need to annotate our models with what types are accepted for each field and raise exceptions if the actual types doesn't match our expectations.

@jodal jodal added A-core and removed A-mpd labels Sep 24, 2014

@jodal jodal changed the title from Adding malformed track to the tracklist breaks MPD 'playlist' command to Malformed model object stuctures breaks stuff, including MPD server Sep 24, 2014

@jodal jodal added this to the v1.0 - Core cleanup milestone Sep 24, 2014

@kingosticks

This comment has been minimized.

Member

kingosticks commented Sep 24, 2014

I see, of course you are right. I actually noticed this when using musicbox-webclient so hopefully @woutervanwijk will be able to fix it there also. Oddly, I don't think I've heard of anyone else run into this issue.

@woutervanwijk

This comment has been minimized.

Contributor

woutervanwijk commented Oct 10, 2014

First time for me too. I'll look into that.

@adamcik

This comment has been minimized.

Member

adamcik commented Oct 20, 2014

#700 is also relevant for this, and http://paste.ubuntu.com/8606932/ looks like an other case of this reported on IRC.

@adamcik adamcik modified the milestones: v0.21 - Gapless playback, v1.0 - Core cleanup Mar 13, 2015

@kingosticks

This comment has been minimized.

Member

kingosticks commented Apr 4, 2015

With the 1.0 API, it probably makes most sense to add to tracklist by uri rather than track whenever possible. That should remove the issue in some cases.

@adamcik

This comment has been minimized.

Member

adamcik commented Apr 4, 2015

Oh yes, I really hope so. As I can't really blame people for getting this wrong with the old APIs. Next step after this model validation fixing and more robust backend calls is likely to add type checks on core calls. And hopefully we can at least mostly trust backends to gives us the right types - though coding defensively there might not be a bad idea either.

@jodal

This comment has been minimized.

Member

jodal commented Apr 6, 2015

Fixed by the merge of PR #1105.

@jodal jodal closed this Apr 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment