From fcec13c548be3fb6ed9d94e2d7bec406aaed4c72 Mon Sep 17 00:00:00 2001 From: Joar Wandborg Date: Thu, 26 Dec 2013 20:44:28 +0100 Subject: [PATCH] [ledgercli] Versioning, error handling - Switched to passing the command via argv instead of stdin to ledger. We might as well as we don't use ledger's long-running mode in an effective manner. - Added version control of the ledger file using pygit2.A - Added error handling in the case of an unbalanced ledger, and cruder error handling in the case of any stderr output from ledger. - [web] Separated transaction_get into transaction_get and transaction_get_all. --- accounting/exceptions.py | 8 ++ accounting/storage/ledgercli.py | 185 +++++++++++++------------------- accounting/web.py | 21 ++-- 3 files changed, 95 insertions(+), 119 deletions(-) diff --git a/accounting/exceptions.py b/accounting/exceptions.py index a95cc42..6d751ca 100644 --- a/accounting/exceptions.py +++ b/accounting/exceptions.py @@ -15,3 +15,11 @@ class AccountingException(Exception): class TransactionNotFound(AccountingException): pass + + +class LedgerNotBalanced(AccountingException): + pass + + +class TransactionIDCollision(AccountingException): + pass diff --git a/accounting/storage/ledgercli.py b/accounting/storage/ledgercli.py index 513edc2..919d8f7 100644 --- a/accounting/storage/ledgercli.py +++ b/accounting/storage/ledgercli.py @@ -2,17 +2,20 @@ # https://gitorious.org/conservancy/accounting-api # License: AGPLv3-or-later +import os import sys import subprocess import logging import time import re +import pygit2 from datetime import datetime from xml.etree import ElementTree from contextlib import contextmanager -from accounting.exceptions import AccountingException, TransactionNotFound +from accounting.exceptions import AccountingException, TransactionNotFound, \ + LedgerNotBalanced, TransactionIDCollision from accounting.models import Account, Transaction, Posting, Amount from accounting.storage import Storage @@ -31,141 +34,93 @@ class Ledger(Storage): self.ledger_file = ledger_file _log.info('ledger file: %s', ledger_file) - self.locked = False - self.ledger_process = None + try: + self.repository = pygit2.Repository( + os.path.join(os.path.dirname(self.ledger_file), '.git')) + except KeyError: + self.repository = None + _log.warning('ledger_file directory does not contain a .git' + ' directory, will not track changes.') - @contextmanager - def locked_process(self): - r''' - Context manager that checks that the ledger process is not already - locked, then "locks" the process and yields the process handle and - unlocks the process when execution is returned. - - Since this decorated as a :func:`contextlib.contextmanager` the - recommended use is with the ``with``-statement. - - .. code-block:: python - - with self.locked_process() as p: - p.stdin.write(b'bal\n') - - output = self.read_until_prompt(p) + # The signature used as author and committer in git commits + self.signature = pygit2.Signature( + name='accounting-api', + email='accounting-api@accounting.example') + def commit_changes(self, message): ''' - if self.locked: - raise RuntimeError('The process has already been locked,' - ' something\'s out of order.') + Commits any changes to :attr:`self.ledger_file` to the git repository + ''' + if self.repository is None: + return - # XXX: This code has no purpose in a single-threaded process - timeout = 5 # Seconds + # Add the ledger file + self.repository.index.read() + self.repository.index.add(os.path.basename(self.ledger_file)) + tree_id = self.repository.index.write_tree() + self.repository.index.write() - for i in range(1, timeout + 2): - if i > timeout: - raise RuntimeError('Ledger process is already locked') + parents = [] + try: + parents.append(self.repository.head.target) + except pygit2.GitError: + _log.info('Repository has no head, creating initial commit') - if not self.locked: - break - else: - _log.info('Waiting for one second... %d/%d', i, timeout) - time.sleep(1) + commit_id = self.repository.create_commit( + 'HEAD', + self.signature, + self.signature, + message, + tree_id, + parents) - process = self.get_process() - - self.locked = True - _log.debug('Lock enabled') - - yield process - - self.locked = False - _log.debug('Lock disabled') - - def assemble_arguments(self): + def assemble_arguments(self, command=None): ''' Returns a list of arguments suitable for :class:`subprocess.Popen` based on :attr:`self.ledger_bin` and :attr:`self.ledger_file`. ''' - return [ + args = [ self.ledger_bin, '-f', self.ledger_file, ] + if command is not None: + args.append(command) - def init_process(self): + return args + + def send_command(self, command): ''' - Creates a new (presumably) ledger subprocess based on the args from - :meth:`Ledger.assemble_arguments()` and then runs - :meth:`Ledger.read_until_prompt()` once (which should return the banner - text) and discards the output. + Creates a new ledger process with the specified :data:`command` and + returns the output. + + Raises an :class:`~accounting.exceptions.AccountingException`-based + Exception based on the ledger-cli stderr. ''' - _log.debug('Starting ledger process...') - self.ledger_process = subprocess.Popen( - self.assemble_arguments(), + _log.debug('Sending command: %r', command) + _log.debug('Starting ledger...') + p = subprocess.Popen( + self.assemble_arguments(command=command), stdout=subprocess.PIPE, stdin=subprocess.PIPE, stderr=subprocess.PIPE) - # Swallow the banner - with self.locked_process() as p: - self.read_until_prompt(p) + output = p.stdout.read() + stderr = p.stderr.read().decode('utf8') - return self.ledger_process + if stderr: + lines = stderr.split('\n') + if 'While balancing transaction from' in lines[1]: + raise LedgerNotBalanced('\n'.join(lines[2:])) - def get_process(self): - ''' - Returns :attr:`self.ledger_process` if it evaluates to ``True``. If - :attr:`self.ledger_process` is not set the result of - :meth:`self.init_process() ` is returned. - ''' - return self.ledger_process or self.init_process() + raise AccountingException(stderr) - def read_until_prompt(self, process): - r''' - Reads from the subprocess instance :data:`process` until it finds a - combination of ``\n]\x20`` (the prompt), then returns the output - without the prompt. - ''' - output = b'' - - while True: - line = process.stdout.read(1) # XXX: This is a hack - - if len(line) > 0: - pass - #_log.debug('line: %s', line) - - output += line - - if b'\n] ' in output: - _log.debug('Found prompt!') - break - - output = output[:-3] # Cut away the prompt - - _log.debug('output: %s', output) + p.send_signal(subprocess.signal.SIGTERM) + _log.debug('Waiting for ledger to shut down') + p.wait() return output - def send_command(self, command): - output = None - - with self.locked_process() as p: - if isinstance(command, str): - command = command.encode('utf8') - - _log.debug('Sending command: %r', command) - - p.stdin.write(command + b'\n') - p.stdin.flush() - - output = self.read_until_prompt(p) - - self.ledger_process.send_signal(subprocess.signal.SIGTERM) - _log.debug('Waiting for ledger to shut down') - self.ledger_process.wait() - self.ledger_process = None - - return output - def add_transaction(self, transaction): ''' Writes a transaction to the ledger file by opening it in 'ab' mode and @@ -177,6 +132,13 @@ class Ledger(Storage): _log.debug('No ID found. Generating an ID.') transaction.generate_id() + exists = self.get_transaction(transaction.id) + + if exists is not None: + raise TransactionIDCollision( + 'A transaction with the id %s already exists: %s' % + (transaction.id, exists)) + transaction.metadata.update({'Id': transaction.id}) transaction_template = ('\n{date} {t.payee}\n' @@ -211,6 +173,8 @@ class Ledger(Storage): with open(self.ledger_file, 'ab') as f: f.write(output) + self.commit_changes('Added transaction %s' % transaction.id) + _log.info('Added transaction %s', transaction.id) _log.debug('written to file: %s', output) @@ -271,9 +235,6 @@ class Ledger(Storage): if transaction.id == transaction_id: return transaction - raise TransactionNotFound( - 'No transaction with id {0} found'.format(transaction_id)) - def reg(self): output = self.send_command('xml') @@ -432,6 +393,8 @@ class Ledger(Storage): for line in lines: f.write(line) + self.commit_changes('Removed transaction %s' % transaction_id) + def update_transaction(self, transaction): ''' Update a transaction in the ledger file. diff --git a/accounting/web.py b/accounting/web.py index 546d955..809f2ff 100644 --- a/accounting/web.py +++ b/accounting/web.py @@ -63,21 +63,26 @@ def transaction_by_id_options(transaction_id=None): @app.route('/transaction', methods=['GET']) +@cors() +@jsonify_exceptions +def transaction_get_all(transaction_id=None): + ''' + Returns the JSON-serialized output of :meth:`accounting.Ledger.reg` + ''' + return jsonify(transactions=app.ledger.get_transactions()) + + @app.route('/transaction/', methods=['GET']) @cors() @jsonify_exceptions def transaction_get(transaction_id=None): - ''' - Returns the JSON-serialized output of :meth:`accounting.Ledger.reg` - ''' - if transaction_id is None: - return jsonify(transactions=app.ledger.get_transactions()) + transaction = app.ledger.get_transaction(transaction_id) - try: - return jsonify(transaction=app.ledger.get_transaction(transaction_id)) - except TransactionNotFound: + if transaction is None: abort(404) + return jsonify(transaction=transaction) + @app.route('/transaction/', methods=['POST']) @cors()