remote: http: raise exception when response with error status code

[pared]

• ❗ Have you followed the guidelines in the Contributing to DVC list?

• 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

• ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Fixes #2510

[Suor]

Do we really need to invent a new exception class for each situation? This would be a nightmare if people will some time in the future use Repo class and try to handle errors. Also creates bloat.

This is question goes to everybody not only @pared.

[pared]

I agree that might be excessive in some situations, though, what alternative do we have?
Do you think I should use requests.HTTPError?

[Suor]

We should probably discuss it separately. I didn't mean this a blocker for this particular PR, just used it as an example.

[Suor]

And it can't be requests.HTTPError as we need to descend this from DvcException().

[Suor]

Code and reason should go next to each other, separate by space. This how it is presented everywhere. We might also want to say that this is an HTTP error in the message.

[Suor]

We don't split imports like that anymore)

[Suor]

Is there any value in testing it 3 times?

[Suor]

Simply use URLInfo(url) / "file.txt" or use URLInfo from the start. This will save you from an error of using os.path instead of posixpath.

[Suor]

Do you really need to store this to an attribute?

[efiop]

@Suor @MrOutis Please review.

[efiop]

@pared Please rebase 😉

[Suor]

This will show:

HTTPError: HTTP Error: 404 Not Found

[Suor]

Suggested change

	total=None if no_progress_bar else self._content_length(from_info),
	total=None if no_progress_bar else self._content_length(response),

I wonder why we are still doing this extra request, also ._content_length() doesn't need to work with url really.

[Suor]

This is outside of your PR actually. But since you are here)

[pared]

But isn't the extra request done only if response does not contain Content-Length?

[Suor]

If you pass url (path_info) and not response then you always make a second request. _content_length() should stop accepting urls and might be even inlined, this will prevent this inefficiency.

[pared]

That is right, sorry, I'll change accordingly. Though, I would leave _content_length as it is, in order to not obstruct _download method content.

[Suor]

I don't think we need flush. It is probably a remnant from the time we did http resume.

[Suor]

Do we actually need it? Can we simply ask for some missing url?

[pared]

Well, 404 is quite easy to generate

[pared]

The question now is, should we leave changes made to StaticFileServer.
@Suor what do you think? I think they should stay, as the previous version was mapping string to its handler class, which was unnecessary.

[Suor]

It's cleaner the new way.

[Suor]

Maybe use "missing.txt" to make it more obvious.

[pared]

done

[ghost]

thanks, @pared ! looks good to me)

[efiop]

@Suor Please take a look 🙂

[Suor]

If you pass url (path_info) and not response then you always make a second request. _content_length() should stop accepting urls and might be even inlined, this will prevent this inefficiency.

[Suor]

Looks good.

[Suor]

No need for this getattr() anymore, simply request.headers.

[pared]

done