Skip to content

Commit c0ca583

Browse files
mgallienmgallien
authored
Merge pull request #8947 from nextcloud/bugfix/8915/lscol-db-items
fix(vfs/cfapi): avoid creating invalid db entries when using a different sync root
2 parents d2881a3 + f7799c9 commit c0ca583

File tree

6 files changed

+153
-104
lines changed

6 files changed

+153
-104
lines changed

‎src/libsync/account.cpp‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "accountfwd.h"
1010
#include "capabilities.h"
1111
#include "clientsideencryptionjobs.h"
12+
#include "common/utility.h"
1213
#include "configfile.h"
1314
#include "cookiejar.h"
1415
#include "creds/abstractcredentials.h"
@@ -1174,61 +1175,64 @@ void Account::setAskUserForMnemonic(const bool ask)
11741175
emit askUserForMnemonicChanged();
11751176
}
11761177

1177-
void Account::listRemoteFolder(QPromise<OCC::PlaceholderCreateInfo> *promise, const QString &path, SyncJournalDb *journalForFolder)
1178+
void Account::listRemoteFolder(QPromise<OCC::PlaceholderCreateInfo> *promise, const QString &remoteSyncRootPath, const QString &subPath, SyncJournalDb *journalForFolder)
11781179
{
1179-
qCInfo(lcAccount()) << "ls col job requested for" << path;
1180+
qCInfo(lcAccount()) << "ls col job requested for" << subPath;
11801181

11811182
if (!credentials()->ready()) {
1182-
qCWarning(lcAccount()) << "credentials are not ready" << path;
1183+
qCWarning(lcAccount()) << "credentials are not ready" << subPath;
11831184
promise->finish();
11841185
return;
11851186
}
11861187

1187-
auto listFolderJob = new OCC::LsColJob{sharedFromThis(), path};
1188+
auto listFolderJob = new OCC::LsColJob{sharedFromThis(), subPath};
11881189

11891190
const auto props = LsColJob::defaultProperties(LsColJob::FolderType::ChildFolder, sharedFromThis());
11901191

11911192
listFolderJob->setProperties(props);
11921193

1193-
QObject::connect(listFolderJob, &OCC::LsColJob::networkError, this, [promise, path] (QNetworkReply *reply) {
1194+
QObject::connect(listFolderJob, &OCC::LsColJob::networkError, this, [promise, subPath] (QNetworkReply *reply) {
11941195
if (reply) {
1195-
qCWarning(lcAccount()) << "ls col job" << path << "error" << reply->errorString();
1196+
qCWarning(lcAccount()) << "ls col job" << subPath << "error" << reply->errorString();
11961197
}
11971198

1198-
qCWarning(lcAccount()) << "ls col job" << path << "error without a reply";
1199+
qCWarning(lcAccount()) << "ls col job" << subPath << "error without a reply";
11991200
promise->finish();
12001201
});
12011202

1202-
QObject::connect(listFolderJob, &OCC::LsColJob::finishedWithError, this, [promise, path] (QNetworkReply *reply) {
1203+
QObject::connect(listFolderJob, &OCC::LsColJob::finishedWithError, this, [promise, subPath] (QNetworkReply *reply) {
12031204
if (reply) {
1204-
qCWarning(lcAccount()) << "ls col job" << path << "error" << reply->errorString();
1205+
qCWarning(lcAccount()) << "ls col job" << subPath << "error" << reply->errorString();
12051206
}
12061207

1207-
qCWarning(lcAccount()) << "ls col job" << path << "error without a reply";
1208+
qCWarning(lcAccount()) << "ls col job" << subPath << "error without a reply";
12081209
promise->finish();
12091210
});
12101211

1211-
QObject::connect(listFolderJob, &OCC::LsColJob::finishedWithoutError, this, [promise, path] () {
1212-
qCInfo(lcAccount()) << "ls col job" << path << "finished";
1212+
QObject::connect(listFolderJob, &OCC::LsColJob::finishedWithoutError, this, [promise, subPath] () {
1213+
qCInfo(lcAccount()) << "ls col job" << subPath << "finished";
12131214
promise->finish();
12141215
});
12151216

1216-
QObject::connect(listFolderJob, &OCC::LsColJob::directoryListingIterated, this, [promise, path, journalForFolder, this](const QString &name, const QMap<QString, QString> &properties) {
1217-
// `name` is e.g. "/remote.php/dav/files/admin" or "/remote.php/dav/files/admin/SomeFolder"; whereas `path` is e.g. "" or "SomeFolder/"
1218-
// in case these two are equal we are currently iterating the entry for the current directory
1219-
const auto serverPath = name.mid(this->davPath().size());
1220-
const auto isRootCollection = serverPath.isEmpty() && path.isEmpty();
1221-
const auto isCurrentCollection = isRootCollection || serverPath == Utility::noTrailingSlashPath(path);
1222-
if (isCurrentCollection) {
1217+
listFolderJob->setProperty("ignoredFirst", false);
1218+
const auto baseRemotePath = Utility::trailingSlashPath(Utility::noLeadingSlashPath(remoteSyncRootPath));
1219+
auto syncRootPath = subPath;
1220+
if (subPath.startsWith(baseRemotePath)) {
1221+
syncRootPath = syncRootPath.mid(baseRemotePath.size());
1222+
}
1223+
1224+
QObject::connect(listFolderJob, &OCC::LsColJob::directoryListingIterated, this, [promise, remoteSyncRootPath, syncRootPath, journalForFolder, this](const QString &completeDavPath, const QMap<QString, QString> &properties) {
1225+
if (!sender()->property("ignoredFirst").toBool()) {
12231226
qCDebug(lcAccount()) << "skip first item";
1227+
sender()->setProperty("ignoredFirst", true);
12241228
return;
12251229
}
12261230

1227-
qCInfo(lcAccount()) << "ls col job" << path << "new file" << name << properties.count();
1231+
qCInfo(lcAccount()) << "ls col job" << syncRootPath << "new file" << completeDavPath << properties.count();
12281232

1229-
const auto slash = name.lastIndexOf('/');
1230-
const auto itemFileName = name.mid(slash + 1);
1231-
const auto absoluteItemPathName = (path.isEmpty() ? itemFileName : path + "/" + itemFileName);
1233+
const auto slash = completeDavPath.lastIndexOf('/');
1234+
const auto itemFileName = completeDavPath.mid(slash + 1);
1235+
const auto absoluteItemPathName = syncRootPath.isEmpty() ? itemFileName : Utility::noTrailingSlashPath(syncRootPath) + '/' + itemFileName;
12321236

12331237
auto currentItemDbRecord = SyncJournalFileRecord{};
12341238
if (journalForFolder->getFileRecord(absoluteItemPathName, &currentItemDbRecord) && currentItemDbRecord.isValid()) {

‎src/libsync/account.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -425,7 +425,8 @@ public slots:
425425
void setAskUserForMnemonic(const bool ask);
426426

427427
void listRemoteFolder(QPromise<OCC::PlaceholderCreateInfo> *promise,
428-
const QString &path,
428+
const QString &remoteSyncRootPath,
429+
const QString &subPath,
429430
SyncJournalDb *journalForFolder);
430431

431432
signals:

‎src/libsync/discoveryphase.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -111,10 +111,6 @@ void DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path,
111111
return callback(false);
112112
}
113113

114-
if (_syncOptions._vfs->mode() == Vfs::WindowsCfApi) {
115-
return callback(true);
116-
}
117-
118114
checkFolderSizeLimit(path, [this, path, callback](const bool bigFolder) {
119115
if (bigFolder) {
120116
// we tell the UI there is a new folder

‎src/libsync/vfs/cfapi/cfapiwrapper.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,8 @@ void CALLBACK cfApiFetchPlaceHolders(const CF_CALLBACK_INFO *callbackInfo, const
553553
sendTransferError();
554554
return;
555555
}
556-
const auto serverPath = QString{vfs->params().remotePath + pathString.mid(rootPath.length() + 1)}.mid(1);
556+
const auto remoteSyncRootPath = vfs->params().remotePath; // with leading slash
557+
const auto serverPath = QString{remoteSyncRootPath + pathString.mid(rootPath.length() + 1)}.mid(1);
557558

558559
qCDebug(lcCfApiWrapper) << "fetch placeholder:" << path << serverPath << requestId;
559560

@@ -584,7 +585,7 @@ void CALLBACK cfApiFetchPlaceHolders(const CF_CALLBACK_INFO *callbackInfo, const
584585
qCInfo(lcCfApiWrapper()) << "ls prop started";
585586
});
586587

587-
QMetaObject::invokeMethod(vfs->params().account.data(), &OCC::Account::listRemoteFolder, &lsPropPromise, serverPath, vfs->params().journal);
588+
QMetaObject::invokeMethod(vfs->params().account.data(), &OCC::Account::listRemoteFolder, &lsPropPromise, remoteSyncRootPath, serverPath, vfs->params().journal);
588589

589590
qCInfo(lcCfApiWrapper()) << "ls prop requested" << path << serverPath;
590591

‎test/testaccount.cpp‎

Lines changed: 59 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,10 @@
1212
#include <QTemporaryDir>
1313
#include <QtTest>
1414

15-
#include "common/utility.h"
16-
#include "folderman.h"
1715
#include "account.h"
1816
#include "accountstate.h"
19-
#include "configfile.h"
20-
#include "testhelper.h"
2117
#include "logger.h"
18+
#include "syncenginetestutils.h"
2219

2320
using namespace OCC;
2421

@@ -100,7 +97,64 @@ private slots:
10097
setLimitSettings(LimitSetting::LegacyGlobalLimit);
10198
verifyLimitSettings(LimitSetting::NoLimit);
10299
}
100+
101+
void testAccount_listRemoteFolder_data()
102+
{
103+
QTest::addColumn<QString>("remotePath");
104+
QTest::addColumn<QString>("subPath");
105+
QTest::addColumn<QStringList>("expectedPaths");
106+
107+
QTest::newRow("root = /; requesting ''") << "" << "" << QStringList{ "A", "B", "C", "S" };
108+
QTest::newRow("root = /; requesting A/") << "" << "A/" << QStringList{ "A/a1", "A/a2", "A/sub1" };
109+
QTest::newRow("root = /; requesting A/sub1/") << "" << "A/sub1/" << QStringList{ "A/sub1/sub2" };
110+
QTest::newRow("root = /; requesting A/sub1/sub2") << "" << "A/sub1/sub2/" << QStringList{};
111+
QTest::newRow("root = /A; requesting A/") << "/A" << "A/" << QStringList{ "a1", "a2", "sub1" };
112+
QTest::newRow("root = /A; requesting A/sub1") << "/A" << "A/sub1/" << QStringList{ "sub1/sub2" };
113+
QTest::newRow("root = /A; requesting A/sub1/sub2") << "/A" << "A/sub1/sub2/" << QStringList{};
114+
}
115+
116+
void testAccount_listRemoteFolder()
117+
{
118+
QFETCH(QString, remotePath);
119+
QFETCH(QString, subPath);
120+
QFETCH(QStringList, expectedPaths);
121+
122+
FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
123+
fakeFolder.remoteModifier().mkdir("A/sub1");
124+
fakeFolder.remoteModifier().mkdir("A/sub1/sub2");
125+
const auto account = fakeFolder.account();
126+
127+
QEventLoop localEventLoop;
128+
auto lsPropPromise = QPromise<OCC::PlaceholderCreateInfo>{};
129+
auto lsPropFuture = lsPropPromise.future();
130+
auto lsPropFutureWatcher = QFutureWatcher<OCC::PlaceholderCreateInfo>{};
131+
lsPropFutureWatcher.setFuture(lsPropFuture);
132+
133+
QList<QString> receivedPaths;
134+
135+
QObject::connect(&lsPropFutureWatcher, &QFutureWatcher<QStringList>::finished,
136+
&localEventLoop, [&localEventLoop] () {
137+
qInfo() << "ls prop finished";
138+
localEventLoop.quit();
139+
});
140+
141+
QObject::connect(&lsPropFutureWatcher, &QFutureWatcher<QStringList>::resultReadyAt,
142+
&localEventLoop, [&receivedPaths, &lsPropFutureWatcher] (int resultIndex) {
143+
qInfo() << "ls prop result at index" << resultIndex;
144+
const auto &newResultEntries = lsPropFutureWatcher.resultAt(resultIndex);
145+
receivedPaths.append(newResultEntries.fullPath);
146+
});
147+
148+
fakeFolder.syncJournal().clearFileTable(); // pretend we only need to fetch placeholders
149+
150+
account->listRemoteFolder(&lsPropPromise, remotePath, subPath, &fakeFolder.syncJournal());
151+
localEventLoop.exec();
152+
153+
receivedPaths.sort();
154+
expectedPaths.sort();
155+
QCOMPARE_EQ(receivedPaths, expectedPaths);
156+
}
103157
};
104158

105-
QTEST_APPLESS_MAIN(TestAccount)
159+
QTEST_GUILESS_MAIN(TestAccount)
106160
#include "testaccount.moc"

0 commit comments

Comments
 (0)