From d0986e2a87f762c30586e31278d819967a4d40bb Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Wed, 15 Jan 2025 14:45:34 +0100 Subject: [PATCH 1/6] Add serverPath column to db metadata table This is a first step to fix issues where file names are normalized differently on the server vs. the client. --- src/common/syncjournaldb.cpp | 40 +++++++++++++++++++++--------- src/common/syncjournalfilerecord.h | 1 + 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index 5ecc5d8f06d..d86b5d64cf2 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -47,11 +47,12 @@ Q_LOGGING_CATEGORY(lcDb, "sync.database", QtInfoMsg) namespace { // base query used to select file record objects, used in combination with WHERE statements. -const auto getFileRecordQueryC = QByteArrayLiteral("SELECT path, inode, modtime, type, md5, fileid, remotePerm, filesize," - " ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum," - " hasDirtyPlaceholder" - " FROM metadata" - " LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id "); +const auto getFileRecordQueryC = + QByteArrayLiteral("SELECT path, inode, modtime, type, md5, fileid, remotePerm, filesize," + " ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum," + " hasDirtyPlaceholder, serverPath" + " FROM metadata" + " LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id "); void fillFileRecordFromGetQuery(OCC::SyncJournalFileRecord &rec, OCC::SqlQuery &query) @@ -66,6 +67,7 @@ void fillFileRecordFromGetQuery(OCC::SyncJournalFileRecord &rec, OCC::SqlQuery & rec._fileSize = query.int64Value(7); rec._serverHasIgnoredFiles = (query.intValue(8) > 0); rec._checksumHeader = query.baValue(9); + rec._serverPath = query.baValue(11); } QByteArray defaultJournalMode(const QString &dbPath) @@ -347,6 +349,7 @@ bool SyncJournalDb::checkConnect() // contentChecksum // contentChecksumTypeId // hasDirtyPlaceholder + // serverPath "PRIMARY KEY(phash)" ");"); @@ -714,6 +717,17 @@ bool SyncJournalDb::updateMetadataTableStructure() commitInternal(QStringLiteral("update database structure: add hasDirtyPlaceholder col")); } + if (columns.indexOf("serverPath") == -1) { + SqlQuery addDirtyQuery(_db); + addDirtyQuery.prepare("ALTER TABLE metadata ADD COLUMN serverPath VARCHAR(4096);"); + if (!addDirtyQuery.exec()) { + sqlFail(QStringLiteral("updateMetadataTableStructure: add serverPath column"), addDirtyQuery); + re = false; + } + // TODO: add index? + commitInternal(QStringLiteral("update database structure: add serverPath col")); + } + auto uploadInfoColumns = tableColumns("uploadinfo"); if (uploadInfoColumns.isEmpty()) return false; @@ -848,10 +862,9 @@ Result SyncJournalDb::setFileRecord(const SyncJournalFileRecord & } } OC_ASSERT(!record._remotePerm.isNull()); - qCInfo(lcDb) << "Updating file record for path:" << record._path << "inode:" << record._inode - << "modtime:" << record._modtime << "type:" << record._type - << "etag:" << record._etag << "fileId:" << record._fileId << "remotePerm:" << record._remotePerm.toString() - << "fileSize:" << record._fileSize << "checksum:" << record._checksumHeader << "hasDirtyPlaceholder:" << record._hasDirtyPlaceholder; + qCInfo(lcDb) << "Updating file record for path:" << record._path << "inode:" << record._inode << "modtime:" << record._modtime << "type:" << record._type + << "etag:" << record._etag << "fileId:" << record._fileId << "remotePerm:" << record._remotePerm.toString() << "fileSize:" << record._fileSize + << "checksum:" << record._checksumHeader << "hasDirtyPlaceholder:" << record._hasDirtyPlaceholder << "serverPath:" << record._serverPath; const qint64 phash = getPHash(record._path); if (checkConnect()) { @@ -867,9 +880,11 @@ Result SyncJournalDb::setFileRecord(const SyncJournalFileRecord & const auto checksumHeader = ChecksumHeader::parseChecksumHeader(record._checksumHeader); int contentChecksumTypeId = mapChecksumType(checksumHeader.type()); - const auto query = _queryManager.get(PreparedSqlQueryManager::SetFileRecordQuery, QByteArrayLiteral("INSERT OR REPLACE INTO metadata " - "(phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, contentChecksum, contentChecksumTypeId, hasDirtyPlaceholder) " - "VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7, ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17);"), + const auto query = _queryManager.get(PreparedSqlQueryManager::SetFileRecordQuery, + QByteArrayLiteral("INSERT OR REPLACE INTO metadata " + "(phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, " + "contentChecksum, contentChecksumTypeId, hasDirtyPlaceholder, serverPath) " + "VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7, ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18);"), _db); if (!query) { return query->error(); @@ -892,6 +907,7 @@ Result SyncJournalDb::setFileRecord(const SyncJournalFileRecord & query->bindValue(15, checksumHeader.checksum()); query->bindValue(16, contentChecksumTypeId); query->bindValue(17, record._hasDirtyPlaceholder); + query->bindValue(18, record._serverPath); if (!query->exec()) { return query->error(); diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h index 5c3106d6084..3d1b9342922 100644 --- a/src/common/syncjournalfilerecord.h +++ b/src/common/syncjournalfilerecord.h @@ -60,6 +60,7 @@ class OCSYNC_EXPORT SyncJournalFileRecord bool _serverHasIgnoredFiles = false; bool _hasDirtyPlaceholder = false; QByteArray _checksumHeader; + QByteArray _serverPath; }; bool OCSYNC_EXPORT From 9a861f16ab3a8a5f902792365f71b8215b83a30b Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Thu, 16 Jan 2025 16:07:19 +0100 Subject: [PATCH 2/6] Add `_remoteName` member to `SyncFileItem` --- src/libsync/syncfileitem.cpp | 1 + src/libsync/syncfileitem.h | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libsync/syncfileitem.cpp b/src/libsync/syncfileitem.cpp index fbe7dfad833..613ec4ae282 100644 --- a/src/libsync/syncfileitem.cpp +++ b/src/libsync/syncfileitem.cpp @@ -72,6 +72,7 @@ SyncFileItemPtr SyncFileItem::fromSyncJournalFileRecord(const SyncJournalFileRec item->_remotePerm = rec._remotePerm; item->_serverHasIgnoredFiles = rec._serverHasIgnoredFiles; item->_checksumHeader = rec._checksumHeader; + item->_remoteName = QString::fromUtf8(rec._serverPath); return item; } diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h index 6f65fc0279b..3e552313622 100644 --- a/src/libsync/syncfileitem.h +++ b/src/libsync/syncfileitem.h @@ -247,6 +247,9 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem QString localName() const { return _localName; } void setLocalName(const QString &newName) { _localName = newName; } + QString remoteName() const { return _remoteName.isEmpty() ? _localName : _remoteName; } + QString rawRemoteName() const { return _remoteName; } + private: /** The syncfolder-relative filesystem path that the operation is about * @@ -254,7 +257,12 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem */ QString _localName; - // QString _remoteName; + /** + * The remote file name, when the local unicode normalization differs from the remote + * normalization. When this string is empty, (and the local name is not), the normalization is + * the same, and the local name can be used. + */ + QString _remoteName; public: /** for renames: the name _file should be renamed to From ad7862de9d21f2b8e24661af202a3b693618f94b Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Thu, 16 Jan 2025 16:08:20 +0100 Subject: [PATCH 3/6] Add `remoteName` column to blacklist table --- src/common/syncjournaldb.cpp | 33 +++++++++++++++++++++--------- src/common/syncjournalfilerecord.h | 1 + src/libsync/owncloudpropagator.cpp | 1 + 3 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index d86b5d64cf2..f637f15f9ab 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -543,8 +543,9 @@ bool SyncJournalDb::checkConnect() return sqlFail(QStringLiteral("prepare _deleteUploadInfoQuery"), *deleteUploadInfoQuery); } - QByteArray sql("SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId " - "FROM blacklist WHERE path=?1"); + QByteArray sql( + "SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId, remoteName " + "FROM blacklist WHERE path=?1"); if (Utility::fsCasePreserving()) { // if the file system is case preserving we have to check the blacklist // case insensitively @@ -806,7 +807,17 @@ bool SyncJournalDb::updateErrorBlacklistTableStructure() sqlFail(QStringLiteral("updateBlacklistTableStructure: Add requestId"), query); re = false; } - commitInternal(QStringLiteral("update database structure: add errorCategory col")); + commitInternal(QStringLiteral("update database structure: add requestId col")); + } + + if (columns.indexOf("remoteName") == -1) { + SqlQuery query(_db); + query.prepare("ALTER TABLE blacklist ADD COLUMN remoteName VARCHAR(4096);"); + if (!query.exec()) { + sqlFail(QStringLiteral("updateBlacklistTableStructure: Add remoteName"), query); + re = false; + } + commitInternal(QStringLiteral("update database structure: add remoteName col")); } SqlQuery query(_db); @@ -1553,6 +1564,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry(const QString query->intValue(7)); entry._requestId = query->baValue(8); entry._file = file; + entry._remoteName = QString::fromUtf8(query->baValue(9)); } } } @@ -1691,18 +1703,18 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord { QMutexLocker locker(&_mutex); - qCInfo(lcDb) << "Setting blacklist entry for" << item._file << item._retryCount - << item._errorString << item._lastTryTime << item._ignoreDuration - << item._lastTryModtime << item._lastTryEtag << item._renameTarget - << item._errorCategory; + qCInfo(lcDb) << "Setting blacklist entry for" << item._file << item._retryCount << item._errorString << item._lastTryTime << item._ignoreDuration + << item._lastTryModtime << item._lastTryEtag << item._renameTarget << item._errorCategory << item._requestId << item._remoteName; if (!checkConnect()) { return; } - const auto query = _queryManager.get(PreparedSqlQueryManager::SetErrorBlacklistQuery, QByteArrayLiteral("INSERT OR REPLACE INTO blacklist " - "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId) " - "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)"), + const auto query = _queryManager.get(PreparedSqlQueryManager::SetErrorBlacklistQuery, + QByteArrayLiteral( + "INSERT OR REPLACE INTO blacklist " + "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory, requestId, remoteName) " + "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)"), _db); if (!query) { return; @@ -1718,6 +1730,7 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord query->bindValue(8, item._renameTarget); query->bindValue(9, item._errorCategory); query->bindValue(10, item._requestId); + query->bindValue(11, item._remoteName); query->exec(); } diff --git a/src/common/syncjournalfilerecord.h b/src/common/syncjournalfilerecord.h index 3d1b9342922..b51cc7fd687 100644 --- a/src/common/syncjournalfilerecord.h +++ b/src/common/syncjournalfilerecord.h @@ -108,6 +108,7 @@ class OCSYNC_EXPORT SyncJournalErrorBlacklistRecord QString _file; QString _renameTarget; + QString _remoteName; /// The last X-Request-ID of the request that failled QByteArray _requestId; diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 1f40a39b418..5e1f6360efa 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -153,6 +153,7 @@ static SyncJournalErrorBlacklistRecord createBlacklistEntry( entry._renameTarget = item._renameTarget; entry._retryCount = old._retryCount + 1; entry._requestId = item._requestId; + entry._remoteName = item.rawRemoteName(); static qint64 minBlacklistTime(getMinBlacklistTime()); static qint64 maxBlacklistTime(qMax(getMaxBlacklistTime(), minBlacklistTime)); From 7242fd5906dbbecec6d2b7d7fd2394e8699dbf1c Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Mon, 20 Jan 2025 18:28:16 +0100 Subject: [PATCH 4/6] Use server file name when talking to the server In follow-up commits, handling for different normalizations between the server and the client is added. So when doing a server request, the recorded remote name has to be used. This also adds a bit of documentation to clarify what the propagator jobs do. --- src/libsync/propagateremotedelete.cpp | 7 ++++--- src/libsync/propagateremotedelete.h | 2 +- src/libsync/propagateremotemkdir.cpp | 13 +++++++++---- src/libsync/propagateremotemkdir.h | 2 +- src/libsync/propagateremotemove.cpp | 4 ++-- src/libsync/propagateremotemove.h | 2 +- src/libsync/propagateuploadng.cpp | 2 +- src/libsync/propagateuploadv1.cpp | 2 +- src/libsync/propagatorjobs.h | 6 +++--- 9 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/libsync/propagateremotedelete.cpp b/src/libsync/propagateremotedelete.cpp index 74be9690ec6..dab09cb4389 100644 --- a/src/libsync/propagateremotedelete.cpp +++ b/src/libsync/propagateremotedelete.cpp @@ -43,12 +43,13 @@ void DeleteJob::finished() void PropagateRemoteDelete::start() { - if (propagator()->_abortRequested) + if (propagator()->_abortRequested) { return; + } - qCDebug(lcPropagateRemoteDelete) << _item->localName(); + qCDebug(lcPropagateRemoteDelete).nospace() << _item->remoteName() << " (local name:" << _item->localName() << ")"; - _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->localName()), this); + _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->remoteName()), this); connect(_job.data(), &DeleteJob::finishedSignal, this, &PropagateRemoteDelete::slotDeleteJobFinished); propagator()->_activeJobList.append(this); _job->start(); diff --git a/src/libsync/propagateremotedelete.h b/src/libsync/propagateremotedelete.h index 8a996286fe1..76a812f0d84 100644 --- a/src/libsync/propagateremotedelete.h +++ b/src/libsync/propagateremotedelete.h @@ -33,7 +33,7 @@ class DeleteJob : public AbstractNetworkJob }; /** - * @brief The PropagateRemoteDelete class + * @brief Propagate a local delete to the server * @ingroup libsync */ class PropagateRemoteDelete : public PropagateItemJob diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index 34757b08fa1..4aaa9f78169 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -34,8 +34,9 @@ Q_LOGGING_CATEGORY(lcPropagateRemoteMkdir, "sync.propagator.remotemkdir", QtInfo void PropagateRemoteMkdir::start() { - if (propagator()->_abortRequested) + if (propagator()->_abortRequested) { return; + } qCDebug(lcPropagateRemoteMkdir) << _item->localName(); @@ -45,19 +46,23 @@ void PropagateRemoteMkdir::start() return slotStartMkcolJob(); } - _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->localName()), this); + _job = new DeleteJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->remoteName()), this); connect(qobject_cast(_job), &DeleteJob::finishedSignal, this, &PropagateRemoteMkdir::slotStartMkcolJob); _job->start(); } void PropagateRemoteMkdir::slotStartMkcolJob() { - if (propagator()->_abortRequested) + if (propagator()->_abortRequested) { return; + } qCDebug(lcPropagateRemoteMkdir) << _item->localName(); - _job = new MkColJob(propagator()->account(), propagator()->webDavUrl(), propagator()->fullRemotePath(_item->localName()), {}, this); + // Use the localName (not the remoteName), because it is a new local directory that gets propagated. + const QString newDirName = propagator()->fullRemotePath(_item->localName()); + + _job = new MkColJob(propagator()->account(), propagator()->webDavUrl(), newDirName, {}, this); connect(qobject_cast(_job), &MkColJob::finishedWithError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); connect(qobject_cast(_job), &MkColJob::finishedWithoutError, this, &PropagateRemoteMkdir::slotMkcolJobFinished); _job->start(); diff --git a/src/libsync/propagateremotemkdir.h b/src/libsync/propagateremotemkdir.h index 97b554804ec..6c9794ac42a 100644 --- a/src/libsync/propagateremotemkdir.h +++ b/src/libsync/propagateremotemkdir.h @@ -19,7 +19,7 @@ namespace OCC { /** - * @brief The PropagateRemoteMkdir class + * @brief Propagate a local mkdir to the server * @ingroup libsync */ class PropagateRemoteMkdir : public PropagateItemJob diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index d769d762850..7cd248504b6 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -59,8 +59,8 @@ void PropagateRemoteMove::start() if (propagator()->_abortRequested) return; - QString origin = propagator()->adjustRenamedPath(_item->localName()); - qCDebug(lcPropagateRemoteMove) << origin << _item->_renameTarget; + const QString origin = propagator()->adjustRenamedPath(_item->remoteName()); + qCDebug(lcPropagateRemoteMove).nospace() << origin << " (local name: " << _item->localName() << ") " << _item->_renameTarget; if (origin == _item->_renameTarget) { // The parent has been renamed already so there is nothing more to do. finalize(); diff --git a/src/libsync/propagateremotemove.h b/src/libsync/propagateremotemove.h index 01a44d81195..05db27bbbc3 100644 --- a/src/libsync/propagateremotemove.h +++ b/src/libsync/propagateremotemove.h @@ -37,7 +37,7 @@ class MoveJob : public AbstractNetworkJob }; /** - * @brief The PropagateRemoteMove class + * @brief Propagate a local move (or rename) to the server * @ingroup libsync */ class PropagateRemoteMove : public PropagateItemJob diff --git a/src/libsync/propagateuploadng.cpp b/src/libsync/propagateuploadng.cpp index 52b0acb7b1b..6d34f7d1897 100644 --- a/src/libsync/propagateuploadng.cpp +++ b/src/libsync/propagateuploadng.cpp @@ -328,7 +328,7 @@ void PropagateUploadFileNG::doFinalMove() _finished = true; // Finish with a MOVE - QString destination = QDir::cleanPath(propagator()->webDavUrl().path() + propagator()->fullRemotePath(_item->localName())); + QString destination = QDir::cleanPath(propagator()->webDavUrl().path() + propagator()->fullRemotePath(_item->remoteName())); auto headers = PropagateUploadFileCommon::headers(); // "If-Match applies to the source, but we are interested in comparing the etag of the destination diff --git a/src/libsync/propagateuploadv1.cpp b/src/libsync/propagateuploadv1.cpp index 286679e3fb3..ba865d0dde1 100644 --- a/src/libsync/propagateuploadv1.cpp +++ b/src/libsync/propagateuploadv1.cpp @@ -93,7 +93,7 @@ void PropagateUploadFileV1::startNextChunk() headers[QByteArrayLiteral("OC-Total-Length")] = QByteArray::number(fileSize); headers[QByteArrayLiteral("OC-Chunk-Size")] = QByteArray::number(chunkSize()); - QString path = _item->localName(); + QString path = _item->remoteName(); qint64 chunkStart = 0; qint64 currentChunkSize = fileSize; diff --git a/src/libsync/propagatorjobs.h b/src/libsync/propagatorjobs.h index 1e9b25860fa..a4d6d05eb6b 100644 --- a/src/libsync/propagatorjobs.h +++ b/src/libsync/propagatorjobs.h @@ -28,7 +28,7 @@ static const char checkSumHeaderC[] = "OC-Checksum"; static const char contentMd5HeaderC[] = "Content-MD5"; /** - * @brief Declaration of the other propagation jobs + * @brief Remove (or move to the trash) a file or folder that was removed remotely * @ingroup libsync */ class PropagateLocalRemove : public PropagateItemJob @@ -47,7 +47,7 @@ class PropagateLocalRemove : public PropagateItemJob }; /** - * @brief The PropagateLocalMkdir class + * @brief Make a local directory after discovering it on the server * @ingroup libsync */ class PropagateLocalMkdir : public PropagateItemJob @@ -74,7 +74,7 @@ class PropagateLocalMkdir : public PropagateItemJob }; /** - * @brief The PropagateLocalRename class + * @brief Rename a local file/directory after discovering a rename on the server * @ingroup libsync */ class PropagateLocalRename : public PropagateItemJob From 266252f497e22c69aa9e3e8475130e9e3b8e3f20 Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Tue, 21 Jan 2025 14:38:16 +0100 Subject: [PATCH 5/6] Add file/dir name normalization test --- test/testlocaldiscovery.cpp | 63 +++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp index 58e0b32878a..1aba621a9ad 100644 --- a/test/testlocaldiscovery.cpp +++ b/test/testlocaldiscovery.cpp @@ -238,6 +238,69 @@ private Q_SLOTS: QVERIFY(!fakeFolder.currentRemoteState().find(QStringLiteral("C/.foo"))); QVERIFY(!fakeFolder.currentRemoteState().find(QStringLiteral("C/bar"))); } + + void testDirNameEncoding() + { + QSKIP("Known bug, is being worked on"); + + QFETCH_GLOBAL(Vfs::Mode, vfsMode); + QFETCH_GLOBAL(bool, filesAreDehydrated); + + const unsigned char a_umlaut_composed_bytes[] = {0xc3, 0xa4, 0x00}; + const QString a_umlaut_composed = QString::fromUtf8(reinterpret_cast(a_umlaut_composed_bytes)); + const QString a_umlaut_decomposed = a_umlaut_composed.normalized(QString::NormalizationForm_D); + + FakeFolder fakeFolder({FileInfo{}}, vfsMode, filesAreDehydrated); + fakeFolder.remoteModifier().mkdir(QStringLiteral("P")); + fakeFolder.remoteModifier().mkdir(QStringLiteral("P/A")); + fakeFolder.remoteModifier().insert(QStringLiteral("P/A/") + a_umlaut_decomposed); + fakeFolder.remoteModifier().mkdir(QStringLiteral("P/B") + a_umlaut_decomposed); + fakeFolder.remoteModifier().insert(QStringLiteral("P/B") + a_umlaut_decomposed + QStringLiteral("/b")); + + LocalDiscoveryTracker tracker; + connect(&fakeFolder.syncEngine(), &SyncEngine::itemCompleted, &tracker, &LocalDiscoveryTracker::slotItemCompleted); + connect(&fakeFolder.syncEngine(), &SyncEngine::finished, &tracker, &LocalDiscoveryTracker::slotSyncFinished); + + QVERIFY(fakeFolder.applyLocalModificationsAndSync()); + + { + auto localState = fakeFolder.currentLocalState(); + FileInfo *localFile = localState.find(QStringLiteral("P/A/") + a_umlaut_composed); + QVERIFY(localFile != nullptr); // check if the file exists + } + { + auto localState = fakeFolder.currentLocalState(); + FileInfo *localFile = localState.find(QStringLiteral("P/B") + a_umlaut_composed + QStringLiteral("/b")); + QVERIFY(localFile != nullptr); // check if the file exists + } + + qDebug() << "*** MARK"; + + fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem, {QStringLiteral("P")}); + tracker.startSyncFullDiscovery(); + QVERIFY(fakeFolder.applyLocalModificationsAndSync()); + + { + auto remoteState = fakeFolder.currentRemoteState(); + FileInfo *remoteFile = remoteState.find(QStringLiteral("P/A/") + a_umlaut_composed); + QVERIFY(remoteFile == nullptr); // check if the file exists + } + { + auto remoteState = fakeFolder.currentRemoteState(); + FileInfo *remoteFile = remoteState.find(QStringLiteral("P/A/") + a_umlaut_decomposed); + QVERIFY(remoteFile != nullptr); // check if the file exists + } + { + auto remoteState = fakeFolder.currentRemoteState(); + FileInfo *remoteFile = remoteState.find(QStringLiteral("P/B") + a_umlaut_composed + QStringLiteral("/b")); + QVERIFY(remoteFile != nullptr); // check if the file exists + } + { + auto remoteState = fakeFolder.currentRemoteState(); + FileInfo *remoteFile = remoteState.find(QStringLiteral("P/B") + a_umlaut_decomposed + QStringLiteral("/b")); + QVERIFY(remoteFile != nullptr); // check if the file exists + } + } }; QTEST_GUILESS_MAIN(TestLocalDiscovery) From eef42284ae4d1cf0dff87a4c55c8c9530405a609 Mon Sep 17 00:00:00 2001 From: Erik Verbruggen Date: Wed, 22 Jan 2025 17:23:50 +0100 Subject: [PATCH 6/6] Set remote filename when different from local filename --- src/common/filesystembase.cpp | 9 +++++++++ src/common/filesystembase.h | 2 ++ src/libsync/discovery.cpp | 15 ++++++++++++--- src/libsync/discovery.h | 8 ++++---- src/libsync/syncfileitem.h | 6 ++++++ 5 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 50bf9c51652..8112b312309 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -563,6 +563,15 @@ QString FileSystem::createPortableFileName(const QString &path, const QString &f return QDir::cleanPath(path + QLatin1Char('/') + tmp); } +QString FileSystem::localNormalizedFileName(const QString &name) +{ + if (Utility::isMac()) { + return name.normalized(QString::NormalizationForm_C); + } + + return name; +} + QString FileSystem::pathEscape(const QString &s) { QString tmp = s; diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index 2687b3907ff..57f7b5a8510 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -181,6 +181,8 @@ namespace FileSystem { */ QString OCSYNC_EXPORT createPortableFileName(const QString &path, const QString &fileName, qsizetype reservedSize = 0); + QString OCSYNC_EXPORT localNormalizedFileName(const QString &name); + /* * Replace path navigation elements from the string */ diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 7d787d8ee66..4e569fc5b37 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -82,7 +82,15 @@ void ProcessDirectoryJob::process() }; std::map entries; for (auto &e : _serverNormalQueryEntries) { - entries[e.name].serverEntry = std::move(e); + const QString localName = FileSystem::localNormalizedFileName(e.name); + auto it = entries.find(localName); + if (it == entries.end()) { + entries[localName].serverEntry = std::move(e); + } else { + // Normalization collision with another file + qCWarning(lcDisco) << "Normalization collision with another filename on the server for" << e.name; + // FIXME: generate an error for the user + } } _serverNormalQueryEntries.clear(); @@ -140,8 +148,7 @@ void ProcessDirectoryJob::process() for (const auto &f : entries) { const auto &e = f.second; - PathTuple path; - path = _currentFolder.addName(e.nameOverride.isEmpty() ? f.first : e.nameOverride); + PathTuple path = _currentFolder.addName(e.nameOverride.isEmpty() ? f.first : e.nameOverride, f.second.serverEntry.name); if (isVfsWithSuffix()) { // Without suffix vfs the paths would be good. But since the dbEntry and localEntry @@ -336,6 +343,8 @@ void ProcessDirectoryJob::processFile(const PathTuple &path, auto item = SyncFileItem::fromSyncJournalFileRecord(dbEntry); item->setLocalName(path._target); + item->maybeSetRemoteName(path._server); + item->_originalFile = path._original; item->_previousSize = dbEntry._fileSize; item->_previousModtime = dbEntry._modtime; diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index 8ceb4e06ed0..a2d3d82d203 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -115,16 +115,16 @@ class ProcessDirectoryJob : public QObject { return base.isEmpty() ? name : base + QLatin1Char('/') + name; } - PathTuple addName(const QString &name) const + PathTuple addName(const QString &localName, const QString &serverName) const { PathTuple result; - result._original = pathAppend(_original, name); + result._original = pathAppend(_original, localName); auto buildString = [&](const QString &other) { // Optimize by trying to keep all string implicitly shared if they are the same (common case) - return other == _original ? result._original : pathAppend(other, name); + return other == _original ? result._original : pathAppend(other, localName); }; result._target = buildString(_target); - result._server = buildString(_server); + result._server = localName == serverName ? buildString(_server) : pathAppend(_server, serverName); result._local = buildString(_local); return result; } diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h index 3e552313622..69c97feae43 100644 --- a/src/libsync/syncfileitem.h +++ b/src/libsync/syncfileitem.h @@ -249,6 +249,12 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem QString remoteName() const { return _remoteName.isEmpty() ? _localName : _remoteName; } QString rawRemoteName() const { return _remoteName; } + void maybeSetRemoteName(const QString &remoteName) + { + if (!remoteName.isEmpty() && remoteName != _localName) { + _remoteName = remoteName; + } + } private: /** The syncfolder-relative filesystem path that the operation is about