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

App crashes when trying modify account if default transaction account no longer exist. #654

Closed
pingu8007 opened this issue Feb 16, 2017 · 10 comments

Comments

@pingu8007
Copy link

commented Feb 16, 2017

Steps to reproduce the behaviour

  1. Use double entry mode.
  2. Create account A and B.
  3. Set B's default transaction account to A.
  4. Remove account A.
  5. Modify account B, crash occurred.

Expected behaviour

Open account modifying interface.

Actual behaviour

App crashed.

Software specifications

  • GnuCash Android version: 2.1.4
  • System Android version: Nougat 7.1.1
  • Device type: Google Pixel
@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Feb 26, 2017

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) 

@rivaldi8 rivaldi8 added the bug label Feb 26, 2017

@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Mar 23, 2017

@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 (f10b4e4) instead? Does it cause problems in other situations? In my tests I didn't find any issue.

@codinguser

This comment has been minimized.

Copy link
Owner

commented Mar 24, 2017

@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?

@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2017

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.

@rivaldi8 rivaldi8 self-assigned this Mar 24, 2017

@codinguser

This comment has been minimized.

Copy link
Owner

commented Mar 25, 2017

I see... sneaky. :)
So I see 3 ways to go about this:

  • Override the method deleteRecord(long) in AccountsDbAdapter
  • Change the call from the deleteRecord(long) to deleteRecord(String).
  • Both of the above (preferred)

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

@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Mar 26, 2017

Ok, I'll avoid adding the constraint for now.

@pingu8007

This comment has been minimized.

Copy link
Author

commented Mar 27, 2017

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?

@codinguser

This comment has been minimized.

Copy link
Owner

commented Mar 27, 2017

@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

@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2017

Yes, I'm working on it.

rivaldi8 added a commit to rivaldi8/gnucash-android that referenced this issue Mar 27, 2017

Ensure default_transfer_account_uid fields don't reference deleted ac…
…counts

For some reason we don't have a foreign key constraint on this field, so we
have to enforce it from code. We do so from AccountsDbAdapter.deleteRecord(String)
but not from DatabaseAdapter.deleteRecord(long), which is not overriden
by AccountsDbAdapter (called by AccountsListFragment).

We don't override deleteRecord(long) to avoid infinite recursion (if we call
deleteRecord(String) to avoid code duplication). So we just change
AccountsListFragment to call deleteRecord(String). The proper solution, with a
SQL constraint, will be implemented later when we restructure the database to
make it compatible with the desktop.

Fixes codinguser#654

codinguser added a commit that referenced this issue Mar 27, 2017

Merge pull request #662 from rivaldi8/bugfix-654-modify-account-defau…
…lt-removed

Fixes #654: App crashes when trying modify account if default transaction account no longer exist
@rivaldi8

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

Fixed in develop.

@rivaldi8 rivaldi8 closed this Mar 28, 2017

@codinguser codinguser added this to the v2.2.0 milestone Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.