Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#162 Bad jGnash file causes unreadable "account list"

closed
Tom Edelson
3
2010-04-12
2010-03-31
Tom Edelson
No

Problems when loading (what is supposed to be) a jGnash data file (file name suffix ".jgnash.xml") can cause problems displaying the "account list" in the main window ... even though jGnash "thinks" that it has successfully loaded the file. Error messages will also be found in the log file.

It is generally not possible to provide a perfect workaround for bad input data. But we should be able to do better than jGnash [1.x] currently does, in two ways:

1) currently a NullPointerException is being caught by vendor code which is part of Swing, meaning that the stack trace is huge, and consists mostly of entries which are also from classes in vendor code. It would be better to (a) generate our own NullPointerException, containing more information about the cause; and (b) catch it in our own code, making the stack trace smaller and easier to understand.

2) the "account list" is left in a strange state, in which parts of the display pane appear to be blank ... until you click on them. It would be better, if possible, to have an "account list" which was structurally and visually as normal as possible, but which contained strings like "*ERR*" in place of information which the program was unable to provide.

Details on the symptoms: multiple stack traces may be printed, because an exception is caught, its stack trace is printed, and then the program is allowed to continue ... only to encounter more exceptions.

Some of these stack traces have, near the beginning, a line like this:

at jgnash.ui.list.AccountListTreeTablePane$AccountBalanceCellRenderer.setValue

Others have, near the beginning, a line like this:

at jgnash.engine.DoubleEntryTransaction.getAmount

Details on how the problem is triggered: the root cause is (almost certainly) a problem in the input file; but I don't yet know exactly what that problem is.

I do think I know, however, that a more proximate cause, at least for the first type of error message, is that displaying the "account list" requires a CommodityNode object representing the "commodity" (usually a currency) associated with the account whose balance is to be displayed. But the method "Engine.getCommodity", expected to return this CommodityNode, is returning null instead.

Discussion

  • Tom Edelson
    Tom Edelson
    2010-04-12

    does not contain a "currencyList" entry for the default currency

     
  • Tom Edelson
    Tom Edelson
    2010-04-12

    Fixed 2010-04-09, in the commit which created revision 2119.

    I'm calling it "fixed", though the behavior of the program is not improved nearly as much as I had hoped.

    At least now we know the cause: every jGnash Account has a currency (or, potentially for some investment accounts, a security) attached to it; this is the "unit of value" for the account, meaning that balances in the account are reported in this currency (or as a number of shares of this security). Internally and ultimately, a currency or security is represented by a CommodityNode object.

    In this file, the "unit of value" for most (or all) accounts is the U.S. dollar. But in the "currencyList" part of the XML file, there is no entry for this currency. This is the problem in the file that causes the symptoms described.

    When jGnash 1.x loads a file, the entries in the "currencyList" part of the XML file become entries in the CommodityMap object associated with the Engine object. Since this particular file doesn't have an entry for the US dollar, no entry is created for it in the CommodityMap, either; that is, no entry is created whose key is the string "USD.en.US".

    However (both with and without this fix), nothing goes visibly wrong while the file is actually being loaded. The problems occur after that, when the program is attempting to display the "Account list". Part if the display of an account, in this list, is the name of its "unit of value" (my term: jGnash does not use this phrase). So in building the screen display of the accounts, for each account, the program needs to retrieve the name of its "unit of value"; as a first step in this, it needs the CommodityNode for the unit of value.

    The method "Engine.getCommodity" is called, at one point in the proceedings, to get this CommodityNode. In the current case, it is looking for the CommodityNode associated with the string "USD.en.US". But since there is no entry in the CommodityMap with that string as its key, Engine.getCommodity returns null.

    The calling method is Account.getCommodityNode. Before this fix, it was assuming that it got a non-null result from Engine.getCommodity, and proceeding to try to use this CommodityNode reference. Since the reference is null, a NullPointerException was thrown.

    Part of the fix is within Account.getCommodityNode: it now checks whether the result from Engine.getCommodity is null before trying to use it. If it is null, the method *explicitly* throws a NullPointerException.

    While this change is not the only one made for this bug, it is the only one which unambiguously improves anything. The improvements are (1) that the explicitly-thrown exception has a message identifying which account is involved, which will show up, when the exception is caught, in the log file; and (2) that it makes the code more self-documenting: reading the code for the method, you now know that a null result from Engine.getCommodity is possible in some circumstance.

    And that's the sole signicant difference, from the user point of view. It is still true, for instance, that in the UI, the "Account list" appears to be [mostly] blank ... although if you click within that blank space, the line of information about one account, the one whose description should be where you happened to click, then becomes visible. And also, when you do that, some additional error messages show up in the log file.

    I had said, when I created this bug report, that I wanted to catch the exception earlier, so that all lines of the "Account list" would be visible (albeit with a string like "*ERR* in place of the missing commodity name.

    I actually got that working, but then undid most of it. I decided that, in doing so, I had changed too much code for too little improvement. A lot of code had to be changed, because it turns out that Account.getCommodityNode is called from multiple places during the process of displaying the "Account list". It's not "just" called for each account to be displayed; it's called several times for each account.

    I also came to the view that making the "Account list" seem to be complete -- although sprinkled with "*ERR* strings -- might not be an improvement at all. It might lull the user into a false sense of security, to the effect that usable data had been loaded from this file. And this is really not the case.

    For the program really to recover from the lack of a necessary "currencyList" entry in the input file, it would have to notice this problem while actually loading the file. Perhaps such a fix will be added one day, but I didn't consider it to be within the scope of this bug.

    For what it's worth, there is now attached to this bug a sample input file, "no-currency-node.jgnash.xml", which demonstrates the problem. Since this file was created by hand, it also contains some XML comments which could be helpful in understanding the issue.

     
  • Tom Edelson
    Tom Edelson
    2010-04-12

    • status: open --> closed