Closed pingu8007 closed 7 years ago
Confirmed. Stack trace:
java.lang.RuntimeException: Unable to start activity ComponentInfo{org.gnucash.android.devel/org.gnucash.android.ui.common.FormActivity}: java.lang.IllegalArgumentException: accounts with GUID 08b2c273aa3e96307fc6f4bbcc48563b does not exist in the db
at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2416)
at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476)
at android.app.ActivityThread.access$900(ActivityThread.java:150)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
Caused by: java.lang.IllegalArgumentException: accounts with GUID 08b2c273aa3e96307fc6f4bbcc48563b does not exist in the db
at org.gnucash.android.db.adapter.DatabaseAdapter.getID(DatabaseAdapter.java:542)
at org.gnucash.android.ui.account.AccountFormFragment.initializeViewsWithAccount(AccountFormFragment.java:395)
at org.gnucash.android.ui.account.AccountFormFragment.onActivityCreated(AccountFormFragment.java:351)
at android.support.v4.app.Fragment.performActivityCreated(Fragment.java:2089)
at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1133)
at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1290)
at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:801)
at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1677)
at android.support.v4.app.FragmentController.execPendingActions(FragmentController.java:388)
at android.support.v4.app.FragmentActivity.onStart(FragmentActivity.java:604)
at android.support.v7.app.AppCompatActivity.onStart(AppCompatActivity.java:178)
at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1238)
at android.app.Activity.performStart(Activity.java:6302)
at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2379)
at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476)
at android.app.ActivityThread.access$900(ActivityThread.java:150)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
@codinguser I've been looking at this issue and it seems it's solved by just uncommenting this constraint in the accounts
table. Why did you leave it commented and implemented it in code (f10b4e433aa353cf0ef913f563be4025b5baa0ec) instead? Does it cause problems in other situations? In my tests I didn't find any issue.
@rivaldi8 I'm not exactly sure why I removed that constraint. But I recall that at some point, we removed constraints because they were taking a long time to process (via triggers) in the database. Therefore, we removed some and just made sure to perform the necessary operations directly via SQL. In this particular case, it might be okay to re-add it and modify the constraint to use "ON DELETE SET NULL"
But I have a question about this bug because I don't quite get why it exists (haven't looked deeper) Using the example the OP gave,, why does account B still have a reference to A after A has been deleted?
I thought we set A to NULL in all places where it is referenced after deletion?
Yes, I was confused at first too. The culprit is DatabaseAdapter.deleteRecord(long rowId) (not overriden by AccountsDbAdapter
). The unit test checks AccountsDbAdapter.deleteRecord(String uid) instead.
That's the nice thing about defining constraints in SQL. They are concise and you can't forget about them, they are always enforced.
I see... sneaky. :) So I see 3 ways to go about this:
Adding the foreign key constraint now will mean having to do a database migration, and since we cannot just alter tables in Sqlite, we have to drop the whole table, re-create it and then re-import it just in order to add that constraint. I would rather not put this effort now, especially as a few releases down the road, we will be having a major db restructuring
Ok, I'll avoid adding the constraint for now.
It seems we can prevent new invalid record created. But without DB restructuring, is it enough to solve invalid record that already exist? I mean this function only called when deleting account, right?
@pingu8007 yes it would be possible for @rivaldi8 to add a migration which does not change the db schema, but just sets the values to NULL where the account with the ID no longer exists. Given the other changes in the code mentioned above, no further invalid records would be created
Yes, I'm working on it.
Fixed in develop.
Steps to reproduce the behaviour
Expected behaviour
Open account modifying interface.
Actual behaviour
App crashed.
Software specifications