scdl-org / scdl

Soundcloud Music Downloader
GNU General Public License v2.0
3.29k stars 331 forks source link

Migrate from `cgi.parse_header` to `email.message.Message` due to PEP 594 #492

Closed es3n1n closed 2 months ago

es3n1n commented 2 months ago

Hey,

Because of the PEP 594 the CGI module would be removed in python 3.13. When importing cgi in python 3.11.4 it already produces a warning, which is very annoying when embedding scdl in some other projects:

In [1]: import cgi
<ipython-input-1-9eb0c0c38b7b>:1: DeprecationWarning: 'cgi' is deprecated and slated for removal in Python 3.13
  import cgi

We can't just use email.message.Message because it doesn't seem to be working well on our stuff.

cgi:

In [17]: cgi.parse_header('''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3''')
Out[17]:
('attachment',
 {'filename': 'SoundCloud%20Download', 'filename*': "utf-8''track.mp3"})

email.message.Message:

In [15]: m['content-type'] = '''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'''

In [16]: dict(m.get_params())
Out[16]: {'attachment': '', 'filename': (None, None, 'utf-8track.mp3')}

As a side note, I tested this via downloading a track from my account and everything seemed to be working well, but unfortunately, I don't think I can add this as a test because I still don't understand how soundcloud decides whether the original mp3 should be allowed to be downloaded or not.

es3n1n commented 2 months ago

There are also a few warnings and issues with the used library clint, I will take a look at them if/when this PR is merged.

7x11x13 commented 2 months ago

The email.message.Message method seems to work well for me (tested on Python 3.11.6 and 3.7.9):

>>> from email.message import Message
>>> m = Message()
>>> m["content-type"] = '''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'''
>>> dict(m.get_params())
{'attachment': '', 'filename': ('utf-8', '', 'track.mp3')}
7x11x13 commented 2 months ago

I still don't understand how soundcloud decides whether the original mp3 should be allowed to be downloaded or not.

In the track settings on SoundCloud you can enable downloads which makes the original file available. Unfortunately for free users the download limit is capped at 100. I'm working on getting a test track hosted by someone with SoundCloud Next Pro so we can re-enable the original download tests.

es3n1n commented 2 months ago

The email.message.Message method seems to work well for me (tested on Python 3.11.6 and 3.7.9):

>>> from email.message import Message
>>> m = Message()
>>> m["content-type"] = '''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'''
>>> dict(m.get_params())
{'attachment': '', 'filename': ('utf-8', '', 'track.mp3')}

Good catch, I tested this a few more times with a clean interpreter and it seems like my previous tests were broken because of the other attributes I set for this message or something like that, my bad.

Here's the recovered and reproduced input of mine where it was failing:

In [1]: import email.message

In [2]: m = email.message.Message()

In [3]: m['content-disposition'] = 'attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'

In [4]: m.get_params()

In [5]: m['content-type'] = 'attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'

In [6]: m['content-type'] = '''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'''

In [7]: m.get_params()
Out[7]:
[('attachment', ''),
 ('filename', 'SoundCloud%20Download'),
 ('filename', (None, None, 'utf-8track.mp3'))]

# Now testing on a new, blank message

In [8]: m2 = email.message.Message()

In [9]: m2['content-type'] = '''attachment;filename="SoundCloud%20Download"; filename*=utf-8''track.mp3'''

In [10]: m2.get_params()
Out[10]:
[('attachment', ''),
 ('filename', 'SoundCloud%20Download'),
 ('filename', ('utf-8', '', 'track.mp3'))]

In [11]:

What's interesting is that this exact order of steps is needed to trigger this, its not just about the content-disposition set. Not sure what's happening but I should've given this a few more tries before reimplementing stuff, oh well.

7x11x13 commented 2 months ago

Thanks for this, I'll merge as soon as I get original download tests working again

es3n1n commented 2 months ago

Thanks for this, I'll merge as soon as I get original download tests working again

Sure, please let me know if something would be needed from my end before merging.