NewPipeExtractor
NewPipeExtractor copied to clipboard
Add Content-Type header to all POST requests without an empty body
- [x] I carefully read the contribution guidelines and agree to them.
- [x] I have tested the API against NewPipe.
This PR adds the Content-Type
header to all POST requests without an empty body made by the extractor. They are made only in YouTube and Bandcamp services, according to the usages of the post
methods in the Downloader
class.
Currently, in NewPipe and in tests, this header is partially guessed and added by the network library we use, OkHttp. But this is not the case of all network libraries (e.g. Cronet), where 400 responses would be encountered on YouTube because the HTTP specifications for POST requests are not required.
I also improved related code in the changed files, and fixed some issues on the fly.
Uhm, instead of all of this duplicate code, shouldn't we change the Downloader.post()
methods so that they take a String contentType
parameter? Then the implementers of Downloader will take care of setting Content-Length
(which can be derived from body.size
) and Content-Type
(which is the provided value). Since, as you said, having those two headers is a requirement for post
requests, imo it makes sense to change the post()
signature, so that we will surely not forget to put the content type anymore as the compiler would complain, while the length is set always.
This will not be merged before the next version, I don't think it is a big deal for the NewPipe app. But we can make a specific extractor version after this PR is merged if needed.
I'm not sure if Content-Length
, should be set since for example, what if gzip/brotli
is being used by the downloader?
I'm not sure if
Content-Length
, should be set since for example, what ifgzip/brotli
is being used by the downloader?
Thank you for the catch! I removed the addition of this header, and kept only the Content-Type
one.
Note that the CI will fail because I didn't updated YoutubeStreamExtractorLivestreamTest
, see why in https://github.com/TeamNewPipe/NewPipeExtractor/pull/863/commits/438fb69431bc62fc4101c3d122ca9bb3fe293544 .
Requested changes have been applied.
Note: I reverted update of YoutubeLiveStreamExtractorTest
and removal of YoutubeSearchExtractorTest.Suggestion
, as both tests will be fixed in #882. That's the reason why some tests will fail because of missing mocks.
@Stypox Could you please review again this pull request? Thank you in advance :)
@Stypox Indeed, you are almost right:
-
the first one may be related to handles introduction: the channel ID is not provided in the
yt:channelId
global element of the RSS feed anymore (nothing is provided, the channel ID can be extracted in other ways); -
the second one is due to #976;
-
the third one is due to the to the fact that not all the "learn more" button is uppercase anymore, that's only the case for the first letter (fixed with 2ec296e6746884c13962311fe2dee7c62dc1c65b):
-
the fourth one is related to due to time extraction failures, which are not breaking changes, (when the duration of streams is unknown) ~~it may be related to the fact that duration can be hidden by creators on search results as far I remember, and so inaccessible for us~~ YouTube sometimes return running livestreams as regular videos, and in this case you can't distinguish these content as livestreams.
I ran again the test to get mocks with only results which have a known duration (in 34d79bd2679d1b851dea67b4e05acb52b09bc8ed);
-
the last one is country dependent, at least I could not reproduce this. Your new mocks doesn't contain the suggestion, so I created new ones (in 34d79bd2679d1b851dea67b4e05acb52b09bc8ed). See the conversation and the changes of #813 and #882 for related details.
By the way, the related code should be not index array based I think.
fixed some issues on the fly
Anything worth mentioning in the blog post?