From 9bea2637e5a10c52e49299b22598297b35412373 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 13:09:58 +0100 Subject: initial filesystem transactions This should solve issues that arise when exceptions occur during upload. Hooks into the transaction/pyramid_tm machinery. --- fietsboek/__init__.py | 10 +- fietsboek/actions.py | 46 ++++----- fietsboek/data.py | 155 +++++++++---------------------- fietsboek/fstrans.py | 231 ++++++++++++++++++++++++++++++++++++++++++++++ fietsboek/views/upload.py | 9 +- 5 files changed, 308 insertions(+), 143 deletions(-) create mode 100644 fietsboek/fstrans.py diff --git a/fietsboek/__init__.py b/fietsboek/__init__.py index 2ac41e3..b533e97 100644 --- a/fietsboek/__init__.py +++ b/fietsboek/__init__.py @@ -31,6 +31,7 @@ from pyramid.response import Response from pyramid.session import SignedCookieSessionFactory from . import config as mod_config +from . import fstrans from . import jinja2 as mod_jinja2 from . import transformers from .data import DataManager @@ -92,7 +93,10 @@ def maintenance_mode( """ def tween(request: Request) -> Response: - maintenance = request.data_manager.maintenance_mode() + # We don't want to mess around with transactioned data mangers here, + # so we create a new one without transaction + data_manager = DataManager(Path(request.config.data_dir)) + maintenance = data_manager.maintenance_mode() if maintenance is None: return handler(request) @@ -155,7 +159,9 @@ def main(global_config, **settings): check_db_engine(parsed_config.sqlalchemy_url) def data_manager(request): - return DataManager(Path(request.config.data_dir)) + data_dir = Path(request.config.data_dir) + lock_file = data_dir / "lock" + return DataManager(data_dir, txn=fstrans.begin(lock_file, request.tm)) def redis_(request): return redis.from_url(request.config.redis_url) diff --git a/fietsboek/actions.py b/fietsboek/actions.py index 2d23d97..f9f36ac 100644 --- a/fietsboek/actions.py +++ b/fietsboek/actions.py @@ -93,29 +93,29 @@ def add_track( # Save the GPX data LOGGER.debug("Creating a new data folder for %d", track.id) assert track.id is not None - with data_manager.initialize(track.id) as manager: - LOGGER.debug("Saving backup to %s", manager.backup_path()) - manager.compress_backup(gpx_data) - - for transformer in transformers: - LOGGER.debug("Running %s with %r", transformer, transformer.parameters) - transformer.execute(path) - track.transformers = [ - [tfm.identifier(), tfm.parameters.model_dump()] for tfm in transformers - ] - - track.fast_set_path(path) - - # Best time to build the cache is right after the upload, but *after* the - # transformers have been applied! - track.ensure_cache(path) - dbsession.add(track.cache) - - LOGGER.debug("Building preview image for %s", track.id) - preview_image = trackmap.render(path, layer, tile_requester) - image_io = io.BytesIO() - preview_image.save(image_io, "PNG") - manager.set_preview(image_io.getvalue()) + manager = data_manager.initialize(track.id) + LOGGER.debug("Saving backup to %s", manager.backup_path()) + manager.compress_backup(gpx_data) + + for transformer in transformers: + LOGGER.debug("Running %s with %r", transformer, transformer.parameters) + transformer.execute(path) + track.transformers = [ + [tfm.identifier(), tfm.parameters.model_dump()] for tfm in transformers + ] + + track.fast_set_path(path) + + # Best time to build the cache is right after the upload, but *after* the + # transformers have been applied! + track.ensure_cache(path) + dbsession.add(track.cache) + + LOGGER.debug("Building preview image for %s", track.id) + preview_image = trackmap.render(path, layer, tile_requester) + image_io = io.BytesIO() + preview_image.save(image_io, "PNG") + manager.set_preview(image_io.getvalue()) return track diff --git a/fietsboek/data.py b/fietsboek/data.py index 3fcd922..c75a451 100644 --- a/fietsboek/data.py +++ b/fietsboek/data.py @@ -19,6 +19,7 @@ import brotli from filelock import FileLock from . import util +from .fstrans import Transaction LOGGER = logging.getLogger(__name__) @@ -50,8 +51,9 @@ class DataManager: :ivar data_dir: Path to the data folder. """ - def __init__(self, data_dir: Path): + def __init__(self, data_dir: Path, *, txn: Transaction | None = None): self.data_dir: Path = data_dir + self.txn = txn def _track_data_dir(self, track_id): return self.data_dir / "tracks" / str(track_id) @@ -81,8 +83,11 @@ class DataManager: :return: The manager that can be used to manage this track's data. """ path = self._track_data_dir(track_id) - path.mkdir(parents=True) - return TrackDataDir(track_id, path, journal=True, is_fresh=True) + if self.txn: + self.txn.make_dir(path) + else: + path.mkdir(parents=True) + return TrackDataDir(track_id, path, txn=self.txn) def initialize_user(self, user_id: int) -> "UserDataDir": """Creates the data directory for a user. @@ -92,8 +97,11 @@ class DataManager: :return: The manager that can be used to manage this user's data. """ path = self._user_data_dir(user_id) - path.mkdir(parents=True) - return UserDataDir(user_id, path) + if self.txn: + self.txn.make_dir(path) + else: + path.mkdir(parents=True) + return UserDataDir(user_id, path, txn=self.txn) def purge(self, track_id: int): """Forcefully purges all data from the given track. @@ -101,9 +109,9 @@ class DataManager: This function logs errors but raises no exception, as such it can always be used to clean up after a track. """ - TrackDataDir(track_id, self._track_data_dir(track_id)).purge() + TrackDataDir(track_id, self._track_data_dir(track_id), txn=self.txn).purge() - def open(self, track_id: int) -> "TrackDataDir": + def open(self, track_id: int, *, force: bool = False) -> "TrackDataDir": """Opens a track's data directory. :raises FileNotFoundError: If the track directory does not exist. @@ -111,9 +119,9 @@ class DataManager: :return: The manager that can be used to manage this track's data. """ path = self._track_data_dir(track_id) - if not path.is_dir(): + if not force and not path.is_dir(): raise FileNotFoundError(f"The path {path} is not a directory") from None - return TrackDataDir(track_id, path) + return TrackDataDir(track_id, path, txn=self.txn) def open_user(self, user_id: int) -> "UserDataDir": """Opens a user's data directory. @@ -125,7 +133,7 @@ class DataManager: path = self._user_data_dir(user_id) if not path.is_dir(): raise FileNotFoundError(f"The path {path} is not a directory") from None - return UserDataDir(user_id, path) + return UserDataDir(user_id, path, txn=self.txn) def size(self) -> int: """Returns the size of all data. @@ -164,86 +172,10 @@ class TrackDataDir: semantics, use ``journal = False``. """ - def __init__(self, track_id: int, path: Path, *, journal: bool = False, is_fresh: bool = False): + def __init__(self, track_id: int, path: Path, *, txn: Transaction | None = None): self.track_id: int = track_id self.path: Path = path - self.journal: Optional[list] = [] if journal else None - self.is_fresh = is_fresh - - def __enter__(self) -> "TrackDataDir": - if self.journal is None: - self.journal = [] - return self - - def __exit__(self, exc_type, exc_val, exc_tb) -> Literal[False]: - if exc_type is None and exc_val is None and exc_tb is None: - self.commit() - else: - self.rollback() - return False - - def rollback(self): - """Rolls back the journal, e.g. in case of error. - - :raises ValueError: If the data directory was opened without the - journal, this raises :exc:`ValueError`. - """ - LOGGER.debug("Rolling back state of %s", self.path) - - if self.journal is None: - raise ValueError("Rollback on a non-journalling data directory") - - if self.is_fresh: - # Shortcut if the directory is fresh, simply remove everything - self.journal = None - self.purge() - return - - for action, *rest in reversed(self.journal): - if action == "purge": - (new_name,) = rest - shutil.move(new_name, self.path) - elif action == "add_image": - (image_path,) = rest - image_path.unlink() - elif action == "delete_image": - path, data = rest - path.write_bytes(data) - elif action == "set_preview": - old_data = rest - if old_data is None: - self.preview_path().unlink() - else: - self.preview_path().write_bytes(old_data) - - self.journal = None - - def commit(self): - """Commits all changed and deletes the journal. - - Note that this function will do nothing if the journal is disabled, - meaning it can always be called. - """ - LOGGER.debug("Committing journal for %s", self.path) - - if self.journal is None: - return - - for action, *rest in reversed(self.journal): - if action == "purge": - (new_name,) = rest - shutil.rmtree(new_name, ignore_errors=False, onerror=self._log_deletion_error) - elif action == "add_image": - # Nothing to do here, the image is already saved - pass - elif action == "delete_image": - # Again, nothing to do here, we simply discard the in-memory image data - pass - elif action == "set_preview": - # Still nothing to do here - pass - - self.journal = None + self.txn = txn def lock(self) -> FileLock: """Returns a FileLock that can be used to lock access to the track's @@ -263,13 +195,11 @@ class TrackDataDir: This function logs errors but raises no exception, as such it can always be used to clean up after a track. """ - if self.journal is None: + if self.txn: + self.txn.purge(self.path) + else: if self.path.is_dir(): shutil.rmtree(self.path, ignore_errors=False, onerror=self._log_deletion_error) - else: - new_name = self.path.with_name("trash-" + self.path.name) - shutil.move(self.path, new_name) - self.journal.append(("purge", new_name)) def size(self) -> int: """Returns the size of the data that this track entails. @@ -289,7 +219,10 @@ class TrackDataDir: quality but slowest compression speed. """ compressed = brotli.compress(data, quality=quality) - self.backup_path().write_bytes(compressed) + if self.txn: + self.txn.write_bytes(self.backup_path(), compressed) + else: + self.backup_path().write_bytes(compressed) def decompress_backup(self) -> bytes: """Returns the backup bytes decompressed. @@ -337,15 +270,18 @@ class TrackDataDir: :return: The ID of the saved image. """ image_dir = self.path / "images" - image_dir.mkdir(parents=True, exist_ok=True) + if self.txn: + self.txn.make_dir(image_dir, exist_ok=True) + else: + image_dir.mkdir(parents=True, exist_ok=True) filename = generate_filename(filename) path = image_dir / filename - with open(path, "wb") as fobj: - shutil.copyfileobj(image, fobj) - - if self.journal is not None: - self.journal.append(("add_image", path)) + if self.txn: + self.txn.write_bytes(path, image.read()) + else: + with open(path, "wb") as fobj: + shutil.copyfileobj(image, fobj) return filename @@ -361,10 +297,10 @@ class TrackDataDir: return path = self.image_path(image_id) - if self.journal is not None: - self.journal.append(("delete_image", path, path.read_bytes())) - - path.unlink() + if self.txn: + self.txn.unlink(path) + else: + path.unlink() def preview_path(self) -> Path: """Gets the path to the "preview image". @@ -378,13 +314,10 @@ class TrackDataDir: :param data: The data of the preview image. """ - if self.journal is not None: - try: - previous_preview = self.preview_path().read_bytes() - except FileNotFoundError: - previous_preview = None - self.journal.append(("set_preview", previous_preview)) - self.preview_path().write_bytes(data) + if self.txn: + self.txn.write_bytes(self.preview_path(), data) + else: + self.preview_path().write_bytes(data) class UserDataDir: diff --git a/fietsboek/fstrans.py b/fietsboek/fstrans.py new file mode 100644 index 0000000..8c73f94 --- /dev/null +++ b/fietsboek/fstrans.py @@ -0,0 +1,231 @@ +"""Filesystem transactions.""" + +import enum +import logging +import shutil +import transaction +import uuid +from filelock import FileLock +from pathlib import Path + +LOGGER = logging.getLogger(__name__) + + +def _log_deletion_error(_, path, exc_info): + LOGGER.warning("Failed to remove %s", path, exc_info=exc_info) + + +class TransactionalError(Exception): + pass + + +class State(enum.Enum): + OPEN = enum.auto() + COMMITTING = enum.auto() + COMMITTED = enum.auto() + TAINTED = enum.auto() + + +class Action: + def commit_1(self): + pass + + def commit_2(self): + pass + + def undo(self): + pass + + +class WriteBytes(Action): + def __init__(self, path: Path, data: bytes): + self.path = path + self.data = data + self.old = None + + def __repr__(self): + return f"" + + def commit_1(self): + try: + self.old = self.path.read_bytes() + except FileNotFoundError: + self.old = None + self.path.write_bytes(self.data) + + def undo(self): + if self.old is None: + self.path.unlink() + else: + self.path.write_bytes(self.old) + + +class Unlink(Action): + def __init__(self, path: Path): + self.path = path + self.old = None + + def __repr__(self): + return f"" + + def commit_1(self): + self.old = self.path.read_bytes() + self.path.unlink() + + def undo(self): + self.path.write_bytes(self.old) + + +class MakeDir(Action): + def __init__(self, path: Path, exist_ok: bool = False): + self.path = path + self.exist_ok = exist_ok + self.existed = None + + def __repr__(self): + return f"" + + def commit_1(self): + self.existed = self.path.is_dir() + self.path.mkdir(exist_ok=self.exist_ok) + + def undo(self): + if not self.existed: + self.path.rmdir() + + +class RemoveDir(Action): + def __init__(self, path: Path): + self.path = path + + def __repr__(self): + return f"" + + def commit_1(self): + self.path.rmdir() + + def undo(self): + self.path.mkdir() + + +class Purge(Action): + def __init__(self, path: Path): + self.path = path + + def __repr__(self): + return f"" + + def commit_2(self): + shutil.rmtree(self.path, ignore_errors=False, onerror=_log_deletion_error) + + +class Transaction: + def __init__(self, lock_path: Path): + self.actions: list[Action] = [] + self.actions_done: list[Action] = [] + self.state = State.OPEN + self.lock_path = lock_path + self.id = uuid.uuid4().hex + + def tpc_begin(self, _trans): + pass + + def tpc_vote(self, _trans): + if not self.actions: + return + + with FileLock(self.lock_path): + self.commit_1() + + def tpc_finish(self, _trans): + if not self.actions: + return + + with FileLock(self.lock_path): + self.commit_2() + + def tpc_abort(self, _trans): + if not self.actions_done: + return + + with FileLock(self.lock_path): + self.undo() + + def sortKey(self): + return f"filesystem:{self.id}" + + def undo(self): + for action in reversed(self.actions_done): + LOGGER.debug("Undoing %s", action) + try: + action.undo() + except Exception as exc_inner: + LOGGER.debug("Exception ignored: %s", exc_inner) + + def commit(self, _trans): + pass + + def commit_1(self): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + + self.state = State.COMMITTING + + try: + for action in self.actions: + LOGGER.debug("Executing 1st phase %s", action) + action.commit_1() + self.actions_done.append(action) + except Exception as exc: + LOGGER.debug("Exception while committing") + self.state = State.TAINTED + raise exc + + def commit_2(self): + if self.state != State.COMMITTING: + raise TransactionalError(f"Transaction is not committing but {self.state}") + + self.state = State.TAINTED + for action in self.actions: + LOGGER.debug("Executing 2nd phase %s", action) + action.commit_2() + self.state = State.COMMITTED + + def abort(self, _trans=None): + self.actions.clear() + self.state = State.TAINTED + + def write_bytes(self, path: Path, data: bytes): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + self.actions.append(WriteBytes(path, data)) + + def unlink(self, path: Path): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + self.actions.append(Unlink(path)) + + def make_dir(self, path: Path, *, exist_ok: bool = False): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + self.actions.append(MakeDir(path, exist_ok=exist_ok)) + + def remove_dir(self, path: Path): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + self.actions.append(RemoveDir(path)) + + def purge(self, path: Path): + if self.state != State.OPEN: + raise TransactionalError(f"Transaction is not open but {self.state}") + self.actions.append(Purge(path)) + + +def begin(lock_path: Path, tm=None) -> Transaction: + """Begin and register a new transaction.""" + trans = Transaction(lock_path) + if tm: + tm.get().join(trans) + else: + transaction.manager.get().join(trans) + return trans diff --git a/fietsboek/views/upload.py b/fietsboek/views/upload.py index 5c86b29..7be5a42 100644 --- a/fietsboek/views/upload.py +++ b/fietsboek/views/upload.py @@ -170,14 +170,9 @@ def do_finish_upload(request): request.dbsession.delete(upload) # Don't forget to add the images + manager = request.data_manager.open(track.id, force=True) LOGGER.debug("Starting to edit images for the upload") - try: - actions.edit_images(request, track) - except Exception: - # We just created the folder, so we'll be fine deleting it - LOGGER.info("Deleting partially created folder for track %d", track.id) - request.data_manager.open(track.id).purge() - raise + actions.edit_images(request, track, manager=manager) request.session.flash(request.localizer.translate(_("flash.upload_success"))) -- cgit v1.2.3 From aab0428c54cb742058de5bae72276f720e89f980 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 16:38:40 +0100 Subject: more doc strings --- fietsboek/data.py | 2 +- fietsboek/fstrans.py | 197 ++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 188 insertions(+), 11 deletions(-) diff --git a/fietsboek/data.py b/fietsboek/data.py index c75a451..70e52dc 100644 --- a/fietsboek/data.py +++ b/fietsboek/data.py @@ -13,7 +13,7 @@ import shutil import string import uuid from pathlib import Path -from typing import BinaryIO, Literal, Optional +from typing import BinaryIO, Optional import brotli from filelock import FileLock diff --git a/fietsboek/fstrans.py b/fietsboek/fstrans.py index 8c73f94..c68ca48 100644 --- a/fietsboek/fstrans.py +++ b/fietsboek/fstrans.py @@ -1,13 +1,76 @@ -"""Filesystem transactions.""" +"""Filesystem transactions. + +Motivation +########## + +Fietsboek does a lot of filesystem stuff, such as saving images and track +backups. Like database actions, we want to ensure that these actions happen +"atomically" -- if an error occurs during one action, we want to undo the +previous ones. Similarly, if an error occurs after things have been sent to the +database/filesystem, we want to ensure that we "clean up" (see `issue 98`_). + +.. _issue 98: https://gitlab.com/dunj3/fietsboek/-/issues/98 + +By having "transactionized" file system actions, we can ensure that we do not +not have such issues: + +* Actions are reversible in case changes need to be rolled back. +* Actions are done all-or-nothing. +* Transactions are combined with other transactions (such as SQLAlchemy); if + one fails, the others will be rolled back. + +Implementation +############## + +The main mechanism is :class:`Transaction`. It provides the commit/abort +interface as required by the transaction_ module, and user-facing methods to +enqueue filesystem modifications. + +.. _transaction: https://github.com/zopefoundation/transaction + +A transaction is started by :func:`begin`, which also joins the transaction +to the other transactions managed by ``transaction``. + +A transaction records :class:`Action`, which represent modifications to be done +to the filesystem. Each action knows how it should be executed. Additionally, +each action records how to undo it -- the :class:`WriteBytes` action, for +example, records the previous file state so it can be restored. + +This implementation has some drawbacks: + +* Modifications are kept in-memory, which might be an issue for larger files. +* Extra time is needed to record previous states (read a file before it is + overwritten). +* Changes are not visible to other programs until they are committed. + +But the advantage is the ease of implementation: Cancelling a transaction just +involves clearing the in-memory buffer, and there are no additional temporary +files needed. + +To further ensure that the filesystem is in a consistent state, the +transactions use a lock file to get exclusive access. Currently, this lock +spans the complete data directory. In the future, more fine-grained locking can +be implemented. + +Usage +##### + +The transaction is automatically used by the +:class:`fietsboek.data.DataManager`. + +Interface +######### +""" import enum import logging import shutil -import transaction import uuid -from filelock import FileLock from pathlib import Path +import transaction +from filelock import FileLock + LOGGER = logging.getLogger(__name__) @@ -16,28 +79,52 @@ def _log_deletion_error(_, path, exc_info): class TransactionalError(Exception): - pass + """An exception that occurs when committing filesystem transactions.""" class State(enum.Enum): + """State of the transaction.""" + OPEN = enum.auto() + """Transaction is open for further actions.""" + COMMITTING = enum.auto() + """Transaction is in the process of being committed.""" + COMMITTED = enum.auto() + """Transaction has been committed.""" + TAINTED = enum.auto() + """Transaction is tainted.""" class Action: + """Base class for any actions that can be applied to the filesystem.""" + def commit_1(self): - pass + """Commit this action (phase 1). + + This corresponds to ``tpc_vote``, and may raise an exception if + committing should be cancelled. + """ def commit_2(self): - pass + """Commit this action (phase 2). + + This corresponds to ``tpc_finish``, and should not raise an exception. + """ def undo(self): - pass + """Undo this action. + + This is called if commiting fails, to undo any changes that were + already committed. + """ class WriteBytes(Action): + """Write bytes to the given file.""" + def __init__(self, path: Path, data: bytes): self.path = path self.data = data @@ -61,6 +148,8 @@ class WriteBytes(Action): class Unlink(Action): + """Remove the given file.""" + def __init__(self, path: Path): self.path = path self.old = None @@ -77,6 +166,8 @@ class Unlink(Action): class MakeDir(Action): + """Create the given directory.""" + def __init__(self, path: Path, exist_ok: bool = False): self.path = path self.exist_ok = exist_ok @@ -95,6 +186,8 @@ class MakeDir(Action): class RemoveDir(Action): + """Remove the given (empty) directory.""" + def __init__(self, path: Path): self.path = path @@ -109,6 +202,8 @@ class RemoveDir(Action): class Purge(Action): + """Purge (recursively remove) the given directory.""" + def __init__(self, path: Path): self.path = path @@ -116,10 +211,13 @@ class Purge(Action): return f"" def commit_2(self): + # pylint: disable=deprecated-argument shutil.rmtree(self.path, ignore_errors=False, onerror=_log_deletion_error) class Transaction: + """A transaction, recording pending filesystem changes.""" + def __init__(self, lock_path: Path): self.actions: list[Action] = [] self.actions_done: list[Action] = [] @@ -128,9 +226,19 @@ class Transaction: self.id = uuid.uuid4().hex def tpc_begin(self, _trans): - pass + """Begin the transaction. + + This is required by the two-phase commit protocol of ``transaction``. + """ def tpc_vote(self, _trans): + """Commit (phase 1) the pending transaction. + + This is required by the two-phase commit protocol of ``transaction``. + + This method may raise exceptions to signal that the transaction (and + all linked transactions) should be aborted. + """ if not self.actions: return @@ -138,6 +246,12 @@ class Transaction: self.commit_1() def tpc_finish(self, _trans): + """Commit (phase 2) the pending transaction. + + This is required by the two-phase commit protocol of ``transaction``. + + This method should not raise an exception. + """ if not self.actions: return @@ -145,16 +259,25 @@ class Transaction: self.commit_2() def tpc_abort(self, _trans): + """Abort the transaction, undoing all previously done changes. + + This is required by the two-phase commit protocol of ``transaction``. + """ if not self.actions_done: return with FileLock(self.lock_path): self.undo() + # Needs to conform to transaction API: + # pylint: disable=invalid-name def sortKey(self): + """Returns the sort key to sort this transaction in relation to others. + """ return f"filesystem:{self.id}" def undo(self): + """Undo all actions that have already been applied.""" for action in reversed(self.actions_done): LOGGER.debug("Undoing %s", action) try: @@ -163,9 +286,16 @@ class Transaction: LOGGER.debug("Exception ignored: %s", exc_inner) def commit(self, _trans): - pass + """Commit this transaction. + + This is required by the interface of ``transaction``. + """ def commit_1(self): + """Start the first phase of committing. + + This method is called automatically by the transaction manager. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") @@ -182,6 +312,10 @@ class Transaction: raise exc def commit_2(self): + """Start the second phase of committing. + + This method is called automatically by the transaction manager. + """ if self.state != State.COMMITTING: raise TransactionalError(f"Transaction is not committing but {self.state}") @@ -192,37 +326,80 @@ class Transaction: self.state = State.COMMITTED def abort(self, _trans=None): + """Abort this transaction.""" self.actions.clear() self.state = State.TAINTED def write_bytes(self, path: Path, data: bytes): + """Write the given bytes to the given path. + + This is a transactioned version of :meth:`Path.write_bytes`. + + :param path: Path where to write the bytes to. + :param data: The data to write. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") self.actions.append(WriteBytes(path, data)) def unlink(self, path: Path): + """Unlinks (removes) the given file. + + This is a transactioned version of :math:`Path.unlink`. + + :param path: The path to the file to unlink. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") self.actions.append(Unlink(path)) def make_dir(self, path: Path, *, exist_ok: bool = False): + """Creates the directory. + + This is a transactioned version of :meth:`Path.mkdir`. + + :param path: The directory to create. + :param exist_ok: If ``True``, no error will be raised if the directory + already exists. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") self.actions.append(MakeDir(path, exist_ok=exist_ok)) def remove_dir(self, path: Path): + """Removes the (empty) directory. + + This is a transactioned version of :meth:`Path.rmdir`. + + :param path: The directory to remove. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") self.actions.append(RemoveDir(path)) def purge(self, path: Path): + """Completely remove (recursively) the given path. + + This uses :func:`shutil.rmtree` to delete the path. + + Unlike other actions, this cannot be undone! + + :param path: The directory to remove. + """ if self.state != State.OPEN: raise TransactionalError(f"Transaction is not open but {self.state}") self.actions.append(Purge(path)) def begin(lock_path: Path, tm=None) -> Transaction: - """Begin and register a new transaction.""" + """Begin a new transaction and register it. + + :param lock_path: The path to the lock file to use in order to synchronize + the transaction. + :param tm: The transaction manager from ``transaction`` in which to join + this transaction. + :return: The :class:`Transaction`. + """ trans = Transaction(lock_path) if tm: tm.get().join(trans) -- cgit v1.2.3 From 0a702bcdcd81bae0d05d029b15430ee5861b3277 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 17:08:01 +0100 Subject: better logging of exceptions during rollback --- fietsboek/fstrans.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fietsboek/fstrans.py b/fietsboek/fstrans.py index c68ca48..13b0018 100644 --- a/fietsboek/fstrans.py +++ b/fietsboek/fstrans.py @@ -278,12 +278,19 @@ class Transaction: def undo(self): """Undo all actions that have already been applied.""" + # pylint: disable=broad-exception-caught for action in reversed(self.actions_done): LOGGER.debug("Undoing %s", action) try: action.undo() except Exception as exc_inner: - LOGGER.debug("Exception ignored: %s", exc_inner) + # Hide "during the handling of ... another exception occurred" + exc_inner.__context__ = None + LOGGER.exception( + "Exception ignored during rollback of %s", + action, + exc_info=exc_inner, + ) def commit(self, _trans): """Commit this transaction. -- cgit v1.2.3 From a967b1103b4446528e641cf13c6986c542de56fd Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 17:09:58 +0100 Subject: actually add txn parameter to UserDataDir --- fietsboek/data.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fietsboek/data.py b/fietsboek/data.py index 70e52dc..d4bbb07 100644 --- a/fietsboek/data.py +++ b/fietsboek/data.py @@ -323,9 +323,10 @@ class TrackDataDir: class UserDataDir: """Manager for a single user's data.""" - def __init__(self, user_id: int, path: Path): + def __init__(self, user_id: int, path: Path, *, txn: Transaction | None = None): self.user_id = user_id self.path = path + self.txn = txn def heatmap_path(self) -> Path: """Returns the path for the heatmap tile file. -- cgit v1.2.3 From 95e137f053caaa4c424915ef408cd2920dadc516 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 17:15:16 +0100 Subject: fix types in fstrans --- fietsboek/fstrans.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/fietsboek/fstrans.py b/fietsboek/fstrans.py index 13b0018..35cc992 100644 --- a/fietsboek/fstrans.py +++ b/fietsboek/fstrans.py @@ -68,7 +68,7 @@ import shutil import uuid from pathlib import Path -import transaction +import transaction # type: ignore from filelock import FileLock LOGGER = logging.getLogger(__name__) @@ -128,7 +128,7 @@ class WriteBytes(Action): def __init__(self, path: Path, data: bytes): self.path = path self.data = data - self.old = None + self.old: bytes | None = None def __repr__(self): return f"" @@ -152,7 +152,7 @@ class Unlink(Action): def __init__(self, path: Path): self.path = path - self.old = None + self.old: bytes | None = None def __repr__(self): return f"" @@ -162,7 +162,10 @@ class Unlink(Action): self.path.unlink() def undo(self): - self.path.write_bytes(self.old) + # This should not happen, unless an exception occurs when we read the + # file + if self.old is not None: + self.path.write_bytes(self.old) class MakeDir(Action): @@ -171,7 +174,7 @@ class MakeDir(Action): def __init__(self, path: Path, exist_ok: bool = False): self.path = path self.exist_ok = exist_ok - self.existed = None + self.existed: bool | None = None def __repr__(self): return f"" -- cgit v1.2.3 From f4b27f3400995ffabce02cfc01f6a02895ae66a4 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 17:17:07 +0100 Subject: remove TrackDataDir context manager use in edit --- fietsboek/views/edit.py | 77 ++++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/fietsboek/views/edit.py b/fietsboek/views/edit.py index d5e3a92..2b559d4 100644 --- a/fietsboek/views/edit.py +++ b/fietsboek/views/edit.py @@ -84,46 +84,45 @@ def do_edit(request): data: TrackDataDir = request.data_manager.open(track.id) tz_offset = datetime.timedelta(minutes=int(request.params["date-tz"])) date = datetime.datetime.fromisoformat(request.params["date"]) - with data, data.lock(): - redo_cache = False + redo_cache = False + try: + gpx_bytes = request.POST["gpx"].file.read() + except AttributeError: + pass + else: + LOGGER.info("Setting new track for %s", track.id) try: - gpx_bytes = request.POST["gpx"].file.read() - except AttributeError: - pass - else: - LOGGER.info("Setting new track for %s", track.id) - try: - new_track = convert.smart_convert(gpx_bytes) - except convert.ConversionError as exc: - request.session.flash(request.localizer.translate(_("flash.invalid_file"))) - LOGGER.info("Could not parse gpx: %s", exc) - return HTTPFound(request.route_url("edit", track_id=track.id)) - data.compress_backup(gpx_bytes) - track.fast_set_path(new_track.path()) - track.transformers = [] - redo_cache = True - - track.date = date.replace(tzinfo=datetime.timezone(tz_offset)) - - track.tagged_people = tagged_people - track.title = request.params["title"] - track.visibility = Visibility[request.params["visibility"]] - track.type = TrackType[request.params["type"]] - track.description = request.params["description"] - track.badges = badges - tags = request.params.getall("tag[]") - track.sync_tags(tags) - - actions.edit_images(request, request.context, manager=data) - actions.execute_transformers(request, request.context) - - # actions.execute_transformers automatically rebuilds the cache, so we only need to do - # this if execute_transformers didn't do it - if redo_cache: - LOGGER.info("New file detected, rebuilding cache for %s", track.id) - track.cache = None - track.ensure_cache() - request.dbsession.add(track.cache) + new_track = convert.smart_convert(gpx_bytes) + except convert.ConversionError as exc: + request.session.flash(request.localizer.translate(_("flash.invalid_file"))) + LOGGER.info("Could not parse gpx: %s", exc) + return HTTPFound(request.route_url("edit", track_id=track.id)) + data.compress_backup(gpx_bytes) + track.fast_set_path(new_track.path()) + track.transformers = [] + redo_cache = True + + track.date = date.replace(tzinfo=datetime.timezone(tz_offset)) + + track.tagged_people = tagged_people + track.title = request.params["title"] + track.visibility = Visibility[request.params["visibility"]] + track.type = TrackType[request.params["type"]] + track.description = request.params["description"] + track.badges = badges + tags = request.params.getall("tag[]") + track.sync_tags(tags) + + actions.edit_images(request, request.context, manager=data) + actions.execute_transformers(request, request.context) + + # actions.execute_transformers automatically rebuilds the cache, so we only need to do + # this if execute_transformers didn't do it + if redo_cache: + LOGGER.info("New file detected, rebuilding cache for %s", track.id) + track.cache = None + track.ensure_cache() + request.dbsession.add(track.cache) return HTTPFound(request.route_url("details", track_id=track.id)) -- cgit v1.2.3 From 9c46c821c5a31bfb70fcdd2c15f058780d4ae147 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 17:20:02 +0100 Subject: fix formatting --- fietsboek/actions.py | 4 +--- fietsboek/fstrans.py | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/fietsboek/actions.py b/fietsboek/actions.py index f9f36ac..cdddaa2 100644 --- a/fietsboek/actions.py +++ b/fietsboek/actions.py @@ -100,9 +100,7 @@ def add_track( for transformer in transformers: LOGGER.debug("Running %s with %r", transformer, transformer.parameters) transformer.execute(path) - track.transformers = [ - [tfm.identifier(), tfm.parameters.model_dump()] for tfm in transformers - ] + track.transformers = [[tfm.identifier(), tfm.parameters.model_dump()] for tfm in transformers] track.fast_set_path(path) diff --git a/fietsboek/fstrans.py b/fietsboek/fstrans.py index 35cc992..d402266 100644 --- a/fietsboek/fstrans.py +++ b/fietsboek/fstrans.py @@ -275,8 +275,7 @@ class Transaction: # Needs to conform to transaction API: # pylint: disable=invalid-name def sortKey(self): - """Returns the sort key to sort this transaction in relation to others. - """ + """Returns the sort key to sort this transaction in relation to others.""" return f"filesystem:{self.id}" def undo(self): -- cgit v1.2.3 From be903b49006e9fcdcb2d5e73431762411b94df86 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 18:24:44 +0100 Subject: ensure tracks/users folders exist Since we don't use parents=True anymore to create the folders (which I think is good), we now need to ensure that those exist. So 1. when fietsboek starts up, we create those folders, and 2. when we delete them in the tests, we recreate them. --- fietsboek/__init__.py | 13 +++++++++++++ tests/conftest.py | 2 ++ 2 files changed, 15 insertions(+) diff --git a/fietsboek/__init__.py b/fietsboek/__init__.py index b533e97..737ee53 100644 --- a/fietsboek/__init__.py +++ b/fietsboek/__init__.py @@ -143,6 +143,18 @@ def check_db_engine(sqlalchemy_uri: str): ) +def create_data_folders(data_dir: str): + """Creates the subfolders of the data directory. + + :param data_dir: Path to the data directory, from the config. + """ + ddir = Path(data_dir) + LOGGER.debug("Creating %s/tracks/", ddir) + (ddir / "tracks").mkdir(exist_ok=True) + LOGGER.debug("Creating %s/users/", ddir) + (ddir / "users").mkdir(exist_ok=True) + + def main(global_config, **settings): """This function returns a Pyramid WSGI application.""" # Avoid a circular import by not importing at the top level @@ -157,6 +169,7 @@ def main(global_config, **settings): settings["jinja2.newstyle"] = True check_db_engine(parsed_config.sqlalchemy_url) + create_data_folders(parsed_config.data_dir) def data_manager(request): data_dir = Path(request.config.data_dir) diff --git a/tests/conftest.py b/tests/conftest.py index b49dad2..732c8d2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -76,8 +76,10 @@ def _cleanup_data(app_settings): data_dir = Path(app_settings["fietsboek.data_dir"]) if (data_dir / "tracks").is_dir(): shutil.rmtree(data_dir / "tracks") + (data_dir / "tracks").mkdir() if (data_dir / "users").is_dir(): shutil.rmtree(data_dir / "users") + (data_dir / "users").mkdir() @pytest.fixture(scope='module') def app(app_settings, dbengine, tmp_path_factory): -- cgit v1.2.3 From 4e9d8ed4315dad30b027b310494636921ff1ee6e Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Mon, 29 Dec 2025 18:34:11 +0100 Subject: fix type of create_data_folders --- fietsboek/__init__.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/fietsboek/__init__.py b/fietsboek/__init__.py index 737ee53..4c3a340 100644 --- a/fietsboek/__init__.py +++ b/fietsboek/__init__.py @@ -143,16 +143,15 @@ def check_db_engine(sqlalchemy_uri: str): ) -def create_data_folders(data_dir: str): +def create_data_folders(data_dir: Path): """Creates the subfolders of the data directory. :param data_dir: Path to the data directory, from the config. """ - ddir = Path(data_dir) - LOGGER.debug("Creating %s/tracks/", ddir) - (ddir / "tracks").mkdir(exist_ok=True) - LOGGER.debug("Creating %s/users/", ddir) - (ddir / "users").mkdir(exist_ok=True) + LOGGER.debug("Creating %s/tracks/", data_dir) + (data_dir / "tracks").mkdir(exist_ok=True) + LOGGER.debug("Creating %s/users/", data_dir) + (data_dir / "users").mkdir(exist_ok=True) def main(global_config, **settings): -- cgit v1.2.3