Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sync errors when trying to delete video component of live photos #7435

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/common/syncjournaldb.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in src/common/syncjournaldb.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on src/common/syncjournaldb.cpp

File src/common/syncjournaldb.cpp does not conform to Custom style guidelines. (lines 52, 971, 972, 973, 999, 1000)
* Copyright (C) by Klaas Freitag <[email protected]>
*
* This library is free software; you can redistribute it and/or
Expand All @@ -16,7 +16,7 @@
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/

#include <QCryptographicHash>

Check failure on line 19 in src/common/syncjournaldb.cpp

View workflow job for this annotation

GitHub Actions / build

src/common/syncjournaldb.cpp:19:10 [clang-diagnostic-error]

'QCryptographicHash' file not found
#include <QFile>
#include <QLoggingCategory>
#include <QStringList>
Expand Down Expand Up @@ -49,7 +49,7 @@
#define GET_FILE_RECORD_QUERY \
"SELECT path, inode, modtime, type, md5, fileid, remotePerm, filesize," \
" ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum, e2eMangledName, isE2eEncrypted, " \
" lock, lockOwnerDisplayName, lockOwnerId, lockType, lockOwnerEditor, lockTime, lockTimeout, lockToken, isShared, lastShareStateFetchedTimestmap, sharedByMe" \
" lock, lockOwnerDisplayName, lockOwnerId, lockType, lockOwnerEditor, lockTime, lockTimeout, lockToken, isShared, lastShareStateFetchedTimestmap, sharedByMe, isLivePhoto, livePhotoFile" \
" FROM metadata" \
" LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id"

Expand Down Expand Up @@ -78,6 +78,8 @@
rec._isShared = query.intValue(20) > 0;
rec._lastShareStateFetchedTimestamp = query.int64Value(21);
rec._sharedByMe = query.intValue(22) > 0;
rec._isLivePhoto = query.intValue(23) > 0;
rec._livePhotoFile = query.stringValue(24);
}

static QByteArray defaultJournalMode(const QString &dbPath)
Expand Down Expand Up @@ -837,6 +839,9 @@
}
commitInternal(QStringLiteral("update database structure: add basePath index"));

addColumn(QStringLiteral("isLivePhoto"), QStringLiteral("INTEGER"));
addColumn(QStringLiteral("livePhotoFile"), QStringLiteral("TEXT"));

return re;
}

Expand Down Expand Up @@ -963,7 +968,9 @@
<< "lock editor:" << record._lockstate._lockEditorApp
<< "sharedByMe:" << record._sharedByMe
<< "isShared:" << record._isShared
<< "lastShareStateFetchedTimestamp:" << record._lastShareStateFetchedTimestamp;
<< "lastShareStateFetchedTimestamp:" << record._lastShareStateFetchedTimestamp
<< "isLivePhoto" << record._isLivePhoto
<< "livePhotoFile" << record._livePhotoFile;

