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 5, 2023 · 1a3bfa4 · 1a3bfa4
1 parent 54706dc
commit 1a3bfa4
Show file tree

Hide file tree

Showing 12 changed files with 81 additions and 24 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 = 0 >> 1,
+        FileMetadata = 0 >> 2,
+        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/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
@@ -862,3 +862,32 @@ 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)
+{
+    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);
+
+    const qint64 result = CfUpdatePlaceholder(handleForPath(path).get(), 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, NoRecurse)) {
+        return { "Couldn't restore pin state" };
+    }
+
+    return OCC::Vfs::ConvertToPlaceholderResult::Ok;
+}
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;

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

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