Skip to content
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.

Sign up for GitHub

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

Download files from direct URLs via a command line parameter #1873

Merged
merged 11 commits into from
Sep 18, 2023
Merged

Download files from direct URLs via a command line parameter #1873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6228644
Download files via a command line parameter
eddoursul Sep 14, 2023
f0494dc
Use a GET parameter to predetermine Content-Disposition header in S3-…
eddoursul Sep 14, 2023
dfa9800
Reverted resumeDownloadInt
eddoursul Sep 15, 2023
2821606
Revert "Reverted resumeDownloadInt"
eddoursul Sep 15, 2023
c1f41f2
Moved downloading to a dedicated command
eddoursul Sep 15, 2023
72727c7
Enforce HTTPS download links
eddoursul Sep 15, 2023
a6d950b
Merge branch 'master' into qt6
eddoursul Sep 15, 2023
cf45dfd
Return numeric error codes
eddoursul Sep 15, 2023
8908df4
Return numeric error codes
eddoursul Sep 15, 2023
1c6ad48
clang-format pass
eddoursul Sep 15, 2023
1ffcc6f
Merge branch 'master' into qt6
eddoursul Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions src/commandline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "env.h"
#include "instancemanager.h"
#include "loglist.h"
#include "messagedialog.h"
#include "multiprocess.h"
#include "organizercore.h"
#include "shared/appconfig.h"
Expand Down Expand Up @@ -49,8 +50,8 @@ CommandLine::CommandLine() : m_command(nullptr)
{
createOptions();

add<RunCommand, ReloadPluginCommand, RefreshCommand, CrashDumpCommand,
LaunchCommand>();
add<RunCommand, ReloadPluginCommand, DownloadFileCommand, RefreshCommand,
CrashDumpCommand, LaunchCommand>();
}

std::optional<int> CommandLine::process(const std::wstring& line)
Expand Down Expand Up @@ -833,6 +834,51 @@ std::optional<int> ReloadPluginCommand::runPostOrganizer(OrganizerCore& core)
return {};
}

Command::Meta DownloadFileCommand::meta() const
{
return {"download", "downloads a file", "URL", ""};
}

po::options_description DownloadFileCommand::getInternalOptions() const
{
po::options_description d;

d.add_options()("URL", po::value<std::string>()->required(), "file URL");

return d;
}

po::positional_options_description DownloadFileCommand::getPositional() const
{
po::positional_options_description d;

d.add("URL", 1);

return d;
}

bool DownloadFileCommand::canForwardToPrimary() const
{
return true;
}

std::optional<int> DownloadFileCommand::runPostOrganizer(OrganizerCore& core)
{
const QString url = QString::fromStdString(vm()["URL"].as<std::string>());

if (!url.startsWith("https://")) {
reportError(QObject::tr("Download URL must start with https://"));
return 1;
}

log::debug("starting direct download from command line: {}", url.toStdString());
MessageDialog::showMessage(QObject::tr("Download started"), qApp->activeWindow(),
eddoursul marked this conversation as resolved.
Show resolved Hide resolved
false);
core.downloadManager()->startDownloadURLs(QStringList() << url);

return {};
}

Command::Meta RefreshCommand::meta() const
{
return {"refresh", "refreshes MO (same as F5)", "", ""};
Expand Down
14 changes: 14 additions & 0 deletions src/commandline.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,20 @@ class ReloadPluginCommand : public Command
std::optional<int> runPostOrganizer(OrganizerCore& core) override;
};

// downloads a file
//
class DownloadFileCommand : public Command
{
protected:
Meta meta() const override;

po::options_description getInternalOptions() const override;
po::positional_options_description getPositional() const override;

bool canForwardToPrimary() const override;
std::optional<int> runPostOrganizer(OrganizerCore& core) override;
};

// refreshes mo
//
class RefreshCommand : public Command
Expand Down
38 changes: 32 additions & 6 deletions src/downloadmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,23 @@ bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int
QString fileName = QFileInfo(URLs.first()).fileName();
if (fileName.isEmpty()) {
fileName = "unknown";
} else {
fileName = QUrl::fromPercentEncoding(fileName.toUtf8());
}

// Temporary URLs for S3-compatible storage are signed for a single method, removing
// the ability to make HEAD requests to such URLs. We can use the
// response-content-disposition GET parameter, setting the Content-Disposition header,
// to predetermine intended file name without a subrequest.
if (fileName.contains("response-content-disposition=")) {
std::regex exp("filename=\"(.+)\"");
std::cmatch result;
if (std::regex_search(fileName.toStdString().c_str(), result, exp)) {
fileName = MOBase::sanitizeFileName(QString::fromUtf8(result.str(1).c_str()));
if (fileName.isEmpty()) {
fileName = "unknown";
}
}
}

