Sync with merge #2260
Sync with merge #2260
Conversation
| SupportSQLiteDatabase tmpDBReadable = tmpDBHelper.getReadableDatabase(); | ||
| // prepare local db (writeable) | ||
| SupportSQLiteDatabase localDB = MmexApplication.getApp().openHelperAtomicReference.get().getWritableDatabase(); | ||
| MmxDate lastLocalSyncDate = getLastLocalSyncDate(storage.getContext()); |
There was a problem hiding this comment.
hi @golesny, this file-level info should be ready from the database metadata.
- sync date/time is for file level
- suid is helpful on row level
- Some fields might be helpful for the column level.
There was a problem hiding this comment.
Thanks, I understood the principle, now. Working on an abstraction and I need to add last modified time to each entry for all tables. I will upload it for payee, first, to let you check the changes. Only the SUID is not helping when an entry was modified.
I guess, I need to implement the manual merge, also.
There was a problem hiding this comment.
How about starting with row-level/suid first? there is no perfect solution for the field/column level, and there is even no updated_at field
There was a problem hiding this comment.
Yes, you are right.
I just don't know what to do, when both entries are modified after creation.
The SUIDs solves only the creation problem.
In the transaction table is the last updated field which is needed for merge. The other tables don't have this.
Or do I miss something?
There was a problem hiding this comment.
@guanlisheng I tried to implement it without the last Modified Date.
This is not possible, because I don't know which side is modified. (I updated the PR)
And then I would need to ignore the changes (either remote or local). This would end up in and endless loop of ignoring the new values.
Example:
User 1 and User 2 are sharing the file and U1 is modifying one payee.
On prefering the local version I would use the new value on sync and upload the new file.
U2 now syncs the file back but I would not take the changes because I would prefer the local changes.
And the file with the old value would be uploaded.
U1 would always upload the local value and U2 always the old value.
I need to add a lastModifiedDate and a LastSyncDate to determine what has changed when.
Otherwise we need to inactivate old rows and create always a new one, but this is a very big change.
What do you think?
My proposal:
- Add a modifiedDateTime to each table in an extra PR (this can be introduced easiliy, default is 1900 or so) -> who can do that? Shall I try it?
- Based on that I provide my PR with the merge
There was a problem hiding this comment.
Hey @golesny
Thank you for continuing on this hard topic. Your understanding is correct that SUID is to mitigate the issue instead.
Modifications on both sides will result in conflict, no matter in which level.
- for file level today, it will raise an error and let the end user decide: either force download or upload.
- for row level, we may consider a similar proposal and leave it to the user to choose one as well.
- for column level, the user may pick a more fine grain when dealing with conflicts.
There was a problem hiding this comment.
@guanlisheng I would not go to Column Level with this PR
I agree, that manual decision will solve the conflict problems. Then, we also don't need the last modified time on each table. It might be a bit annoying when a lot of changes (from edit operation) are incoming (but we can improve later, e.g. checkbox).
I will prepare the PR with manual merging decision.
There was a problem hiding this comment.
@guanlisheng @wolfsolver I have the UI now implemented roughly (currently just for testing).
The merge seems to work now (currently only for Payee and Transaction)
Missing things:
- Show the entity as text (for the conflict dialog)
- After sync/merge the current table needs to be refreshed (currently you have to go out and in again)
|
Please wait to push until 5.1.1 was release Thanks |
No problem, I need some time to finalize the UI. |
|
hi @golesny, eventually yes while it takes time. with your help, android might go first. |
|
I don't think that we have to wait for all platforms ready to merge. Is enough when all platforms handle new suid |
5.1 is gone. :) |
|
@wolfsolver @guanlisheng Next steps: Add all entities |
|
thanks, @golesny , will run it locally with some test data. also + @velmuruganc |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces changes to support sync functionality by exposing entity creation and table name information for multiple repositories, along with adjustments to sync-related methods.
- Changed access modifiers for createEntity() methods from protected to public and added getTableName() implementations
- Updated AccountTransactionRepository to utilize a different date format for last updated time
- Introduced new helper methods in FileStorageHelper and added a constant in InfoKeys to support sync metadata
Reviewed Changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| StockRepository.java | Changed createEntity() access modifier and added getTableName() |
| StockHistoryRepository.java | Changed createEntity() access modifier and added getTableName() |
| SplitScheduledCategoryRepository.java | Changed createEntity() access modifier and added getTableName() (note the entity type returned) |
| SplitCategoryRepository.java | Changed createEntity() access modifier and added getTableName() |
| ScheduledTransactionRepository.java | Changed createEntity() access modifier and added getTableName() |
| RepositoryBase.java | Updated abstract methods' access and added getTableName() declaration |
| ReportRepository.java | Changed createEntity() access modifier and added getTableName() |
| PayeeRepository.java | Changed table name constant to public and updated createEntity() access |
| IModificationTraceable.java | Added new interface for modification traceability |
| CategoryRepository.java, BudgetRepository.java, BudgetEntryRepository.java, AttachmentRepository.java, CurrencyRepository.java | Updated createEntity() access modifiers and added getTableName() implementations |
| AccountTransactionRepository.java | Updated createEntity() access modifier, added getTableName(), and modified date format in update() |
| AccountRepository.java | Changed createEntity() access modifier and added getTableName() |
| QueryAllDataRepository.java | Changed createEntity() access modifier and added getTableName() (with a FIXME placeholder) |
| FileStorageHelper.java | Introduced pullDatabaseToTmpFile() method and modified uploadDatabase() to handle sync rollback |
| InfoKeys.java | Added LAST_SYNC_DATE constant |
Files not reviewed (1)
- app/build.gradle: Language not supported
Comments suppressed due to low confidence (2)
app/src/main/java/com/money/manager/ex/datalayer/SplitScheduledCategoryRepository.java:46
- The repository name is SplitScheduledCategoryRepository, but the createEntity() method returns a SplitRecurringCategory. Verify if this type is correct or if it should be SplitScheduledCategory instead.
public SplitRecurringCategory createEntity() {
app/src/main/java/com/money/manager/ex/datalayer/AccountTransactionRepository.java:106
- Changing from toIsoCombinedString() to toIsoString() may affect the expected date format. Please verify that toIsoString() returns the format required by downstream consumers.
item.setLastUpdatedTime((new MmxDate()).toIsoString());
| return new AccountTransactionDisplay(); | ||
| } | ||
|
|
||
| @Override | ||
| public String getTableName() { | ||
| return ""; // FIXME |
There was a problem hiding this comment.
The getTableName() method currently returns an empty string with a FIXME comment. Please update this to return the correct table name to avoid potential runtime issues when querying data.
| return ""; // FIXME | |
| return "queryalldata"; |
Copilot uses AI. Check for mistakes.
|
@wolfsolver @guanlisheng Sorry for being absent so long. I tried to finalize the merge but there are some datalayer classes for some tables missing: ASSETS_, CURRENCYFORMATS, CURRENCYHISTORY, CUSTOMFIELDDATA, CUSTOMFIELD, INFOTABLE, CATEGORY_, CHECKINGACCOUNT, TRANSLINK |
|
Thanks @golesny. |
|
I added all missing tables, now everything should be complete. Thanks for adding missing ORM @guanlisheng |
|
I think its ready now for experimental use. I added a switch in the sync settings. |
|
Great work. Tomorrow I will compile and test on my android to verify and perform some test before meege |
Hi @guanlisheng
This is the first code for the sync.
the sync code should work (when categories and the rest is not modified).
I could now force a both modified. What is strange is that the metadata check is only working when I download the remote file to the tmp file before checking the modified date.