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

Subsonic API #2321

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@magne4000

magne4000 commented Dec 15, 2016

This PR adds Subsonic API support (version 1.10.1 of the protocol).
With it you can connect with any subsonic client as long as web plugin is enabled (if client ask for login/password, put anything, those are not read): i.e. good android client: D-Sub.

NB: encoding parameters for stream mode are not taken into account, file is streamed as-is.

Implemented methods

Category Methods
System ping getLicense
Browsing getMusicFolders getIndexes getMusicDirectory getArtists getArtist getAlbum getSong
Album/song lists getAlbumList getAlbumList2 getRandomSongs
Searching search2 search3

Unimplemented methods:

All unimplemented methods generate a valid XML/JSON that should be interpreted by any client, so that they can show an appropriate error message without any crash.

Category Methods
Browsing getGenres getVideos getVideoInfo getArtistInfo getArtistInfo2 getAlbumInfo getAlbumInfo2 getSimilarSongs getSimilarSongs2 getTopSongs
Album/song lists getSongsByGenre getNowPlaying getStarred getStarred2
Searching search
Playlists getPlaylists getPlaylist createPlaylist updatePlaylist deletePlaylist
Media retrieval stream download hls getCaptions getCoverArt getLyrics getAvatar
Media annotation star unstar setRating scrobble
Sharing getShares createShare updateShare deleteShare
Podcast getPodcasts getNewestPodcasts refreshPodcasts createPodcastChannel deletePodcastChannel deletePodcastEpisode downloadPodcastEpisode
Jukebox jukeboxControl
Internet radio getInternetRadioStations
Chat getChatMessages addChatMessage
User management getUser getUsers createUser updateUser deleteUser changePassword
Bookmarks getBookmarks createBookmark deleteBookmark getPlayQueue savePlayQueue

@magne4000 magne4000 referenced this pull request Dec 15, 2016

Closed

Web audio #1002

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 16, 2016

Member

Interesting! Any comments about how this works in general? How broadly have you observed its compatibility?

Also, it looks like changes to a few other files got wrapped up in this PR.

Member

sampsyo commented Dec 16, 2016

Interesting! Any comments about how this works in general? How broadly have you observed its compatibility?

Also, it looks like changes to a few other files got wrapped up in this PR.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 16, 2016

I edited the first post.

Some comments on why I had to change others files (feel free to guide me on how I could have done it better):

  • beetsplug/random.py: Patched to be reuseable by subsonic plugin.
  • beetsplug/web/__init__.py: Patched to add subsonic parameter. It uses Flask blueprint.
  • beetsplug/web/subsonic.py: Code for the plugin.
  • docs/plugins/web.rst: Doc updated for new config parameter in web plugin.
  • setup.py and tox.ini: New dev dependency xmlunittest.
  • test/test_web_subsonic.py: Test cases.

Also, pagination is done entirely in python, which is rather inefficient. We'll need to to some work as stated in #750.

magne4000 commented Dec 16, 2016

I edited the first post.

Some comments on why I had to change others files (feel free to guide me on how I could have done it better):

  • beetsplug/random.py: Patched to be reuseable by subsonic plugin.
  • beetsplug/web/__init__.py: Patched to add subsonic parameter. It uses Flask blueprint.
  • beetsplug/web/subsonic.py: Code for the plugin.
  • docs/plugins/web.rst: Doc updated for new config parameter in web plugin.
  • setup.py and tox.ini: New dev dependency xmlunittest.
  • test/test_web_subsonic.py: Test cases.

Also, pagination is done entirely in python, which is rather inefficient. We'll need to to some work as stated in #750.

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 16, 2016

Member

Got it; thanks for elaborating!

The one thing that looks like it might be a problem is the dependency on random.py. Cross-plugin dependencies can lead to plugins getting "accidentally" enabled when the user didn't request them. We may need a different tactic here.

And, to talk more about the long-term picture: I'm really interested in moving toward AURA, our new API that should replace our hacked-together initial API. There's even been some talk about constructing an AURA-to-Subsonic "adapter," more or less exactly like this, to connect to clients that support that API already. Would you be interested in exploring that direction?

Member

sampsyo commented Dec 16, 2016

Got it; thanks for elaborating!

The one thing that looks like it might be a problem is the dependency on random.py. Cross-plugin dependencies can lead to plugins getting "accidentally" enabled when the user didn't request them. We may need a different tactic here.

And, to talk more about the long-term picture: I'm really interested in moving toward AURA, our new API that should replace our hacked-together initial API. There's even been some talk about constructing an AURA-to-Subsonic "adapter," more or less exactly like this, to connect to clients that support that API already. Would you be interested in exploring that direction?

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 19, 2016

I'll move reusable part of random.py outside the beetsplug structure, so it can be reused.

And regarding AURA, here are my remarks:

  • festival also implements its own API (that kinda look like AURA) , but with hindsight, It doesn't have any added value over subsonic (or other) API. So if I would have to change it I would stick to existent APIs (that way, you already have a lot of client to make your test and unit tests)
  • Will AURA API do more than subsonic API ?
  • AURA-to-subsonic API could be a way to do that, but is that necessary ? What are the advantages over this PR ? My opinion is that we should have a sane reusable base (python methods and classes) that we can use to EASILY implement any API, not an API to rule them all 💍.

So the real question is "Do you have a real need for a new API ?"

magne4000 commented Dec 19, 2016

I'll move reusable part of random.py outside the beetsplug structure, so it can be reused.

And regarding AURA, here are my remarks:

  • festival also implements its own API (that kinda look like AURA) , but with hindsight, It doesn't have any added value over subsonic (or other) API. So if I would have to change it I would stick to existent APIs (that way, you already have a lot of client to make your test and unit tests)
  • Will AURA API do more than subsonic API ?
  • AURA-to-subsonic API could be a way to do that, but is that necessary ? What are the advantages over this PR ? My opinion is that we should have a sane reusable base (python methods and classes) that we can use to EASILY implement any API, not an API to rule them all 💍.

So the real question is "Do you have a real need for a new API ?"

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 19, 2016

Also, to fix appveyor build, I would need to modify appveyor.yml file. xmlunittest package requires lxml package to be installed, and it seems to be kinda complicated under windows ... http://help.appveyor.com/discussions/problems/5330-pip-install-lxml-fails-with-missing-symbols

magne4000 commented Dec 19, 2016

Also, to fix appveyor build, I would need to modify appveyor.yml file. xmlunittest package requires lxml package to be installed, and it seems to be kinda complicated under windows ... http://help.appveyor.com/discussions/problems/5330-pip-install-lxml-fails-with-missing-symbols

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 19, 2016

@sampsyo as you see I made some test trying to get lxml build working, without success...
I would like to try another one, but it would require including corresponding .whl files from http://www.lfd.uci.edu/~gohlke/pythonlibs/#lxml into the repository (under test/prereq folder for example). Would you be okay with that ?

magne4000 commented Dec 19, 2016

@sampsyo as you see I made some test trying to get lxml build working, without success...
I would like to try another one, but it would require including corresponding .whl files from http://www.lfd.uci.edu/~gohlke/pythonlibs/#lxml into the repository (under test/prereq folder for example). Would you be okay with that ?

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 20, 2016

Member

OK! Factoring out the random functionality sounds like a fine strategy if we can do it cleanly.

Here's my motivation for focusing on AURA in the future: interoperation. I'd like to be able to unify music software of all stripes, not just beets, so people don't have to invent their own APIs or use the cumbersome ones that already exist. In particular, it should be easy to get new tools—new players, new caching layers, etc.—up and running. That rules out XML in my opinion, as you're discovering with the lxml dependency hell. 😃

I know that's something of a dream that's low on practicality, so I don't object to adding a Subsonic API directly to beets for now. I'm just letting you know that, maybe someday, we might want to supplant it with the more grand vision.


Back to the matter at hand: I think we need to avoid checking other people's code into this repository… maybe we should just mark the tests as skippable if lxml isn't available? That's what we've done for the tests that require GStreamer, for example.

