fava icon indicating copy to clipboard operation
fava copied to clipboard

Error when importing entries with Decimal metadata

Open kpine opened this issue 1 year ago • 2 comments

I have a CSV importer that adds Decimal metadata to entries, e.g. Decimal(114) which renders to foo: 114. When I attempt to import a file with these transactions, an error is raised. The transactions are accepted by beancount when using the importer directly (Numbers (Decimal) are a valid metadata type), but fails when importing via fava.

Exception on /beancount/api/add_entries [PUT]
Traceback (most recent call last):
  File "/app/lib/python3.11/site-packages/flask/app.py", line 2528, in wsgi_app
    response = self.full_dispatch_request()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/flask/app.py", line 1825, in full_dispatch_request
    rv = self.handle_user_exception(e)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/flask/app.py", line 1823, in full_dispatch_request
    rv = self.dispatch_request()
         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/flask/app.py", line 1799, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/fava/json_api.py", line 152, in _wrapper
    res = func(*validator(data))
          ^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/fava/json_api.py", line 349, in put_add_entries
    g.ledger.file.insert_entries(entries)
  File "/app/lib/python3.11/site-packages/fava/core/file.py", line 171, in insert_entries
    fava_options.insert_entry = insert_entry(
                                ^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/fava/core/file.py", line 328, in insert_entry
    content = _format_entry(entry, currency_column, indent)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/fava/core/file.py", line 360, in _format_entry
    string = align(format_entry(entry, prefix=" " * indent), currency_column)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/beancount/parser/printer.py", line 369, in format_entry
    return EntryPrinter(dcontext, render_weights, prefix=prefix)(entry)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/lib/python3.11/site-packages/beancount/parser/printer.py", line 123, in __call__
    method(obj, oss)
  File "/app/lib/python3.11/site-packages/beancount/parser/printer.py", line 182, in Transaction
    self.write_metadata(entry.meta, oss)
  File "/app/lib/python3.11/site-packages/beancount/parser/printer.py", line 159, in write_metadata
    raise ValueError("Unexpected value: '{!r}'".format(value))
ValueError: Unexpected value: '114'

If I manually add a transaction through the UI, with the same metadata, it looks like fava converts it to a string, as the ledger file contains foo: "114" instead. I suppose numbers that come through the API are not converted any valid type.

I can update my importer to use a string, which will solve it, but could fava either convert to string or Decimal in this case?

kpine avatar May 06 '23 17:05 kpine

Yes, like with #980, Fava does not really support all kinds of metadata like Decimals or Balances to be round-tripped through the import interface. A PR fixing this would be welcome :)

yagebu avatar May 24 '23 16:05 yagebu

I also hit this bug. Then I found the type mismatch between fava and beancount. The basic flow of fava importer is like:

  1. Fava backend calls importer to extract entries.
  2. The entries are serialized as JSON and sent back to the front end.
  3. User modifies the entries in the frontend page.
  4. User sends the entries as JSON back to the Fava backend.

So the original data type given by the importer is lost during the JSON serialization and deserialization.

Usually it's not an issue as beancount tries best at handling all kinds of data. But in this Decimal metadata case, the Decimal is serialized into JSON and finally deserialized as int or float, and beancount has a strict type check. Hence the error.

chen-chao avatar Oct 28 '23 16:10 chen-chao

hi @yagebu

I noticed that the interface has used the deserialize function to classify the entries and perform type verification and conversion. The existing bug has been fixed.

If project team need bug-fixes or raise issues in the roadmap, I can provide PR.

Here are just some suggestions

  1. Server-side constraints and validation
  2. Pydantic provides model definition, validation, conversion and parsing.
  3. Pydantic is good at handling nested data types. And it provides a unified paradigm without having to invest too much in if-else processing for each field.
  4. Provide a consistent way to handle the same field type in different models, instead of processing it in each trade operation serializer function.

Example Code

"""Pydantic Model
"""
import re
import datetime
from enum import Enum
from typing import TypeAlias, Mapping, Union

from pydantic import BaseModel, field_validator
from pydantic import constr
from decimal import Decimal
# ---- test case ----
import unittest
from pydantic import ValidationError
from datetime import date
import json

MetaValue: TypeAlias = Union[str, int, bool, Decimal, date]
Meta: TypeAlias = Mapping[str, MetaValue]

CURRENCY_RE = r'[A-Z][A-Z0-9\'\.\_\-]{0,22}[A-Z0-9]'


class EnumOperation(str, Enum):
    TRANSACTION = "Transaction"
    BALANCE = "Balance"
    POSTING = "Posting"


class ModelAmount(BaseModel):
    number: Decimal
    currency: constr(to_upper=True, min_length=3, max_length=3)

    @field_validator('currency')
    def validate_currency(cls, v):
        if not re.match(CURRENCY_RE, v):
            raise ValueError('Invalid currency code')
        return v


class ModelBalance(BaseModel):
    t: EnumOperation = EnumOperation.BALANCE
    meta: Meta
    date: datetime.date
    account: str
    amount: ModelAmount
    tolerance: Decimal | None = None
    diff_amount: str | None = None


class TestModelBalance(unittest.TestCase):

    def test_valid_json(self):
        json_data = '''
        {
            "t": "Balance",
            "meta": {
                "key": "example"
            },
            "date": "2024-08-14",
            "account": "Assets:Cash",
            "amount": {
                "number": "1000.00",
                "currency": "usd"
            },
            "tolerance": "0.01",
            "diff_amount": "10"
        }
        '''
        data = json.loads(json_data)
        model = ModelBalance(**data)

        # type check
        assert isinstance(model.t, EnumOperation)
        # assert isinstance(model.meta, Meta)
        assert isinstance(model.date, date)
        assert isinstance(model.account, str)
        assert isinstance(model.amount, ModelAmount)
        assert isinstance(model.amount.number, Decimal)
        assert isinstance(model.amount.currency, str)
        assert isinstance(model.tolerance, Decimal)
        assert isinstance(model.diff_amount, str)

        # value check
        self.assertEqual(model.t, EnumOperation.BALANCE)
        self.assertEqual(model.meta['key'], "example")
        self.assertEqual(model.date, datetime.datetime(2024, 8, 14).date())
        self.assertEqual(model.account, "Assets:Cash")
        self.assertEqual(model.amount.number, Decimal("1000.00"))
        self.assertEqual(model.amount.currency, "USD")
        self.assertEqual(model.tolerance, Decimal("0.01"))
        self.assertEqual(model.diff_amount, "10")


if __name__ == '__main__':
    unittest.main()

AntiKnot avatar Aug 15 '24 08:08 AntiKnot