avoid modifying a placeholder (virtual files) when not needed

acoid modifying some metadata of the placeholder when this placeholder has just been uploaded to the server (will avoid truncating the timestamps) Close #6190 Signed-off-by: Matthieu Gallien <[email protected]>
nextcloud · Dec 6, 2023 · 6e1619c · 6e1619c
1 parent 37b15f2
commit 6e1619c
Show file tree

Hide file tree

Showing 13 changed files with 130 additions and 87 deletions.
diff --git a/src/common/vfs.h b/src/common/vfs.h
@@ -113,6 +113,15 @@ class OCSYNC_EXPORT Vfs : public QObject
     };
     Q_ENUM(ConvertToPlaceholderResult)
 
+    enum UpdateMetadataType {
+        DatabaseMetadata = 1 << 0,
+        FileMetadata = 1 << 1,
+        AllMetadata = DatabaseMetadata | FileMetadata,
+    };
+
+    Q_DECLARE_FLAGS(UpdateMetadataTypes, UpdateMetadataType)
+    Q_FLAG(UpdateMetadataType)
+
     static QString modeToString(Mode mode);
     static Optional<Mode> modeFromString(const QString &str);
 
@@ -203,10 +212,10 @@ class OCSYNC_EXPORT Vfs : public QObject
      * new placeholder shall supersede, for rename-replace actions with new downloads,
      * for example.
      */
-    Q_REQUIRED_RESULT virtual Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(
-        const QString &filename,
-        const SyncFileItem &item,
-        const QString &replacesFile = QString()) = 0;
+    Q_REQUIRED_RESULT virtual Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename,
+                                                                                                    const SyncFileItem &item,
+                                                                                                    const QString &replacesFile = {},
+                                                                                                    UpdateMetadataTypes updateType = AllMetadata) = 0;
 
     /// Determine whether the file at the given absolute path is a dehydrated placeholder.
     Q_REQUIRED_RESULT virtual bool isDehydratedPlaceholder(const QString &filePath) = 0;
@@ -311,7 +320,7 @@ class OCSYNC_EXPORT VfsOff : public Vfs
     Result<void, QString> updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; }
     Result<void, QString> createPlaceholder(const SyncFileItem &) override { return {}; }
     Result<void, QString> dehydratePlaceholder(const SyncFileItem &) override { return {}; }
-    Result<ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return ConvertToPlaceholderResult::Ok; }
+    Result<ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &, const SyncFileItem &, const QString &, UpdateMetadataTypes) override { return ConvertToPlaceholderResult::Ok; }
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }
     bool isDehydratedPlaceholder(const QString &) override { return false; }

diff --git a/src/libsync/bulkpropagatorjob.cpp b/src/libsync/bulkpropagatorjob.cpp
@@ -479,7 +479,7 @@ void BulkPropagatorJob::adjustLastJobTimeout(AbstractNetworkJob *job, qint64 fil
 void BulkPropagatorJob::finalizeOneFile(const BulkUploadItem &oneFile)
 {
     // Update the database entry
-    const auto result = propagator()->updateMetadata(*oneFile._item);
+    const auto result = propagator()->updateMetadata(*oneFile._item, Vfs::UpdateMetadataType::DatabaseMetadata);
     if (!result) {
         done(oneFile._item, SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()), ErrorCategory::GenericError);
         return;

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -1001,16 +1001,19 @@ QString OwncloudPropagator::adjustRenamedPath(const QString &original) const
     return OCC::adjustRenamedPath(_renamedDirectories, original);
 }
 
-Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::updateMetadata(const SyncFileItem &item)
+Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::updateMetadata(const SyncFileItem &item, Vfs::UpdateMetadataTypes updateType)
 {
-    return OwncloudPropagator::staticUpdateMetadata(item, _localDir, syncOptions()._vfs.data(), _journal);
+    return OwncloudPropagator::staticUpdateMetadata(item, _localDir, syncOptions()._vfs.data(), _journal, updateType);
 }
 
-Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::staticUpdateMetadata(const SyncFileItem &item, const QString localDir,
-                                                                                          Vfs *vfs, SyncJournalDb *const journal)
+Result<Vfs::ConvertToPlaceholderResult, QString> OwncloudPropagator::staticUpdateMetadata(const SyncFileItem &item,
+                                                                                          const QString localDir,
+                                                                                          Vfs *vfs,
+                                                                                          SyncJournalDb *const journal,
+                                                                                          Vfs::UpdateMetadataTypes updateType)
 {
     const QString fsPath = localDir + item.destination();
-    const auto result = vfs->convertToPlaceholder(fsPath, item);
+    const auto result = vfs->convertToPlaceholder(fsPath, item, {}, updateType);
     if (!result) {
         return result.error();
     } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) {
@@ -1582,7 +1585,7 @@ void CleanupPollsJob::slotPollFinished()
     } else if (job->_item->_status != SyncFileItem::Success) {
         qCWarning(lcCleanupPolls) << "There was an error with file " << job->_item->_file << job->_item->_errorString;
     } else {
-        if (!OwncloudPropagator::staticUpdateMetadata(*job->_item, _localPath, _vfs.data(), _journal)) {
+        if (!OwncloudPropagator::staticUpdateMetadata(*job->_item, _localPath, _vfs.data(), _journal, Vfs::AllMetadata)) {
             qCWarning(lcCleanupPolls) << "database error";
             job->_item->_status = SyncFileItem::FatalError;
             job->_item->_errorString = tr("Error writing metadata to the database");

diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h
@@ -27,13 +27,15 @@
 
 #include "accountfwd.h"
 #include "bandwidthmanager.h"
-#include "common/syncjournaldb.h"
-#include "common/utility.h"
 #include "csync.h"
 #include "progressdispatcher.h"
 #include "syncfileitem.h"
 #include "syncoptions.h"
 
+#include "common/syncjournaldb.h"
+#include "common/utility.h"
+#include "common/vfs.h"
+
 #include <deque>
 
 namespace OCC {
@@ -592,7 +594,7 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject
      *
      * Will also trigger a Vfs::convertToPlaceholder.
      */
-    Result<Vfs::ConvertToPlaceholderResult, QString> updateMetadata(const SyncFileItem &item);
+    Result<Vfs::ConvertToPlaceholderResult, QString> updateMetadata(const SyncFileItem &item, Vfs::UpdateMetadataTypes updateType = Vfs::AllMetadata);
 
     /** Update the database for an item.
      *
@@ -601,8 +603,11 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject
      *
      * Will also trigger a Vfs::convertToPlaceholder.
      */
-    static Result<Vfs::ConvertToPlaceholderResult, QString> staticUpdateMetadata(const SyncFileItem &item, const QString localDir,
-                                                                                 Vfs *vfs, SyncJournalDb * const journal);
+    static Result<Vfs::ConvertToPlaceholderResult, QString> staticUpdateMetadata(const SyncFileItem &item,
+                                                                                 const QString localDir,
+                                                                                 Vfs *vfs,
+                                                                                 SyncJournalDb * const journal,
+                                                                                 Vfs::UpdateMetadataTypes updateType);
 
     Q_REQUIRED_RESULT bool isDelayedUploadItem(const SyncFileItemPtr &item) const;
 

diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp
@@ -800,7 +800,7 @@ void PropagateUploadFileCommon::finalize()
         quotaIt.value() -= _fileToUpload._size;
 
     // Update the database entry
-    const auto result = propagator()->updateMetadata(*_item);
+    const auto result = propagator()->updateMetadata(*_item, Vfs::DatabaseMetadata);
     if (!result) {
         done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()));
         return;

diff --git a/src/libsync/vfs/cfapi/cfapiwrapper.cpp b/src/libsync/vfs/cfapi/cfapiwrapper.cpp
@@ -48,6 +48,30 @@ constexpr auto syncRootFlagsNoCfApiContextMenu = 2;
 
 constexpr auto syncRootManagerRegKey = R"(SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SyncRootManager)";
 
+DWORD sizeToDWORD(size_t size)
+{
+    return OCC::Utility::convertSizeToDWORD(size);
+}
+
+OCC::PinState cfPinStateToPinState(CF_PIN_STATE state)
+{
+    switch (state) {
+    case CF_PIN_STATE_UNSPECIFIED:
+        return OCC::PinState::Unspecified;
+    case CF_PIN_STATE_PINNED:
+        return OCC::PinState::AlwaysLocal;
+    case CF_PIN_STATE_UNPINNED:
+        return OCC::PinState::OnlineOnly;
+    case CF_PIN_STATE_INHERIT:
+        return OCC::PinState::Inherited;
+    case CF_PIN_STATE_EXCLUDED:
+        return OCC::PinState::Excluded;
+    default:
+        Q_UNREACHABLE();
+        return OCC::PinState::Inherited;
+    }
+}
+
 void cfApiSendTransferInfo(const CF_CONNECTION_KEY &connectionKey, const CF_TRANSFER_KEY &transferKey, NTSTATUS status, void *buffer, qint64 offset, qint64 currentBlockLength, qint64 totalLength)
 {
 
@@ -237,6 +261,53 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const
         sendTransferError();
     }
 }
+
+enum class CfApiUpdateMetadataType {
+    OnlyBasicMetadata,
+    AllMetadata,
+};
+
+OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> updatePlaceholderState(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath, CfApiUpdateMetadataType updateType)
+{
+    if (updateType == CfApiUpdateMetadataType::AllMetadata && modtime <= 0) {
+            return {QString{"Could not update metadata due to invalid modification time for %1: %2"}.arg(path).arg(modtime)};
+        }
+
+        const auto info = replacesPath.isEmpty() ? OCC::CfApiWrapper::findPlaceholderInfo(path)
+                                                 : OCC::CfApiWrapper::findPlaceholderInfo(replacesPath);
+        if (!info) {
+            return { "Can't update non existing placeholder info" };
+        }
+
+        const auto previousPinState = cfPinStateToPinState(info->PinState);
+        const auto fileIdentity = QString::fromUtf8(fileId).toStdWString();
+        const auto fileIdentitySize = (fileIdentity.length() + 1) * sizeof(wchar_t);
+
+        CF_FS_METADATA metadata;
+        metadata.FileSize.QuadPart = size;
+        OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.CreationTime);
+        OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastWriteTime);
+        OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastAccessTime);
+        OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.ChangeTime);
+        metadata.BasicInfo.FileAttributes = 0;
+
+        qCInfo(lcCfApiWrapper) << "updatePlaceholderState" << path << modtime;
+        const qint64 result = CfUpdatePlaceholder(OCC::CfApiWrapper::handleForPath(path).get(), updateType == CfApiUpdateMetadataType::AllMetadata ? &metadata : nullptr,
+                                                  fileIdentity.data(), sizeToDWORD(fileIdentitySize),
+                                                  nullptr, 0, CF_UPDATE_FLAG_MARK_IN_SYNC, nullptr, nullptr);
+
+        if (result != S_OK) {
+            qCWarning(lcCfApiWrapper) << "Couldn't update placeholder info for" << path << ":" << QString::fromWCharArray(_com_error(result).ErrorMessage()) << replacesPath;
+            return { "Couldn't update placeholder info" };
+        }
+
+               // Pin state tends to be lost on updates, so restore it every time
+        if (!setPinState(path, previousPinState, OCC::CfApiWrapper::NoRecurse)) {
+            return { "Couldn't restore pin state" };
+        }
+
+        return OCC::Vfs::ConvertToPlaceholderResult::Ok;
+}
 }
 
 void CALLBACK cfApiCancelFetchData(const CF_CALLBACK_INFO *callbackInfo, const CF_CALLBACK_PARAMETERS * /*callbackParameters*/)
