Minor changes to Group API to make it more explicit

* Include check for group as recycle bin directly into the Group::isRecycled() function

* Return the original root group from Database::setRootGroup(...) to force memory management transfer
This commit is contained in:
Jonathan White 2023-08-28 07:19:12 -04:00
parent b4ff1fa36c
commit a02bceabd2
14 changed files with 45 additions and 69 deletions

View File

@ -66,9 +66,9 @@ Database::Database()
// block modified signal and set root group
setEmitModified(false);
setRootGroup(new Group());
rootGroup()->setUuid(QUuid::createUuid());
rootGroup()->setName(tr("Passwords", "Root group name"));
// Note: oldGroup is nullptr but need to respect return value capture
auto oldGroup = setRootGroup(new Group());
Q_UNUSED(oldGroup)
m_modified = false;
setEmitModified(true);
@ -481,9 +481,8 @@ void Database::releaseData()
m_data.clear();
m_metadata->clear();
auto oldGroup = rootGroup();
setRootGroup(new Group());
// explicitly delete old group, otherwise it is only deleted when the database object is destructed
// Reset and delete the root group
auto oldGroup = setRootGroup(new Group());
delete oldGroup;
m_fileWatcher->stop();
@ -559,14 +558,12 @@ const Group* Database::rootGroup() const
return m_rootGroup;
}
/**
* Sets group as the root group and takes ownership of it.
* Warning: Be careful when calling this method as it doesn't
* emit any notifications so e.g. models aren't updated.
* The caller is responsible for cleaning up the previous
root group.
/* Set the root group of the database and return
* the old root group. It is the responsibility
* of the calling function to dispose of the old
* root group.
*/
void Database::setRootGroup(Group* group)
Group* Database::setRootGroup(Group* group)
{
Q_ASSERT(group);
@ -574,8 +571,17 @@ void Database::setRootGroup(Group* group)
emit databaseDiscarded();
}
auto oldRoot = m_rootGroup;
m_rootGroup = group;
m_rootGroup->setParent(this);
// Initialize the root group if not done already
if (m_rootGroup->uuid().isNull()) {
m_rootGroup->setUuid(QUuid::createUuid());
m_rootGroup->setName(tr("Passwords", "Root group name"));
}
return oldRoot;
}
Metadata* Database::metadata()

View File

@ -112,7 +112,7 @@ public:
const Metadata* metadata() const;
Group* rootGroup();
const Group* rootGroup() const;
void setRootGroup(Group* group);
Q_REQUIRED_RESULT Group* setRootGroup(Group* group);
QVariantMap& publicCustomData();
const QVariantMap& publicCustomData() const;
void setPublicCustomData(const QVariantMap& customData);

View File

@ -463,7 +463,7 @@ bool Entry::isRecycled() const
return false;
}
return m_group == db->metadata()->recycleBin() || m_group->isRecycled();
return m_group->isRecycled();
}
bool Entry::isAttributeReference(const QString& key) const

View File

@ -223,18 +223,16 @@ Entry* Group::lastTopVisibleEntry() const
bool Group::isRecycled() const
{
auto group = this;
if (!group->database()) {
if (!group->database() || !group->m_db->metadata()) {
return false;
}
do {
if (group->m_parent && group->m_db->metadata()) {
if (group->m_parent == group->m_db->metadata()->recycleBin()) {
return true;
}
if (group == group->m_db->metadata()->recycleBin()) {
return true;
}
group = group->m_parent;
} while (group && group->m_parent && group->m_parent != group->m_db->rootGroup());
} while (group);
return false;
}

View File

@ -228,9 +228,7 @@ private:
bool m_updateTimeinfo;
friend void Database::setRootGroup(Group* group);
friend Entry::~Entry();
friend void Entry::setGroup(Group* group, bool trackPrevious);
friend Group* Database::setRootGroup(Group* group);
};
Q_DECLARE_OPERATORS_FOR_FLAGS(Group::CloneFlags)

View File