One other design point: might it make sense to make this its own plugin? It doesn't seem to share much with the existing web plugin, except that they both use Flask. It might be simpler to explain and use the different set of requirements for two different plugins.

Member

sampsyo commented Dec 20, 2016

OK! Factoring out the random functionality sounds like a fine strategy if we can do it cleanly.

Here's my motivation for focusing on AURA in the future: interoperation. I'd like to be able to unify music software of all stripes, not just beets, so people don't have to invent their own APIs or use the cumbersome ones that already exist. In particular, it should be easy to get new tools—new players, new caching layers, etc.—up and running. That rules out XML in my opinion, as you're discovering with the lxml dependency hell. 😃

I know that's something of a dream that's low on practicality, so I don't object to adding a Subsonic API directly to beets for now. I'm just letting you know that, maybe someday, we might want to supplant it with the more grand vision.


Back to the matter at hand: I think we need to avoid checking other people's code into this repository… maybe we should just mark the tests as skippable if lxml isn't available? That's what we've done for the tests that require GStreamer, for example.

One other design point: might it make sense to make this its own plugin? It doesn't seem to share much with the existing web plugin, except that they both use Flask. It might be simpler to explain and use the different set of requirements for two different plugins.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 20, 2016

Got it!
I'll try to remove subsonic test from AppVeyor CI.

We can move subsonic to it's own plugin, but that would make beets listen on another port. I think of the web plugin as something that should be extended with web functionalities, without bothering with redundant things as "What port should I use for my 34th web-ish plugin".

magne4000 commented Dec 20, 2016

Got it!
I'll try to remove subsonic test from AppVeyor CI.

We can move subsonic to it's own plugin, but that would make beets listen on another port. I think of the web plugin as something that should be extended with web functionalities, without bothering with redundant things as "What port should I use for my 34th web-ish plugin".

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 20, 2016

Member

Yeah, we'd like to have an extensible server mode—see #718. That direction would allow plugins to extend the web functionality, instead of needing to roll all possible web-related functionality into a single monolithic plugin.

Member

sampsyo commented Dec 20, 2016

Yeah, we'd like to have an extensible server mode—see #718. That direction would allow plugins to extend the web functionality, instead of needing to roll all possible web-related functionality into a single monolithic plugin.

Show outdated Hide outdated appveyor.yml
Show outdated Hide outdated docs/plugins/web.rst
@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 21, 2016

I took a look at #718, the ideas there are really good. But for now we still just have the web plugin all by himself, and I think that, for now, just extending it for further APIs is the way to go (simpler to dev, simpler for users).
If we just add things like a single python file that creates a Flask Blueprint, for each new API, this is really easy to maintain.

magne4000 commented Dec 21, 2016

I took a look at #718, the ideas there are really good. But for now we still just have the web plugin all by himself, and I think that, for now, just extending it for further APIs is the way to go (simpler to dev, simpler for users).
If we just add things like a single python file that creates a Flask Blueprint, for each new API, this is really easy to maintain.

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Dec 21, 2016

Member

OK, cool. I get the convenience argument, and I now see that this doesn't have any extra dependencies beyond what we require for the web plugin itself. (In part because you hand-rolled your own XML serialization! Wow!)

One small argument on the other side is that I imagine the Subsonic API would, in an ideal world, prefer to "live" on a different port (4040?) by default. Putting all the APIs under one roof means that they all get our funky joke port, 8337, by default. That's not the biggest deal in the world, but it wouldn't be the case for separate plugins.

Member

sampsyo commented Dec 21, 2016

OK, cool. I get the convenience argument, and I now see that this doesn't have any extra dependencies beyond what we require for the web plugin itself. (In part because you hand-rolled your own XML serialization! Wow!)

One small argument on the other side is that I imagine the Subsonic API would, in an ideal world, prefer to "live" on a different port (4040?) by default. Putting all the APIs under one roof means that they all get our funky joke port, 8337, by default. That's not the biggest deal in the world, but it wouldn't be the case for separate plugins.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Dec 21, 2016

