-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: cobalt v10 api - parse filename | fix #27 #28
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,82 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defmodule SmallSdk.WebDownloader do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
require Logger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# FIXME:TODAY return {:ok, file_name, file_content} | {:error, reason} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def download_files(urls) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger.info("download_files started, urls: #{inspect(urls)}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
res = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
urls | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> Enum.map(&download_file/1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> Enum.reduce_while([], fn | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:ok, filename, file_content}, acc -> {:cont, [{filename, file_content} | acc]} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:error, reason}, _ -> {:halt, {:error, reason}} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:ok, res} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# TODO: have to handle Stream data | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def download_file(url) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger.info("download_file started, url: #{url}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case Req.get(url) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:ok, %{status: status, body: ""}} -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger.warning("Downloaded an empty file, status: #{status}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:error, "💔 Downloaded an empty file"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:ok, %{status: status, body: body, headers: headers}} -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case status do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
status when status in 200..209 -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
filename = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
cond do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
String.contains?(url, "/tunnel") -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
parse_filename(url, :content_disposition, headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
true -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
parse_filename(url, :content_type, headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:ok, filename, body} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_ -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger.error("download_file failed, status: #{status}, body: #{inspect(body)}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:error, "💔 Failed to download file"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:error, reason} -> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Logger.error("download_file failed, reason: #{inspect(reason)}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{:error, "💔 Failed to download file"} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+20
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate URLs to prevent potential security risks The function Consider adding validation to ensure that URLs are safe and conform to expected patterns, or limit requests to trusted domains. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defp parse_filename(url, :content_type, headers) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ext = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
headers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> Map.get("content-type") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> List.first() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> String.split("/") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> List.last() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
gen_file_name(url) <> "." <> ext | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+53
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling for missing or malformed 'content-type' headers The function assumes that the 'content-type' header exists and is properly formatted. If the header is missing or has an unexpected format, it could lead to runtime errors. Consider adding safeguards to handle such cases safely: ext =
headers
|> Enum.find_value(fn {key, value} -> if key == "content-type", do: value end)
|> case do
nil -> "bin"
content_type ->
content_type
|> String.split("/")
|> List.last()
end Ensure correct extraction of 'content-type' from headers In To fix this issue, consider using Apply this diff: - headers
- |> Map.get("content-type")
+ headers
+ |> Enum.find_value(fn {key, value} -> if key == "content-type", do: value end) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defp parse_filename(_url, :content_disposition, headers) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
filename = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
headers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> Map.get("content-disposition") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> List.first() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> String.split(";") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> Enum.find(fn x -> String.contains?(x, "filename") end) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> String.split("=") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> List.last() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> String.trim() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|> String.replace("\"", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
filename | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+64
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle potential errors when parsing 'content-disposition' header The function assumes that the 'content-disposition' header exists and follows a specific format. If the header is missing or malformed, it could cause runtime errors. Add checks or default values to handle cases where the filename cannot be extracted: filename =
headers
|> Enum.find_value(fn {key, value} -> if key == "content-disposition", do: value end)
|> case do
nil -> gen_file_name(url)
content_disposition ->
content_disposition
|> String.split(";")
|> Enum.find(fn x -> String.contains?(x, "filename") end)
|> case do
nil -> gen_file_name(url)
filename_part ->
filename_part
|> String.split("=")
|> List.last()
|> String.trim("\"")
end
end Correct the retrieval of 'content-disposition' header In Update the code to properly extract the 'content-disposition' header: - headers
- |> Map.get("content-disposition")
+ headers
+ |> Enum.find_value(fn {key, value} -> if key == "content-disposition", do: value end) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
defp gen_file_name(url) do | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
:crypto.hash(:sha256, url) |> Base.url_encode64(padding: false) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure
download_files/1
returns consistent success and error tuplesCurrently,
download_files/1
may return{:ok, {:error, reason}}
when an error occurs during file downloads, which can be confusing. The function should return{:error, reason}
in case of an error to maintain consistent return types.Apply this diff to adjust the return value:
📝 Committable suggestion