@ -408,7 +408,7 @@ namespace FdoSecrets
auto newUuid = FdoSecrets::settings()->exposedGroup(m_backend->database());
auto newGroup = m_backend->database()->rootGroup()->findGroupByUuid(newUuid);
if (!newGroup || inRecycleBin(newGroup)) {
if (!newGroup || newGroup->isRecycled()) {
// no exposed group, delete self
removeFromDBus();
return;
@ -444,7 +444,7 @@ namespace FdoSecrets
});
// Another possibility is the group being moved to recycle bin.
connect(m_exposedGroup.data(), &Group::modified, this, [this]() {
if (inRecycleBin(m_exposedGroup)) {
if (m_exposedGroup->isRecycled()) {
// reset the exposed group to none
FdoSecrets::settings()->setExposedGroup(m_backend->database().data(), {});
}
@ -490,7 +490,7 @@ namespace FdoSecrets
void Collection::onEntryAdded(Entry* entry, bool emitSignal)
{
if (inRecycleBin(entry)) {
if (entry->isRecycled()) {
return;
}
@ -524,7 +524,7 @@ namespace FdoSecrets
void Collection::connectGroupSignalRecursive(Group* group)
{
if (inRecycleBin(group)) {
if (group->isRecycled()) {
return;
}
@ -627,7 +627,7 @@ namespace FdoSecrets
bool Collection::doDeleteEntry(Entry* entry)
{
// Confirm entry removal before moving forward
bool permanent = inRecycleBin(entry) || !m_backend->database()->metadata()->recycleBinEnabled();
bool permanent = entry->isRecycled() || !m_backend->database()->metadata()->recycleBinEnabled();
if (FdoSecrets::settings()->confirmDeleteItem()
&& !GuiTools::confirmDeleteEntries(m_backend, {entry}, permanent)) {
return false;
@ -664,29 +664,6 @@ namespace FdoSecrets
return group;
}
bool Collection::inRecycleBin(Group* group) const
{
Q_ASSERT(m_backend);
Q_ASSERT(group);
if (!m_backend->database()->metadata()) {
return false;
}
auto recycleBin = m_backend->database()->metadata()->recycleBin();
if (!recycleBin) {
return false;
}
return group->uuid() == recycleBin->uuid() || group->isRecycled();
}
bool Collection::inRecycleBin(Entry* entry) const
{
Q_ASSERT(entry);
return inRecycleBin(entry->group());
}
Item* Collection::doNewItem(const DBusClientPtr& client, QString itemPath)
{
Q_ASSERT(m_backend);

View File

@ -107,11 +107,6 @@ namespace FdoSecrets
DatabaseWidget* backend() const;
QString backendFilePath() const;
Service* service() const;
/**
* similar to Group::isRecycled, but we also return true when the group itself is the recycle bin
*/
bool inRecycleBin(Group* group) const;
bool inRecycleBin(Entry* entry) const;
static EntrySearcher::SearchTerm attributeToTerm(const QString& key, const QString& value);

View File

@ -463,8 +463,7 @@ bool KdbxXmlReader::parseRoot()
Group* rootGroup = parseGroup();
if (rootGroup) {
Group* oldRoot = m_db->rootGroup();
m_db->setRootGroup(rootGroup);
auto oldRoot = m_db->setRootGroup(rootGroup);
delete oldRoot;
groupParsedSuccessfully = true;
}

View File

@ -92,8 +92,7 @@ namespace
key->addKey(QSharedPointer<PasswordKey>::create(reference.password));
targetDb->setKey(key);
auto* obsoleteRoot = targetDb->rootGroup();
targetDb->setRootGroup(targetRoot);
auto obsoleteRoot = targetDb->setRootGroup(targetRoot);
delete obsoleteRoot;
targetDb->metadata()->setName(sourceRoot->name());

View File

@ -54,6 +54,7 @@ ShareObserver::ShareObserver(QSharedPointer<Database> db, QObject* parent)
ShareObserver::~ShareObserver()
{
m_db->disconnect(this);
}
void ShareObserver::deinitialize()

View File

@ -62,8 +62,7 @@ void TestAutoType::init()
m_db = QSharedPointer<Database>::create();
m_dbList.clear();
m_dbList.append(m_db);
m_group = new Group();
m_db->setRootGroup(m_group);
m_group = m_db->rootGroup();
AutoTypeAssociations::Association association;

View File

@ -1799,7 +1799,8 @@ void TestCli::testMergeWithKeys()
entry->setPassword("secretsecretsecret");
group->addEntry(entry);
sourceDatabase->setRootGroup(rootGroup);
auto oldGroup = sourceDatabase->setRootGroup(rootGroup);
delete oldGroup;
auto* otherRootGroup = new Group();
otherRootGroup->setName("root");
@ -1815,7 +1816,8 @@ void TestCli::testMergeWithKeys()
otherEntry->setPassword("secretsecretsecret 2");
otherGroup->addEntry(otherEntry);
targetDatabase->setRootGroup(otherRootGroup);
oldGroup = targetDatabase->setRootGroup(otherRootGroup);
delete oldGroup;
sourceDatabase->saveAs(sourceDatabaseFilename);
targetDatabase->saveAs(targetDatabaseFilename);

View File

@ -576,7 +576,9 @@ void TestKeePass2Format::testKdbxKeyChange()
buffer.seek(0);
QSharedPointer<Database> db(new Database());
db->changeKdf(fastKdf(KeePass2::uuidToKdf(m_kdbxSourceDb->kdf()->uuid())));
db->setRootGroup(m_kdbxSourceDb->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
auto oldGroup =
db->setRootGroup(m_kdbxSourceDb->rootGroup()->clone(Entry::CloneNoFlags, Group::CloneIncludeEntries));
delete oldGroup;
db->setKey(key1);
writeKdbx(&buffer, db.data(), hasError, errorString);

View File

@ -1456,8 +1456,8 @@ Database* TestMerge::createTestDatabase()
Database* TestMerge::createTestDatabaseStructureClone(Database* source, int entryFlags, int groupFlags)
{
auto db = new Database();
// the old root group is deleted by QObject::parent relationship
db->setRootGroup(source->rootGroup()->clone(static_cast<Entry::CloneFlag>(entryFlags),
static_cast<Group::CloneFlag>(groupFlags)));
auto oldGroup = db->setRootGroup(source->rootGroup()->clone(static_cast<Entry::CloneFlag>(entryFlags),
static_cast<Group::CloneFlag>(groupFlags)));
delete oldGroup;
return db;
}