const qint64 phash = getPHash(record._path);
if (!checkConnect()) {
Expand All @@ -989,8 +996,8 @@
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, e2eMangledName, isE2eEncrypted, lock, lockType, lockOwnerDisplayName, lockOwnerId, "
"lockOwnerEditor, lockTime, lockTimeout, lockToken, isShared, lastShareStateFetchedTimestmap, sharedByMe) "
"VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7, ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18, ?19, ?20, ?21, ?22, ?23, ?24, ?25, ?26, ?27, ?28, ?29);"),
"lockOwnerEditor, lockTime, lockTimeout, lockToken, isShared, lastShareStateFetchedTimestmap, sharedByMe, isLivePhoto, livePhotoFile) "
"VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7, ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16, ?17, ?18, ?19, ?20, ?21, ?22, ?23, ?24, ?25, ?26, ?27, ?28, ?29, ?30, ?31);"),
_db);
if (!query) {
qCDebug(lcDb) << "database error:" << query->error();
Expand Down Expand Up @@ -1026,6 +1033,8 @@
query->bindValue(27, record._isShared);
query->bindValue(28, record._lastShareStateFetchedTimestamp);
query->bindValue(29, record._sharedByMe);
query->bindValue(30, record._isLivePhoto);
query->bindValue(31, record._livePhotoFile);

if (!query->exec()) {
qCDebug(lcDb) << "database error:" << query->error();
Expand Down
2 changes: 2 additions & 0 deletions src/common/syncjournalfilerecord.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#ifndef SYNCJOURNALFILERECORD_H
#define SYNCJOURNALFILERECORD_H

#include <QString>

Check failure on line 22 in src/common/syncjournalfilerecord.h

View workflow job for this annotation

GitHub Actions / build

src/common/syncjournalfilerecord.h:22:10 [clang-diagnostic-error]

'QString' file not found
#include <QDateTime>

#include "csync.h"
Expand Down Expand Up @@ -88,6 +88,8 @@
bool _isShared = false;
qint64 _lastShareStateFetchedTimestamp = 0;
bool _sharedByMe = false;
bool _isLivePhoto = false;
QString _livePhotoFile;
};

QDebug& operator<<(QDebug &stream, const SyncJournalFileRecord::EncryptionStatus status);
Expand Down
10 changes: 10 additions & 0 deletions src/libsync/discovery.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in src/libsync/discovery.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on src/libsync/discovery.cpp

File src/libsync/discovery.cpp does not conform to Custom style guidelines. (lines 550)
* Copyright (C) by Olivier Goffart <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
Expand Down Expand Up @@ -547,6 +547,7 @@
<< " | e2eeMangledName: " << dbEntry.e2eMangledName() << "/" << serverEntry.e2eMangledName
<< " | file lock: " << localFileIsLocked << "//" << serverFileIsLocked
<< " | file lock type: " << localFileLockType << "//" << serverFileLockType
<< " | live photo: " << dbEntry._isLivePhoto << "//" << serverEntry.isLivePhoto
<< " | metadata missing: /" << localEntry.isMetadataMissing << '/';

qCInfo(lcDisco).nospace() << processingLog;
Expand Down Expand Up @@ -718,6 +719,9 @@
item->_lockTimeout = serverEntry.lockTimeout;
item->_lockToken = serverEntry.lockToken;

item->_isLivePhoto = serverEntry.isLivePhoto;
item->_livePhotoFile = serverEntry.livePhotoFile;

// Check for missing server data
{
QStringList missingData;
Expand Down Expand Up @@ -1119,6 +1123,12 @@
qCWarning(lcDisco) << "Failed to delete a file record from the local DB" << path._original;
}
return;
} else if (dbEntry._isLivePhoto && QMimeDatabase().mimeTypeForFile(item->_file).inherits(QStringLiteral("video/quicktime"))) {
// This is a live photo's video file; the server won't allow deletion of this file
// so we need to *not* propagate the .mov deletion to the server and redownload the file
qCInfo(lcDisco) << "Live photo video file deletion detected, redownloading" << item->_file;
item->_direction = SyncFileItem::Down;
item->_instruction = CSYNC_INSTRUCTION_SYNC;
} else if (!serverModified) {
// Removed locally: also remove on the server.
if (!dbEntry._serverHasIgnoredFiles) {
Expand Down
7 changes: 6 additions & 1 deletion src/libsync/discoveryphase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ void DiscoverySingleDirectoryJob::start()
<< "http://owncloud.org/ns:dDC"
<< "http://owncloud.org/ns:permissions"
<< "http://owncloud.org/ns:checksums"
<< "http://nextcloud.org/ns:is-encrypted";
<< "http://nextcloud.org/ns:is-encrypted"
<< "http://nextcloud.org/ns:metadata-files-live-photo";

if (_isRootPath)
props << "http://owncloud.org/ns:data-fingerprint";
Expand Down Expand Up @@ -550,6 +551,10 @@ static void propertyMapToRemoteInfo(const QMap<QString, QString> &map, RemotePer
if (property == "lock-token") {
result.lockToken = value;
}
if (property == "metadata-files-live-photo") {
result.livePhotoFile = value;
result.isLivePhoto = true;
}
}

if (result.isDirectory && map.contains("size")) {
Expand Down
3 changes: 3 additions & 0 deletions src/libsync/discoveryphase.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

#pragma once

#include <QObject>

Check failure on line 17 in src/libsync/discoveryphase.h

View workflow job for this annotation

GitHub Actions / build

src/libsync/discoveryphase.h:17:10 [clang-diagnostic-error]

'QObject' file not found
#include <QElapsedTimer>
#include <QStringList>
#include <csync.h>
Expand Down Expand Up @@ -87,6 +87,9 @@
qint64 lockTime = 0;
qint64 lockTimeout = 0;
QString lockToken;

bool isLivePhoto = false;
QString livePhotoFile;
};

struct LocalInfo
Expand Down
9 changes: 9 additions & 0 deletions src/libsync/syncfileitem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ SyncJournalFileRecord SyncFileItem::toSyncJournalFileRecordWithInode(const QStri
rec._lockstate._lockTime = _lockTime;
rec._lockstate._lockTimeout = _lockTimeout;
rec._lockstate._lockToken = _lockToken;
rec._isLivePhoto = _isLivePhoto;
rec._livePhotoFile = _livePhotoFile;

// Update the inode if possible
rec._inode = _inode;
Expand Down Expand Up @@ -167,6 +169,8 @@ SyncFileItemPtr SyncFileItem::fromSyncJournalFileRecord(const SyncJournalFileRec
item->_sharedByMe = rec._sharedByMe;
item->_isShared = rec._isShared;
item->_lastShareStateFetchedTimestamp = rec._lastShareStateFetchedTimestamp;
item->_isLivePhoto = rec._isLivePhoto;
item->_livePhotoFile = rec._livePhotoFile;
return item;
}

Expand Down Expand Up @@ -237,6 +241,11 @@ SyncFileItemPtr SyncFileItem::fromProperties(const QString &filePath, const QMap
item->_checksumHeader = findBestChecksum(properties.value("checksums").toUtf8());
}

if (properties.contains(QStringLiteral("metadata-files-live-photo"))) {
item->_isLivePhoto = true;
item->_livePhotoFile = properties.value(QStringLiteral("metadata-files-live-photo"));
}

// direction and instruction are decided later
item->_direction = SyncFileItem::None;
item->_instruction = CSYNC_INSTRUCTION_NONE;
Expand Down
3 changes: 3 additions & 0 deletions src/libsync/syncfileitem.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#ifndef SYNCFILEITEM_H
#define SYNCFILEITEM_H

#include <QVector>

Check failure on line 18 in src/libsync/syncfileitem.h

View workflow job for this annotation

GitHub Actions / build

src/libsync/syncfileitem.h:18:10 [clang-diagnostic-error]

'QVector' file not found
#include <QString>
#include <QDateTime>
#include <QMetaType>
Expand Down Expand Up @@ -340,6 +340,9 @@
bool _isAnyInvalidCharChild = false;
bool _isAnyCaseClashChild = false;

bool _isLivePhoto = false;
QString _livePhotoFile;

QString _discoveryResult;
};

Expand Down
8 changes: 8 additions & 0 deletions test/syncenginetestutils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,14 @@
file->lastModified = modTime;
}

void FileInfo::setIsLivePhoto(const QString &relativePath, const bool isLivePhoto)

Check warning on line 226 in test/syncenginetestutils.cpp

View workflow job for this annotation

GitHub Actions / build

test/syncenginetestutils.cpp:226:16 [readability-convert-member-functions-to-static]

method 'setIsLivePhoto' can be made static
{
const auto file = find(relativePath);
Q_ASSERT(file);
file->isLivePhoto = isLivePhoto;
}

void FileInfo::modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout)

