From caca1576c8cafaf6f7b57c46ea8125e26819ec0d Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Tue, 3 Jan 2023 00:37:28 +0200 Subject: [PATCH] Correct way to allow setting context with existing This reverts commit 5c4d3eeffd3ddc2d024ee0b6d0807a91c78d5df8. Original solution has introduced special workaround (strict flag) which contradicts the concept of immutable context. Moreover, it introduces possible side-effects, because child process will use the one set by parent instead of having own one. The correct solution is to re-create context in process entry point Sorry, it was Jan 1 and I was drunk :( --- src/ahriman/core/__init__.py | 9 +++----- src/ahriman/core/repository/repository.py | 23 ++++++++++++------- .../core/repository/test_repository.py | 10 ++++---- tests/ahriman/core/test_context_init.py | 11 --------- 4 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/ahriman/core/__init__.py b/src/ahriman/core/__init__.py index 3bcc143a..a75f51b5 100644 --- a/src/ahriman/core/__init__.py +++ b/src/ahriman/core/__init__.py @@ -58,26 +58,23 @@ class _Context: raise ValueError(f"Value {value} is not an instance of {key.return_type}") return value - def set(self, key: ContextKey[T], value: T, strict: bool = True) -> None: + def set(self, key: ContextKey[T], value: T) -> None: """ set value for the specified key Args: key(ContextKey[T]): context key name value(T): context value associated with the specified key - strict(bool, optional): check if key already exists (Default value = True) Raises: KeyError: in case if the specified context variable already exists ValueError: in case if type of value is not an instance of specified return type """ - has_key = key.key in self._content - if strict and has_key: + if key.key in self._content: raise KeyError(key.key) if not isinstance(value, key.return_type): raise ValueError(f"Value {value} is not an instance of {key.return_type}") - if not has_key: - self._content[key.key] = value + self._content[key.key] = value def __iter__(self) -> Iterator[str]: """ diff --git a/src/ahriman/core/repository/repository.py b/src/ahriman/core/repository/repository.py index e1e39abd..4b19d0ad 100644 --- a/src/ahriman/core/repository/repository.py +++ b/src/ahriman/core/repository/repository.py @@ -22,7 +22,7 @@ from __future__ import annotations from pathlib import Path from typing import Dict, Iterable, List, Optional, Type -from ahriman.core import context +from ahriman.core import _Context, context from ahriman.core.alpm.pacman import Pacman from ahriman.core.configuration import Configuration from ahriman.core.database import SQLite @@ -79,16 +79,23 @@ class Repository(Executor, UpdateHandler): def _set_context(self) -> None: """ - set context variables + create context variables and set their values """ - ctx = context.get() + # there is a reason why do we always create fresh context here. + # Issue is that if we are going to spawn child process (e.g. from web service), we will use context variables + # from parent process which we would like to avoid (at least they can have different flags). + # In the another hand, this class is the entry point of the application, so we will always create context + # exactly on the start of the application. + # And, finally, context still provides default not-initialized value, in case if someone would like to use it + # directly without loader + ctx = _Context() - ctx.set(ContextKey("database", SQLite), self.database, strict=False) - ctx.set(ContextKey("configuration", Configuration), self.configuration, strict=False) - ctx.set(ContextKey("pacman", Pacman), self.pacman, strict=False) - ctx.set(ContextKey("sign", GPG), self.sign, strict=False) + ctx.set(ContextKey("database", SQLite), self.database) + ctx.set(ContextKey("configuration", Configuration), self.configuration) + ctx.set(ContextKey("pacman", Pacman), self.pacman) + ctx.set(ContextKey("sign", GPG), self.sign) - ctx.set(ContextKey("repository", type(self)), self, strict=False) + ctx.set(ContextKey("repository", type(self)), self) context.set(ctx) diff --git a/tests/ahriman/core/repository/test_repository.py b/tests/ahriman/core/repository/test_repository.py index 6051d5fd..ee05c55a 100644 --- a/tests/ahriman/core/repository/test_repository.py +++ b/tests/ahriman/core/repository/test_repository.py @@ -32,11 +32,11 @@ def test_set_context(configuration: Configuration, database: SQLite, mocker: Moc instance = Repository.load("x86_64", configuration, database, report=False, unsafe=False) set_mock.assert_has_calls([ - MockCall(ContextKey("database", SQLite), instance.database, strict=False), - MockCall(ContextKey("configuration", Configuration), instance.configuration, strict=False), - MockCall(ContextKey("pacman", Pacman), instance.pacman, strict=False), - MockCall(ContextKey("sign", GPG), instance.sign, strict=False), - MockCall(ContextKey("repository", Repository), instance, strict=False), + MockCall(ContextKey("database", SQLite), instance.database), + MockCall(ContextKey("configuration", Configuration), instance.configuration), + MockCall(ContextKey("pacman", Pacman), instance.pacman), + MockCall(ContextKey("sign", GPG), instance.sign), + MockCall(ContextKey("repository", Repository), instance), ]) diff --git a/tests/ahriman/core/test_context_init.py b/tests/ahriman/core/test_context_init.py index ede41f24..ba4425f2 100644 --- a/tests/ahriman/core/test_context_init.py +++ b/tests/ahriman/core/test_context_init.py @@ -57,17 +57,6 @@ def test_set_value_exception() -> None: ctx.set(ContextKey("key", str), 42) -def test_set_value_exists() -> None: - """ - must skip key set in case if key already exists and strict check is disabled - """ - key, value = ContextKey("key", int), 42 - ctx = _Context() - ctx.set(key, value) - - ctx.set(key, value, strict=False) - - def test_contains() -> None: """ must correctly check if element is in list