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.

Sign up for GitHub

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

Add serverPath column to db metadata table #12033

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+183 −47
Draft

Add serverPath column to db metadata table #12033

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0986e2
Add serverPath column to db metadata table
erikjv Jan 15, 2025
9a861f1
Add `_remoteName` member to `SyncFileItem`
erikjv Jan 16, 2025
ad7862d
Add `remoteName` column to blacklist table
erikjv Jan 16, 2025
7242fd5
Use server file name when talking to the server
erikjv Jan 20, 2025
266252f
Add file/dir name normalization test
erikjv Jan 21, 2025
eef4228
Set remote filename when different from local filename
erikjv Jan 22, 2025
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
73 changes: 51 additions & 22 deletions src/common/syncjournaldb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -347,6 +349,7 @@ bool SyncJournalDb::checkConnect()
// contentChecksum
// contentChecksumTypeId
// hasDirtyPlaceholder
// serverPath
"PRIMARY KEY(phash)"
");");

Expand Down Expand Up @@ -540,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
Expand Down Expand Up @@ -714,6 +718,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;
Expand Down Expand Up @@ -792,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);");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a central mechanism taking care of db migrations? This feels so wrong

if (!query.exec()) {
sqlFail(QStringLiteral("updateBlacklistTableStructure: Add remoteName"), query);
re = false;
}
commitInternal(QStringLiteral("update database structure: add remoteName col"));
}

SqlQuery query(_db);
Expand Down Expand Up @@ -848,10 +873,9 @@ Result<void, QString> 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()) {
Expand All @@ -867,9 +891,11 @@ Result<void, QString> 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();
Expand All @@ -892,6 +918,7 @@ Result<void, QString> 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();
Expand Down Expand Up @@ -1537,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));
}
}
}
Expand Down Expand Up @@ -1675,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;
Expand All @@ -1702,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();
}

Expand Down
2 changes: 2 additions & 0 deletions src/common/syncjournalfilerecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class OCSYNC_EXPORT SyncJournalFileRecord
bool _serverHasIgnoredFiles = false;
bool _hasDirtyPlaceholder = false;
QByteArray _checksumHeader;
QByteArray _serverPath;
};

bool OCSYNC_EXPORT
Expand Down Expand Up @@ -107,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;
Expand Down
1 change: 1 addition & 0 deletions src/libsync/owncloudpropagator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
1 change: 1 addition & 0 deletions src/libsync/syncfileitem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we have the same name remoteName for DB as well? Just to be consistent ...

return item;
}

Expand Down
10 changes: 9 additions & 1 deletion src/libsync/syncfileitem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,14 +247,22 @@ 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
*
* For rename operation this is the rename source and the target is in _renameTarget.
*/
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
Expand Down
Loading