@@ -262,11 +333,6 @@ CF_CALLBACK_REGISTRATION cfApiCallbacks[] = {
     CF_CALLBACK_REGISTRATION_END
 };
 
-DWORD sizeToDWORD(size_t size)
-{
-    return OCC::Utility::convertSizeToDWORD(size);
-}
-
 void deletePlaceholderInfo(CF_PLACEHOLDER_BASIC_INFO *info)
 {
     auto byte = reinterpret_cast<char *>(info);
@@ -281,25 +347,6 @@ std::wstring pathForHandle(const OCC::CfApiWrapper::FileHandle &handle)
     return std::wstring(buffer);
 }
 
-OCC::PinState cfPinStateToPinState(CF_PIN_STATE state)
-{
-    switch (state) {
-    case CF_PIN_STATE_UNSPECIFIED:
-        return OCC::PinState::Unspecified;
-    case CF_PIN_STATE_PINNED:
-        return OCC::PinState::AlwaysLocal;
-    case CF_PIN_STATE_UNPINNED:
-        return OCC::PinState::OnlineOnly;
-    case CF_PIN_STATE_INHERIT:
-        return OCC::PinState::Inherited;
-    case CF_PIN_STATE_EXCLUDED:
-        return OCC::PinState::Excluded;
-    default:
-        Q_UNREACHABLE();
-        return OCC::PinState::Inherited;
-    }
-}
-
 CF_PIN_STATE pinStateToCfPinState(OCC::PinState state)
 {
     switch (state) {
@@ -750,44 +797,7 @@ OCC::Result<void, QString> OCC::CfApiWrapper::createPlaceholderInfo(const QStrin
 
 OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> OCC::CfApiWrapper::updatePlaceholderInfo(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath)
 {
-
-    if (modtime <= 0) {
-        return {QString{"Could not update metadata due to invalid modification time for %1: %2"}.arg(path).arg(modtime)};
-    }
-
-    const auto info = replacesPath.isEmpty() ? findPlaceholderInfo(path)
-                                             : findPlaceholderInfo(replacesPath);
-    if (!info) {
-        return { "Can't update non existing placeholder info" };
-    }
-
-    const auto previousPinState = cfPinStateToPinState(info->PinState);
-    const auto fileIdentity = QString::fromUtf8(fileId).toStdWString();
-    const auto fileIdentitySize = (fileIdentity.length() + 1) * sizeof(wchar_t);
-
-    CF_FS_METADATA metadata;
-    metadata.FileSize.QuadPart = size;
-    OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.CreationTime);
-    OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastWriteTime);
-    OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastAccessTime);
-    OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.ChangeTime);
-    metadata.BasicInfo.FileAttributes = 0;
-
-    const qint64 result = CfUpdatePlaceholder(handleForPath(path).get(), &metadata,
-                                              fileIdentity.data(), sizeToDWORD(fileIdentitySize),
-                                              nullptr, 0, CF_UPDATE_FLAG_MARK_IN_SYNC, nullptr, nullptr);
-
-    if (result != S_OK) {
-        qCWarning(lcCfApiWrapper) << "Couldn't update placeholder info for" << path << ":" << QString::fromWCharArray(_com_error(result).ErrorMessage()) << replacesPath;
-        return { "Couldn't update placeholder info" };
-    }
-
-    // Pin state tends to be lost on updates, so restore it every time
-    if (!setPinState(path, previousPinState, NoRecurse)) {
-        return { "Couldn't restore pin state" };
-    }
-
-    return OCC::Vfs::ConvertToPlaceholderResult::Ok;
+    return updatePlaceholderState(path, modtime, size, fileId, replacesPath, CfApiUpdateMetadataType::AllMetadata);
 }
 
 OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> OCC::CfApiWrapper::dehydratePlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId)
