Merge lp:~mandel/ubuntu-download-manager/follow-redirects into lp:ubuntu-download-manager

Proposed by Manuel de la Peña
Status: Merged
Approved by: Manuel de la Peña
Approved revision: 229
Merged at revision: 227
Proposed branch: lp:~mandel/ubuntu-download-manager/follow-redirects
Merge into: lp:ubuntu-download-manager
Diff against target: 397 lines (+258/-7)
8 files modified
ubuntu-download-manager-priv/downloads/file_download.cpp (+61/-7)
ubuntu-download-manager-priv/downloads/file_download.h (+3/-0)
ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp (+6/-0)
ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h (+2/-0)
ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp (+2/-0)
ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h (+3/-0)
ubuntu-download-manager-tests/downloads/test_download.cpp (+175/-0)
ubuntu-download-manager-tests/downloads/test_download.h (+6/-0)
To merge this branch: bzr merge lp:~mandel/ubuntu-download-manager/follow-redirects
Reviewer Review Type Date Requested Status
Mike McCracken (community) Approve
PS Jenkins bot continuous-integration Approve
Roberto Alsina (community) Approve
Review via email: mp+204666@code.launchpad.net

Commit message

Ensure that redirects are followed from udm unless we are in a loop. If the download gets in a redirect loop a network error is emitted.

Description of the change

Ensure that redirects are followed from udm unless we are in a loop. If the download gets in a redirect loop a network error is emitted.

To post a comment you must log in.
228. By Manuel de la Peña

Raise an error if we have file system issues.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Roberto Alsina (ralsina) :
review: Approve
229. By Manuel de la Peña