Check warning on line 233 in test/syncenginetestutils.cpp

View workflow job for this annotation

GitHub Actions / build

test/syncenginetestutils.cpp:233:16 [readability-convert-member-functions-to-static]

method 'modifyLockState' can be made static

Check warning on line 233 in test/syncenginetestutils.cpp

View workflow job for this annotation

GitHub Actions / build

test/syncenginetestutils.cpp:233:82 [bugprone-easily-swappable-parameters]

6 adjacent parameters of 'modifyLockState' of similar type are easily swapped by mistake
{
FileInfo *file = findInvalidatingEtags(relativePath);
Q_ASSERT(file);
Expand Down Expand Up @@ -411,6 +418,7 @@
xml.writeTextElement(ncUri, QStringLiteral("lock-time"), QString::number(fileInfo.lockTime));
xml.writeTextElement(ncUri, QStringLiteral("lock-timeout"), QString::number(fileInfo.lockTimeout));
xml.writeTextElement(ncUri, QStringLiteral("is-encrypted"), fileInfo.isEncrypted ? QString::number(1) : QString::number(0));
xml.writeTextElement(ncUri, QStringLiteral("metadata-files-live-photo"), fileInfo.isLivePhoto ? QString::number(1) : QString::number(0));
buffer.write(fileInfo.extraDavProperties);
xml.writeEndElement(); // prop
xml.writeTextElement(davUri, QStringLiteral("status"), QStringLiteral("HTTP/1.1 200 OK"));
Expand Down
3 changes: 3 additions & 0 deletions test/syncenginetestutils.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
#pragma once

#include "account.h"

Check failure on line 9 in test/syncenginetestutils.h

View workflow job for this annotation

GitHub Actions / build

test/syncenginetestutils.h:9:10 [clang-diagnostic-error]

'account.h' file not found
#include "common/result.h"
#include "creds/abstractcredentials.h"
#include "logger.h"
Expand Down Expand Up @@ -142,6 +142,8 @@

void setModTimeKeepEtag(const QString &relativePath, const QDateTime &modTime);

void setIsLivePhoto(const QString &relativePath, bool isLivePhoto);

void modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout) override;