@@ -862,3 +872,8 @@ OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> OCC::CfApiWrapper::co
         return stateResult;
     }
 }
+
+OCC::Result<OCC::Vfs::ConvertToPlaceholderResult, QString> OCC::CfApiWrapper::updatePlaceholderMarkInSync(const QString &path, const QByteArray &fileId, const QString &replacesPath)
+{
+    return updatePlaceholderState(path, {}, {}, fileId, replacesPath, CfApiUpdateMetadataType::OnlyBasicMetadata);
+}
diff --git a/src/libsync/vfs/cfapi/cfapiwrapper.h b/src/libsync/vfs/cfapi/cfapiwrapper.h
@@ -97,6 +97,7 @@ NEXTCLOUD_CFAPI_EXPORT Result<void, QString> createPlaceholderInfo(const QString
 NEXTCLOUD_CFAPI_EXPORT Result<OCC::Vfs::ConvertToPlaceholderResult, QString> updatePlaceholderInfo(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath = QString());
 NEXTCLOUD_CFAPI_EXPORT Result<OCC::Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath);
 NEXTCLOUD_CFAPI_EXPORT Result<OCC::Vfs::ConvertToPlaceholderResult, QString> dehydratePlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId);
+NEXTCLOUD_CFAPI_EXPORT Result<OCC::Vfs::ConvertToPlaceholderResult, QString> updatePlaceholderMarkInSync(const QString &path, const QByteArray &fileId, const QString &replacesPath = QString());
 
 }
 

diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.cpp b/src/libsync/vfs/cfapi/vfs_cfapi.cpp
@@ -222,7 +222,7 @@ Result<void, QString> VfsCfApi::dehydratePlaceholder(const SyncFileItem &item)
     }
 }
 
-Result<Vfs::ConvertToPlaceholderResult, QString> VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile)
+Result<Vfs::ConvertToPlaceholderResult, QString> VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile, UpdateMetadataTypes updateType)
 {
     const auto localPath = QDir::toNativeSeparators(filename);
 
@@ -238,7 +238,11 @@ Result<Vfs::ConvertToPlaceholderResult, QString> VfsCfApi::convertToPlaceholder(
     const auto replacesPath = QDir::toNativeSeparators(replacesFile);
 
     if (cfapi::findPlaceholderInfo(localPath)) {
-        return cfapi::updatePlaceholderInfo(localPath, item._modtime, item._size, item._fileId, replacesPath);
+        if (updateType & Vfs::UpdateMetadataType::FileMetadata) {
+            return cfapi::updatePlaceholderInfo(localPath, item._modtime, item._size, item._fileId, replacesPath);
+        } else {
+            return cfapi::updatePlaceholderMarkInSync(localPath, item._fileId, replacesPath);
+        }
     } else {
         return cfapi::convertToPlaceholder(localPath, item._modtime, item._size, item._fileId, replacesPath);
     }

diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.h b/src/libsync/vfs/cfapi/vfs_cfapi.h
@@ -45,7 +45,7 @@ class VfsCfApi : public Vfs
 
     Result<void, QString> createPlaceholder(const SyncFileItem &item) override;
     Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) override;
-    Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) override;
+    Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile, UpdateMetadataTypes updateType) override;
 
     bool needsMetadataUpdate(const SyncFileItem &) override;
     bool isDehydratedPlaceholder(const QString &filePath) override;

diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp
@@ -138,7 +138,7 @@ Result<void, QString> VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
     return {};
 }
 
-Result<Vfs::ConvertToPlaceholderResult, QString> VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &)
+Result<Vfs::ConvertToPlaceholderResult, QString> VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &, UpdateMetadataTypes)
 {
     // Nothing necessary
     return Vfs::ConvertToPlaceholderResult::Ok;

diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h
@@ -42,7 +42,7 @@ class VfsSuffix : public Vfs
 
     Result<void, QString> createPlaceholder(const SyncFileItem &item) override;
     Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) override;
-    Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override;
+    Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &, UpdateMetadataTypes updateType) override;
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }
     bool isDehydratedPlaceholder(const QString &filePath) override;