QUrl preferredUrl = QUrl::fromEncoded(URLs.first().toLocal8Bit());
Expand All @@ -446,6 +463,9 @@ bool DownloadManager::addDownload(const QStringList& URLs, QString gameName, int
QNetworkRequest request(preferredUrl);
request.setHeader(QNetworkRequest::UserAgentHeader,
m_NexusInterface->getAccessManager()->userAgent());
request.setAttribute(QNetworkRequest::CacheSaveControlAttribute, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for these two added attributes?

Copy link

@vensko vensko Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same reason they are used in Nexus downloads https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/nexusinterface.cpp#L821 . Spent half a day pulling my hair out on this.

By default, MO2 caches repeating requests, especially with small files. When this happens, the finished() signal is sent immediately, causing downloadFinished to fire before downloading a single byte. Then the check for size==0 kicks in here https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/downloadmanager.cpp#L2100 , and the download gets marked as failed without showing a single message, because messages don't cover this case.

These two lines disable cache for the request.

request.setAttribute(QNetworkRequest::CacheLoadControlAttribute,
QNetworkRequest::AlwaysNetwork);
request.setHttp2Configuration(h2Conf);
return addDownload(m_NexusInterface->getAccessManager()->get(request), URLs, fileName,
gameName, modID, fileID, fileInfo);
Expand Down Expand Up @@ -597,7 +617,10 @@ void DownloadManager::startDownload(QNetworkReply* reply, DownloadInfo* newDownl
if (newDownload->m_State != STATE_DOWNLOADING &&
newDownload->m_State != STATE_READY &&
newDownload->m_State != STATE_FETCHINGMODINFO && reply->isFinished()) {
downloadFinished(indexByInfo(newDownload));
int index = indexByInfo(newDownload);
if (index >= 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The needs for this should be investigated, although probably not relevant for this PR, but maybe add a TODO comment.

Copy link

@vensko vensko Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the fix to downloadFinished (below) is accepted, this may be redundant (I need to re-check). If it's not accepted, downloadFinished may fire with invalid -1 value, causing the no download index warning.

downloadFinished(index);
}
return;
}
} else
Expand Down Expand Up @@ -945,8 +968,7 @@ void DownloadManager::resumeDownloadInt(int index)

// Check for finished download;
if (info->m_TotalSize <= info->m_Output.size() && info->m_Reply != nullptr &&
info->m_Reply->isOpen() && info->m_Reply->isFinished() &&
info->m_State != STATE_ERROR) {
info->m_Reply->isFinished() && info->m_State != STATE_ERROR) {
setState(info, STATE_DOWNLOADING);
downloadFinished(index);
return;
Expand Down Expand Up @@ -1508,7 +1530,7 @@ QString DownloadManager::getDownloadFileName(const QString& baseName, bool renam
QString DownloadManager::getFileNameFromNetworkReply(QNetworkReply* reply)
{
if (reply->hasRawHeader("Content-Disposition")) {
std::regex exp("filename=\"(.*)\"");
std::regex exp("filename=\"(.+)\"");

std::cmatch result;
if (std::regex_search(reply->rawHeader("Content-Disposition").constData(), result,
Expand Down Expand Up @@ -2142,10 +2164,14 @@ void DownloadManager::nxmRequestFailed(QString gameName, int modID, int fileID,
void DownloadManager::downloadFinished(int index)
{
DownloadInfo* info;
if (index)
if (index > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for this change (and the one below)?

Copy link

@vensko vensko Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

downloadFinished is used in two ways:

  • as a function, accepting an integer argument,
  • as a signal callback.

Currently, downloadFinished has a bug, when it does not run for the first download in the list (index 0), because of the logical mistake here 0==false https://github.com/ModOrganizer2/modorganizer/blob/qt6/src/downloadmanager.cpp#L2079

When it runs with downloadFinished(0), if (index) skips execution and jumps to detecting the signal sender, which yields nothing, because it is not a signal call. As a result, the first file may never see the finishing routine, causing cascade of errors and discrepancy in interface and actual status. In case of the proposed command, this looks like a duplicating paused file, when user tries to redownload the first file.

The first idea was to add if (index >= 0), but then the signal callback info = findDownload(this->sender(), &index) never fires, because during the callback index is also 0.

So I implemented detection of the first row as a fallback after all paths are exhausted.

I expected opposition on this fix and added the additional check in startDownload, catching possible downloadFinished(-1) call, which is an invalid value, causing the no download index warning. If the fix is accepted, the startDownload change probably becomes redundant.

info = m_ActiveDownloads[index];
else
else {
info = findDownload(this->sender(), &index);
if (info == nullptr && index == 0) {
info = m_ActiveDownloads[index];
}
}

if (info != nullptr) {
QNetworkReply* reply = info->m_Reply;
Expand Down
Loading