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

Fix Bug 1147: Check balance assertions against the amount AFTER #451

Merged
merged 1 commit into from Mar 23, 2016

Conversation

mk12
Copy link
Contributor

@mk12 mk12 commented Mar 23, 2016

Fixes Bug 1147

  • Made balance assertions succeed only against the amount after the posting, not before.
  • Changed the error message to reflect this (the expected value is the amount after).
  • Added a regression test.
  • Updated a couple other regression tests to reflect this change.

@mk12 mk12 force-pushed the bug-1147-balance-assertion branch from 4d207e3 to d082b64 Compare March 23, 2016 06:09
@jwiegley jwiegley merged commit b08c03f into ledger:next Mar 23, 2016
@mk12
Copy link
Contributor Author

mk12 commented Mar 23, 2016

Thanks for merging so quickly.

I amended the commit and force pushed to this branch to fix feat-balance_assert-off.test and feat-balance_assert_split.test just seconds before you merged this, so you may not have noticed. Those were the two failing on Travis — they should pass now.

@jwiegley
Copy link
Member

Can you submit a new pull request for the updated tests? Thanks for the fix!

@mk12
Copy link
Contributor Author

mk12 commented Mar 23, 2016

It already went into next in the latest commit (b08c03f). I was just giving you a heads up because of the coincidental timing (my force push just seconds before your merge). It looks like those tests are passing on next now, so everything is fine.

@mk12 mk12 deleted the bug-1147-balance-assertion branch March 23, 2016 15:47
@thdox
Copy link
Contributor

thdox commented Oct 4, 2016

This is related to bug #1147 (http://bugs.ledger-cli.org/show_bug.cgi?id=1147), and the fix of this bug by github pull request #451 (#451).

Up to now, I was using a ledger exec that was 1 year old. Recently I compiled ledger from latest next. It appears that this PR #451 makes my ledger file to fail at parsing. Here is a simplified use case:

2006/08/17 * Au Bon Bec
    Dépense:Alimentation:Restaurant      100,00 €
    Passif:Crédit:Banque

2006/08/20 * Retrait
    Dépense:Liquide                       60,00 €
    Passif:Crédit:Banque                 -60,00 €
    Passif:Crédit:Banque                  60,00 € = -100,00 €
    Actif:Courant:Cc                     -60,00 €

Here is the error

$ ledger --args-only --decimal-comma --file=test.ledger bal
While parsing file "/media/thierry/W2K8/toto.ledger.bak2", line 8:
While parsing posting:
  Passif:Crédit:Banque                  60,00 € = -100,00 €
                                                     ^^^^^^^^^^^
Error: Balance assertion off by -60,00 € (expected to see -40,00 €)

The balance assertion that I have is correct (-100-60+60=-100), but it is now failing.

I have 137 lines like that in my ledger file.

I can confirm that doing git revert on commit d082b64 on next makes my file passing through ledger bal.

$ ledger --args-only --decimal-comma --file=test.ledger bal
            -60,00 €  Actif:Courant:BnpCc
            160,00 €  Dépense
            100,00 €    Alimentation:Restaurant
             60,00 €    Liquide
           -100,00 €  Passif:Crédit:BanqueAccord
--------------------
                   0

I have opened Bug 1187.

@mk12, is this something that you could look at? I could then propose that exemple as regress test.

@mk12
Copy link
Contributor Author

mk12 commented Oct 5, 2016

This does look wrong, but I don't think it's a regression from my changes.

When you revert d082b64, you're getting the desired behaviour simply because -100,00 € happens to also be the balance in the account before the transaction. The fact that balance assertions used to succeed against the before-balance and the after-balance was unexpected and undocumented, which is why I changed it to only match the after-balance in this pull request.

I've never tried balance assertions on transactions containing multiple postings to the same account. The manual doesn't give any indication of how this would work. It would be a nice improvement to make it work, though.

@colindean
Copy link
Contributor

I'm leaving a note here so that it's discoverable by others. I encountered a problem in my tx record related to this PR. As of this writing, Homebrew ships ledger v3.1.1 at that tag while Ubuntu 18.04 ships a slightly newer revision at d082b64.

Ubuntu 18.04, which is now what I'm using in Gitlab CI as of a few days ago (docker ubuntu:latest), has a newer version of Ledger than what's in Homebrew. That newer version has this commit that appears to change how balance assertions work: checking at the end of every transaction instead of at the end of a day's transactions. I've always thought it was that way but apparently it wasn't!

v3.1.1 to what's in Ubuntu 18.04:

v3.1.1...3a00e1c

The commit that changed behavior:

d082b64

So, if your tx records are suddenly failing balance assertions, read over the errors and you might find a some transactions that occurred on the same day and the balance asserted on one makes it clear that there should be an ordering to those transactions as recorded. For me, I had to swap two transactions to make it work.

tbm added a commit to tbm/ledger2beancount that referenced this pull request May 2, 2018
Colin Dean observed recently that the way balance assertions are done
in ledger has changed after 3.1.1:
ledger/ledger#451 (comment)
tbm added a commit to tbm/ledger2beancount that referenced this pull request May 3, 2018
Colin Dean observed recently that the way balance assertions are done
in ledger has changed after 3.1.1:
ledger/ledger#451 (comment)
@colindean
Copy link
Contributor

colindean commented Mar 6, 2019

I've got another tx record that is getting hit by this change or something related to it badly. I've got nearly 100 transactions with balance assertions that are no longer valid when I unknowingly moved to ledger 3.1.2 on my workstation.

The easy fix is to go back to ledger 3.1.1 but, to me, such a change is a backwards-incompatible change :-/

I extracted ledger@3.1.1: brew install colindean/personal/ledger@3.1.1. When running with it, my existing balance assertions work fine. I'll have to figure out the changes in a future tx record…

@mk12
Copy link
Contributor Author

mk12 commented Mar 7, 2019

If the assertions were valid before because they intentially matched the pre-transaction balance, then it should just be a matter of updating all those assertions to reflect the post-transaction amount. If they were always invalid due to some other mistakes in the journal, but passed beforehand because they happened to match the pre-transaction balances, I suppose that could be harder to track down.

I don’t think it’s a breaking change unless people were intentially asserting pre-transaction balances (which was undocumented). The new failures caused by the update shown have been failures before too, but weren’t — that’s the bug.

@colindean
Copy link
Contributor

colindean commented Mar 7, 2019

I don't believe I was asserting pre-transaction balances. The balance assertions I use are the balances provided by my banks on each line on the statement. I believe this to be the post-transaction balance of the account.

Inside of my ledger, commingled with hundreds of other transactions, ledger 3.1.2 fails on the last transaction for Assets:Cash:Banks:Bank1:Savings:

2018/01/01 * Opening balance
    Assets:Cash:Banks:Bank2:Checking   20,000.00 USD
    Equity:OpeningBalance

2018/10/05 * Transfer from Bank2 to Bank1 Savings
    Assets:Cash:Banks:Bank1:Savings    10,000.00 USD
    Assets:Cash:Banks:Bank2:Checking   -10,000.00 USD = 10,000.00 USD

2018/11/01 * Bank1 Interest
    Assets:Cash:Banks:Bank1:Savings       12.05 USD = 10,012.05 USD
    Income:Interest:Bank1

Error message, among nearly 100 others mostly around Bank2:Checking assertions:

While parsing file "/Users/colin/Documents/Finance/Personal/2018/reports/2018.ledger", line 3817:
While parsing posting:
  Assets:Cash:Banks:Simple:Savings       12.05 USD = 10,012.05 USD
                                                     ^^^^^^^^^^^^^
Error: Balance assertion off by 10,000.00 USD (expected to see 12.05 USD)

Extracted like this, it passes with both versions of ledger. I've only changed Bank2:Checking amount for privacy and there are of course a lot more transactions between 1 Jan and 1 Nov.

@mk12
Copy link
Contributor Author

mk12 commented Mar 7, 2019

@colindean Note that when a transaction fails because of an invalid balance assertion, ledger will ignore the whole transaction and proceed with the rest of the journal. Balance assertions on later transactions with the same account will then fail as well, because they assumed the earlier transaction occurred. So "hundreds" of error messages probably does not mean hundreds of incorrect assertions. It's possible only the first one is incorrect. You should start by looking at the first one.

@colindean
Copy link
Contributor

You were right. There was one balance assertion that caused a cascading series of failures, probably 90 of them. Add in an issue with a balance adjustment transaction that refused to work and a missing commodity on another, and my record is back at parity with 3.1.1: both are failing on a balance adjustment transaction (i.e., all posts are just Account = 9999.99 USD except one that is left blank to let the tx balance. I'll comment it out for now or just manually enter the adjustment amount.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants