From 72ac617678a9dfd6c1977bbea9c116aa5391d997 Mon Sep 17 00:00:00 2001 From: Dimitri Pisarev Date: Sat, 7 Feb 2026 23:22:03 +0100 Subject: [PATCH] refactor: decompose GenerateQRHandler into focused components (004) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract DynamicQRCodeResolver service from GenerateQRHandler to follow SRP: - DynamicQRCodeResolver: short code generation, collision resolution, redirect URL building - DynamicQRSetup: immutable DTO for dynamic QR preparation result Refactor GenerateQRHandler into 4-phase architecture: 1. _prepare_content() — build content + optional dynamic setup 2. _generate_image() — QR image I/O (outside transaction) 3. _persist_with_retry() — DB writes with collision retry 4. Event publishing with BaseEvent.with_updated() Key improvements: - I/O (image generation) now happens OUTSIDE database transactions - Collision handling simplified: single check per tx, retry externally - BaseEvent.with_updated() replaces fragile manual event reconstruction - DI container wired with new singleton services Tests: 1667 passed (18 new tests for DynamicQRCodeResolver + BaseEvent.with_updated) --- .../qr/generate_qr_handler.py | 549 +++++++++++------- bot/application/services/__init__.py | 3 + .../services/dynamic_qr_resolver.py | 133 +++++ bot/di/container.py | 14 + bot/di/modules/command_handlers.py | 19 +- bot/domain/events/base.py | 20 + tests/infrastructure/test_unit_of_work.py | 20 +- .../qr/test_generate_qr_handler.py | 15 +- .../services/test_dynamic_qr_resolver.py | 269 +++++++++ tests/unit/domain/events/test_base.py | 65 +++ 10 files changed, 876 insertions(+), 231 deletions(-) create mode 100644 bot/application/services/dynamic_qr_resolver.py create mode 100644 tests/unit/application/services/test_dynamic_qr_resolver.py diff --git a/bot/application/command_handlers/qr/generate_qr_handler.py b/bot/application/command_handlers/qr/generate_qr_handler.py index 1d09411..8eccd74 100644 --- a/bot/application/command_handlers/qr/generate_qr_handler.py +++ b/bot/application/command_handlers/qr/generate_qr_handler.py @@ -4,12 +4,18 @@ Handles QR code generation with validation, saving to history, updating user stats, and event publishing. -Uses Unit of Work pattern for transactional consistency: -- All DB operations (save QR, create redirect, update user) in single transaction -- Events published AFTER successful commit -- Cache invalidated AFTER successful commit - -Uses Rich Domain Model: events are emitted by entities. +Architecture (after decomposition — see docs/imrovements/004): +- PHASE 1: Validation & preparation (no I/O, pure logic) +- PHASE 2: QR image generation (I/O, outside transaction) +- PHASE 3: DB operations (inside transaction, only DB I/O) +- PHASE 4: Side effects (after commit — events, cache) + +Key invariant: QR image generation (I/O) NEVER happens inside +an open database transaction. On short code collision, we exit +the transaction, regenerate the image, and retry. + +Uses DynamicQRCodeResolver for short code management. +Uses BaseEvent.with_updated() instead of manual event reconstruction. """ import hashlib @@ -19,38 +25,77 @@ from bot.application.command_handlers.base import ICommandHandler from bot.application.commands import GenerateQRCommand -from bot.application.services.redirect_url_builder import RedirectURLBuilder -from bot.config import Config +from bot.application.services.dynamic_qr_resolver import ( + DynamicQRCodeResolver, + DynamicQRSetup, +) from bot.domain.entities import QRCode, QRRedirect from bot.domain.ports import IEventBus, IQRCodeGenerator, IUnitOfWork from bot.domain.value_objects import URL, QRFormat, ShortCode logger = logging.getLogger(__name__) -# Maximum number of short code generation attempts before giving up -_MAX_SHORT_CODE_RETRIES = 10 - # QR types that encode data directly and cannot be dynamic _NON_DYNAMIC_QR_TYPES = frozenset({"wifi", "payment", "vcard", "text"}) +# Maximum attempts for the generate-check-regenerate cycle +_MAX_TRANSACTION_RETRIES = 3 + + +class _PreparedContent: + """Internal DTO holding validated & prepared content for QR generation. + + Not exported — only used within GenerateQRHandler to pass data + between phases without a dozen parameters. + """ + + __slots__ = ( + "encode_url", + "storage_url", + "target_url", + "url_hash", + "qr_format", + "is_dynamic", + "is_non_url_type", + "dynamic_setup", + ) + + def __init__( + self, + *, + encode_url, + storage_url: URL, + target_url: URL | None, + url_hash: str, + qr_format: QRFormat, + is_dynamic: bool, + is_non_url_type: bool, + dynamic_setup: DynamicQRSetup | None, + ): + self.encode_url = encode_url + self.storage_url = storage_url + self.target_url = target_url + self.url_hash = url_hash + self.qr_format = qr_format + self.is_dynamic = is_dynamic + self.is_non_url_type = is_non_url_type + self.dynamic_setup = dynamic_setup + class GenerateQRHandler(ICommandHandler[GenerateQRCommand, tuple[bytes, int | None]]): """ Command handler for QR code generation. - Responsibilities: - - Validate URL and format - - Generate QR code binary via QRCodeGenerator (stateless, outside transaction) - - Save QR code to history (inside transaction) - - Create redirect for dynamic QR (inside transaction) - - Increment user's QR count (inside transaction) - - Invalidate user stats cache (AFTER commit) - - Publish events from entities (AFTER commit) + Orchestrates the generation flow: + 1. Validate and prepare content (pure logic) + 2. Generate QR image (I/O, outside transaction) + 3. Persist to DB (transaction, only DB I/O) + 4. Publish domain events (after commit) Transaction Safety: - - Uses Unit of Work for atomic DB operations - - If any step fails, entire transaction is rolled back - - Events and cache invalidation happen only after successful commit + - QR image generation happens OUTSIDE the transaction + - On short code collision, transaction is retried with new image + - Events are published only after successful commit Rich Domain Model: Events are emitted by QRCode and User entities, not created manually in the handler. @@ -61,16 +106,12 @@ def __init__( qr_generator: IQRCodeGenerator, uow_factory: Callable[[], IUnitOfWork], event_bus: IEventBus, - config: Config, - redirect_url_builder: RedirectURLBuilder | None = None, + dynamic_resolver: DynamicQRCodeResolver, ): self._qr_generator = qr_generator self._create_uow = uow_factory self._event_bus = event_bus - self._config = config - self._redirect_url_builder = redirect_url_builder or RedirectURLBuilder( - config.app_url - ) + self._dynamic_resolver = dynamic_resolver async def handle( self, @@ -87,211 +128,39 @@ async def handle( Raises: ValueError: If URL or format is invalid + RuntimeError: If unique short code cannot be generated """ start_time = time.time() - pending_events = [] try: # ============================================ - # PHASE 1: Validation and Preparation (outside transaction) + # PHASE 1: Validation and Preparation (no I/O) # ============================================ + content = self._prepare_content(command) - # For WiFi and Payment QR codes, the data is not a URL but a special format - # WIFI:T:WPA;S:ssid;P:password;; - # CARD PAYMENT\nNumber: 1234... - is_non_url_type = command.qr_type in _NON_DYNAMIC_QR_TYPES - - if is_non_url_type: - # Skip URL validation for non-URL types - use raw string - qr_url = command.url - url = None # No URL object for non-URL types - else: - # Validate URL value object - url = URL(command.url) - qr_url = url - - qr_format = QRFormat(command.format) - - # Dynamic QR is only supported for URL-type content. - # Non-URL types (WiFi, vCard, text, payment) encode data directly - # into the QR image and cannot redirect through a short URL. - is_dynamic = command.is_dynamic and not is_non_url_type - - if command.is_dynamic and is_non_url_type: - logger.warning( - f"Dynamic QR requested for non-URL type '{command.qr_type}', " - f"falling back to static QR for user {command.user_id}" - ) - - # Generate short code for dynamic QR (will verify uniqueness in transaction) - short_code = None - if is_dynamic: - short_code = str(ShortCode.generate()) - - # For dynamic QR, encode redirect URL instead of target URL - qr_encode_url = qr_url - if is_dynamic and short_code: - # Dynamic QR encodes redirect URL (APP_URL/{short_code}) - # instead of target URL, allowing the target to be changed later - redirect_url = self._redirect_url_builder.build(short_code) - qr_encode_url = URL(redirect_url) - logger.info( - f"Dynamic QR will encode redirect URL: {qr_encode_url.value}" - ) - - # Generate QR code binary data (stateless, outside transaction) - qr_data = await self._qr_generator.generate( - content=str(qr_encode_url), - format=qr_format, - size=command.size, - error_correction=command.error_correction, - design=command.design_config, - ) - - # Calculate generation time + # ============================================ + # PHASE 2: Generate QR image (I/O, outside transaction) + # ============================================ + qr_data = await self._generate_image(content, command) generation_time_ms = int((time.time() - start_time) * 1000) - # Create URL hash for deduplication - if is_non_url_type: - url_hash = hashlib.sha256(command.url.encode()).hexdigest() - # For non-URL types, create a placeholder URL for storage - # Use data: scheme which is allowed and makes sense for payload data - storage_url = URL(f"data:text/plain;base64,{url_hash[:8]}") - else: - url_hash = hashlib.sha256(url.value.encode()).hexdigest() - storage_url = url - - # Create QRCode entity (emits QRCodeGenerated event via factory) - qr_code = QRCode.create( - user_id=command.user_id, - url=storage_url, - url_hash=url_hash, - format=qr_format, - title=command.title, - is_dynamic=is_dynamic, - short_code=short_code, - file_size_bytes=len(qr_data), - generation_time_ms=generation_time_ms, - ) - # ============================================ - # PHASE 2: All DB operations (inside transaction) + # PHASE 3: Persist (transaction) + handle collisions # ============================================ - - async with self._create_uow() as uow: - # Check short code uniqueness (inside transaction for consistency) - if is_dynamic and short_code: - retries = 0 - while await uow.qr_redirects.short_code_exists( - ShortCode(short_code) - ): - retries += 1 - if retries > _MAX_SHORT_CODE_RETRIES: - raise RuntimeError( - f"Failed to generate unique short code " - f"after {_MAX_SHORT_CODE_RETRIES} attempts" - ) - - short_code = str(ShortCode.generate()) - logger.info( - f"Short code collision (attempt {retries}), " - f"regenerated: {short_code}" - ) - - # CRITICAL: Regenerate QR image with new redirect URL. - # The QR image must encode the correct short code, - # otherwise the QR code will point to the wrong URL. - redirect_url = self._redirect_url_builder.build(short_code) - qr_encode_url = URL(redirect_url) - qr_data = await self._qr_generator.generate( - content=str(qr_encode_url), - format=qr_format, - size=command.size, - error_correction=command.error_correction, - design=command.design_config, - ) - - # Recreate entity with new short code and updated file size - qr_code = QRCode.create( - user_id=command.user_id, - url=storage_url, - url_hash=url_hash, - format=qr_format, - title=command.title, - is_dynamic=is_dynamic, - short_code=short_code, - file_size_bytes=len(qr_data), - generation_time_ms=generation_time_ms, - ) - - # Save QR code to history (returns entity with ID) - saved_qr = await uow.qr_codes.save(qr_code) - qr_code_id = saved_qr.id - - # Create redirect for dynamic QR - if is_dynamic and short_code: - redirect = QRRedirect( - id=None, - qr_code_id=qr_code_id, - short_code=ShortCode(short_code), - original_url=storage_url, - target_url=url, # Use the actual URL object for redirects - user_id=command.user_id, - clicks_count=0, - unique_clicks_count=0, - is_active=True, - ) - - await uow.qr_redirects.create(redirect) - - logger.info( - f"✅ Created dynamic QR redirect: " - f"qr_id={qr_code_id}, short_code={short_code}, " - f"target={url.value}" - ) - - # Update the event's qr_code_id since we now have it - # Collect events from QRCode entity - qr_events = qr_code.collect_events() - for event in qr_events: - # Update qr_code_id in the event (it was None before save) - if hasattr(event, "qr_code_id") and event.qr_code_id is None: - # Create new event with correct qr_code_id - from bot.domain.events import QRCodeGenerated - - if isinstance(event, QRCodeGenerated): - event = QRCodeGenerated( - occurred_at=event.occurred_at, - event_id=event.event_id, - user_id=event.user_id, - qr_code_id=qr_code_id, - url=event.url, - format=event.format, - is_dynamic=event.is_dynamic, - file_size_bytes=event.file_size_bytes, - generation_time_ms=event.generation_time_ms, - ) - pending_events.append(event) - - # Get user and increment QR count (Rich Domain Model) - user = await uow.users.get_by_telegram_id(command.user_id) - if user: - user.increment_qr_count() - await uow.users.update(user) - # Collect events from User entity - pending_events.extend(user.collect_events()) - - # Transaction auto-commits on successful exit + qr_data, qr_code_id, pending_events = await self._persist_with_retry( + command, content, qr_data, generation_time_ms + ) # ============================================ - # PHASE 3: Side effects (AFTER successful commit) + # PHASE 4: Side effects (AFTER successful commit) # ============================================ # Cache invalidation is handled by CacheEventHandlers.on_qr_generated() # triggered by the QRCodeGenerated event published below. logger.info( - f"✅ Generated {qr_format.value.upper()} QR code for user {command.user_id} " + f"✅ Generated {content.qr_format.value.upper()} QR code " + f"for user {command.user_id} " f"(id={qr_code_id}, {len(qr_data)} bytes, {generation_time_ms}ms)" ) @@ -302,9 +171,253 @@ async def handle( return qr_data, qr_code_id - except ValueError as e: - logger.error(f"❌ Validation error in GenerateQRCommand: {e}") + except ValueError: + logger.error( + f"❌ Validation error in GenerateQRCommand for user {command.user_id}" + ) raise - except Exception as e: - logger.error(f"❌ Error handling GenerateQRCommand: {e}") + except Exception: + logger.error( + f"❌ Error handling GenerateQRCommand for user {command.user_id}" + ) raise + + # ------------------------------------------------------------------ + # Private: Phase 1 — Preparation (pure logic, no I/O) + # ------------------------------------------------------------------ + + def _prepare_content(self, command: GenerateQRCommand) -> _PreparedContent: + """Validate command and prepare all data needed for generation. + + Pure function — no I/O, no side effects. + """ + is_non_url_type = command.qr_type in _NON_DYNAMIC_QR_TYPES + + # For WiFi and Payment QR codes, the data is not a URL but a special + # format (WIFI:T:WPA;S:ssid;P:password;; or CARD PAYMENT\n...) + if is_non_url_type: + qr_url = command.url + url = None + else: + url = URL(command.url) + qr_url = url + + qr_format = QRFormat(command.format) + + # Dynamic QR is only supported for URL-type content. + # Non-URL types (WiFi, vCard, text, payment) encode data directly + # into the QR image and cannot redirect through a short URL. + is_dynamic = command.is_dynamic and not is_non_url_type + if command.is_dynamic and is_non_url_type: + logger.warning( + f"Dynamic QR requested for non-URL type '{command.qr_type}', " + f"falling back to static QR for user {command.user_id}" + ) + + # Prepare dynamic setup (short code + redirect URL) if needed + dynamic_setup: DynamicQRSetup | None = None + encode_url = qr_url + if is_dynamic: + dynamic_setup = self._dynamic_resolver.prepare(url) + encode_url = dynamic_setup.redirect_url + logger.info(f"Dynamic QR will encode redirect URL: {encode_url.value}") + + # Compute storage URL and hash for deduplication + if is_non_url_type: + url_hash = hashlib.sha256(command.url.encode()).hexdigest() + storage_url = URL(f"data:text/plain;base64,{url_hash[:8]}") + else: + url_hash = hashlib.sha256(url.value.encode()).hexdigest() + storage_url = url + + return _PreparedContent( + encode_url=encode_url, + storage_url=storage_url, + target_url=url, + url_hash=url_hash, + qr_format=qr_format, + is_dynamic=is_dynamic, + is_non_url_type=is_non_url_type, + dynamic_setup=dynamic_setup, + ) + + # ------------------------------------------------------------------ + # Private: Phase 2 — Image generation (I/O, outside transaction) + # ------------------------------------------------------------------ + + async def _generate_image( + self, + content: _PreparedContent, + command: GenerateQRCommand, + ) -> bytes: + """Generate QR code binary image. I/O operation — must be outside tx.""" + return await self._qr_generator.generate( + content=str(content.encode_url), + format=content.qr_format, + size=command.size, + error_correction=command.error_correction, + design=command.design_config, + ) + + # ------------------------------------------------------------------ + # Private: Phase 3 — Persistence (transaction + collision handling) + # ------------------------------------------------------------------ + + async def _persist_with_retry( + self, + command: GenerateQRCommand, + content: _PreparedContent, + qr_data: bytes, + generation_time_ms: int, + ) -> tuple[bytes, int | None, list]: + """Persist QR code inside a transaction. + + If a short code collision is detected, we: + 1. Let the current transaction complete (empty — nothing was written) + 2. Regenerate short code + QR image OUTSIDE the transaction + 3. Retry the transaction with the new data + + This ensures QR image generation (I/O) never blocks a DB connection. + + Returns: + Tuple of (final_qr_data, qr_code_id, pending_events) + """ + current_setup = content.dynamic_setup + current_qr_data = qr_data + + for attempt in range(_MAX_TRANSACTION_RETRIES): + result = await self._try_persist( + command, content, current_setup, current_qr_data, generation_time_ms + ) + if result is not None: + # Success — no collision + return current_qr_data, result[0], result[1] + + # Collision detected — regenerate OUTSIDE transaction + current_setup = self._dynamic_resolver.regenerate(current_setup.target_url) + current_qr_data = await self._qr_generator.generate( + content=str(current_setup.redirect_url), + format=content.qr_format, + size=command.size, + error_correction=command.error_correction, + design=command.design_config, + ) + logger.info( + f"Short code collision on attempt {attempt + 1}, " + f"regenerated image with new short code: " + f"{current_setup.short_code}" + ) + + # Exhausted all transaction retries + raise RuntimeError( + f"Failed to persist QR code after {_MAX_TRANSACTION_RETRIES} " + f"transaction retries due to short code collisions" + ) + + async def _try_persist( + self, + command: GenerateQRCommand, + content: _PreparedContent, + dynamic_setup: DynamicQRSetup | None, + qr_data: bytes, + generation_time_ms: int, + ) -> tuple[int | None, list] | None: + """Attempt to persist QR code in a single transaction. + + Returns: + (qr_code_id, pending_events) on success, None on short code collision. + On collision, the transaction commits empty (nothing was written). + """ + short_code = dynamic_setup.short_code if dynamic_setup else None + + # Create entity (emits QRCodeGenerated event via factory) + qr_code = QRCode.create( + user_id=command.user_id, + url=content.storage_url, + url_hash=content.url_hash, + format=content.qr_format, + title=command.title, + is_dynamic=content.is_dynamic, + short_code=short_code, + file_size_bytes=len(qr_data), + generation_time_ms=generation_time_ms, + ) + + async with self._create_uow() as uow: + # For dynamic QR: verify short code uniqueness inside transaction + if ( + content.is_dynamic + and short_code + and await uow.qr_redirects.short_code_exists(ShortCode(short_code)) + ): + # Collision! Transaction commits empty (nothing written). + # Caller will regenerate and retry. + return None + + return await self._do_persist( + uow, + command, + content, + qr_code, + qr_data, + short_code, + dynamic_setup, + ) + + async def _do_persist( + self, + uow: IUnitOfWork, + command: GenerateQRCommand, + content: _PreparedContent, + qr_code: QRCode, + qr_data: bytes, + short_code: str | None, + dynamic_setup: DynamicQRSetup | None, + ) -> tuple[int | None, list]: + """Execute all DB writes inside the active UoW transaction. + + Returns: + Tuple of (qr_code_id, pending_events) + """ + pending_events = [] + + # Save QR code to history (returns entity with ID) + saved_qr = await uow.qr_codes.save(qr_code) + qr_code_id = saved_qr.id + + # Create redirect for dynamic QR + if content.is_dynamic and short_code and dynamic_setup: + redirect = QRRedirect( + id=None, + qr_code_id=qr_code_id, + short_code=ShortCode(short_code), + original_url=content.storage_url, + target_url=content.target_url, + user_id=command.user_id, + clicks_count=0, + unique_clicks_count=0, + is_active=True, + ) + await uow.qr_redirects.create(redirect) + + logger.info( + f"✅ Created dynamic QR redirect: " + f"qr_id={qr_code_id}, short_code={short_code}, " + f"target={content.target_url.value}" + ) + + # Collect entity events and patch qr_code_id using with_updated() + for event in qr_code.collect_events(): + if hasattr(event, "qr_code_id") and event.qr_code_id is None: + event = event.with_updated(qr_code_id=qr_code_id) + pending_events.append(event) + + # Increment user's QR count (Rich Domain Model) + user = await uow.users.get_by_telegram_id(command.user_id) + if user: + user.increment_qr_count() + await uow.users.update(user) + pending_events.extend(user.collect_events()) + + # Transaction auto-commits on successful __aexit__ + return qr_code_id, pending_events diff --git a/bot/application/services/__init__.py b/bot/application/services/__init__.py index 46558d4..317b2a3 100644 --- a/bot/application/services/__init__.py +++ b/bot/application/services/__init__.py @@ -12,6 +12,7 @@ - Чёткое разделение ответственности """ +from .dynamic_qr_resolver import DynamicQRCodeResolver, DynamicQRSetup from .qr_generation_service import ( GenerationResult, GenerationStatus, @@ -22,6 +23,8 @@ from .user_stats_service import UserStatsService __all__ = [ + "DynamicQRCodeResolver", + "DynamicQRSetup", "QRGenerationService", "GenerationResult", "GenerationStatus", diff --git a/bot/application/services/dynamic_qr_resolver.py b/bot/application/services/dynamic_qr_resolver.py new file mode 100644 index 0000000..b357fcc --- /dev/null +++ b/bot/application/services/dynamic_qr_resolver.py @@ -0,0 +1,133 @@ +""" +Dynamic QR Code Resolver service. + +Encapsulates the logic for generating unique short codes +and building redirect URLs for dynamic QR codes. + +Extracted from GenerateQRHandler to follow SRP: +- Short code generation and collision resolution +- Redirect URL construction +- No I/O — purely stateless preparation logic + +Clean Architecture: This service lives in the application layer +because it depends on RedirectURLBuilder (configuration-aware). +""" + +from dataclasses import dataclass + +from bot.application.services.redirect_url_builder import RedirectURLBuilder +from bot.domain.value_objects import URL, ShortCode + +# Maximum number of short code generation attempts before giving up. +# After this many collisions, we assume something is fundamentally wrong. +MAX_SHORT_CODE_RETRIES = 10 + + +@dataclass(frozen=True) +class DynamicQRSetup: + """Immutable result of dynamic QR preparation. + + Contains all data needed to generate a dynamic QR code image + and persist the redirect mapping. + + Attributes: + short_code: Generated short code string (e.g., "xK9mP2Lq") + redirect_url: URL to encode in the QR image (APP_URL/{short_code}) + target_url: Original URL the user wants the QR to point to + """ + + short_code: str + redirect_url: URL + target_url: URL + + +class DynamicQRCodeResolver: + """Resolves unique short codes for dynamic QR codes. + + Responsibilities: + - Generate initial short code + redirect URL pair + - Regenerate on collision (new random short code) + - Verify uniqueness against repository (inside transaction) + + Usage: + resolver = DynamicQRCodeResolver(redirect_url_builder) + + # Phase 1: Prepare (outside transaction) + setup = resolver.prepare(target_url) + + # Phase 2: Ensure uniqueness (inside transaction) + final_setup = await resolver.ensure_unique(uow, setup) + """ + + def __init__(self, redirect_url_builder: RedirectURLBuilder) -> None: + self._url_builder = redirect_url_builder + + def prepare(self, target_url: URL) -> DynamicQRSetup: + """Generate initial short code and redirect URL. + + This is a pure function (no I/O) — safe to call outside transaction. + + Args: + target_url: The URL the user wants the QR to redirect to + + Returns: + DynamicQRSetup with generated short code and redirect URL + """ + short_code = str(ShortCode.generate()) + redirect_url = URL(self._url_builder.build(short_code)) + return DynamicQRSetup( + short_code=short_code, + redirect_url=redirect_url, + target_url=target_url, + ) + + def regenerate(self, target_url: URL) -> DynamicQRSetup: + """Generate a new short code after collision. + + Same as prepare() — generates a fresh random short code. + + Args: + target_url: The URL the user wants the QR to redirect to + + Returns: + DynamicQRSetup with a new random short code + """ + return self.prepare(target_url) + + async def ensure_unique( + self, + uow, + setup: DynamicQRSetup, + ) -> DynamicQRSetup: + """Check short code uniqueness, regenerate on collision. + + This method MUST be called inside an active UoW transaction + to guarantee consistency (short_code_exists is a DB query). + + The method only regenerates the short code string — it does NOT + regenerate the QR image. The caller is responsible for detecting + that the short code changed and regenerating the image accordingly. + + Args: + uow: Active Unit of Work with qr_redirects repository + setup: Initial DynamicQRSetup to verify + + Returns: + Original setup if unique, or new setup with different short code + + Raises: + RuntimeError: If unique code cannot be found after max retries + """ + retries = 0 + current = setup + + while await uow.qr_redirects.short_code_exists(ShortCode(current.short_code)): + retries += 1 + if retries > MAX_SHORT_CODE_RETRIES: + raise RuntimeError( + f"Failed to generate unique short code " + f"after {MAX_SHORT_CODE_RETRIES} attempts" + ) + current = self.regenerate(setup.target_url) + + return current diff --git a/bot/di/container.py b/bot/di/container.py index 23e007c..735c051 100644 --- a/bot/di/container.py +++ b/bot/di/container.py @@ -49,8 +49,10 @@ GetUserStatsHandler, ) from bot.application.services import ( + DynamicQRCodeResolver, QRGenerationService, QRPreviewService, + RedirectURLBuilder, UserStatsService, ) from bot.config import Config @@ -309,6 +311,18 @@ def qr_preview_service(self) -> QRPreviewService: """Get QR preview application service (lazy).""" return self._lazy_deps["qr_preview_service"].get_sync() + # Shared Application Services (used by command handlers) + + @property + def redirect_url_builder(self) -> RedirectURLBuilder: + """Get redirect URL builder (lazy).""" + return self._lazy_deps["redirect_url_builder"].get_sync() + + @property + def dynamic_qr_resolver(self) -> DynamicQRCodeResolver: + """Get dynamic QR code resolver (lazy).""" + return self._lazy_deps["dynamic_qr_resolver"].get_sync() + # CQRS Command Handlers @property diff --git a/bot/di/modules/command_handlers.py b/bot/di/modules/command_handlers.py index 28c6cdb..89ce7e3 100644 --- a/bot/di/modules/command_handlers.py +++ b/bot/di/modules/command_handlers.py @@ -9,6 +9,7 @@ UpdateUserLanguageHandler, UpgradeUserTariffHandler, ) +from bot.application.services.dynamic_qr_resolver import DynamicQRCodeResolver from bot.application.services.redirect_url_builder import RedirectURLBuilder from .base import DIModule @@ -29,6 +30,21 @@ class CommandHandlersModule(DIModule): """ def register(self, container) -> None: + # Shared services for QR handlers + container.register_lazy( + "redirect_url_builder", + lambda: RedirectURLBuilder(container.config.app_url), + singleton=True, + ) + + container.register_lazy( + "dynamic_qr_resolver", + lambda: DynamicQRCodeResolver( + redirect_url_builder=container.redirect_url_builder, + ), + singleton=True, + ) + # QR Handlers container.register_lazy( "generate_qr_handler", @@ -36,8 +52,7 @@ def register(self, container) -> None: qr_generator=container.qr_generator, uow_factory=container.create_unit_of_work, event_bus=container.event_bus, - config=container.config, - redirect_url_builder=RedirectURLBuilder(container.config.app_url), + dynamic_resolver=container.dynamic_qr_resolver, ), singleton=True, ) diff --git a/bot/domain/events/base.py b/bot/domain/events/base.py index e0154e4..6232358 100644 --- a/bot/domain/events/base.py +++ b/bot/domain/events/base.py @@ -185,6 +185,26 @@ def _get_payload(self) -> dict[str, Any]: if k not in base_fields } + def with_updated(self, **kwargs: Any) -> BaseEvent: + """Create a copy of this event with updated fields. + + Uses dataclasses.replace() under the hood to produce a new + frozen instance. This is safe for frozen dataclasses and avoids + fragile manual reconstruction when only one or two fields change. + + Usage: + event = event.with_updated(qr_code_id=saved_id) + + Args: + **kwargs: Fields to update in the new instance + + Returns: + New event instance with updated fields + """ + import dataclasses + + return dataclasses.replace(self, **kwargs) + @staticmethod def _serialize_value(value: Any) -> Any: """Serialize a single value for JSON compatibility.""" diff --git a/tests/infrastructure/test_unit_of_work.py b/tests/infrastructure/test_unit_of_work.py index f7d322b..5c0ed53 100644 --- a/tests/infrastructure/test_unit_of_work.py +++ b/tests/infrastructure/test_unit_of_work.py @@ -100,11 +100,15 @@ async def test_handler_uses_uow_for_all_db_ops(self): """Test that all DB operations happen inside UoW context.""" from bot.application.command_handlers import GenerateQRHandler from bot.application.commands import GenerateQRCommand + from bot.application.services.dynamic_qr_resolver import DynamicQRCodeResolver + from bot.application.services.redirect_url_builder import RedirectURLBuilder # Mock dependencies event_bus = AsyncMock() - config = MagicMock() - config.app_url = "https://example.com" + redirect_url_builder = RedirectURLBuilder("https://example.com") + dynamic_resolver = DynamicQRCodeResolver( + redirect_url_builder=redirect_url_builder + ) # Track UoW usage uow_entered = False @@ -147,7 +151,7 @@ def uow_factory(): qr_generator=qr_generator, uow_factory=uow_factory, event_bus=event_bus, - config=config, + dynamic_resolver=dynamic_resolver, ) command = GenerateQRCommand( @@ -173,13 +177,17 @@ async def test_events_published_after_commit(self): """Test that events are published only after UoW commits.""" from bot.application.command_handlers import GenerateQRHandler from bot.application.commands import GenerateQRCommand + from bot.application.services.dynamic_qr_resolver import DynamicQRCodeResolver + from bot.application.services.redirect_url_builder import RedirectURLBuilder qr_generator = AsyncMock() qr_generator.generate = AsyncMock(return_value=b"qr_data") event_bus = AsyncMock() - config = MagicMock() - config.app_url = "https://example.com" + redirect_url_builder = RedirectURLBuilder("https://example.com") + dynamic_resolver = DynamicQRCodeResolver( + redirect_url_builder=redirect_url_builder + ) events_published = [] commit_called = False @@ -216,7 +224,7 @@ async def on_exit(self_mock, exc_type, exc_val, exc_tb): qr_generator=qr_generator, uow_factory=lambda: mock_uow, event_bus=event_bus, - config=config, + dynamic_resolver=dynamic_resolver, ) command = GenerateQRCommand( diff --git a/tests/unit/application/command_handlers/qr/test_generate_qr_handler.py b/tests/unit/application/command_handlers/qr/test_generate_qr_handler.py index ebe5649..752c281 100644 --- a/tests/unit/application/command_handlers/qr/test_generate_qr_handler.py +++ b/tests/unit/application/command_handlers/qr/test_generate_qr_handler.py @@ -18,6 +18,7 @@ from bot.application.command_handlers.qr.generate_qr_handler import GenerateQRHandler from bot.application.commands import GenerateQRCommand +from bot.application.services.dynamic_qr_resolver import DynamicQRCodeResolver from bot.application.services.redirect_url_builder import RedirectURLBuilder from bot.domain.entities import QRCode from bot.domain.value_objects import URL, QRFormat @@ -29,21 +30,25 @@ def redirect_url_builder(mock_config): return RedirectURLBuilder(mock_config.app_url) +@pytest.fixture +def dynamic_resolver(redirect_url_builder): + """Create DynamicQRCodeResolver with redirect_url_builder.""" + return DynamicQRCodeResolver(redirect_url_builder=redirect_url_builder) + + @pytest.fixture def handler( mock_qr_generator, mock_unit_of_work, mock_event_bus, - mock_config, - redirect_url_builder, + dynamic_resolver, ): """Create handler with mocked dependencies.""" return GenerateQRHandler( qr_generator=mock_qr_generator, uow_factory=mock_unit_of_work, event_bus=mock_event_bus, - config=mock_config, - redirect_url_builder=redirect_url_builder, + dynamic_resolver=dynamic_resolver, ) @@ -474,7 +479,7 @@ async def test_max_collision_retries_raises_error( ) # Act & Assert - with pytest.raises(RuntimeError, match="Failed to generate unique short code"): + with pytest.raises(RuntimeError, match="Failed to persist QR code"): await handler.handle(command) async def test_dynamic_wifi_falls_back_to_static( diff --git a/tests/unit/application/services/test_dynamic_qr_resolver.py b/tests/unit/application/services/test_dynamic_qr_resolver.py new file mode 100644 index 0000000..2e0b2db --- /dev/null +++ b/tests/unit/application/services/test_dynamic_qr_resolver.py @@ -0,0 +1,269 @@ +"""Tests for DynamicQRCodeResolver service.""" + +from unittest.mock import AsyncMock + +import pytest + +from bot.application.services.dynamic_qr_resolver import ( + MAX_SHORT_CODE_RETRIES, + DynamicQRCodeResolver, + DynamicQRSetup, +) +from bot.application.services.redirect_url_builder import RedirectURLBuilder +from bot.domain.value_objects import URL + +# ============================================================================= +# Fixtures +# ============================================================================= + +APP_URL = "https://go.make-qr.de" + + +@pytest.fixture() +def redirect_url_builder() -> RedirectURLBuilder: + """Create a real RedirectURLBuilder.""" + return RedirectURLBuilder(APP_URL) + + +@pytest.fixture() +def resolver(redirect_url_builder: RedirectURLBuilder) -> DynamicQRCodeResolver: + """Create DynamicQRCodeResolver with real builder.""" + return DynamicQRCodeResolver(redirect_url_builder) + + +@pytest.fixture() +def mock_uow() -> AsyncMock: + """Create a mock Unit of Work with qr_redirects repository.""" + uow = AsyncMock() + uow.qr_redirects = AsyncMock() + uow.qr_redirects.short_code_exists = AsyncMock(return_value=False) + return uow + + +# ============================================================================= +# Test DynamicQRSetup +# ============================================================================= + + +class TestDynamicQRSetup: + """Tests for DynamicQRSetup frozen dataclass.""" + + def test_create_setup(self) -> None: + """Should create a DynamicQRSetup with all fields.""" + target = URL("https://example.com") + redirect = URL("https://go.make-qr.de/abc123") + setup = DynamicQRSetup( + short_code="abc123", + redirect_url=redirect, + target_url=target, + ) + assert setup.short_code == "abc123" + assert setup.redirect_url == redirect + assert setup.target_url == target + + def test_setup_is_immutable(self) -> None: + """DynamicQRSetup is frozen — cannot be modified.""" + setup = DynamicQRSetup( + short_code="abc123", + redirect_url=URL("https://go.make-qr.de/abc123"), + target_url=URL("https://example.com"), + ) + with pytest.raises(AttributeError): + setup.short_code = "changed" # type: ignore + + +# ============================================================================= +# Test DynamicQRCodeResolver.prepare() +# ============================================================================= + + +class TestPrepare: + """Tests for DynamicQRCodeResolver.prepare().""" + + def test_returns_dynamic_qr_setup(self, resolver: DynamicQRCodeResolver) -> None: + """Should return a DynamicQRSetup instance.""" + target_url = URL("https://example.com") + result = resolver.prepare(target_url) + assert isinstance(result, DynamicQRSetup) + + def test_target_url_preserved(self, resolver: DynamicQRCodeResolver) -> None: + """Should preserve the target URL in the result.""" + target_url = URL("https://example.com/page") + result = resolver.prepare(target_url) + assert result.target_url == target_url + + def test_redirect_url_contains_short_code( + self, resolver: DynamicQRCodeResolver + ) -> None: + """Redirect URL should contain the generated short code.""" + result = resolver.prepare(URL("https://example.com")) + assert result.short_code in result.redirect_url.value + + def test_redirect_url_starts_with_app_url( + self, resolver: DynamicQRCodeResolver + ) -> None: + """Redirect URL should start with the configured APP_URL.""" + result = resolver.prepare(URL("https://example.com")) + assert result.redirect_url.value.startswith(APP_URL) + + def test_short_code_is_non_empty_string( + self, resolver: DynamicQRCodeResolver + ) -> None: + """Generated short code should be a non-empty string.""" + result = resolver.prepare(URL("https://example.com")) + assert isinstance(result.short_code, str) + assert len(result.short_code) > 0 + + def test_redirect_url_format(self, resolver: DynamicQRCodeResolver) -> None: + """Redirect URL should be {APP_URL}/{short_code}.""" + result = resolver.prepare(URL("https://example.com")) + expected = f"{APP_URL}/{result.short_code}" + assert result.redirect_url.value == expected + + def test_each_call_generates_unique_code( + self, resolver: DynamicQRCodeResolver + ) -> None: + """Multiple calls should generate different short codes.""" + target = URL("https://example.com") + codes = {resolver.prepare(target).short_code for _ in range(10)} + # With random generation, 10 codes should all be unique + assert len(codes) == 10 + + +# ============================================================================= +# Test DynamicQRCodeResolver.regenerate() +# ============================================================================= + + +class TestRegenerate: + """Tests for DynamicQRCodeResolver.regenerate().""" + + def test_returns_new_setup(self, resolver: DynamicQRCodeResolver) -> None: + """Should return a new DynamicQRSetup.""" + target = URL("https://example.com") + result = resolver.regenerate(target) + assert isinstance(result, DynamicQRSetup) + + def test_different_from_prepare(self, resolver: DynamicQRCodeResolver) -> None: + """Regenerated setup should have different short code than prepared one.""" + target = URL("https://example.com") + first = resolver.prepare(target) + second = resolver.regenerate(target) + # Extremely unlikely to collide with random codes + assert first.short_code != second.short_code + + def test_preserves_target_url(self, resolver: DynamicQRCodeResolver) -> None: + """Should preserve the target URL.""" + target = URL("https://example.com/path") + result = resolver.regenerate(target) + assert result.target_url == target + + +# ============================================================================= +# Test DynamicQRCodeResolver.ensure_unique() +# ============================================================================= + + +class TestEnsureUnique: + """Tests for DynamicQRCodeResolver.ensure_unique().""" + + @pytest.mark.asyncio() + async def test_returns_same_setup_when_unique( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """Should return the original setup when short code is unique.""" + mock_uow.qr_redirects.short_code_exists.return_value = False + setup = resolver.prepare(URL("https://example.com")) + + result = await resolver.ensure_unique(mock_uow, setup) + + assert result.short_code == setup.short_code + assert result.redirect_url == setup.redirect_url + mock_uow.qr_redirects.short_code_exists.assert_awaited_once() + + @pytest.mark.asyncio() + async def test_regenerates_on_collision( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """Should regenerate short code when collision is detected.""" + # First call: collision, second call: unique + mock_uow.qr_redirects.short_code_exists.side_effect = [True, False] + setup = resolver.prepare(URL("https://example.com")) + + result = await resolver.ensure_unique(mock_uow, setup) + + assert result.short_code != setup.short_code + assert mock_uow.qr_redirects.short_code_exists.await_count == 2 + + @pytest.mark.asyncio() + async def test_regenerates_multiple_times( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """Should handle multiple consecutive collisions.""" + # 3 collisions, then unique + mock_uow.qr_redirects.short_code_exists.side_effect = [ + True, + True, + True, + False, + ] + setup = resolver.prepare(URL("https://example.com")) + + result = await resolver.ensure_unique(mock_uow, setup) + + assert result.short_code != setup.short_code + assert mock_uow.qr_redirects.short_code_exists.await_count == 4 + + @pytest.mark.asyncio() + async def test_raises_after_max_retries( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """Should raise RuntimeError after exceeding max retries.""" + # All attempts collide + mock_uow.qr_redirects.short_code_exists.return_value = True + setup = resolver.prepare(URL("https://example.com")) + + with pytest.raises(RuntimeError, match="Failed to generate unique short code"): + await resolver.ensure_unique(mock_uow, setup) + + # 1 initial check + MAX_SHORT_CODE_RETRIES retries + expected_checks = MAX_SHORT_CODE_RETRIES + 1 + assert mock_uow.qr_redirects.short_code_exists.await_count == expected_checks + + @pytest.mark.asyncio() + async def test_preserves_target_url_after_collision( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """Target URL should remain the same after collision resolution.""" + mock_uow.qr_redirects.short_code_exists.side_effect = [True, False] + target = URL("https://example.com/important-page") + setup = resolver.prepare(target) + + result = await resolver.ensure_unique(mock_uow, setup) + + assert result.target_url == target + + @pytest.mark.asyncio() + async def test_redirect_url_matches_new_short_code( + self, + resolver: DynamicQRCodeResolver, + mock_uow: AsyncMock, + ) -> None: + """After collision, redirect URL should match the new short code.""" + mock_uow.qr_redirects.short_code_exists.side_effect = [True, False] + setup = resolver.prepare(URL("https://example.com")) + + result = await resolver.ensure_unique(mock_uow, setup) + + expected_redirect = f"{APP_URL}/{result.short_code}" + assert result.redirect_url.value == expected_redirect diff --git a/tests/unit/domain/events/test_base.py b/tests/unit/domain/events/test_base.py index 9114e26..a9fde5e 100644 --- a/tests/unit/domain/events/test_base.py +++ b/tests/unit/domain/events/test_base.py @@ -286,3 +286,68 @@ def test_event_hashable(self): event_dict = {event: "value"} assert event_dict[event] == "value" + + +# ============================================================================= +# Test BaseEvent.with_updated() +# ============================================================================= + + +class TestWithUpdated: + """Tests for BaseEvent.with_updated() copy-with-changes method.""" + + def test_returns_new_instance(self): + """with_updated() should return a different object.""" + event = SampleEvent(user_id=1, message="original") + updated = event.with_updated(message="changed") + assert updated is not event + + def test_updates_specified_field(self): + """Should update the specified field.""" + event = SampleEvent(user_id=1, message="original") + updated = event.with_updated(message="changed") + assert updated.message == "changed" + + def test_preserves_unmodified_fields(self): + """Should keep fields that were not updated.""" + event = SampleEvent(user_id=42, message="original") + updated = event.with_updated(message="changed") + assert updated.user_id == 42 + + def test_preserves_event_id(self): + """Should keep the same event_id.""" + event = SampleEvent(user_id=1, message="test") + updated = event.with_updated(message="changed") + assert updated.event_id == event.event_id + + def test_preserves_occurred_at(self): + """Should keep the same occurred_at timestamp.""" + event = SampleEvent(user_id=1, message="test") + updated = event.with_updated(message="changed") + assert updated.occurred_at == event.occurred_at + + def test_update_multiple_fields(self): + """Should update multiple fields at once.""" + event = SampleEvent(user_id=1, message="original") + updated = event.with_updated(user_id=99, message="new") + assert updated.user_id == 99 + assert updated.message == "new" + + def test_no_changes_returns_copy(self): + """with_updated() with no kwargs returns an equal copy.""" + event = SampleEvent(user_id=1, message="test") + copy = event.with_updated() + assert copy == event + assert copy is not event + + def test_returns_correct_type(self): + """Returned object should be the same subclass.""" + event = SampleEvent(user_id=1, message="test") + updated = event.with_updated(message="changed") + assert isinstance(updated, SampleEvent) + + def test_original_unchanged(self): + """Original frozen event must remain unchanged.""" + event = SampleEvent(user_id=1, message="original") + event.with_updated(message="changed") + assert event.message == "original"