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

Password problem when reusing MPD protocol module #646

Closed
nathanharper opened this issue Jan 17, 2014 · 7 comments
Closed

Password problem when reusing MPD protocol module #646

nathanharper opened this issue Jan 17, 2014 · 7 comments
Assignees
Labels
C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal
Milestone

Comments

@nathanharper
Copy link
Contributor

I've been working on an extension that was basically meant to override and "enhance" the basic MPD frontend, giving the ability to configure what actions users can perform on a playlist, how many requests can be sent from a particular IP, etc. As a proof of concept I made an extension that just overrides the base mpd module, and it worked perfectly except for a problem I ran into with this line:

https://github.com/mopidy/mopidy/blob/develop/mopidy/mpd/protocol/connection.py#L42

I had disabled the mpd module, but wanted to reuse the protocol, and mopidy.mpd.protocol was still referencing the mpd configuration in just that one instance. I was able to get around this by writing a special case to override that particular handler method, but it felt a bit dirty.

My question is: would it make sense to abstract the protocol module a bit more to make it more reusable? Or am I approaching this whole thing the wrong way?

@adamcik
Copy link
Member

adamcik commented Jan 17, 2014

Hmm, as I'm sure you've noticed this code was never written with this type of use in mind :-)

Immediate ideas that come to mind is:

  • the option to unregister handlers, or at least register a new one over an existing one. That way you could replace the one in question.
  • if this is the only place we use the config in the protocol module via context we could move the password to Context(config['mpd']['password']) which would allow you to break that dependence I believe.
  • Our dispatcher code currently has a chain of things to do. Which combined with our recent registry work could mean that we could add hooks to install something in that chain. Essentially equivalent to a WSGI middleware just for our MPD request handling.

#591 might also play in to this as we would likely end up with tokenizing the requests instead of regexp mathcing them, making some of these ideas easier to achieve.

@adamcik
Copy link
Member

adamcik commented Jan 17, 2014

Not sure which one I prefer yet, so last comment is really just thinking aloud to see what might work :-)

@nathanharper
Copy link
Contributor Author

Cool, thanks for the response! For what it's worth, I poked around the module and I think that was the only usage of config, so option 2 might be pretty easy.

@txomon
Copy link
Member

txomon commented Jan 19, 2014

@nathanharper I have done so by using stickers in the mpd frontend, and creating Metadata Provider feature in the core.

The usage would be to use sticker set song service:user:txomon and so. Maybe you are interested on that branch https://github.com/txomon/mopidy/tree/feature/metadata-provider

@nathanharper
Copy link
Contributor Author

Thanks, @txomon . I'm not even familiar with stickers really, but I'll check it out!

@adamcik
Copy link
Member

adamcik commented Jan 24, 2014

Is this solved give the merging of #659?

@nathanharper
Copy link
Contributor Author

Yep! Thanks. I'm closing it.

@jodal jodal added this to the 0.18.2 milestone Feb 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal
Projects
None yet
Development

No branches or pull requests

4 participants