From 01ea6c2b8bb25ee222b1e778d94619b3dd7c4ac7 Mon Sep 17 00:00:00 2001 From: "James R. Barlow" Date: Mon, 15 Dec 2025 01:15:08 -0800 Subject: [PATCH] feat: add ValidationCoordinator for cross-cutting validation Co-authored-by: aider (openrouter/anthropic/claude-sonnet-4) --- src/ocrmypdf/_options.py | 36 +---- src/ocrmypdf/_validation.py | 16 +++ src/ocrmypdf/_validation_coordinator.py | 131 ++++++++++++++++++ src/ocrmypdf/builtin_plugins/optimize.py | 40 ++++-- src/ocrmypdf/builtin_plugins/tesseract_ocr.py | 66 +++++++-- 5 files changed, 232 insertions(+), 57 deletions(-) create mode 100644 src/ocrmypdf/_validation_coordinator.py diff --git a/src/ocrmypdf/_options.py b/src/ocrmypdf/_options.py index 38590090..499fea1e 100644 --- a/src/ocrmypdf/_options.py +++ b/src/ocrmypdf/_options.py @@ -293,40 +293,8 @@ class OCROptions(BaseModel): data['output_file'] = '/dev/null' # Placeholder return data - @model_validator(mode='after') - def validate_exclusive_ocr_options(self): - """Ensure only one of force_ocr, skip_text, redo_ocr is set.""" - exclusive_options = sum( - 1 for opt in [self.force_ocr, self.skip_text, self.redo_ocr] if opt - ) - if exclusive_options >= 2: - raise ValueError("Choose only one of --force-ocr, --skip-text, --redo-ocr.") - return self - - @model_validator(mode='after') - def validate_output_type_compatibility(self): - """Validate output type is compatible with output file.""" - if self.output_type == 'none' and str(self.output_file) not in ( - os.devnull, - '-', - ): - raise ValueError( - "Since you specified `--output-type none`, the output file " - f"{self.output_file} cannot be produced. Set the output file to " - f"`-` to suppress this message." - ) - return self - - @model_validator(mode='after') - def validate_redo_ocr_options(self): - """Validate options compatible with redo_ocr.""" - if self.redo_ocr: - if self.deskew or self.clean_final or self.remove_background: - raise ValueError( - "--redo-ocr is not currently compatible with --deskew, " - "--clean-final, and --remove-background" - ) - return self + # Note: Cross-cutting validation moved to ValidationCoordinator + # Basic field validation remains here, complex validation moved to coordinator @property def lossless_reconstruction(self): diff --git a/src/ocrmypdf/_validation.py b/src/ocrmypdf/_validation.py index 8269de98..6c7482af 100644 --- a/src/ocrmypdf/_validation.py +++ b/src/ocrmypdf/_validation.py @@ -121,12 +121,28 @@ def _check_plugin_invariant_options(options: OCROptions) -> None: def _check_plugin_options(options: OCROptions, plugin_manager: PluginManager) -> None: + # First, let plugins check their external dependencies plugin_manager.hook.check_options(options=options) + + # Then check OCR engine language support ocr_engine_languages = plugin_manager.hook.get_ocr_engine().languages(options) check_options_languages(options, ocr_engine_languages) + + # Finally, run comprehensive validation using the coordinator + from ocrmypdf._validation_coordinator import ValidationCoordinator + coordinator = ValidationCoordinator(plugin_manager) + coordinator.validate_all_options(options) def check_options(options: OCROptions, plugin_manager: PluginManager) -> None: + """Check options for validity and consistency. + + This function coordinates validation across the entire system: + 1. Core validation (platform, files, preprocessing) + 2. Plugin external dependency validation + 3. Plugin-specific validation (handled by plugin models) + 4. Cross-cutting validation (handled by validation coordinator) + """ _check_plugin_invariant_options(options) _check_plugin_options(options, plugin_manager) diff --git a/src/ocrmypdf/_validation_coordinator.py b/src/ocrmypdf/_validation_coordinator.py new file mode 100644 index 00000000..8ac640df --- /dev/null +++ b/src/ocrmypdf/_validation_coordinator.py @@ -0,0 +1,131 @@ +# SPDX-FileCopyrightText: 2024 James R. Barlow +# SPDX-License-Identifier: MPL-2.0 + +"""Validation coordinator for plugin options and cross-cutting concerns.""" + +from __future__ import annotations + +import logging +import os +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + import pluggy + from ocrmypdf._options import OCROptions + +log = logging.getLogger(__name__) + + +class ValidationCoordinator: + """Coordinates validation across plugin models and core options.""" + + def __init__(self, plugin_manager: pluggy.PluginManager): + self.plugin_manager = plugin_manager + self.registry = getattr(plugin_manager, '_option_registry', None) + + def validate_all_options(self, options: OCROptions) -> None: + """Run comprehensive validation on all options. + + This runs validation in the correct order: + 1. Plugin self-validation (already done by Pydantic) + 2. Plugin context validation (requires external context) + 3. Cross-cutting validation (between plugins and core) + + Args: + options: The options to validate + """ + # Step 1: Plugin context validation + self._validate_plugin_contexts(options) + + # Step 2: Cross-cutting validation + self._validate_cross_cutting_concerns(options) + + def _validate_plugin_contexts(self, options: OCROptions) -> None: + """Validate plugin options that require external context.""" + if not self.registry: + return + + registered_models = self.registry.get_registered_models() + + # Validate Tesseract options with language context + if 'tesseract' in registered_models: + from ocrmypdf.builtin_plugins.tesseract_ocr import TesseractOptions + + # Create TesseractOptions from legacy fields for validation + tesseract_data = { + 'config': options.tesseract_config, + 'pagesegmode': options.tesseract_pagesegmode, + 'oem': options.tesseract_oem, + 'thresholding': options.tesseract_thresholding, + 'timeout': options.tesseract_timeout, + 'non_ocr_timeout': options.tesseract_non_ocr_timeout or 180.0, + 'downsample_large_images': options.tesseract_downsample_large_images, + 'downsample_above': options.tesseract_downsample_above, + 'user_words': options.user_words, + 'user_patterns': options.user_patterns, + } + # Remove None values + tesseract_data = {k: v for k, v in tesseract_data.items() if v is not None} + + tesseract_options = TesseractOptions(**tesseract_data) + tesseract_options.validate_with_context(options.languages) + + # Validate Optimize options with external program context + if 'optimize' in registered_models: + from ocrmypdf.builtin_plugins.optimize import OptimizeOptions + from ocrmypdf._exec import jbig2enc, pngquant + + optimize_data = { + 'level': options.optimize, + 'jpeg_quality': options.jpeg_quality or 0, + 'png_quality': options.png_quality or 0, + 'jbig2_lossy': options.jbig2_lossy or False, + 'jbig2_page_group_size': options.jbig2_page_group_size or 0, + 'jbig2_threshold': options.jbig2_threshold, + } + + optimize_options = OptimizeOptions(**optimize_data) + external_programs = { + 'pngquant': pngquant.available(), + 'jbig2enc': jbig2enc.available(), + } + optimize_options.validate_with_context(external_programs) + + def _validate_cross_cutting_concerns(self, options: OCROptions) -> None: + """Validate cross-cutting concerns that span multiple plugins.""" + # Validate mutually exclusive OCR options + exclusive_options = sum( + 1 for opt in [options.force_ocr, options.skip_text, options.redo_ocr] if opt + ) + if exclusive_options >= 2: + from ocrmypdf.exceptions import BadArgsError + raise BadArgsError("Choose only one of --force-ocr, --skip-text, --redo-ocr.") + + # Validate redo_ocr compatibility + if options.redo_ocr: + if options.deskew or options.clean_final or options.remove_background: + from ocrmypdf.exceptions import BadArgsError + raise BadArgsError( + "--redo-ocr is not currently compatible with --deskew, " + "--clean-final, and --remove-background" + ) + + # Validate output type compatibility + if options.output_type == 'none' and str(options.output_file) not in ( + os.devnull, '-' + ): + from ocrmypdf.exceptions import BadArgsError + raise BadArgsError( + "Since you specified `--output-type none`, the output file " + f"{options.output_file} cannot be produced. Set the output file to " + "`-` to suppress this message." + ) + + # Validate PDF/A image compression compatibility + if (options.pdfa_image_compression and + options.pdfa_image_compression != 'auto' and + not options.output_type.startswith('pdfa')): + log.warning( + "--pdfa-image-compression argument only applies when " + "--output-type is one of 'pdfa', 'pdfa-1', or 'pdfa-2'" + ) diff --git a/src/ocrmypdf/builtin_plugins/optimize.py b/src/ocrmypdf/builtin_plugins/optimize.py index 3a8716b6..8412b16c 100644 --- a/src/ocrmypdf/builtin_plugins/optimize.py +++ b/src/ocrmypdf/builtin_plugins/optimize.py @@ -18,6 +18,7 @@ from ocrmypdf._pipeline import get_pdf_save_settings from ocrmypdf.cli import numeric from ocrmypdf.optimize import optimize from ocrmypdf.subprocess import check_external_program +from pydantic import model_validator log = logging.getLogger(__name__) @@ -139,6 +140,36 @@ class OptimizeOptions(BaseModel): ), ) + @model_validator(mode='after') + def validate_optimization_consistency(self): + """Validate optimization options are consistent.""" + if self.level == 0 and any([ + self.jbig2_lossy, + self.png_quality > 0, + self.jpeg_quality > 0 + ]): + log.warning( + "The arguments --jbig2-lossy, --png-quality, and --jpeg-quality " + "will be ignored because --optimize=0." + ) + return self + + def validate_with_context(self, external_programs_available: dict[str, bool]) -> None: + """Validate options that require external context. + + Args: + external_programs_available: Dict of program name -> availability + """ + if self.level >= 2: + if not external_programs_available.get('pngquant', False): + log.warning( + "pngquant is not available, so PNG optimization will be limited" + ) + if not external_programs_available.get('jbig2enc', False): + log.warning( + "jbig2enc is not available, so JBIG2 optimization will be limited" + ) + @hookimpl def register_options(): @@ -154,6 +185,7 @@ def add_options(parser): @hookimpl def check_options(options): + """Check external dependencies for optimization.""" if options.optimize >= 2: check_external_program( program='pngquant', @@ -175,14 +207,6 @@ def check_options(options): recommended=True if not options.jbig2_lossy else False, ) - if options.optimize == 0 and any( - [options.jbig2_lossy, options.png_quality, options.jpeg_quality] - ): - log.warning( - "The arguments --jbig2-lossy, --png-quality, and --jpeg-quality " - "will be ignored because --optimize=0." - ) - @hookimpl def optimize_pdf( diff --git a/src/ocrmypdf/builtin_plugins/tesseract_ocr.py b/src/ocrmypdf/builtin_plugins/tesseract_ocr.py index e9ca80d9..90d33b45 100644 --- a/src/ocrmypdf/builtin_plugins/tesseract_ocr.py +++ b/src/ocrmypdf/builtin_plugins/tesseract_ocr.py @@ -21,6 +21,7 @@ from ocrmypdf.helpers import available_cpu_count, clamp from ocrmypdf.imageops import calculate_downsample, downsample_image from ocrmypdf.pluginspec import OcrEngine from ocrmypdf.subprocess import check_external_program +from pydantic import field_validator, model_validator log = logging.getLogger(__name__) @@ -204,6 +205,54 @@ class TesseractOptions(BaseModel): help="Specify the location of the Tesseract user patterns file.", ) + @field_validator('timeout', 'non_ocr_timeout') + @classmethod + def validate_timeout_reasonable(cls, v): + """Validate timeout values are reasonable.""" + if v > 3600: # 1 hour + log.warning(f"Timeout of {v} seconds is very long and may cause issues") + return v + + @field_validator('pagesegmode') + @classmethod + def validate_pagesegmode_warning(cls, v): + """Validate page segmentation mode and warn about problematic values.""" + if v in (0, 2): + log.warning( + "The tesseract-pagesegmode you selected will disable OCR. " + "This may cause processing to fail." + ) + return v + + @model_validator(mode='after') + def validate_downsample_consistency(self): + """Validate downsample options are consistent.""" + if ( + self.downsample_above != 32767 + and not self.downsample_large_images + ): + log.warning( + "The --tesseract-downsample-above argument will have no effect unless " + "--tesseract-downsample-large-images is also given." + ) + return self + + def validate_with_context(self, languages: list[str]) -> None: + """Validate options that require external context. + + Args: + languages: List of languages being used for OCR + """ + # Validate languages are not internal Tesseract languages + DENIED_LANGUAGES = {'equ', 'osd'} + if DENIED_LANGUAGES & set(languages): + raise BadArgsError( + "The following languages are for Tesseract's internal use and should not " + "be issued explicitly: " + f"{', '.join(DENIED_LANGUAGES & set(languages))}\n" + "Remove them from the -l/--language argument." + ) + @hookimpl def register_options(): @@ -219,6 +268,7 @@ def add_options(parser): @hookimpl def check_options(options): + """Check external dependencies and version compatibility for Tesseract.""" check_external_program( program='tesseract', package={'linux': 'tesseract-ocr'}, @@ -233,27 +283,13 @@ def check_options(options): "Please upgrade to a newer or supported older version." ) - # Validate Tesseract-specific options using the new model - # For now, we still access options directly for backward compatibility + # Check version-specific feature compatibility if not tesseract.has_thresholding() and options.tesseract_thresholding != 0: log.warning( "The installed version of Tesseract does not support changes to its " "thresholding method. The --tesseract-threshold argument will be " "ignored." ) - if options.tesseract_pagesegmode in (0, 2): - log.warning( - "The --tesseract-pagesegmode argument you select will disable OCR. " - "This may cause processing to fail." - ) - DENIED_LANGUAGES = {'equ', 'osd'} - if DENIED_LANGUAGES & set(options.languages): - raise BadArgsError( - "The following languages for Tesseract's internal use and should not " - "be issued explicitly: " - f"{', '.join(DENIED_LANGUAGES & set(options.languages))}\n" - "Remove them from the -l/--language argument." - ) @hookimpl