import-url: allow queries in URL

[casperdcl]

Looks like disallowing URLs containing queries happened in b83564d (#2707), i.e. dvc>0.66.1... Not sure why. Seems fine to me to remove the assert not p.query restriction.

• fix URLInfo for http URLs (via subclass HTTPURLInfo(URLInfo))
  • add support for params
  • add support for queries
    • fixes import-url: does not work for URL that contains a query string #3424 dvc import-url -v https://www.dropbox.com/?test data
  • add support for fragments
• add tests

[Suor]

This basically breaks URLInfo. Many of its methods will stop to work:
url, __str__, __eq__, __hash__, ... So info will be lost and they will start to behave weird when comparing or when using as dict keys etc.

You should either add full support of params or go around of it. If you are going to implement it then you should leave the old class for anything but http urls for efficiency.

[casperdcl]

ok so this is still a bug then. We need to support queries because some sites need them for downloadable links (I seem to recall dropbox for example returns a html webpage if you leave out ?dl=1 - not sure if this is still the case but you get the point)

[casperdcl]

I didn't bother overriding fill_parts as it's not used publicly anywhere else

[Suor]

Then don't. It used via .replace(), .__div__(), .parent, If the inherited implementation works then you may skip though.

[casperdcl]

Those use from_parts (which would use this overridden method). The point is nothing else explicitly uses fill_parts directly so there's no need to override fill_parts for now.

[Suor]

You keep saying don't need to override, but you override. Not sure I understand what you are up to here.

[casperdcl]

I mean technically the correct way to do things would be:

def fill_parts(self, scheme, host, user, port, path, params, query, fragment):
    super().fill_parts(self, scheme, host, user, port, path)
    self.params = params
    self.query = query
    self.fragment = fragment

but this isn't required right now as fill_parts is really a private method not used anywhere else.

[casperdcl]

@shcheklein / @jorgeorpinel / @Suor ready for review

[Suor]

Some simplifications are possible. Sorry for late review.

[Suor]

Then don't. It used via .replace(), .__div__(), .parent, If the inherited implementation works then you may skip though.

[Suor]

Why do we use restringification? May use .from_parts() or .fill_parts().

[casperdcl]

More future-proof to use super() to use the parent's logic. Otherwise we'd need

p = urlparse(url)
-stripped = p._replace(params=None, query=None, fragment=None)
-super().__init__(stripped.geturl())
+assert p.password is None
+self.fill_parts(p.scheme, p.hostname, p.username, p.port, p.path)

and not call super(), which is allowed but not great practice.

[Suor]

First, we can ignore all of this as long as it works. Some tech debt, but might be resolved later.

Needing to parse, restringify and parse is a kludge.