void setE2EE(const QString &relativepath, const bool enabled) override;
Expand Down Expand Up @@ -188,6 +190,7 @@
quint64 lockTime = 0;
quint64 lockTimeout = 0;
bool isEncrypted = false;
bool isLivePhoto = false;

// Sorted by name to be able to compare trees
QMap<QString, FileInfo> children;
Expand Down
35 changes: 35 additions & 0 deletions test/testlocaldiscovery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*
*/

#include <QtTest>

Check failure on line 8 in test/testlocaldiscovery.cpp

View workflow job for this annotation

GitHub Actions / build

test/testlocaldiscovery.cpp:8:10 [clang-diagnostic-error]

'QtTest' file not found
#include "syncenginetestutils.h"
#include <syncengine.h>
#include <localdiscoverytracker.h>
Expand Down Expand Up @@ -333,6 +333,41 @@
QVERIFY(!fakeFolder.currentRemoteState().find("C/filename.ext"));
}

void testRedownloadDeletedLivePhotoMov()
{
FakeFolder fakeFolder{FileInfo{}};
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
const auto livePhotoImg = QStringLiteral("IMG_0001.heic");
const auto livePhotoMov = QStringLiteral("IMG_0001.mov");
fakeFolder.localModifier().insert(livePhotoImg);
fakeFolder.localModifier().insert(livePhotoMov);

ItemCompletedSpy completeSpy(fakeFolder);
QVERIFY(fakeFolder.syncOnce());

QCOMPARE(completeSpy.findItem(livePhotoImg)->_status, SyncFileItem::Status::Success);
QCOMPARE(completeSpy.findItem(livePhotoMov)->_status, SyncFileItem::Status::Success);

fakeFolder.remoteModifier().setIsLivePhoto(livePhotoImg, true);
fakeFolder.remoteModifier().setIsLivePhoto(livePhotoMov, true);
QVERIFY(fakeFolder.syncOnce());

SyncJournalFileRecord imgRecord;
QVERIFY(fakeFolder.syncJournal().getFileRecord(livePhotoImg, &imgRecord));
QVERIFY(imgRecord._isLivePhoto);

SyncJournalFileRecord movRecord;
QVERIFY(fakeFolder.syncJournal().getFileRecord(livePhotoMov, &movRecord));
QVERIFY(movRecord._isLivePhoto);

completeSpy.clear();
fakeFolder.localModifier().remove(livePhotoMov);
QVERIFY(fakeFolder.syncOnce());
QCOMPARE(completeSpy.findItem(livePhotoMov)->_status, SyncFileItem::Status::Success);
QCOMPARE(completeSpy.findItem(livePhotoMov)->_instruction, CSYNC_INSTRUCTION_SYNC);
QCOMPARE(completeSpy.findItem(livePhotoMov)->_direction, SyncFileItem::Direction::Down);
}

void testCreateFileWithTrailingSpaces_localAndRemoteTrimmedDoNotExist_renameAndUploadFile()
{
FakeFolder fakeFolder{FileInfo{}};
Expand Down
Loading