Regarding subsonic case, I don't think there is a "default" port to use. Other APIs may indeed use such ports, and would require some other Flask App instead of Flask Blueprints.

And you're right, there is no new dependency for this API, only the ones already there for the web plugin.

magne4000 commented Dec 21, 2016

Regarding subsonic case, I don't think there is a "default" port to use. Other APIs may indeed use such ports, and would require some other Flask App instead of Flask Blueprints.

And you're right, there is no new dependency for this API, only the ones already there for the web plugin.

@cobra2

This comment has been minimized.

Show comment
Hide comment
@cobra2

cobra2 Dec 23, 2016

I agree with the statement of there is not a 'default' port to use. Sure subsonic, libresonic, madsonic all point their ports to 4040 by default. However; most of the installs that I've seen and used change those ports to 80/443 or something personal to the user.

cobra2 commented Dec 23, 2016

I agree with the statement of there is not a 'default' port to use. Sure subsonic, libresonic, madsonic all point their ports to 4040 by default. However; most of the installs that I've seen and used change those ports to 80/443 or something personal to the user.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 12, 2017

I moved random functionnalities to a dedicated util module.

magne4000 commented Jan 12, 2017

I moved random functionnalities to a dedicated util module.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 12, 2017

I think that we can consider this PR as mergeable now, tests with real clients are conclusive.

magne4000 commented Jan 12, 2017

I think that we can consider this PR as mergeable now, tests with real clients are conclusive.

@sampsyo

This comment has been minimized.

Show comment
Hide comment
@sampsyo

sampsyo Jan 12, 2017

Member

OK; thank you! I'm still a little dissatisfied with the unfortunate impact the tests have on the Tox config, but I'll keep looking for a solution for this…

Member

sampsyo commented Jan 12, 2017

OK; thank you! I'm still a little dissatisfied with the unfortunate impact the tests have on the Tox config, but I'll keep looking for a solution for this…

@jrobeson

This comment has been minimized.

Show comment
Hide comment
@jrobeson

jrobeson Jan 12, 2017

Contributor

@magne4000 : I don't see the tests being skipped if lxml is missing as per #2321 (comment) Or did i miss it?

Contributor

jrobeson commented Jan 12, 2017

@magne4000 : I don't see the tests being skipped if lxml is missing as per #2321 (comment) Or did i miss it?

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 13, 2017

xmlunittest can't be installed without lxml previously installed, and testing xmlunittest is the real functionnality that we want to test for presence.

magne4000 commented Jan 13, 2017

xmlunittest can't be installed without lxml previously installed, and testing xmlunittest is the real functionnality that we want to test for presence.

@irskep

This comment has been minimized.

Show comment
Hide comment
@irskep

irskep Jan 16, 2017

