Refactor DeleteJob and DeleteApiJob to use SimpleFileJob.

Signed-off-by: alex-z <blackslayer4@gmail.com>
nextcloud · Jan 11, 2022 · 1b33852 · 1b33852
1 parent 7a3b641
commit 1b33852
Show file tree

Hide file tree

Showing 10 changed files with 97 additions and 93 deletions.
diff --git a/src/common/checksums.h b/src/common/checksums.h
@@ -175,7 +175,7 @@ class OCSYNC_EXPORT ValidateChecksumHeader : public QObject
 signals:
     void validated(const QByteArray &checksumType, const QByteArray &checksum);
     void validationFailed(const QString &errMsg, const QByteArray &calculatedChecksumType,
-        const QByteArray &calculatedChecksum, ValidateChecksumHeader::FailureReason reason);
+        const QByteArray &calculatedChecksum, const ValidateChecksumHeader::FailureReason reason);
 
 private slots:
     void slotChecksumCalculated(const QByteArray &checksumType, const QByteArray &checksum);

diff --git a/src/libsync/deletejob.cpp b/src/libsync/deletejob.cpp
@@ -21,12 +21,12 @@ namespace OCC {
 Q_LOGGING_CATEGORY(lcDeleteJob, "nextcloud.sync.networkjob.delete", QtInfoMsg)
 
 DeleteJob::DeleteJob(AccountPtr account, const QString &path, QObject *parent)
-    : AbstractNetworkJob(account, path, parent)
+    : SimpleFileJob(account, path, parent)
 {
 }
 
 DeleteJob::DeleteJob(AccountPtr account, const QUrl &url, QObject *parent)
-    : AbstractNetworkJob(account, QString(), parent)
+    : SimpleFileJob(account, QString(), parent)
     , _url(url)
 {
 }
@@ -39,24 +39,10 @@ void DeleteJob::start()
     }
 
     if (_url.isValid()) {
-        sendRequest("DELETE", _url, req);
+        startRequest("DELETE", _url, req);
     } else {
-        sendRequest("DELETE", makeDavUrl(path()), req);
+        startRequest("DELETE", req);
     }
-
-    if (reply()->error() != QNetworkReply::NoError) {
-        qCWarning(lcDeleteJob) << " Network error: " << reply()->errorString();
-    }
-    AbstractNetworkJob::start();
-}
-
-bool DeleteJob::finished()
-{
-    qCInfo(lcDeleteJob) << "DELETE of" << reply()->request().url() << "FINISHED WITH STATUS"
-                       << replyStatusString();
-
-    emit finishedSignal();
-    return true;
 }
 
 QByteArray DeleteJob::folderToken() const

diff --git a/src/libsync/deletejob.h b/src/libsync/deletejob.h
@@ -23,22 +23,18 @@ namespace OCC {
  * @brief The DeleteJob class
  * @ingroup libsync
  */
-class DeleteJob : public AbstractNetworkJob
+class DeleteJob : public SimpleFileJob
 {
     Q_OBJECT
 public:
     explicit DeleteJob(AccountPtr account, const QString &path, QObject *parent = nullptr);
     explicit DeleteJob(AccountPtr account, const QUrl &url, QObject *parent = nullptr);
 
     void start() override;
-    bool finished() override;
 
     QByteArray folderToken() const;
     void setFolderToken(const QByteArray &folderToken);
 
-signals:
-    void finishedSignal();
-
 private:
     QUrl _url; // Only used if the constructor taking a url is taken.
     QByteArray _folderToken;

diff --git a/src/libsync/networkjobs.cpp b/src/libsync/networkjobs.cpp
@@ -55,6 +55,7 @@ Q_LOGGING_CATEGORY(lcMkColJob, "nextcloud.sync.networkjob.mkcol", QtInfoMsg)
 Q_LOGGING_CATEGORY(lcProppatchJob, "nextcloud.sync.networkjob.proppatch", QtInfoMsg)
 Q_LOGGING_CATEGORY(lcJsonApiJob, "nextcloud.sync.networkjob.jsonapi", QtInfoMsg)
 Q_LOGGING_CATEGORY(lcDetermineAuthTypeJob, "nextcloud.sync.networkjob.determineauthtype", QtInfoMsg)
+Q_LOGGING_CATEGORY(lcSimpleFileJob, "nextcloud.sync.networkjob.simplefilejob", QtInfoMsg)
 const int notModifiedStatusCode = 304;
 
 QByteArray parseEtag(const char *header)
@@ -1090,22 +1091,33 @@ SimpleFileJob::SimpleFileJob(AccountPtr account, const QString &filePath, QObjec
 }
 
 QNetworkReply *SimpleFileJob::startRequest(
-    const QByteArray &verb, QNetworkRequest req, QIODevice *requestBody)
+    const QByteArray &verb, const QNetworkRequest req, QIODevice *requestBody)
 {
-    const auto davUrlString = makeDavUrl(path()).toString();
-    auto reply = sendRequest(verb, makeDavUrl(path()), req, requestBody);
-    start();
+    return startRequest(verb, makeDavUrl(path()), req, requestBody);
+}
+
+QNetworkReply *SimpleFileJob::startRequest(
+    const QByteArray &verb, const QUrl &url, const QNetworkRequest req, QIODevice *requestBody)
+{
+    _verb = verb;
+    const auto reply = sendRequest(verb, url, req, requestBody);
+
+    if (reply->error() != QNetworkReply::NoError) {
+        qCWarning(lcSimpleFileJob) << verb << " Network error: " << reply->errorString();
+    }
+    AbstractNetworkJob::start();
     return reply;
 }
 
 bool SimpleFileJob::finished()
 {
+    qCInfo(lcSimpleFileJob) << _verb << "for" << reply()->request().url() << "FINISHED WITH STATUS" << replyStatusString();
     emit finishedSignal(reply());
     return true;
 }
 
 DeleteApiJob::DeleteApiJob(AccountPtr account, const QString &path, QObject *parent)
-    : AbstractNetworkJob(account, path, parent)
+    : SimpleFileJob(account, path, parent)
 {
 
 }
@@ -1114,14 +1126,13 @@ void DeleteApiJob::start()
 {
     QNetworkRequest req;
     req.setRawHeader("OCS-APIREQUEST", "true");
-    QUrl url = Utility::concatUrlPath(account()->url(), path());
-    sendRequest("DELETE", url, req);
-    AbstractNetworkJob::start();
+
+    startRequest("DELETE", req);
 }
 
 bool DeleteApiJob::finished()
 {
-    qCInfo(lcJsonApiJob) << "JsonApiJob of" << reply()->request().url() << "FINISHED WITH STATUS"
+    qCInfo(lcJsonApiJob) << "DeleteApiJob of" << reply()->request().url() << "FINISHED WITH STATUS"
                          << reply()->error()
                          << (reply()->error() == QNetworkReply::NoError ? QLatin1String("") : errorString());
 
@@ -1137,7 +1148,7 @@ bool DeleteApiJob::finished()
     const auto replyData = QString::fromUtf8(reply()->readAll());
     qCInfo(lcJsonApiJob()) << "TMX Delete Job" << replyData;
     emit result(httpStatus);
-    return true;
+    return SimpleFileJob::finished();
 }
 
 void fetchPrivateLinkUrl(AccountPtr account, const QString &remotePath,

diff --git a/src/libsync/networkjobs.h b/src/libsync/networkjobs.h
@@ -60,14 +60,39 @@ private slots:
     bool finished() override;
 };
 
+/**
+ * @brief A basic file manipulation job
+ * @ingroup libsync
+ */
+class OWNCLOUDSYNC_EXPORT SimpleFileJob : public AbstractNetworkJob
+{
+    Q_OBJECT
+public:
+    explicit SimpleFileJob(AccountPtr account, const QString &filePath, QObject *parent = nullptr);
+
+    QNetworkReply *startRequest(
+        const QByteArray &verb, const QNetworkRequest req = QNetworkRequest(), QIODevice *requestBody = nullptr);
+
+    QNetworkReply *startRequest(const QByteArray &verb, const QUrl &url, const QNetworkRequest req = QNetworkRequest(),
+        QIODevice *requestBody = nullptr);
+
+signals:
+    void finishedSignal(QNetworkReply *reply);
+protected slots:
+    bool finished() override;
+
+private:
+    QByteArray _verb;
+};
+
 /**
  * @brief sends a DELETE http request to a url.
  *
  * See Nextcloud API usage for the possible DELETE requests.
  *
  * This does *not* delete files, it does a http request.
  */
-class OWNCLOUDSYNC_EXPORT DeleteApiJob : public AbstractNetworkJob
+class OWNCLOUDSYNC_EXPORT DeleteApiJob : public SimpleFileJob
 {
     Q_OBJECT
 public:
@@ -498,24 +523,6 @@ private slots:
     bool finished() override;
 };
 
-/**
- * @brief A basic file manipulation job
- * @ingroup libsync
- */
-class OWNCLOUDSYNC_EXPORT SimpleFileJob : public AbstractNetworkJob
-{
-    Q_OBJECT
-public:
-    explicit SimpleFileJob(AccountPtr account, const QString &filePath, QObject *parent = nullptr);
-
-    QNetworkReply *startRequest(const QByteArray &verb, QNetworkRequest req = QNetworkRequest(),  QIODevice *requestBody = nullptr);
-
-signals:
-    void finishedSignal(QNetworkReply *reply);
-private slots:
-    bool finished() override;
-};
-
 /**
  * @brief Runs a PROPFIND to figure out the private link url
  *

diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp
@@ -923,53 +923,55 @@ void PropagateDownloadFile::slotGetFinished()
 }
 
 void PropagateDownloadFile::slotChecksumFail(const QString &errMsg,
-    const QByteArray &calculatedChecksumType, const QByteArray &calculatedChecksum, ValidateChecksumHeader::FailureReason reason)
+    const QByteArray &calculatedChecksumType, const QByteArray &calculatedChecksum, const ValidateChecksumHeader::FailureReason reason)
 {
-    const auto processChecksumFailure = [this, errMsg]() {
-        FileSystem::remove(_tmpFile.fileName());
-        propagator()->_anotherSyncNeeded = true;
-        done(SyncFileItem::SoftError, errMsg); // tr("The file downloaded with a broken checksum, will be redownloaded."));
-    };
-
-    if (reason == ValidateChecksumHeader::FailureReason::ChecksumMismatch
-        && propagator()->account()->isChecksumRecalculateRequestSupported()) {
+    if (reason == ValidateChecksumHeader::FailureReason::ChecksumMismatch && propagator()->account()->isChecksumRecalculateRequestSupported()) {
             const QByteArray calculatedChecksumHeader(calculatedChecksumType + ':' + calculatedChecksum);
             const QString fullRemotePathForFile(propagator()->fullRemotePath(_isEncrypted ? _item->_encryptedFileName : _item->_file));
             auto *job = new SimpleFileJob(propagator()->account(), fullRemotePathForFile);
-            QObject::connect(job, &SimpleFileJob::finishedSignal, this, [this, calculatedChecksumHeader, processChecksumFailure](QNetworkReply *reply) {
-                if (reply->error() == QNetworkReply::NoError) {
-                    const auto newChecksumHeaderFromServer = reply->rawHeader(checkSumHeaderC);
-                    if (newChecksumHeaderFromServer == calculatedChecksumHeader) {
-                        const auto newChecksumHeaderFromServerSplit = newChecksumHeaderFromServer.split(':');
-                        if (newChecksumHeaderFromServerSplit.size() > 1) {
-                            transmissionChecksumValidated(
-                                newChecksumHeaderFromServerSplit.first(), newChecksumHeaderFromServerSplit.last());
-                            return;
-                        }
-                    }
-
-                    qCCritical(lcPropagateDownload) << "Checksum recalculation has failed for file:" << reply->url()
-                                                    << " " << checkSumHeaderC << " received is:" << newChecksumHeaderFromServer;
-                }
-
-                if (reply->error() != QNetworkReply::NoError) {
-                    qCCritical(lcPropagateDownload)
-                        << "Checksum recalculation has failed for file:" << reply->url()
-                        << " Recalculation request has finished with error:" << reply->errorString();
-                }
-
-                processChecksumFailure();
+            QObject::connect(job, &SimpleFileJob::finishedSignal, this,
+                [this, calculatedChecksumHeader, errMsg](const QNetworkReply *reply) { processChecksumRecalculate(reply, calculatedChecksumHeader, errMsg);
             });
 
             qCWarning(lcPropagateDownload) << "Checksum validation has failed for file:" << fullRemotePathForFile
                                            << " Requesting checksum recalculation on the server...";
             QNetworkRequest req;
-            req.setRawHeader(checksumRecalculateOnServer, calculatedChecksumType);
+            req.setRawHeader(checksumRecalculateOnServerHeaderC, calculatedChecksumType);
             job->startRequest(QByteArrayLiteral("PATCH"), req);
             return;
     }
 
-    processChecksumFailure();
+    checksumValidateFailedAbortDownload(errMsg);
+}
+
+void PropagateDownloadFile::processChecksumRecalculate(const QNetworkReply *reply, const QByteArray &originalChecksumHeader, const QString &errorMessage)
+{
+    if (reply->error() != QNetworkReply::NoError) {
+        qCCritical(lcPropagateDownload) << "Checksum recalculation has failed for file:" << reply->url()
+                                        << " Recalculation request has finished with error:" << reply->errorString();
+        checksumValidateFailedAbortDownload(errorMessage);
+        return;
+    }
+
+    const auto newChecksumHeaderFromServer = reply->rawHeader(checkSumHeaderC);
+    if (newChecksumHeaderFromServer == originalChecksumHeader) {
+        const auto newChecksumHeaderFromServerSplit = newChecksumHeaderFromServer.split(':');
+        if (newChecksumHeaderFromServerSplit.size() > 1) {
+            transmissionChecksumValidated(newChecksumHeaderFromServerSplit.first(), newChecksumHeaderFromServerSplit.last());
+            return;
+        }
+    }
+
+    qCCritical(lcPropagateDownload) << "Checksum recalculation has failed for file:" << reply->url() << " "
+                                    << checkSumHeaderC << " received is:" << newChecksumHeaderFromServer;
+    checksumValidateFailedAbortDownload(errorMessage);
+}
+
+void PropagateDownloadFile::checksumValidateFailedAbortDownload(const QString &errMsg)
+{
+    FileSystem::remove(_tmpFile.fileName());
+    propagator()->_anotherSyncNeeded = true;
+    done(SyncFileItem::SoftError, errMsg); // tr("The file downloaded with a broken checksum, will be redownloaded."));
 }
 
 void PropagateDownloadFile::deleteExistingFolder()

diff --git a/src/libsync/propagatedownload.h b/src/libsync/propagatedownload.h
@@ -237,7 +237,9 @@ private slots:
     void abort(PropagatorJob::AbortType abortType) override;
     void slotDownloadProgress(qint64, qint64);
     void slotChecksumFail(const QString &errMsg, const QByteArray &calculatedChecksumType,
-        const QByteArray &calculatedChecksum, ValidateChecksumHeader::FailureReason reason);
+        const QByteArray &calculatedChecksum, const ValidateChecksumHeader::FailureReason reason);
+    void processChecksumRecalculate(const QNetworkReply *reply, const QByteArray &originalChecksumHeader, const QString &errorMessage);
+    void checksumValidateFailedAbortDownload(const QString &errMsg);
 
 private:
     void startAfterIsEncryptedIsChecked();

diff --git a/src/libsync/propagatorjobs.h b/src/libsync/propagatorjobs.h
@@ -24,9 +24,9 @@ namespace OCC {
  * Tags for checksum header.
  * It's here for being shared between Upload- and Download Job
  */
-static const char checkSumHeaderC[] = "OC-Checksum";
-static const char contentMd5HeaderC[] = "Content-MD5";
-static const char checksumRecalculateOnServer[] = "X-Recalculate-Hash";
+constexpr auto checkSumHeaderC = "OC-Checksum";
+constexpr auto contentMd5HeaderC = "Content-MD5";
+constexpr auto checksumRecalculateOnServerHeaderC = "X-Recalculate-Hash";
 
 /**
  * @brief Declaration of the other propagation jobs

diff --git a/test/testchecksumvalidator.cpp b/test/testchecksumvalidator.cpp
@@ -44,7 +44,7 @@ using namespace OCC::Utility;
     }
 
     void slotDownError(const QString &errMsg, const QByteArray &calculatedChecksumType,
-        const QByteArray &calculatedChecksum, ValidateChecksumHeader::FailureReason reason)
+        const QByteArray &calculatedChecksum, const ValidateChecksumHeader::FailureReason reason)
     {
         Q_UNUSED(calculatedChecksumType);
         Q_UNUSED(calculatedChecksum);

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -565,7 +565,7 @@ private slots:
                     reply->setRawHeader(OCC::contentMd5HeaderC, contentMd5Value);
                 return reply;
             } else if (op == QNetworkAccessManager::CustomOperation) {
-                if (request.hasRawHeader(OCC::checksumRecalculateOnServer)) {
+                if (request.hasRawHeader(OCC::checksumRecalculateOnServerHeaderC)) {
                     if (!isChecksumRecalculateSupported) {
                         return new FakeErrorReply(op, request, &parent, 402);
                     }