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

Pad directive fails to add metadata to postings. Causes beanquery any_meta() to assert in queries. #767

Open
muonzoo opened this issue Aug 16, 2023 · 3 comments

Comments

@muonzoo
Copy link

muonzoo commented Aug 16, 2023

Description

TL;DR: bean-query (standalone) throws an uncaught TypeError if you use any_meta() in the query and have a pad directive in the ledger.

E           TypeError: 'NoneType' object is not subscriptable

This is an interesting bug because while I thought the problem was with the latest beanquery while chasing it I think the root cause was a lack of metadata on the postings created by the Pad directive within beancount v2 (lines 139 and 144).

One could argue that (if there is no invariant for metadata on a posting) this is only a bug in the new beanquery code. In which case simply mark this wont_fix and I'll pursue the fix in beanquery's repo.

Repro

I've created a repro test case in my fork of beanquery that fails when run against the v2 release of beancount.

This same test passes when run against my fork of beancount where I add metadata to the `Postings' for the transaction generated by the Pad directive. I chose to copy the directive's metadata but equally it could be set to being empty (vs None).

Environment

  • Make a clean Python virtual env (I used 3.10.8)
  • Install beancount v2
  • Install beanquery
  • Run the sample query below and the given sample file, test.bc.

Input File

# cat test-pad.bc
2019-01-01 commodity USD
2019-01-01 open Assets:Main USD
2019-01-01 open Assets:Other USD
2019-01-15 pad Assets:Main Assets:Other
2019-01-16 balance Assets:Main 1000.00 USD

Command to Run

❯ bean-query test-pad.bc 'SELECT ANY_META("X")'
Traceback (most recent call last):
  File "/.../versions/3.10.8/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/cmd.py", line 214, in onecmd
    func = getattr(self, 'do_' + cmd)/
AttributeError: 'BQLShell' object has no attribute 'do_select'. Did you mean: 'on_Select'?

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/.../.pyenv/versions/bean24/bin/bean-query", line 33, in <module>
    sys.exit(load_entry_point('beanquery', 'console_scripts', 'bean-query')())
  File "/.../.pyenv/versions/3.10.8/envs/bean24/lib/python3.10/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/.../.pyenv/versions/3.10.8/envs/bean24/lib/python3.10/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/.../.pyenv/versions/3.10.8/envs/bean24/lib/python3.10/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/.../.pyenv/versions/3.10.8/envs/bean24/lib/python3.10/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/.../Documents/scm/p/beanquery/beanquery/shell.py", line 741, in main
    shell.onecmd(query)
  File "/.../.pyenv/versions/3.10.8/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/cmd.py", line 216, in onecmd
    return self.default(line)
  File "/.../Documents/scm/p/beanquery/beanquery/shell.py", line 317, in default
    self.execute(line)
  File "/.../Documents/scm/p/beanquery/beanquery/shell.py", line 326, in execute
    self.dispatch(statement)
  File "/.../Documents/scm/p/beanquery/beanquery/shell.py", line 309, in dispatch
    return method(statement)
  File "/.../Documents/scm/p/beanquery/beanquery/shell.py", line 515, in on_Select
    cursor = self.context.execute(statement)
  File "/.../Documents/scm/p/beanquery/beanquery/__init__.py", line 45, in execute
    return self.cursor().execute(query, params)
  File "/.../Documents/scm/p/beanquery/beanquery/cursor.py", line 93, in execute
    description, rows = query_execute.execute_query(query)
  File "/.../Documents/scm/p/beanquery/beanquery/query_execute.py", line 135, in execute_query
    return execute_select(query)
  File "/.../Documents/scm/p/beanquery/beanquery/query_execute.py", line 213, in execute_select
    values = [c_expr(context) for c_expr in c_target_exprs]
  File "/.../Documents/scm/p/beanquery/beanquery/query_execute.py", line 213, in <listcomp>
    values = [c_expr(context) for c_expr in c_target_exprs]
  File "/.../Documents/scm/p/beanquery/beanquery/query_env.py", line 52, in __call__
    return func(context, *args)
  File "/.../Documents/scm/p/beanquery/beanquery/query_env.py", line 342, in any_meta
    return context.posting.meta[key]
TypeError: 'NoneType' object is not subscriptable

Discussion

I see a few options for resolving this and would like the maintainer's opinion on which is more acceptable (there are likely other options).

  1. If all postings should have some kind of metadata as an invariant then this is a bug in beancount.
  2. This is a bug in beanquery (and I'll move this to that project) as we can simply improve AnyMeta to deal with the metadata being None - likely by adding TypeError to the 'safe' exceptions.
  3. Both - fair - we can do both.

I have a test case and fix for case 1 but it might be more general a problem if there are other places in beancount where we create postings without metadata. I also have a test case for 2 (haven't implemented the trivial fix yet).

The test case and fix for 2 is also quite simple but would be done in beanquery not beancount's repository.

dnicolodi added a commit to dnicolodi/beanquery that referenced this issue Aug 16, 2023
These functions need to handle the meta field to be None because
transactions inserted by Beancount for pad directives have postings
which the meta field set to None instead than to an empty dictionary.

See beancount/beancount#767.
dnicolodi added a commit to beancount/beanquery that referenced this issue Aug 16, 2023
These functions need to handle the meta field to be None because
transactions inserted by Beancount for pad directives have postings
which the meta field set to None instead than to an empty dictionary.

See beancount/beancount#767.
@dnicolodi
Copy link
Collaborator

I've just pushed a fix for beanquery to handle this gracefully. However, this is a bug in Beancount: the meta field is supposed to always contain a dictionary. I'm very surprised that this issue was not caught before.

@dnicolodi
Copy link
Collaborator

Also, in passing, I would like to point out that the meta(), entry_meta(), and any_meta() functions will soon be deprecated. They are replaced by different constructs.

The postings table gained a new meta column of type dict that can be accessed with the subscript operator: meta('foo') becomes meta['foo'].

The postings table also gained a entry column with a structured type pointing to the parent transaction, which itself has a meta field. Therefore entry_meta('foo') becomes entry.meta['foo'].

Combining these two, any_meta('foo') becomes coalesce(entry.meta['foo'], meta['foo']) which is longer to type, but has the advantage of making it explicit whether the posting or transaction metadata entry has precedence (something I never remember), and to allows to change it.

@muonzoo
Copy link
Author

muonzoo commented Aug 16, 2023

#768

muonzoo pushed a commit to muonzoo/beancount that referenced this issue Aug 16, 2023
- Add test case for None Meta on pads.
- Fix the missing metadata in pad postings.
muonzoo pushed a commit to muonzoo/beancount that referenced this issue Aug 16, 2023
- Add test case for None Meta on pads.
- Fix the missing metadata in pad postings.
muonzoo added a commit to muonzoo/beancount that referenced this issue Aug 17, 2023
muonzoo pushed a commit to muonzoo/beancount that referenced this issue Aug 17, 2023
- Add test case for None Meta on pads.
- Fix the missing metadata in pad postings.
muonzoo added a commit to muonzoo/beancount that referenced this issue Aug 18, 2023
- Add test case for None Meta on pads.
- Fix the missing metadata in pad postings.
blais added a commit that referenced this issue Sep 17, 2023
Ensure there is (empty) metadata on postings when generating pad transaction #767
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

No branches or pull requests

2 participants