Trim trailing spaces before uploading files

Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

src/libsync/discovery.cpp

@@ -15,9 +15,12 @@
 #include "discovery.h"
 #include "common/filesystembase.h"
 #include "common/syncjournaldb.h"
+#include "filesystem.h"
 #include "syncfileitem.h"
 #include <QDebug>
 #include <algorithm>
+#include <QEventLoop>
+#include <QDir>
 #include <set>
 #include <QTextCodec>
 #include "vio/csync_vio_local.h"
@@ -34,6 +37,50 @@ namespace OCC {
 
 Q_LOGGING_CATEGORY(lcDisco, "sync.discovery", QtInfoMsg)
 
+
+bool ProcessDirectoryJob::checkForInvalidFileName(const PathTuple &path,
+    const std::map<QString, Entries> &entries, Entries &entry)
+{
+    const auto originalFileName = entry.localEntry.name;
+    const auto newFileName = originalFileName.trimmed();
+
+    if (originalFileName == newFileName) {
+        return true;
+    }
+
+    const auto entriesIter = entries.find(newFileName);
+    if (entriesIter != entries.end()) {
+        QString errorMessage;
+        const auto newFileNameEntry = entriesIter->second;
+        if (newFileNameEntry.serverEntry.isValid()) {
+            errorMessage = tr("File contains trailing spaces and coudn't be renamed, because a file with the same name already exists on the server.");
+        }
+        if (newFileNameEntry.localEntry.isValid()) {
+            errorMessage = tr("File contains trailing spaces and coudn't be renamed, because a file with the same name already exists locally.");
+        }
+
+        if (!errorMessage.isEmpty()) {
+            auto item = SyncFileItemPtr::create();
+            if (entry.localEntry.isDirectory) {
+                item->_type = CSyncEnums::ItemTypeDirectory;
+            } else {
+                item->_type = CSyncEnums::ItemTypeFile;
+            }
+            item->_file = path._target;
+            item->_originalFile = path._target;
+            item->_instruction = CSYNC_INSTRUCTION_ERROR;
+            item->_status = SyncFileItem::NormalError;
+            item->_errorString = errorMessage;
+            emit _discoveryData->itemDiscovered(item);
+            return false;
+        }
+    }
+
+    entry.localEntry.renameName = newFileName;
+
+    return true;
+}
+
 void ProcessDirectoryJob::start()
 {
     qCInfo(lcDisco) << "STARTING" << _currentFolder._server << _queryServer << _currentFolder._local << _queryLocal;
@@ -73,12 +120,6 @@ void ProcessDirectoryJob::process()
     // However, if foo and foo.owncloud exists locally, there'll be "foo"
     // with local, db, server entries and "foo.owncloud" with only a local
     // entry.
-    struct Entries {
-        QString nameOverride;
-        SyncJournalFileRecord dbEntry;
-        RemoteInfo serverEntry;
-        LocalInfo localEntry;
-    };
     std::map<QString, Entries> entries;
     for (auto &e : _serverNormalQueryEntries) {
         entries[e.name].serverEntry = std::move(e);
@@ -136,8 +177,8 @@ void ProcessDirectoryJob::process()
     //
     // Iterate over entries and process them
     //
-    for (const auto &f : entries) {
-        const auto &e = f.second;
+    for (auto &f : entries) {
+        auto &e = f.second;
 
         PathTuple path;
         path = _currentFolder.addName(e.nameOverride.isEmpty() ? f.first : e.nameOverride);
@@ -192,6 +233,9 @@ void ProcessDirectoryJob::process()
             processBlacklisted(path, e.localEntry, e.dbEntry);
             continue;
         }
+        if (!checkForInvalidFileName(path, entries, e)) {
+            continue;
+        }
         processFile(std::move(path), e.localEntry, e.serverEntry, e.dbEntry);
     }
     QTimer::singleShot(0, _discoveryData, &DiscoveryPhase::scheduleMoreJobs);
@@ -345,6 +389,7 @@ void ProcessDirectoryJob::processFile(PathTuple path,
     item->_originalFile = path._original;
     item->_previousSize = dbEntry._fileSize;
     item->_previousModtime = dbEntry._modtime;
+    item->_renameTarget = localEntry.renameName;
 
     if (dbEntry._modtime == localEntry.modtime && dbEntry._type == ItemTypeVirtualFile && localEntry.type == ItemTypeFile) {
         item->_type = ItemTypeFile;

src/libsync/discovery.h

@@ -104,6 +104,13 @@ public:
     SyncFileItemPtr _dirItem;
 
 private:
+    struct Entries
+    {
+        QString nameOverride;
+        SyncJournalFileRecord dbEntry;
+        RemoteInfo serverEntry;
+        LocalInfo localEntry;
+    };
 
     /** Structure representing a path during discovery. A same path may have different value locally
      * or on the server in case of renames.
@@ -143,6 +150,8 @@ private:
         }
     };
 
+    bool checkForInvalidFileName(const PathTuple &path, const std::map<QString, Entries> &entries, Entries &entry);
+
     /** Iterate over entries inside the directory (non-recursively).
      *
      * Called once _serverEntries and _localEntries are filled

src/libsync/discoveryphase.h

@@ -70,6 +70,7 @@ struct LocalInfo
 {
     /** FileName of the entry (this does not contains any directory or path, just the plain name */
     QString name;
+    QString renameName;
     time_t modtime = 0;
     int64_t size = 0;
     uint64_t inode = 0;

src/libsync/propagateupload.cpp

@@ -217,6 +217,20 @@ void PropagateUploadFileCommon::start()
     const auto slashPosition = path.lastIndexOf('/');
     const auto parentPath = slashPosition >= 0 ? path.left(slashPosition) : QString();
 
+
+    if (!_item->_renameTarget.isEmpty() && _item->_file != _item->_renameTarget) {
+        // Try to rename the file
+        const auto originalFilePathAbsolute = propagator()->fullLocalPath(_item->_file);
+        const auto newFilePathAbsolute = propagator()->fullLocalPath(_item->_renameTarget);
+        const auto renameSuccess = QFile::rename(originalFilePathAbsolute, newFilePathAbsolute);
+        if (!renameSuccess) {
+            done(SyncFileItem::NormalError, "File contains trailing spaces and couldn't be renamed");
+            return;
+        }
+        _item->_file = _item->_renameTarget;
+        _item->_modtime = FileSystem::getModTime(newFilePathAbsolute);
+    }
+
     SyncJournalFileRecord parentRec;
     bool ok = propagator()->_journal->getFileRecord(parentPath, &parentRec);
     if (!ok) {

test/testlocaldiscovery.cpp

@@ -204,6 +204,71 @@ private slots:
         QVERIFY(!fakeFolder.currentRemoteState().find("C/.foo"));
         QVERIFY(!fakeFolder.currentRemoteState().find("C/bar"));
     }
+
+    void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameAndUploadFile()
+    {
+        FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces1(" foo");
+        const QString fileWithSpaces2(" bar  ");
+        const QString fileWithSpaces3("bla ");
+
+        fakeFolder.localModifier().insert(fileWithSpaces1);
+        fakeFolder.localModifier().insert(fileWithSpaces2);
+        fakeFolder.localModifier().insert(fileWithSpaces3);
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces1));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces1.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces1));
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces2));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces2.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces2));
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces3));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces3.trimmed()));
+        QVERIFY(!fakeFolder.currentLocalState().find(fileWithSpaces3));
+    }
+
+    void testCreateFileWithTrailingSpaces_localTrimmedDoesExist_dontRenameAndUploadFile()
+    {
+        FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces(" foo");
+        const QString fileTrimmed("foo");
+
+        fakeFolder.localModifier().insert(fileTrimmed);
+        QVERIFY(fakeFolder.syncOnce());
+        fakeFolder.localModifier().insert(fileWithSpaces);
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileTrimmed));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces));
+        QVERIFY(fakeFolder.currentLocalState().find(fileTrimmed));
+    }
+
+    void testCreateFileWithTrailingSpaces_localTrimmedAlsoCreated_dontRenameAndUploadFile()
+    {
+        FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+        const QString fileWithSpaces(" foo");
+        const QString fileTrimmed("foo");
+
+        fakeFolder.localModifier().insert(fileTrimmed);
+        fakeFolder.localModifier().insert(fileWithSpaces);
+        QVERIFY(!fakeFolder.syncOnce());
+
+        QVERIFY(fakeFolder.currentRemoteState().find(fileTrimmed));
+        QVERIFY(!fakeFolder.currentRemoteState().find(fileWithSpaces));
+        QVERIFY(fakeFolder.currentLocalState().find(fileWithSpaces));
+        QVERIFY(fakeFolder.currentLocalState().find(fileTrimmed));
+    }
 };
 
 QTEST_GUILESS_MAIN(TestLocalDiscovery)