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

Detect database file changes using hash #3612

Merged
merged 3 commits into from
Oct 20, 2019
Merged

Conversation

droidmonkey
Copy link
Member

Type of change

  • ✅ Refactor (significant modification to existing code)
  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

Cleanup Database Save Functions

  • Make a clear distinction between saving to the existing file path and saving to a new file path
  • Use proper save function calls in CLI

Move FileWatcher into database class

Every database that has been opened now watch's it's own file. This allows the database class to manage file changes and detect fail conditions during saving. Additionally, all stakeholders of the database can listen for the database file changed notification and respond accordingly.

Performed significant cleanup of the autoreload code within DatabaseWidget. Fixed several issues with handling changes due to merging, not merging, and other scenarios while reloading.

Prevent database saves to the same file if there are changes on disk that have not been merged with the open database.

Improve database and CLI tests

Testing strategy

Tested manually and improved test cases.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • ✅ My change requires a change to the documentation, and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

src/core/Database.cpp Outdated Show resolved Hide resolved
Copy link
Member

@louib louib left a comment

Choose a reason for hiding this comment

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

some comments, mainly reviewed the CLI changes

src/core/Database.cpp Show resolved Hide resolved
tests/TestCli.cpp Outdated Show resolved Hide resolved
tests/TestCli.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member Author

Changes made and fixed tests

@droidmonkey
Copy link
Member Author

@phoerious this is ready for your review

src/core/Database.cpp Outdated Show resolved Hide resolved
src/core/FileWatcher.cpp Outdated Show resolved Hide resolved
src/core/FileWatcher.cpp Outdated Show resolved Hide resolved
src/core/FileWatcher.h Show resolved Hide resolved
@droidmonkey
Copy link
Member Author

Changes made, I recommend a merge commit for this one since a lot of fundamental code was changed.

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

If all three commits compile separately, you can simply to a rebase merge. Otherwise squash them.

@droidmonkey
Copy link
Member Author

I will verify but pretty sure they do.

* Make a clear distinction between saving to the existing file path and saving to a new file path
* Use proper save function calls in CLI
* Fix #3506
* Fix #2389
* Fix #2536
* Fix #2230

Every database that has been opened now watch's it's own file. This allows the database class to manage file changes and detect fail conditions during saving. Additionally, all stakeholders of the database can listen for the database file changed notification and respond accordingly.

Performed significant cleanup of the autoreload code within DatabaseWidget. Fixed several issues with handling changes due to merging, not merging, and other scenarios while reloading.

Prevent database saves to the same file if there are changes on disk that have not been merged with the open database.
@droidmonkey droidmonkey merged commit 1e69427 into develop Oct 20, 2019
@droidmonkey droidmonkey deleted the feature/detect-changes branch October 20, 2019 22:56
@FlorentDotMe
Copy link

That's a great improvement ! Thank you for your work. As the new release isn't published yet, I don't really want to open a new issue. Just to point out an unexpected behavior with this feature.

Expected Behavior

User A and User B are simultaneously using the same keepass file.

User A is editing the new entry E1. At the same time, User B is editing a new entry E2. User A is the first to validate his entry E1 while User B is still editing E2. Database is updated. Then User B validates his entry E2. Database is updated. Both users see E1 and E2 in the database.

Current Behavior

User A is editing the new entry E1. At the same time, User B is adding a new entry E2. User A is the first to validate his entry E1 while User B is still editing E2. Database is updated. Then User B validates his entry E2. Database is NOT updated. Finally, only E1 was added to the database and E2 is lost.

Debug Info

KeePassXC - Version 2.5.0-snapshot
Build Type: Snapshot
Revision: 4cc06f9

Qt 5.11.3
Debugging mode is disabled.

Operating system: Debian GNU/Linux 10 (buster)
CPU architecture: x86_64
Kernel: linux 4.19.0-5-amd64

Enabled extensions:

  • Auto-Type

Cryptographic libraries:
libgcrypt 1.8.4

@droidmonkey
Copy link
Member Author

This scenario would occur without this change. Please open a new issue for this and a more refined sequence of events please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment