NewPipeExtractor icon indicating copy to clipboard operation
NewPipeExtractor copied to clipboard

Add Content-Type header to all POST requests without an empty body

Open AudricV opened this issue 2 years ago • 6 comments

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.

AudricV avatar Jun 19 '22 15:06 AudricV

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.

Stypox avatar Jun 24 '22 13:06 Stypox

I'm not sure if Content-Length, should be set since for example, what if gzip/brotli is being used by the downloader?

FireMasterK avatar Jun 25 '22 00:06 FireMasterK

I'm not sure if Content-Length, should be set since for example, what if gzip/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.

AudricV avatar Jul 11 '22 14:07 AudricV

Note that the CI will fail because I didn't updated YoutubeStreamExtractorLivestreamTest, see why in https://github.com/TeamNewPipe/NewPipeExtractor/pull/863/commits/438fb69431bc62fc4101c3d122ca9bb3fe293544 .

AudricV avatar Jul 11 '22 14:07 AudricV

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.

AudricV avatar Aug 07 '22 22:08 AudricV

@Stypox Could you please review again this pull request? Thank you in advance :)

AudricV avatar Aug 24 '22 14:08 AudricV

@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): YoutubeSearchExtractorTest.MetaInfoTest learn more text difference

  • 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.

AudricV avatar Nov 22 '22 16:11 AudricV

fixed some issues on the fly

Anything worth mentioning in the blog post?

opusforlife2 avatar Dec 16 '22 18:12 opusforlife2