Fix some small space alignment issue in the code.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Mike McCracken (mikemc) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'ubuntu-download-manager-priv/downloads/file_download.cpp'
--- ubuntu-download-manager-priv/downloads/file_download.cpp 2014-02-03 15:30:58 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.cpp 2014-02-04 16:23:22 +0000
@@ -291,16 +291,54 @@
291}291}
292292
293void293void
294FileDownload::onFinished() {294FileDownload::onRedirect(QUrl redirect) {
295 LOG(INFO) << "Following redirect to" << redirect;
296 // update the _url value and perform a second request to try and get the data
297 if (_visitedUrls.contains(redirect)) {
298 // we are in a loop!!! we have to raise an error about this.
299 LOG(WARNING) << "Redirect loop found";
300 NetworkErrorStruct err(QNetworkReply::ContentNotFoundError);
301 emit networkError(err);
302 emitError(NETWORK_ERROR);
303 return;
304 }
305 _visitedUrls.append(_url);
306 _url = redirect;
307
308 // clean the reply
309 disconnectFromReplySignals();
310 _reply->deleteLater();
311 _reply = nullptr;
312
313 // clean the local file
314 cleanUpCurrentData();
315
316 // perform again the request but do not emit started signal
317 _currentData = FileManager::instance()->createFile(_filePath);
318 bool canWrite = _currentData->open(QIODevice::ReadWrite | QFile::Append);
319
320 if (!canWrite) {
321 emitError(FILE_SYSTEM_ERROR);
322 return;
323 }
324
325 _reply = _requestFactory->get(buildRequest());
326 _reply->setReadBufferSize(throttle());
327
328 connectToReplySignals();
329}
330
331void
332FileDownload::onDownloadCompleted() {
295 TRACE << _url;333 TRACE << _url;
296334
297 // if the hash is present we check it335 // if the hash is present we check it
298 if (!_hash.isEmpty()) {336 if (!_hash.isEmpty()) {
299 emit processing(filePath());337 emit processing(filePath());
300 _currentData->reset();338 _currentData->reset();
301 QCryptographicHash hash(_algo);339 QCryptographicHash hash(_algo);
302 // addData is smart enough to not load the entire file in memory340 // addData is smart enough to not load the entire file in memory
303 hash.addData(_currentData->device());341 hash.addData(_currentData->device());
304 QString fileSig = QString(hash.result().toHex());342 QString fileSig = QString(hash.result().toHex());
305343
306 if (fileSig != _hash) {344 if (fileSig != _hash) {
@@ -316,7 +354,8 @@
316 // finish signals once the command was done (or an error ocurred in354 // finish signals once the command was done (or an error ocurred in
317 // the command execution.355 // the command execution.
318 if (metadata().contains(METADATA_COMMAND_KEY)) {356 if (metadata().contains(METADATA_COMMAND_KEY)) {
319 // just emit processing if we DO NOT have a hash because else we already emitted it.357 // just emit processing if we DO NOT have a hash because else we
358 // already emitted it.
320 if (_hash.isEmpty()) {359 if (_hash.isEmpty()) {
321 emit processing(filePath());360 emit processing(filePath());
322 }361 }
@@ -368,6 +407,21 @@
368}407}
369408
370void409void
410FileDownload::onFinished() {
411 TRACE << _url;
412 // the reply has finished but the resource might have been moved
413 // and we must do a new request
414 auto redirect = _reply->attribute(
415 QNetworkRequest::RedirectionTargetAttribute).toUrl();
416
417 if(!redirect.isEmpty() && redirect != _url) {
418 onRedirect(redirect);
419 } else {
420 onDownloadCompleted();
421 }
422}
423
424void
371FileDownload::onSslErrors(const QList<QSslError>& errors) {425FileDownload::onSslErrors(const QList<QSslError>& errors) {
372 TRACE << errors;426 TRACE << errors;
373 if (!_reply->canIgnoreSslErrors(errors)) {427 if (!_reply->canIgnoreSslErrors(errors)) {
@@ -539,7 +593,7 @@
539 if (!secondPart.isEmpty()) {593 if (!secondPart.isEmpty()) {
540 secondPart = "." + secondPart;594 secondPart = "." + secondPart;
541 firstPart = path.left(path.size() - secondPart.size());595 firstPart = path.left(path.size() - secondPart.size());
542 } 596 }
543597
544 // Try with an ever-increasing number suffix, until we've reached a file598 // Try with an ever-increasing number suffix, until we've reached a file
545 // that does not yet exist.599 // that does not yet exist.
@@ -557,7 +611,7 @@
557611
558void612void
559FileDownload::cleanUpCurrentData() {613FileDownload::cleanUpCurrentData() {
560 bool success = true; 614 bool success = true;
561 QFile::FileError error = QFile::NoError;615 QFile::FileError error = QFile::NoError;
562 if (_currentData != nullptr) {616 if (_currentData != nullptr) {
563 success = _currentData->remove();617 success = _currentData->remove();
564618
=== modified file 'ubuntu-download-manager-priv/downloads/file_download.h'
--- ubuntu-download-manager-priv/downloads/file_download.h 2014-02-03 13:34:10 +0000
+++ ubuntu-download-manager-priv/downloads/file_download.h 2014-02-04 16:23:22 +0000
@@ -112,6 +112,8 @@
112 void init();112 void init();
113 void onDownloadProgress(qint64 currentProgress, qint64);113 void onDownloadProgress(qint64 currentProgress, qint64);
114 void onError(QNetworkReply::NetworkError);114 void onError(QNetworkReply::NetworkError);
115 void onRedirect(QUrl redirect);
116 void onDownloadCompleted();
115 void onFinished();117 void onFinished();
116 void onSslErrors(const QList<QSslError>&);118 void onSslErrors(const QList<QSslError>&);
117 void onProcessError(QProcess::ProcessError error);119 void onProcessError(QProcess::ProcessError error);
@@ -131,6 +133,7 @@
131 QCryptographicHash::Algorithm _algo;133 QCryptographicHash::Algorithm _algo;
132 NetworkReply* _reply = nullptr;134 NetworkReply* _reply = nullptr;
133 File* _currentData = nullptr;135 File* _currentData = nullptr;
136 QList<QUrl> _visitedUrls;
134};137};
135138
136} // Daemon139} // Daemon
137140
=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp 2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.cpp 2014-02-04 16:23:22 +0000
@@ -153,6 +153,12 @@
153}153}
154154
155void155void
156FakeNetworkReply::setAttribute(QNetworkRequest::Attribute code,
157 QUrl url) {
158 _attrs[code] = url;
159}
160
161void
156FakeNetworkReply::clearAttribute(QNetworkRequest::Attribute code) {162FakeNetworkReply::clearAttribute(QNetworkRequest::Attribute code) {
157 if (_attrs.contains(code)) {163 if (_attrs.contains(code)) {
158 _attrs.remove(code);164 _attrs.remove(code);
159165
=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h 2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/network_reply.h 2014-02-04 16:23:22 +0000
@@ -65,6 +65,8 @@
65 int value);65 int value);
66 void setAttribute(QNetworkRequest::Attribute code,66 void setAttribute(QNetworkRequest::Attribute code,
67 QString message);67 QString message);
68 void setAttribute(QNetworkRequest::Attribute code,
69 QUrl url);
68 void clearAttribute(QNetworkRequest::Attribute code);70 void clearAttribute(QNetworkRequest::Attribute code);
6971
70 private:72 private:
7173
=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp 2014-01-24 11:54:24 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.cpp 2014-02-04 16:23:22 +0000
@@ -32,6 +32,7 @@
32FakeRequestFactory::FakeRequestFactory(QObject *parent)32FakeRequestFactory::FakeRequestFactory(QObject *parent)
33 : RequestFactory(parent),33 : RequestFactory(parent),
34 Fake() {34 Fake() {
35 qRegisterMetaType<NetworkReply*>("NetworkReply*");
35}36}
3637
37NetworkReply*38NetworkReply*
@@ -53,6 +54,7 @@
53 // if we are recording we do set the recording of the returned reply54 // if we are recording we do set the recording of the returned reply
54 reply->record();55 reply->record();
55 }56 }
57 emit requestCreated(reply);
56 return reply;58 return reply;
57}59}
5860
5961
=== modified file 'ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h'
--- ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h 2014-01-10 16:38:56 +0000
+++ ubuntu-download-manager-test-lib/ubuntu/download_manager/tests/server/request_factory.h 2014-02-04 16:23:22 +0000
@@ -50,6 +50,9 @@
50 NetworkReply* get(const QNetworkRequest& request);50 NetworkReply* get(const QNetworkRequest& request);
51 QList<QSslCertificate> acceptedCertificates() override;51 QList<QSslCertificate> acceptedCertificates() override;
52 void setAcceptedCertificates(const QList<QSslCertificate>& certs) override;52 void setAcceptedCertificates(const QList<QSslCertificate>& certs) override;
53
54 signals:
55 void requestCreated(NetworkReply*);
53};56};
5457
55#endif // FAKE_QNETWORK_ACCESS_MANAGER_H58#endif // FAKE_QNETWORK_ACCESS_MANAGER_H
5659
=== modified file 'ubuntu-download-manager-tests/downloads/test_download.cpp'
--- ubuntu-download-manager-tests/downloads/test_download.cpp 2014-02-03 13:34:10 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.cpp 2014-02-04 16:23:22 +0000
@@ -1986,3 +1986,178 @@
1986 QString("FILE SYSTEM ERROR: %1").arg(QFile::WriteError));1986 QString("FILE SYSTEM ERROR: %1").arg(QFile::WriteError));
1987 _fileManager->setFile(nullptr);1987 _fileManager->setFile(nullptr);
1988}1988}
1989
1990void
1991TestDownload::testRedirectCycle() {
1992 // the test works as follows, create a request, pause and set the redirect
1993 // header, wait until the second request is created via the signal from the
1994 // request factory and set the redirect to a first url
1995 QUrl redirectUrl("http://redirect.example.com");
1996 _reqFactory->record();
1997 QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
1998 _rootPath, _url, _metadata, _headers));
1999
2000 download->start(); // change state
2001 download->startDownload();
2002
2003 // we need to set the data before we pause!!!
2004 QList<MethodData> calledMethods = _reqFactory->calledMethods();
2005 QCOMPARE(1, calledMethods.count());
2006 FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
2007 calledMethods[0].params().outParams()[0]);
2008
2009 // set the attr for a redirect to a new url
2010 // makes the process to be executed
2011 reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
2012 redirectUrl);
2013 QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
2014
2015 // use a spy to wait for the second reply
2016 reply->emitFinished();
2017
2018 QTRY_COMPARE(1, replySpy.count());
2019
2020 reply = qobject_cast<FakeNetworkReply*>(
2021 replySpy.takeFirst().at(0).value<NetworkReply*>());
2022
2023 download->pause();
2024
2025 // set the attr to be a loop
2026 reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
2027 _url);
2028
2029 // emit finished and an errors should be emitted
2030 QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
2031 QSignalSpy networkErrorSpy(download.data(),
2032 SIGNAL(networkError(NetworkErrorStruct)));
2033
2034 reply->emitFinished();
2035
2036 QTRY_COMPARE(networkErrorSpy.count(), 1);
2037 QTRY_COMPARE(errorSpy.count(), 1);
2038}
2039
2040void
2041TestDownload::testSingleRedirect() {
2042 // ensure that a single redirect is followed
2043 QUrl redirectUrl("http://redirect.example.com");
2044 _reqFactory->record();
2045 QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
2046 _rootPath, _url, _metadata, _headers));
2047
2048 download->start(); // change state
2049 download->startDownload();
2050
2051 // we need to set the data before we pause!!!
2052 QList<MethodData> calledMethods = _reqFactory->calledMethods();
2053 QCOMPARE(1, calledMethods.count());
2054 FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
2055 calledMethods[0].params().outParams()[0]);
2056
2057 // set the attr for a redirect to a new url
2058 // makes the process to be executed
2059 reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
2060 redirectUrl);
2061 QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
2062
2063 // use a spy to wait for the second reply
2064 reply->emitFinished();
2065
2066 // ensure that a second request is performed
2067 QTRY_COMPARE(1, replySpy.count());
2068
2069 reply = qobject_cast<FakeNetworkReply*>(
2070 replySpy.takeFirst().at(0).value<NetworkReply*>());
2071
2072 download->pause();
2073
2074 QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
2075 QSignalSpy networkErrorSpy(download.data(),
2076 SIGNAL(networkError(NetworkErrorStruct)));
2077 QSignalSpy finishedSpy(download.data(), SIGNAL(finished(QString)));
2078
2079 reply->emitFinished();
2080
2081 QTRY_COMPARE(networkErrorSpy.count(), 0);
2082 QTRY_COMPARE(errorSpy.count(), 0);
2083 QTRY_COMPARE(finishedSpy.count(), 1);
2084}
2085
2086void
2087TestDownload::testSeveralRedirects_data() {
2088 QTest::addColumn<QStringList>("urls");
2089
2090 QStringList first;
2091 first.append("http://first.com/resource");
2092 first.append("http://first.com/moved_resource");
2093 first.append("http://first.com/last");
2094 QTest::newRow("Three redirects") << first;
2095
2096 QStringList second;
2097 second.append("http://second.com/resource");
2098 second.append("http://second.com/moved1");
2099 second.append("http://second.com/moved2");
2100 second.append("http://second.com/moved3");
2101 second.append("http://second.com/moved4");
2102 QTest::newRow("Five redirects") << second;
2103
2104 QStringList third;
2105 third.append("http://third.com/resource");
2106 third.append("http://third.com/moved1");
2107 third.append("http://third.com/moved2");
2108 third.append("http://third.com/moved3");
2109 third.append("http://third.com/moved4");
2110 third.append("http://third.com/moved5");
2111 third.append("http://third.com/moved6");
2112 QTest::newRow("Seven redirects") << third;
2113
2114}
2115
2116void
2117TestDownload::testSeveralRedirects() {
2118 QFETCH(QStringList, urls);
2119 auto redirectCount = 0;
2120 _reqFactory->record();
2121 QScopedPointer<FileDownload> download(new FileDownload(_id, _path, _isConfined,
2122 _rootPath, _url, _metadata, _headers));
2123
2124 download->start(); // change state
2125 download->startDownload();
2126
2127 // we need to set the data before we pause!!!
2128 QList<MethodData> calledMethods = _reqFactory->calledMethods();
2129 QCOMPARE(1, calledMethods.count());
2130 FakeNetworkReply* reply = qobject_cast<FakeNetworkReply*>(
2131 calledMethods[0].params().outParams()[0]);
2132
2133 // redirect all the required times
2134 foreach(const QString& url, urls) {
2135 reply->setAttribute(QNetworkRequest::RedirectionTargetAttribute,
2136 url);
2137 QSignalSpy replySpy(_reqFactory, SIGNAL(requestCreated(NetworkReply*)));
2138
2139 // use a spy to wait for the second reply
2140 reply->emitFinished();
2141
2142 // ensure that a second request is performed
2143 QTRY_COMPARE(1, replySpy.count());
2144
2145 reply = qobject_cast<FakeNetworkReply*>(
2146 replySpy.takeFirst().at(0).value<NetworkReply*>());
2147 redirectCount++;
2148 }
2149
2150 download->pause();
2151
2152 QSignalSpy errorSpy(download.data(), SIGNAL(error(QString)));
2153 QSignalSpy networkErrorSpy(download.data(),
2154 SIGNAL(networkError(NetworkErrorStruct)));
2155 QSignalSpy finishedSpy(download.data(), SIGNAL(finished(QString)));
2156
2157 reply->emitFinished();
2158
2159 QTRY_COMPARE(networkErrorSpy.count(), 0);
2160 QTRY_COMPARE(errorSpy.count(), 0);
2161 QTRY_COMPARE(finishedSpy.count(), 1);
2162 QCOMPARE(redirectCount, urls.count());
2163}
19892164
=== modified file 'ubuntu-download-manager-tests/downloads/test_download.h'
--- ubuntu-download-manager-tests/downloads/test_download.h 2014-01-27 16:36:44 +0000
+++ ubuntu-download-manager-tests/downloads/test_download.h 2014-02-04 16:23:22 +0000
@@ -154,6 +154,12 @@
154 void testFileSystemErrorProgress();154 void testFileSystemErrorProgress();
155 void testFileSystemErrorPause();155 void testFileSystemErrorPause();
156156
157 // test different redirects
158 void testRedirectCycle();
159 void testSingleRedirect();
160 void testSeveralRedirects_data();
161 void testSeveralRedirects();
162
157 private:163 private:
158 QString _id;164 QString _id;
159 bool _isConfined;165 bool _isConfined;

Subscribers

People subscribed via source and target branches