To future-proof against future web plugin extensibility, perhaps it makes sense to namespace the subsonic API under /subsonic/? (It's possible that it already does this and I just misread, but if so, then the docs should probably explain what the new endpoints are.)

irskep commented Jan 16, 2017

To future-proof against future web plugin extensibility, perhaps it makes sense to namespace the subsonic API under /subsonic/? (It's possible that it already does this and I just misread, but if so, then the docs should probably explain what the new endpoints are.)

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 17, 2017

@irskep subsonic API is defined to run under /rest/ namespace. Some clients won't be able to handle URIs such as /subsonic/rest/.

magne4000 commented Jan 17, 2017

@irskep subsonic API is defined to run under /rest/ namespace. Some clients won't be able to handle URIs such as /subsonic/rest/.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 17, 2017

Moved to dedicated plugin (with corresponding doc).
@sampsyo Eventually, I just added a register_blueprint into web plugin, it's the simplest and most Flask way to do it I presume.

magne4000 commented Jan 17, 2017

Moved to dedicated plugin (with corresponding doc).
@sampsyo Eventually, I just added a register_blueprint into web plugin, it's the simplest and most Flask way to do it I presume.

@irskep

This comment has been minimized.

Show comment
Hide comment
@irskep

irskep Jan 17, 2017

Wow, I don't know about you, but this seems so much better to me.

But reading the code, it occurred to me...why not just import beetsplug.web.app and call app.register_blueprint()? Then there is no more API surface area at all, except that app becomes part of the "public" API of the web plugin rather than just internal.

Sorry for not thinking of this earlier.

irskep commented Jan 17, 2017

Wow, I don't know about you, but this seems so much better to me.

But reading the code, it occurred to me...why not just import beetsplug.web.app and call app.register_blueprint()? Then there is no more API surface area at all, except that app becomes part of the "public" API of the web plugin rather than just internal.

Sorry for not thinking of this earlier.

@magne4000

This comment has been minimized.

Show comment
Hide comment
@magne4000

magne4000 Jan 18, 2017

@irskep that's indeed simpler ! I'll document this in Web plugin then.

magne4000 commented Jan 18, 2017

@irskep that's indeed simpler ! I'll document this in Web plugin then.

@irskep

This comment has been minimized.

Show comment
Hide comment
@irskep

irskep Mar 20, 2017

As a random person who happens to be commenting on your PR, this looks good to me. Nice separation of new plugin, existing web plugin, and core library changes.

irskep commented Mar 20, 2017

As a random person who happens to be commenting on your PR, this looks good to me. Nice separation of new plugin, existing web plugin, and core library changes.

@muff1nman

This comment has been minimized.

Show comment
Hide comment
@muff1nman

muff1nman Apr 22, 2017

@magne4000 which clients do not support arbitrary prefixes? As far as I am aware, you should be able to prefix the rest api at least one level down.

muff1nman commented Apr 22, 2017

@magne4000 which clients do not support arbitrary prefixes? As far as I am aware, you should be able to prefix the rest api at least one level down.

@anarcat

This comment has been minimized.

Show comment
Hide comment
@anarcat

anarcat Jun 29, 2017

Contributor

wow, this is pretty awesome! i think the only thing that needs to be fixed here is to remove the conflicting file, but otherwise - is there a reason why this shouldn't be merged?

Contributor

anarcat commented Jun 29, 2017

wow, this is pretty awesome! i think the only thing that needs to be fixed here is to remove the conflicting file, but otherwise - is there a reason why this shouldn't be merged?

@adamdmoss

This comment has been minimized.

Show comment
Hide comment
@adamdmoss

adamdmoss Jun 29, 2017

Very excited by this so I thought I'd give it a spin!
Using play:Sub on iOS, to log in and get the artist+album count it does this:
GET /rest/ping.view?c=playSub&f=json&p=enc%3A626172&u=fooooo&v=1.9.0 HTTP/1.1
POST /rest/getGenres.view HTTP/1.1
POST /rest/getArtists.view HTTP/1.1

Unfortunately, both POST requests fail with the same traceback:

[2017-06-29 11:34:41,153] ERROR in app: Exception on /rest/getArtists.view [POST]
(or [2017-06-29 11:34:41,231] ERROR in app: Exception on /rest/getIndexes.view [POST])
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/pi/code/adambeets/beetsplug/subsonic.py", line 439, in v_indexes
    letter = artist[0].upper()
IndexError: string index out of range

adamdmoss commented Jun 29, 2017

Very excited by this so I thought I'd give it a spin!
Using play:Sub on iOS, to log in and get the artist+album count it does this:
GET /rest/ping.view?c=playSub&f=json&p=enc%3A626172&u=fooooo&v=1.9.0 HTTP/1.1
POST /rest/getGenres.view HTTP/1.1
POST /rest/getArtists.view HTTP/1.1

Unfortunately, both POST requests fail with the same traceback:

[2017-06-29 11:34:41,153] ERROR in app: Exception on /rest/getArtists.view [POST]
(or [2017-06-29 11:34:41,231] ERROR in app: Exception on /rest/getIndexes.view [POST])
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python2.7/dist-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/pi/code/adambeets/beetsplug/subsonic.py", line 439, in v_indexes
    letter = artist[0].upper()
IndexError: string index out of range
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment