Skip to content

imp: add: Verify balance assertions on each posting (#2355) #2356

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jun 1, 2025
Merged

imp: add: Verify balance assertions on each posting (#2355) #2356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9123d88
imp: add: Verify balance assertions on each posting (#2355)
reesmichael1 Mar 19, 2025
cd003dc
Clarify some code after review
reesmichael1 May 29, 2025
45416fb
Add test for assertion combined with posting date
reesmichael1 May 29, 2025
b2eaf51
Ignore assertions during add with -I
reesmichael1 May 29, 2025
6183f82
Handle partial dates in added transactions
reesmichael1 Jun 1, 2025
8dcacdc
;doc: Update manual to describe assertion checking in hledger add
reesmichael1 Jun 1, 2025
f7cd12a
Always check added transaction, even if no immediate assertion
reesmichael1 Jun 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions hledger-lib/Hledger/Data/Balancing.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ module Hledger.Data.Balancing
, isTransactionBalanced
, balanceTransaction
, balanceTransactionHelper
-- * assertion validation
, checkAssertions
-- * journal balancing
, journalBalanceTransactions
-- * tests
Expand Down Expand Up @@ -146,6 +148,17 @@ transactionCheckBalanced BalancingOpts{commodity_styles_} t = errs
isTransactionBalanced :: BalancingOpts -> Transaction -> Bool
isTransactionBalanced bopts = null . transactionCheckBalanced bopts

-- | Verify that any assertions in this transaction hold
-- when included in the larger journal.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this could be slow if called for a lot of transactions, but we intend it to be called only for one, the one user has just entered during an add session. So, minimal user-visible delay even with a big journal and slow machine, hopefully..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. I'm not entirely sure I understand what you're saying, but this does need to be run with the entire journal (since we're checking that the most recent transaction slots into the rest of the journal). For what it's worth, I don't claim to have a huge journal, but hledger stats says I have over 5000 transactions, and hledger add with assertions is still quite snappy on my machine.

Perhaps a nice compromise is to only do the assertion checking if there's an actual assertion in the given transaction?

I'll note that one side effect of this PR that I hadn't considered until now is that this will also check that any future assertions still hold as a result of the transaction, and we'd lose that by only entering the assertion logic if there is an assertion.

Copy link
Owner

@simonmichael simonmichael May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will also check that any future assertions still hold as a result of the transaction

Aiee! Good point, I hadn't thought of that. We should probably mention in the manual, and I should mention in release notes: entering an amount during add can now fail if it would break any balance assertion in the journal. (Even if you aren't adding a new assertion.) Also it can fail if there was a pre-existing failing assertion. Though in that case, add probably wouldn't run - unless you run it with -I. We should probably make sure that the new assertion check also respects -I.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure that the new assertion check also respects -I.

I thought that this check would skip any assertions, but a quick test seems to indicate otherwise! I'll check this out as well. I'm happy to update the manual as well, and good call on flagging this in the release notes.

It also appears that I might have been ahead of myself and already implemented the "only check if there's an assertion in the transaction" restriction--let me think a bit more about the ramifications of that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that this check would skip any assertions [...]

It would help to not just use defbalancingopts everywhere 🤦‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've corrected it so that -I works to ignore assertions. With that in place, I think I'll propose removing the restriction of only checking that assertions pass when there's an assertion in the transaction. I added that for performance, but even with the 100ktxns-1kaccts.journal example file, it takes less than a second to analyze an assertion on my laptop.

This would let us always make sure that the added transaction doesn't break existing assertions, which I think is a nice feature, and with -I, it can be disabled if it's ever undesirable.

checkAssertions :: BalancingOpts -> Journal -> Transaction -> Either String Transaction
checkAssertions bopts j t =
if (ignore_assertions_ bopts) || noassertions t then Right t else do
j' <- journalStyleAmounts j
let newtxns = sortOn tdate (jtxns j' ++ [ t ])
fmap (\_ -> t) $ journalBalanceTransactions defbalancingopts j'{jtxns = newtxns}
where
noassertions = all (isNothing . pbalanceassertion) . tpostings

-- | Balance this transaction, ensuring that its postings
-- (and its balanced virtual postings) sum to 0,
-- by inferring a missing amount or conversion price(s) if needed.
Expand Down Expand Up @@ -1072,6 +1085,32 @@ tests_Balancing =

]

,testGroup "checkAssertions" $ [
testCase "simple assertion on same day" $ do
assertRight $
checkAssertions defbalancingopts nulljournal{ jtxns = [
transaction (fromGregorian 2025 01 01) [ vpost' "a" (usd 1) Nothing ]
] } (transaction (fromGregorian 2025 01 01) [ vpost' "a" (usd 1) (balassert (usd 2)) ])

,testCase "inclusive assertions" $ do
assertRight $
checkAssertions defbalancingopts nulljournal{ jtxns = [
transaction (fromGregorian 2025 01 01) [ vpost' "a:a" (usd 1) Nothing ]
,transaction (fromGregorian 2025 01 02) [ vpost' "a:b" (usd 2) Nothing]
,transaction (fromGregorian 2025 01 02) [ vpost' "a:c" (usd 5) Nothing]
,transaction (fromGregorian 2025 01 03) [ vpost' "a:d" (eur 10) Nothing]
] } (transaction (fromGregorian 2025 01 04) [ vpost' "a" (usd 2) (balassertParInc (usd 10))])

,testCase "multicommodity assertion" $ do
assertRight $
checkAssertions defbalancingopts nulljournal{ jtxns = [
transaction (fromGregorian 2025 01 01) [ vpost' "a" (usd 1) Nothing ]
,transaction (fromGregorian 2025 01 02) [ vpost' "a:b" (eur 2) Nothing ]
,transaction (fromGregorian 2025 01 02) [ vpost' "a:c" (usd 5) Nothing ]
,transaction (fromGregorian 2025 01 03) [ vpost' "a:b" (eur (-2)) Nothing ]
] } (transaction (fromGregorian 2025 01 03) [ vpost' "a" (usd 2) (balassertTotInc (usd 8)) ])
]

,testGroup "commodityStylesFromAmounts" $ [

-- Journal similar to the one on #1091:
Expand Down
62 changes: 46 additions & 16 deletions hledger/Hledger/Cli/Commands/Add.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import Safe (headDef, headMay, atMay)
import System.Console.CmdArgs.Explicit (flagNone)
import System.Console.Haskeline (runInputT, defaultSettings, setComplete)
import System.Console.Haskeline.Completion (CompletionFunc, completeWord, isFinished, noCompletion, simpleCompletion)
import System.Console.Wizard (Wizard, defaultTo, line, output, retryMsg, linePrewritten, nonEmpty, parser, run)
import System.Console.Wizard (Wizard, defaultTo, line, output, outputLn, retryMsg, linePrewritten, nonEmpty, parser, run)
import System.Console.Wizard.Haskeline
import System.IO ( stderr, hPutStr, hPutStrLn )
import Text.Megaparsec
Expand Down Expand Up @@ -231,16 +231,33 @@ confirmedTransactionWizard prevInput es@EntryState{..} stack@(currentStage : _)
confirmedTransactionWizard prevInput es{esPostings=init esPostings} (dropWhile notPrevAmountAndNotEnterDesc stack)

EnterAmountAndComment txnParams account -> amountAndCommentWizard prevInput es >>= \case
Just (amt, comment) -> do
Just (amt, assertion, (comment, tags, pdate1, pdate2)) -> do
-- This check is necessary because we cons a ';' in the comment parser above,
-- and we don't want to add an empty comment here if it wasn't given.
let pcomment = if T.length comment == 1 then "" else comment
let p = nullposting{paccount=T.pack $ stripbrackets account
,pamount=mixedAmount amt
,pcomment=comment
,pcomment=pcomment
,ptype=accountNamePostingType $ T.pack account
,pbalanceassertion = assertion
,pdate=pdate1
,pdate2=pdate2
,ptags=tags
}
amountAndCommentString = showAmount amt ++ T.unpack (if T.null comment then "" else " ;" <> comment)
prevAmountAndCmnt' = replaceNthOrAppend (length esPostings) amountAndCommentString (prevAmountAndCmnt prevInput)
es' = es{esPostings=esPostings++[p], esArgs=drop 1 esArgs}
confirmedTransactionWizard prevInput{prevAmountAndCmnt=prevAmountAndCmnt'} es' (EnterNewPosting txnParams (Just posting) : stack)
-- Include a dummy posting to balance the unfinished transation in assertion checking
dummytxn = nulltransaction{tpostings = esPostings ++ [p, post "" missingamt]
,tdate = txnDate txnParams
,tdescription = txnDesc txnParams }
validated = balanceTransaction defbalancingopts dummytxn >>= checkAssertions defbalancingopts esJournal
case validated of
Left err -> do
liftIO (hPutStrLn stderr err)
confirmedTransactionWizard prevInput es (EnterAmountAndComment txnParams account : stack)
Right _ ->
confirmedTransactionWizard prevInput{prevAmountAndCmnt=prevAmountAndCmnt'} es' (EnterNewPosting txnParams (Just posting) : stack)
Nothing -> confirmedTransactionWizard prevInput es (drop 1 stack)

EndStage t -> do
Expand Down Expand Up @@ -324,7 +341,10 @@ accountWizard PrevInput{..} EntryState{..} = do
| otherwise = Just t
dbg' = id -- strace

amountAndCommentWizard PrevInput{..} EntryState{..} = do
type Comment = (Text, [Tag], Maybe Day, Maybe Day)

amountAndCommentWizard :: PrevInput -> EntryState -> Wizard Haskeline (Maybe (Amount, Maybe BalanceAssertion, Comment))
amountAndCommentWizard previnput@PrevInput{..} entrystate@EntryState{..} = do
let pnum = length esPostings + 1
(mhistoricalp,followedhistoricalsofar) =
case esSimilarTransaction of
Expand All @@ -339,26 +359,36 @@ amountAndCommentWizard PrevInput{..} EntryState{..} = do
| Just hp <- mhistoricalp, followedhistoricalsofar = showamt $ pamount hp
| pnum > 1 && not (mixedAmountLooksZero balancingamt) = showamt balancingamtfirstcommodity
| otherwise = ""
retryMsg "A valid hledger amount is required. Eg: 1, $2, 3 EUR, \"4 red apples\"." $
parser parseAmountAndComment $
retryMsg "A valid hledger amount is required. Eg: 1, $2, 3 EUR, \"4 red apples\"." $
parser' parseAmountAndComment $
withCompletion (amountCompleter def) $
defaultTo' def $
nonEmpty $
linePrewritten (green' $ printf "Amount %d%s: " pnum (showDefault def)) (fromMaybe "" $ prevAmountAndCmnt `atMay` length esPostings) ""
where
parseAmountAndComment s = if s == "<" then return Nothing else either (const Nothing) (return . Just) $
runParser
(evalStateT (amountandcommentp <* eof) nodefcommodityj)
""
(T.pack s)
-- Custom parser that combines with Wizard to use IO via outputLn
parser' f a = a >>= \input ->
case f input of
Left err -> do
outputLn (customErrorBundlePretty err)
amountAndCommentWizard previnput entrystate
Right res -> pure res
parseAmountAndComment s =
if s == "<" then Right Nothing else
Just <$> runParser
(evalStateT (amountandcommentp <* eof) nodefcommodityj)
""
(T.pack s)
nodefcommodityj = esJournal{jparsedefaultcommodity=Nothing}
amountandcommentp :: JournalParser Identity (Amount, Text)
amountandcommentp :: JournalParser Identity (Amount, Maybe BalanceAssertion, Comment)
amountandcommentp = do
a <- amountp
lift skipNonNewlineSpaces
c <- T.pack <$> fromMaybe "" `fmap` optional (char ';' >> many anySingle)
-- eof
return (a,c)
assertion <- optional balanceassertionp
com <- T.pack <$> fromMaybe "" `fmap` optional (char ';' >> many anySingle)
case rtp (postingcommentp Nothing) (T.cons ';' com) of
Left err -> fail $ customErrorBundlePretty err
Right comment -> return $ (a, assertion, comment)
balancingamt = maNegate . sumPostings $ filter isReal esPostings
balancingamtfirstcommodity = mixed . take 1 $ amounts balancingamt
showamt = wbUnpack . showMixedAmountB defaultFmt . mixedAmountSetPrecision
Expand Down
6 changes: 6 additions & 0 deletions hledger/hledger.m4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1803,6 +1803,12 @@ Eg a [commodity directive](#commodity-directive)
may limit the display precision, but this will not affect balance assertions.
Balance assertion failure messages show exact amounts.

### Assertions and hledger add

Balance assertions can be included in the amounts given in `add`.
All types of assertions are supported, and assertions can be used as
in a normal journal file.

## Posting comments

Text following `;`, at the end of a posting line,
Expand Down
92 changes: 90 additions & 2 deletions hledger/test/add.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,96 @@ $ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> /Amount 3 \[-0.75\]:/
>2 //

## 10. shouldn't add decimals if there aren't any
## 10. Balance assertions with ==

<
2025-05-01
x
a
50 USD
b
-50 USD == 50 USD
.
$ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> //
>2 /Balance assertion failed in b/

## 11. Balance assertions with =

<
2025-05-01
x
a
\$10
a
10 EUR
a
-10 EUR = 0 EUR
a
\$-10 = $0
.
$ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> /Save this transaction to the journal/
>2 //

## 12. Assertions with subaccounts

<
2025-05-01
x
a:b
1000 JPY
a
-500 JPY ==* 500 JPY
c
-500 JPY
.
$ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> /Save this transaction to the journal/
>2 //

## 13. Assertions with posting dates
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test add-ing an amount with an assertion and a posting date ? I'm not sure if that's supported.

Also what about balance assignments, are they expected to work or be rejected ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! It looks like it's not possible to add an assertion and a posting date such that the assertion would be valid on the day of the posting date. However, this is also not valid in a journal file:

2025-05-01 x
    a     50 USD
    b    -50 USD

2025-05-02 x2
    a    100 USD  == 50 USD  ; date:2025-05-05
    c   -100 USD

2025-05-10 x3
    a     50 USD  == 200 USD
    d    -50 USD

$ hledger print -f test.hledger
hledger: Error: /home/michael/builds/hledger/test.hledger:6:19:
  | 2025-05-02 x2
6 |     a         100 USD == 50 USD  ; date:2025-05-05
  |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |     c        -100 USD

This balance assertion failed.
In account:    a
and commodity: USD                             (no other commodities allowed)
this balance was asserted:     50
but the calculated balance is: 150

This might be possible to support eventually, but since this is already the existing behavior, I think that's better for a follow-up PR.

Balance assignments aren't supported in stock hledger:

$ hledger add
Date [2025-05-28]:
Description: Opening Balances
Account 1: Assets:Checking
Amount  1:   = $409.32
A valid hledger amount is required. Eg: 1, $2, 3 EUR, "4 red apples".
Amount  1:

If we're interested, I could look at adding those next.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this is also not valid in a journal file:

I wouldn't expect that journal file to pass. But this should be accepted, shouldn't it ?

$ hledger -f new.j add
Date [2025-05-28]: 
Description: 
Account 1: (a)
Amount  1: 10
Account 2 (or . or enter to finish this transaction): 
2025-05-28
    (a)              10

Save this transaction to the journal ? [y]: 
Saved.
Starting the next transaction (. or ctrl-D/ctrl-C to quit)
Date [2025-05-28]: 
Description: 
Account 1 [(a)]: 
Amount  1 [10]: 3 = 3  ; date:5/1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Equivalent to this journal:)

2025-05-28
    (a)              10

2025-05-28
    (a)               3 = 3  ; date:5/1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Balance assignments aren't supported in stock hledger:

Right you are, current hledger doesn't allow add-ing balance assertions so of course it doesn't allow add-ing balance assignments either.

Once it allows add-ing assertions, people might expect assignments too, let's mention in the manual that add-ing those isn't supported yet.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this should be accepted, shouldn't it ?

And yes that works as desired, I just checked. (Except it required me to enter the year; ideally it would use the transaction date's year as default.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks! I've added a test for this case. Not sure why the partial date doesn't work...I'll dig into what the journal parsing does when handling partial dates.


<
2025-05-01
x
a
50 USD ; date:2025-05-10
b
-50 USD
.
y
2025-05-05
x2
a
10 USD == 10 USD
c
-10 USD
.
# Check the output with c to make sure we get to the final transaction display
# (anything generic is also in the first transaction)
$ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> /c[[:space:]]+-10 USD/
>2 //

## 14. Multi-commodity subaccount assertions
<
2025-05-01
x
a:b
50 EUR
a:c
500 MXN
a
-50 EUR =* 0 EUR
a
-500 MXN =* 0 MXN
.
$ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
> /Save this transaction to the journal/
>2 //

## 15. shouldn't add decimals if there aren't any
## printf '\n\na\n1\nb\n' | hledger -f /dev/null add
# <
#
Expand All @@ -124,4 +213,3 @@ $ rm -f nosuch.journal; hledger -f nosuch.journal add; rm -f nosuch.journal
# b
# $ hledger -f /dev/null add
# > /amount 2 \[-1\]/