From 7f4bac893862c288dc579a2be2564410b31c8e91 Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Tue, 9 Jun 2026 17:03:06 -0400 Subject: [PATCH 1/6] Add oversize submission detection and backward-compatible DB flag Detect oversize submissions at file-upload time and persist the flag to the classic arXiv_submissions.is_oversize column. The flag is a soft gate: the submitter is warned but can still proceed. The auto-hold effect at finalize is a later phase. Adds policy module: - submit_ce/domain/size_limits.py: SizeLimits (three per-archive limits, default 50 MB) and a pure check_sizes() decision function. Enforces the total and per-file uncompressed limits, matching legacy check_sizes; compressed limit defined but not enforced. OVERRIDE/MAXSIZE env escapes. - ui/config.py: MAX_UNCOMPRESSED_TOTAL_KB / _PER_FILE_KB / _COMPRESSED_KB. Adds detection in the file `Event`: - Submission.is_oversize: flag set when files change. - UploadArchive/UploadFiles/RemoveFiles evaluate the authoritative workspace against the limits in execute(), persist the result, and apply it in project() (deterministic on replay); RemoveAllFiles clears it. - SubmitApi.get_size_limits() (defaults) + Flask override reading config, so the domain event reaches limits through the api boundary. - upload controller flashes an oversize warning. Adds submit 1.5 backward-compatible DB projection: - update_from_submission writes arXiv_submissions.is_oversize; to_submission reads it back so rows round-trip. Tests: size_limits unit tests, event detection tests, and UI/DB persistence tests (column write + domain round-trip). Co-Authored-By: Claude Opus 4.8 (1M context) --- submit_ce/api/submit.py | 9 + submit_ce/domain/event/file.py | 37 ++++ .../event/tests/test_oversize_detection.py | 157 +++++++++++++ submit_ce/domain/size_limits.py | 207 ++++++++++++++++++ submit_ce/domain/submission.py | 6 + submit_ce/domain/tests/test_size_limits.py | 163 ++++++++++++++ .../legacy_implementation/db.py | 1 + .../legacy_implementation/flask_impl.py | 13 ++ .../legacy_implementation/models.py | 2 + submit_ce/ui/config.py | 12 + submit_ce/ui/conftest.py | 45 ++++ .../new/tests/test_upload_delete.py | 5 + submit_ce/ui/controllers/new/upload.py | 19 ++ .../ui/tests/test_oversize_persistence.py | 50 +++++ submit_ce/ui/tests/test_oversize_upload.py | 29 +++ 15 files changed, 755 insertions(+) create mode 100644 submit_ce/domain/event/tests/test_oversize_detection.py create mode 100644 submit_ce/domain/size_limits.py create mode 100644 submit_ce/domain/tests/test_size_limits.py create mode 100644 submit_ce/ui/tests/test_oversize_persistence.py create mode 100644 submit_ce/ui/tests/test_oversize_upload.py diff --git a/submit_ce/api/submit.py b/submit_ce/api/submit.py index c5dfc31d..364fc11b 100644 --- a/submit_ce/api/submit.py +++ b/submit_ce/api/submit.py @@ -65,11 +65,20 @@ from submit_ce.api.compile_service import CompileService from submit_ce.domain import Submission, Event, License +from submit_ce.domain.size_limits import SizeLimits from submit_ce.api.file_store import SubmissionFileStore class SubmitApi(ABC): + def get_size_limits(self) -> SizeLimits: + """The size limits used to flag oversize submissions. + + Defaults to the built-in 50 MB limits. Implementations with access to + application config (e.g. the Flask implementation) override this to + honor the configured ``MAX_*_KB`` values.""" + return SizeLimits.defaults() + @abstractmethod def get(self, submission_id: str) -> Submission: """ diff --git a/submit_ce/domain/event/file.py b/submit_ce/domain/event/file.py index f59445e5..b6a1c10b 100644 --- a/submit_ce/domain/event/file.py +++ b/submit_ce/domain/event/file.py @@ -9,6 +9,7 @@ from .base import EventWithSideEffect from ..submission import Submission from ..uploads import SubmitFile +from .. import size_limits import logging logger = logging.getLogger(__name__) @@ -25,6 +26,26 @@ def _common_file_change_execute(api: SubmitApi, submission: Submission) -> None: file_store.delete_preview(str(submission.submission_id)) +def _evaluate_oversize(api: SubmitApi, submission: Submission) -> bool: + """Measure the current workspace against the configured size limits. + + Called from ``execute`` (which has file-store access). The boolean result + is stored on the event so ``project`` can apply it deterministically on + replay, when ``execute`` does not run. Reads the authoritative post-change + workspace so the flag reflects *all* current files, not just the ones this + event touched. + """ + workspace = api.get_file_store().get_workspace(str(submission.submission_id)) + if workspace is None: + return False + per_file = {file.path: file.bytes for file in workspace.files} + total = workspace.size or 0 + category = (submission.primary_classification.category + if submission.primary_classification else None) + return size_limits.is_oversize(total, per_file, primary_category=category, + limits=api.get_size_limits()) + + class UploadArchive(EventWithSideEffect): """Uploads a zip or tgz file to the workspace, unpacking all the files.""" @@ -37,6 +58,9 @@ class UploadArchive(EventWithSideEffect): bytes_added: int = 0 """Bytes added by uploading this archive.""" + oversize: bool = False + """Whether the submission is oversize after this change (set in execute).""" + def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) @@ -45,9 +69,11 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: files = api.get_file_store().store_source_package(str(submission.submission_id), self.file, 4098) self.bytes_added = sum([file.bytes for file in files]) _common_file_change_execute(api, submission) + self.oversize = _evaluate_oversize(api, submission) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added + submission.is_oversize = self.oversize _common_file_change_project(submission) return submission @@ -66,6 +92,9 @@ class UploadFiles(EventWithSideEffect): Field(default_factory=list, exclude=True) bytes_added: int = 0 + oversize: bool = False + """Whether the submission is oversize after this change (set in execute).""" + def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) @@ -76,9 +105,11 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: stat=file_store.store_source_file(str(submission.submission_id), f, chunk_size=4096) self.bytes_added += stat.bytes _common_file_change_execute(api, submission) + self.oversize = _evaluate_oversize(api, submission) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added + submission.is_oversize = self.oversize _common_file_change_project(submission) return submission @@ -96,6 +127,9 @@ class RemoveFiles(EventWithSideEffect): bytes_removed:int = 0 """Bytes removed by removing these files.""" + oversize: bool = False + """Whether the submission is oversize after this change (set in execute).""" + def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) @@ -109,9 +143,11 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: self.bytes_removed += file.bytes _common_file_change_execute(api, submission) + self.oversize = _evaluate_oversize(api, submission) def project(self, submission: Submission) -> Submission: submission.uncompressed_size -= self.bytes_removed + submission.is_oversize = self.oversize _common_file_change_project(submission) return submission @@ -134,5 +170,6 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: def project(self, submission: Submission) -> Submission: submission.source_format = None submission.uncompressed_size = 0 + submission.is_oversize = False _common_file_change_project(submission) return submission diff --git a/submit_ce/domain/event/tests/test_oversize_detection.py b/submit_ce/domain/event/tests/test_oversize_detection.py new file mode 100644 index 00000000..e3a0e6a9 --- /dev/null +++ b/submit_ce/domain/event/tests/test_oversize_detection.py @@ -0,0 +1,157 @@ +"""Tests for oversize detection wired into the file-upload events. + +These exercise ``execute``/``project`` directly with a hand-rolled fake API so +no Flask app or real file store is needed. +""" + +from datetime import datetime +from types import SimpleNamespace + +from pytz import UTC + +from submit_ce.domain import submission as submod, agent +from submit_ce.domain.meta import Classification +from submit_ce.domain.event.file import ( + UploadFiles, + UploadArchive, + RemoveFiles, + RemoveAllFiles, +) +from submit_ce.domain.size_limits import SizeLimits + +MB = 1024 * 1024 + + +class _FakeStore: + def __init__(self, workspace): + self.workspace = workspace + + def store_source_package(self, sid, content, chunk_size): + return [] + + def store_source_file(self, sid, content, chunk_size): + return SimpleNamespace(bytes=0, path=getattr(content, "filename", "f")) + + def delete_source_file(self, sid, name): + return None + + def delete_all_source_files(self, sid): + pass + + def get_workspace(self, sid): + return self.workspace + + def delete_preflight(self, sid): + pass + + def delete_preview(self, sid): + pass + + +class _FakeApi: + def __init__(self, workspace, limits=None): + self._store = _FakeStore(workspace) + self._limits = limits or SizeLimits.defaults() + + def get_file_store(self): + return self._store + + def get_size_limits(self): + return self._limits + + +def _ws(total, per_file=None): + per_file = per_file or {} + files = [SimpleNamespace(path=p, bytes=b) for p, b in per_file.items()] + return SimpleNamespace(size=total, files=files) + + +def _user(uid="u1"): + return agent.PublicUser(name="Test User", user_id=uid, + email=f"{uid}@example.org", endorsements=[]) + + +def _submission(category="astro-ph.GA"): + u = _user() + return submod.Submission( + creator=u, owner=u, created=datetime.now(UTC), + primary_classification=Classification(category=category)) + + +def test_submission_defaults_not_oversize(): + assert _submission().is_oversize is False + + +def test_upload_files_flags_oversize(): + s = _submission() + api = _FakeApi(_ws(60 * MB, {"huge.pdf": 60 * MB})) + e = UploadFiles(creator=s.creator, files=[]) + e.execute(api, s) + assert e.oversize is True + s = e.project(s) + assert s.is_oversize is True + + +def test_upload_files_within_limit_not_oversize(): + s = _submission() + api = _FakeApi(_ws(10 * MB, {"ok.pdf": 10 * MB})) + e = UploadFiles(creator=s.creator, files=[]) + e.execute(api, s) + s = e.project(s) + assert s.is_oversize is False + + +def test_upload_archive_flags_oversize(): + s = _submission() + api = _FakeApi(_ws(80 * MB, {"a.tex": 80 * MB})) + e = UploadArchive(creator=s.creator, file=None) + e.execute(api, s) + s = e.project(s) + assert s.is_oversize is True + + +def test_remove_files_clears_oversize(): + s = _submission() + s.is_oversize = True + api = _FakeApi(_ws(5 * MB, {"small.tex": 5 * MB})) + e = RemoveFiles(creator=s.creator, files=[]) + e.execute(api, s) + s = e.project(s) + assert s.is_oversize is False + + +def test_remove_all_files_clears_oversize(): + s = _submission() + s.is_oversize = True + e = RemoveAllFiles(creator=s.creator) + s = e.project(s) + assert s.is_oversize is False + + +def test_project_uses_persisted_flag_on_replay(): + # On replay execute() does not run; the flag comes from the stored event. + s = _submission() + e = UploadFiles(creator=s.creator, files=[], oversize=True) + s = e.project(s) + assert s.is_oversize is True + + +def test_no_workspace_is_not_oversize(): + s = _submission() + api = _FakeApi(None) + e = UploadFiles(creator=s.creator, files=[]) + e.execute(api, s) + assert e.oversize is False + + +def test_per_archive_limit_used_in_event(): + s = _submission(category="astro-ph.GA") + limits = SizeLimits( + max_uncompressed_total={"default": 100 * MB, "astro-ph": 5 * MB}, + max_uncompressed_per_file={"default": 100 * MB}, + max_compressed={"default": 100 * MB}, + ) + api = _FakeApi(_ws(10 * MB, {"f": 10 * MB}), limits=limits) + e = UploadFiles(creator=s.creator, files=[]) + e.execute(api, s) + assert e.oversize is True # 10 MB exceeds the 5 MB astro-ph total limit diff --git a/submit_ce/domain/size_limits.py b/submit_ce/domain/size_limits.py new file mode 100644 index 00000000..d9207d15 --- /dev/null +++ b/submit_ce/domain/size_limits.py @@ -0,0 +1,207 @@ +"""Size-limit policy for submissions. + +A port of the legacy ``arXiv/Submit/Size_limits.pm``. It keeps *policy* +(the limit values) separate from *enforcement* (the size check), so the +numbers can be tuned without touching the checking logic. + +There are three independent limits, each a mapping keyed by archive with a +``'default'`` fallback (matching legacy, where only ``'default'`` is +populated but per-archive overrides are supported): + +* ``max_uncompressed_total`` -- sum of all extracted files +* ``max_uncompressed_per_file`` -- any single extracted file +* ``max_compressed`` -- size of the compressed upload + +All limits default to 50,000 KB (50 MB), the legacy arXiv size guideline. + +Only the total and per-file uncompressed limits are *enforced* by +:func:`check_sizes` (matching legacy ``check_sizes``); ``max_compressed`` is +defined for completeness and future tuning but is not currently checked. + +Values are handled in **bytes** internally (matching +:attr:`submit_ce.domain.submission.Submission.uncompressed_size` and +:attr:`submit_ce.domain.uploads.FileStatus.bytes`); the documented defaults +are expressed in KB to mirror the legacy module. + +This module is pure domain code and must not depend on Flask. The two +environment escape hatches below mirror the legacy ``OVERRIDE``/``MAXSIZE`` +knobs and are read at call time so tests can raise the limits without +reconfiguring the app. +""" + +import os +from dataclasses import dataclass +from typing import List, Mapping, Optional + +from arxiv.taxonomy.definitions import CATEGORIES + +ONE_KB = 1024 +DEFAULT_MAX_SIZE_KB = 50_000 +"""Legacy arXiv size guideline: 50,000 KB (50 MB).""" + +DEFAULT_MAX_SIZE_BYTES = DEFAULT_MAX_SIZE_KB * ONE_KB + +DEFAULT_ARCHIVE = "default" +"""Fallback key used when a category has no archive-specific override.""" + +OVERRIDE_ENV = "SUBMIT_OVERSIZE_OVERRIDE" +"""When truthy, doubles every limit (port of legacy ``OVERRIDE``).""" + +MAXSIZE_ENV = "SUBMIT_OVERSIZE_MAXSIZE_KB" +"""When set to a number of KB, raises every limit to at least that value +(port of legacy ``MAXSIZE``).""" + + +def _truthy(value: Optional[str]) -> bool: + return bool(value) and value.strip().lower() not in ("0", "false", "no", "") + + +def _apply_env_escapes(base_bytes: int) -> int: + """Apply the ``MAXSIZE``/``OVERRIDE`` escape hatches to a limit.""" + maxsize = os.environ.get(MAXSIZE_ENV) + if maxsize: + try: + base_bytes = max(base_bytes, int(maxsize) * ONE_KB) + except ValueError: + pass + if _truthy(os.environ.get(OVERRIDE_ENV)): + base_bytes *= 2 + return base_bytes + + +def archive_for_category(category: Optional[str]) -> str: + """Return the archive key for a category, or ``'default'`` if unknown.""" + if category and category in CATEGORIES: + return CATEGORIES[category].in_archive + return DEFAULT_ARCHIVE + + +@dataclass(frozen=True) +class SizeLimits: + """The three size limits, each keyed by archive (values in bytes). + + Each mapping must contain a ``'default'`` entry. Per-archive overrides are + optional; lookups fall back to ``'default'``. + """ + + max_uncompressed_total: Mapping[str, int] + max_uncompressed_per_file: Mapping[str, int] + max_compressed: Mapping[str, int] + + @classmethod + def defaults(cls) -> "SizeLimits": + """Limits at the legacy default (50 MB), with env escapes applied.""" + return cls.from_kb(DEFAULT_MAX_SIZE_KB, + DEFAULT_MAX_SIZE_KB, + DEFAULT_MAX_SIZE_KB) + + @classmethod + def from_kb(cls, total_kb: int, per_file_kb: int, + compressed_kb: int) -> "SizeLimits": + """Build limits from KB values (e.g. from config), applying escapes.""" + return cls( + max_uncompressed_total={ + DEFAULT_ARCHIVE: _apply_env_escapes(total_kb * ONE_KB)}, + max_uncompressed_per_file={ + DEFAULT_ARCHIVE: _apply_env_escapes(per_file_kb * ONE_KB)}, + max_compressed={ + DEFAULT_ARCHIVE: _apply_env_escapes(compressed_kb * ONE_KB)}, + ) + + @staticmethod + def _lookup(table: Mapping[str, int], category: Optional[str]) -> int: + return table.get(archive_for_category(category), table[DEFAULT_ARCHIVE]) + + def total_limit(self, category: Optional[str] = None) -> int: + return self._lookup(self.max_uncompressed_total, category) + + def per_file_limit(self, category: Optional[str] = None) -> int: + return self._lookup(self.max_uncompressed_per_file, category) + + def compressed_limit(self, category: Optional[str] = None) -> int: + return self._lookup(self.max_compressed, category) + + +@dataclass(frozen=True) +class OversizeReason: + """One reason a submission is oversize, with human-readable text.""" + + kind: str + """``'total'`` or ``'per_file'``.""" + + message: str + limit: int + """The limit that was exceeded, in bytes.""" + + actual: int + """The measured size, in bytes.""" + + path: Optional[str] = None + """The offending file, for ``'per_file'`` reasons.""" + + +def _mb(num_bytes: int) -> str: + return f"{num_bytes / (ONE_KB * ONE_KB):.1f} MB" + + +def check_sizes(total_uncompressed: int, + per_file_sizes: Optional[Mapping[str, int]] = None, + primary_category: Optional[str] = None, + limits: Optional[SizeLimits] = None) -> List[OversizeReason]: + """Return the reasons a submission is oversize, or ``[]`` if within limits. + + Enforces the total-uncompressed and per-file-uncompressed limits, matching + the legacy ``check_sizes``. + + Parameters + ---------- + total_uncompressed + Sum of all extracted files, in bytes. + per_file_sizes + Mapping of file path to size in bytes. Empty/omitted skips the + per-file check. + primary_category + Primary classification category, used to select per-archive limits. + limits + Limits to enforce. Defaults to :meth:`SizeLimits.defaults`. + """ + limits = limits or SizeLimits.defaults() + per_file_sizes = per_file_sizes or {} + reasons: List[OversizeReason] = [] + + total_limit = limits.total_limit(primary_category) + if total_uncompressed > total_limit: + reasons.append(OversizeReason( + kind="total", + message=(f"Total uncompressed size {_mb(total_uncompressed)} " + f"exceeds the {_mb(total_limit)} limit."), + limit=total_limit, + actual=total_uncompressed, + )) + + per_file_limit = limits.per_file_limit(primary_category) + for path, size in per_file_sizes.items(): + if size > per_file_limit: + reasons.append(OversizeReason( + kind="per_file", + message=(f"File '{path}' is {_mb(size)}, exceeding the " + f"{_mb(per_file_limit)} per-file limit."), + limit=per_file_limit, + actual=size, + path=path, + )) + return reasons + + +def is_oversize(total_uncompressed: int, + per_file_sizes: Optional[Mapping[str, int]] = None, + primary_category: Optional[str] = None, + limits: Optional[SizeLimits] = None) -> bool: + """Return ``True`` if the submission exceeds any enforced limit.""" + return bool(check_sizes(total_uncompressed, per_file_sizes, + primary_category, limits)) + + +def summarize(reasons: List[OversizeReason]) -> str: + """Join reason messages into a single human-readable warning string.""" + return " ".join(reason.message for reason in reasons) diff --git a/submit_ce/domain/submission.py b/submit_ce/domain/submission.py index 66c3f8df..143d67aa 100644 --- a/submit_ce/domain/submission.py +++ b/submit_ce/domain/submission.py @@ -289,6 +289,12 @@ class Submission: source_format: Optional[SourceFormat] = field(default=None) uncompressed_size: int = field(default=0) + is_oversize: bool = field(default=False) + """Canonical oversize flag, set from the size check when files change. + + This is the flag, set at upload time; the auto-hold is a separate effect + applied at finalize. + """ preview: Optional[Preview] = field(default=None) metadata: SubmissionMetadata = field(default_factory=SubmissionMetadata) diff --git a/submit_ce/domain/tests/test_size_limits.py b/submit_ce/domain/tests/test_size_limits.py new file mode 100644 index 00000000..3c454bbf --- /dev/null +++ b/submit_ce/domain/tests/test_size_limits.py @@ -0,0 +1,163 @@ +"""Tests for the size-limit policy module (:mod:`submit_ce.domain.size_limits`).""" + +from submit_ce.domain import size_limits as sl +from submit_ce.domain.size_limits import ( + ONE_KB, + DEFAULT_MAX_SIZE_KB, + DEFAULT_MAX_SIZE_BYTES, + SizeLimits, + archive_for_category, + check_sizes, + is_oversize, + summarize, +) + +MB = ONE_KB * ONE_KB + + +# --- limit values & lookups ------------------------------------------------- + +def test_default_limit_is_50mb(): + assert DEFAULT_MAX_SIZE_KB == 50_000 + assert DEFAULT_MAX_SIZE_BYTES == 50_000 * ONE_KB + + +def test_defaults_populate_all_three_limits(): + limits = SizeLimits.defaults() + assert limits.total_limit() == DEFAULT_MAX_SIZE_BYTES + assert limits.per_file_limit() == DEFAULT_MAX_SIZE_BYTES + assert limits.compressed_limit() == DEFAULT_MAX_SIZE_BYTES + + +def test_from_kb_converts_to_bytes_independently(): + limits = SizeLimits.from_kb(total_kb=10, per_file_kb=20, compressed_kb=30) + assert limits.total_limit() == 10 * ONE_KB + assert limits.per_file_limit() == 20 * ONE_KB + assert limits.compressed_limit() == 30 * ONE_KB + + +def test_archive_for_category(): + assert archive_for_category("math.GT") == "math" + assert archive_for_category("astro-ph.GA") == "astro-ph" + assert archive_for_category("hep-th") == "hep-th" + assert archive_for_category(None) == "default" + assert archive_for_category("not.a.category") == "default" + + +def test_per_archive_override_falls_back_to_default(): + limits = SizeLimits( + max_uncompressed_total={"default": 100, "astro-ph": 999}, + max_uncompressed_per_file={"default": 100}, + max_compressed={"default": 100}, + ) + # astro-ph.GA resolves to the astro-ph override... + assert limits.total_limit("astro-ph.GA") == 999 + # ...while other categories fall back to 'default'. + assert limits.total_limit("math.GT") == 100 + assert limits.total_limit(None) == 100 + + +# --- check_sizes ------------------------------------------------------------ + +def test_within_limits_returns_no_reasons(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + reasons = check_sizes(total_uncompressed=10 * MB, + per_file_sizes={"a.tex": 1 * MB, "b.png": 5 * MB}, + limits=limits) + assert reasons == [] + assert is_oversize(10 * MB, {"a.tex": 1 * MB}, limits=limits) is False + + +def test_total_over_limit_flags_total(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + reasons = check_sizes(total_uncompressed=60 * MB, limits=limits) + assert len(reasons) == 1 + assert reasons[0].kind == "total" + assert reasons[0].actual == 60 * MB + assert reasons[0].limit == DEFAULT_MAX_SIZE_BYTES + assert is_oversize(60 * MB, limits=limits) is True + + +def test_per_file_over_limit_flags_each_file(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + reasons = check_sizes( + total_uncompressed=10 * MB, # total is fine + per_file_sizes={"small.tex": 1 * MB, "huge.dat": 60 * MB}, + limits=limits, + ) + assert len(reasons) == 1 + assert reasons[0].kind == "per_file" + assert reasons[0].path == "huge.dat" + assert reasons[0].actual == 60 * MB + + +def test_both_dimensions_can_trip(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + reasons = check_sizes( + total_uncompressed=120 * MB, + per_file_sizes={"huge.dat": 60 * MB}, + limits=limits, + ) + kinds = sorted(r.kind for r in reasons) + assert kinds == ["per_file", "total"] + + +def test_exactly_at_limit_is_not_oversize(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + assert check_sizes(DEFAULT_MAX_SIZE_BYTES, + {"f": DEFAULT_MAX_SIZE_BYTES}, + limits=limits) == [] + + +def test_compressed_limit_is_not_enforced(): + # A tiny per-file/total but a defined compressed limit: check_sizes only + # looks at total + per-file, never compressed. + limits = SizeLimits.from_kb(50_000, 50_000, compressed_kb=1) + assert check_sizes(10 * MB, {"a": 1 * MB}, limits=limits) == [] + + +def test_per_archive_limit_used_in_check(): + limits = SizeLimits( + max_uncompressed_total={"default": 100 * MB, "astro-ph": 5 * MB}, + max_uncompressed_per_file={"default": 100 * MB}, + max_compressed={"default": 100 * MB}, + ) + # 10MB is over the 5MB astro-ph limit but under the 100MB default. + assert is_oversize(10 * MB, primary_category="astro-ph.GA", limits=limits) + assert not is_oversize(10 * MB, primary_category="math.GT", limits=limits) + + +def test_summarize_joins_messages(): + limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + reasons = check_sizes(120 * MB, {"huge.dat": 60 * MB}, limits=limits) + text = summarize(reasons) + assert "Total uncompressed size" in text + assert "huge.dat" in text + + +# --- environment escape hatches --------------------------------------------- + +def test_override_env_doubles_limits(monkeypatch): + monkeypatch.setenv(sl.OVERRIDE_ENV, "1") + limits = SizeLimits.defaults() + assert limits.total_limit() == 2 * DEFAULT_MAX_SIZE_BYTES + + +def test_override_env_falsey_does_nothing(monkeypatch): + monkeypatch.setenv(sl.OVERRIDE_ENV, "0") + assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES + + +def test_maxsize_env_raises_limits(monkeypatch): + monkeypatch.setenv(sl.MAXSIZE_ENV, "200000") # 200,000 KB + assert SizeLimits.defaults().total_limit() == 200_000 * ONE_KB + + +def test_maxsize_env_only_raises_never_lowers(monkeypatch): + monkeypatch.setenv(sl.MAXSIZE_ENV, "1") # below the default + assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES + + +def test_maxsize_env_ignores_garbage(monkeypatch): + monkeypatch.setenv(sl.MAXSIZE_ENV, "not-a-number") + assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES diff --git a/submit_ce/implementations/legacy_implementation/db.py b/submit_ce/implementations/legacy_implementation/db.py index a6e3acbc..e253ea9d 100644 --- a/submit_ce/implementations/legacy_implementation/db.py +++ b/submit_ce/implementations/legacy_implementation/db.py @@ -680,6 +680,7 @@ def to_submission(row: models.Submission, updated=row.get_updated(), source_format=source_format, uncompressed_size=uncompressed_size, + is_oversize=bool(row.is_oversize), submitter_is_author=bool(row.is_author), submitter_accepts_policy=bool(row.agree_policy), submitter_contact_verified=bool(row.userinfo), diff --git a/submit_ce/implementations/legacy_implementation/flask_impl.py b/submit_ce/implementations/legacy_implementation/flask_impl.py index d3c6383d..83cb603a 100644 --- a/submit_ce/implementations/legacy_implementation/flask_impl.py +++ b/submit_ce/implementations/legacy_implementation/flask_impl.py @@ -1,8 +1,10 @@ from arxiv.db import Session +from flask import current_app, has_app_context from . import LegacySubmitImplementation from sqlalchemy.orm import Session as SqlalchemySession +from submit_ce.domain.size_limits import SizeLimits, DEFAULT_MAX_SIZE_KB from submit_ce.implementations.compile.compile_api_service import CompileApiService def flask_get_session() -> SqlalchemySession: @@ -21,3 +23,14 @@ def __init__(self, compiler = compiler or CompileApiService() super().__init__(store=store, compiler=compiler) self.get_session = flask_get_session + + def get_size_limits(self) -> SizeLimits: + """Build size limits from the configured ``MAX_*_KB`` values.""" + if not has_app_context(): + return SizeLimits.defaults() + cfg = current_app.config + return SizeLimits.from_kb( + cfg.get("MAX_UNCOMPRESSED_TOTAL_KB", DEFAULT_MAX_SIZE_KB), + cfg.get("MAX_UNCOMPRESSED_PER_FILE_KB", DEFAULT_MAX_SIZE_KB), + cfg.get("MAX_COMPRESSED_KB", DEFAULT_MAX_SIZE_KB), + ) diff --git a/submit_ce/implementations/legacy_implementation/models.py b/submit_ce/implementations/legacy_implementation/models.py index f4b78420..b6396b49 100644 --- a/submit_ce/implementations/legacy_implementation/models.py +++ b/submit_ce/implementations/legacy_implementation/models.py @@ -322,6 +322,8 @@ def update_from_submission(self, submission: domain.Submission) -> None: self.source_size = submission.uncompressed_size + self.is_oversize = 1 if submission.is_oversize else 0 + if submission.source_format: self.source_format = submission.source_format.value else: diff --git a/submit_ce/ui/config.py b/submit_ce/ui/config.py index d2fc9073..2fa06943 100644 --- a/submit_ce/ui/config.py +++ b/submit_ce/ui/config.py @@ -99,6 +99,18 @@ def __init__(self, **kwargs): """If true, only admin users can use the system. Intended to allowe closed to the public dev or beta system.""" + MAX_UNCOMPRESSED_TOTAL_KB: int = 50_000 + """Max total uncompressed submission size, in KB, before the submission is + flagged oversize (default 50 MB; the legacy arXiv size guideline).""" + + MAX_UNCOMPRESSED_PER_FILE_KB: int = 50_000 + """Max uncompressed size of any single file, in KB, before the submission + is flagged oversize (default 50 MB).""" + + MAX_COMPRESSED_KB: int = 50_000 + """Max compressed upload size, in KB. Defined for completeness; not + currently enforced (matches legacy check_sizes).""" + COMPILE_API_URL: str = "https://tex2pdf-api-default-874717964009.us-central1.run.app" """The tex2pdf-api url. Do not end with a /. diff --git a/submit_ce/ui/conftest.py b/submit_ce/ui/conftest.py index d7a352c6..1a0ff4e3 100644 --- a/submit_ce/ui/conftest.py +++ b/submit_ce/ui/conftest.py @@ -4,6 +4,7 @@ import tempfile import uuid import logging +from types import SimpleNamespace from unittest.mock import MagicMock import arxiv.db.models as classic @@ -284,6 +285,50 @@ class _FakePdf: mock_store = MagicMock() mock_store.store_source_file.return_value = fake_stat + # The upload events read the workspace to evaluate the size limits. + small_ws = MagicMock() + small_ws.size = 10_000 + small_ws.files = [] + mock_store.get_workspace.return_value = small_ws + + original_store = current_app.api.store + current_app.api.store = mock_store + try: + submission, _ = current_app.api.save( + UploadFiles(creator=user, client=ua, files=[_FakePdf()]), + submission_id=sub_cross.submission_id, + ) + finally: + current_app.api.store = original_store + + return submission + + +@pytest.fixture(scope="function") +def sub_files_oversize(app, authorized_user, sub_cross): + """A submission whose uploaded files exceed the size limit. + + The submission is flagged ``is_oversize`` but is NOT yet on hold; the + auto-hold is only applied at finalize time.""" + with app.app_context(): + user = authorized_user + ua = InternalClient(name=f"test_client_{__file__}") + big = 60 * 1024 * 1024 # 60 MB, over the 50 MB default limit + + class _FakePdf: + filename = "huge.pdf" + content_type = "application/pdf" + stream = io.BytesIO(b"%PDF-1.4\n%%EOF\n") + + fake_stat = MagicMock() + fake_stat.bytes = big + + mock_store = MagicMock() + mock_store.store_source_file.return_value = fake_stat + big_ws = MagicMock() + big_ws.size = big + big_ws.files = [SimpleNamespace(path="huge.pdf", bytes=big)] + mock_store.get_workspace.return_value = big_ws original_store = current_app.api.store current_app.api.store = mock_store diff --git a/submit_ce/ui/controllers/new/tests/test_upload_delete.py b/submit_ce/ui/controllers/new/tests/test_upload_delete.py index 5cd08611..8bfc315a 100644 --- a/submit_ce/ui/controllers/new/tests/test_upload_delete.py +++ b/submit_ce/ui/controllers/new/tests/test_upload_delete.py @@ -40,6 +40,11 @@ def test_delete_file_post_confirmed(app, authorized_client, sub_files, mocker): mock_store = mocker.patch.object(app.api, 'store') mock_store.delete_source_file.return_value.bytes = 1 + # RemoveFiles re-evaluates the size limits, which reads the workspace. + ws = mocker.MagicMock() + ws.size = 0 + ws.files = [] + mock_store.get_workspace.return_value = ws resp = authorized_client.post(url, data={ 'csrf_token': csrf, diff --git a/submit_ce/ui/controllers/new/upload.py b/submit_ce/ui/controllers/new/upload.py index b2a62e29..b62c0e16 100644 --- a/submit_ce/ui/controllers/new/upload.py +++ b/submit_ce/ui/controllers/new/upload.py @@ -226,6 +226,23 @@ def _get_upload(params: MultiDict, session: Session, submission: Submission, +def _flash_oversize_warning(submission: Submission) -> None: + """Warn the submitter that an oversize submission will be held for review. + + The submission is not rejected: the size check is a soft gate. The flag is + persisted during event save; the auto-hold is applied when the submission is + finalized.""" + if not submission.is_oversize: + return + alerts.flash_warning( + Markup( + 'This submission exceeds the arXiv size guideline. You can still ' + 'submit, but it will be placed on hold for moderator review. See ' + 'arxiv.org/help/sizes for ways to reduce ' + 'the size, or to request a size exception.'), + title='Submission is oversize') + + def _upload_archive(form: AddfilesForm, file: FileStorage, submitter: User, client: Client, submission: Submission, rdata: Dict[str, Any], token: str) \ @@ -254,6 +271,7 @@ def _upload_archive(form: AddfilesForm, file: FileStorage, f' package size is {converted_size}. See below for errors.', title='Upload complete, with errors' ) + _flash_oversize_warning(submission) alerts.flash_hidden(workspace.model_dump(), '_status') rdata.update({'status': workspace}) @@ -288,6 +306,7 @@ def _upload_files(form: AddfilesForm, file: FileStorage, f' package size is {converted_size}. See below for errors.', title='Upload complete, with errors' ) + _flash_oversize_warning(submission) alerts.flash_hidden(workspace.model_dump(), '_status') rdata.update({'status': workspace}) diff --git a/submit_ce/ui/tests/test_oversize_persistence.py b/submit_ce/ui/tests/test_oversize_persistence.py new file mode 100644 index 00000000..4002c641 --- /dev/null +++ b/submit_ce/ui/tests/test_oversize_persistence.py @@ -0,0 +1,50 @@ +"""Phase 3: the oversize flag is projected to the classic DB columns. + +Backward-compatibility contract: an oversize upload writes +``arXiv_submissions.is_oversize = 1`` (and leaves ``auto_hold = 0`` until +finalize), and the flag survives a domain -> DB -> domain round-trip. +""" + +from flask import current_app +from arxiv.db import Session +from sqlalchemy import text + + +def _row(sid): + return Session.execute( + text(""" + SELECT is_oversize, auto_hold + FROM arXiv_submissions + WHERE submission_id = :sid + """), + {"sid": sid}, + ).fetchone() + + +def test_oversize_upload_writes_is_oversize_column(app, sub_files_oversize): + with app.app_context(): + row = _row(sub_files_oversize.submission_id) + assert row is not None + assert row.is_oversize == 1 + # The auto-hold effect is not applied at upload time (unset/0). + assert not row.auto_hold + + +def test_normal_upload_writes_is_oversize_zero(app, sub_files): + with app.app_context(): + row = _row(sub_files.submission_id) + assert row is not None + assert row.is_oversize == 0 + + +def test_is_oversize_round_trips(app, sub_files_oversize): + """Reloading the submission reads the flag back from the DB column.""" + with app.app_context(): + reloaded = current_app.api.get(str(sub_files_oversize.submission_id)) + assert reloaded.is_oversize is True + + +def test_normal_submission_round_trips_false(app, sub_files): + with app.app_context(): + reloaded = current_app.api.get(str(sub_files.submission_id)) + assert reloaded.is_oversize is False diff --git a/submit_ce/ui/tests/test_oversize_upload.py b/submit_ce/ui/tests/test_oversize_upload.py new file mode 100644 index 00000000..2e12f4f0 --- /dev/null +++ b/submit_ce/ui/tests/test_oversize_upload.py @@ -0,0 +1,29 @@ +"""Phase 2 oversize tests at the UI/persistence level. + +These use the composable submission fixtures from ``submit_ce/ui/conftest.py``. +""" + +from flask import current_app + +from submit_ce.domain.event.file import UploadFiles + + +def test_oversize_upload_sets_flag(sub_files_oversize): + """An oversize upload flags the submission but does not hold it yet.""" + assert sub_files_oversize.is_oversize is True + # The auto-hold is only applied at finalize; a working submission is not + # on hold yet. + assert sub_files_oversize.is_on_hold is False + + +def test_normal_upload_not_oversize(sub_files): + assert sub_files.is_oversize is False + + +def test_oversize_flag_persists_on_event(app, sub_files_oversize): + """The event's oversize flag round-trips through the event history.""" + with app.app_context(): + _, history = current_app.api.get_with_history( + str(sub_files_oversize.submission_id)) + uploads = [e for e in history if isinstance(e, UploadFiles)] + assert uploads and uploads[-1].oversize is True From 88720facd1d9e61c6a344c838d6300e38ead3146 Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Wed, 10 Jun 2026 10:59:30 -0400 Subject: [PATCH 2/6] Adds Event.consequences() for explicit follow-on events Adds auto-hold oversize on `FinalizeSubmission` Event base class (domain/event/base.py): - consequences(submission) -> List[Event]: follow-on events an event implies, given the post-projection state. Default returns none. - get_consequences(): wraps consequences() and enforces that every emitted type is declared in the new CONSEQUENCE_TYPES class attribute, so the static graph cannot silently rot. - CONSEQUENCE_TYPES: frozenset of event types an event may emit, declared statically so the type graph can be checked for cycles. Save loop (implementations/legacy_implementation/__init__.py): - `_save()` now drives a work-queue instead of a flat loop. Consequences are applied and persisted in the same locked session/transaction, inserted at the front of the queue so a consequence applies to the state immediately after its parent. First use case (domain/event/__init__.py): - FinalizeSubmission declares CONSEQUENCE_TYPES = {AddHold} and emits a SOURCE_OVERSIZE AddHold when the submission is oversize and unwaived. Status stays SUBMITTED; the derived is_on_hold property reports the hold (this model has no ON_HOLD status). Tests: - test_consequences_graph.py: asserts the production consequence type-graph is acyclic therefore chains always terminate and that all declared types are Events. Includes a detector test with a self-referencing throwaway Event to prove find_cycle fires. - test_finalize_oversize_hold.py (domain): unit-tests the oversize/normal/ waiver branches of FinalizeSubmission.consequences(). - test_finalize_oversize_hold.py (ui): end-to-end finalize of an oversize submission applies the hold and persists the AddHold in event history. Co-Authored-By: Claude Opus 4.8 (1M context) --- submit_ce/domain/event/__init__.py | 19 +++- submit_ce/domain/event/base.py | 35 +++++++ .../event/tests/test_consequences_graph.py | 94 +++++++++++++++++++ .../tests/test_finalize_oversize_hold.py | 55 +++++++++++ .../legacy_implementation/__init__.py | 14 ++- .../ui/tests/test_finalize_oversize_hold.py | 86 +++++++++++++++++ 6 files changed, 301 insertions(+), 2 deletions(-) create mode 100644 submit_ce/domain/event/tests/test_consequences_graph.py create mode 100644 submit_ce/domain/event/tests/test_finalize_oversize_hold.py create mode 100644 submit_ce/ui/tests/test_finalize_oversize_hold.py diff --git a/submit_ce/domain/event/__init__.py b/submit_ce/domain/event/__init__.py index 57c68fe0..b740e0a2 100644 --- a/submit_ce/domain/event/__init__.py +++ b/submit_ce/domain/event/__init__.py @@ -72,7 +72,7 @@ ClassifierResult from ..preview import Preview from ..submission import Submission, Author, \ - Classification, License + Classification, License, Hold from ..uploads import SourceFormat from ..exceptions import InvalidEvent @@ -978,6 +978,8 @@ class FinalizeSubmission(Event): ] REQUIRED_METADATA: ClassVar[str] = ['title', 'abstract', 'authors_display'] + CONSEQUENCE_TYPES = frozenset({AddHold}) + def validate(self, submission: Submission) -> None: """Ensure that all required data/steps are complete.""" if submission.is_finalized: @@ -992,6 +994,21 @@ def project(self, submission: Submission) -> Submission: submission.submitted = datetime.now(UTC) return submission + def consequences(self, submission: Submission) -> List[Event]: + """Place an oversize submission on hold when it is finalized. + + Recording a `SOURCE_OVERSIZE` hold (while status stays `SUBMITTED`) is + what makes :attr:`Submission.is_on_hold` report true; there is no + separate hold status in this model. Skipped if a waiver already exists. + """ + if submission.is_oversize \ + and not submission.has_waiver_for(Hold.Type.SOURCE_OVERSIZE): + return [AddHold(creator=System(name=__name__), + submission_id=submission.submission_id, + hold_type=Hold.Type.SOURCE_OVERSIZE, + hold_reason="source is oversize")] + return [] + def _required_fields_are_complete(self, submission: Submission) -> None: """Verify that all required fields are complete.""" for key in self.REQUIRED: diff --git a/submit_ce/domain/event/base.py b/submit_ce/domain/event/base.py index ff04aac0..ce8b9264 100644 --- a/submit_ce/domain/event/base.py +++ b/submit_ce/domain/event/base.py @@ -47,6 +47,13 @@ class Event(BaseModel): NAME: ClassVar[str] = 'base event' NAMED: ClassVar[str] = 'base event' + CONSEQUENCE_TYPES: ClassVar[frozenset] = frozenset() + """Event types this event may emit from :meth:`consequences`. + + Declared statically so the consequence graph over event types can be + checked for cycles. Empty means this event has no consequences. + """ + creator: User """ The agent responsible for the operation represented by this event. @@ -150,6 +157,34 @@ def project(self, submission: Submission) -> Submission: This is how the `Event` changes the `submission`.""" raise NotImplementedError('Must be implemented by subclass') + def consequences(self, submission: Submission) -> List['Event']: + """Follow-on events implied by this event given the resulting state. + + Called by the `SubmitApi.save()` loop with the submission state *after* + this event's projection. The types of the returned instances must be a + subset of :attr:`CONSEQUENCE_TYPES`. This is enforced at runtime in the + `save()`. Default: no consequences. + + This is intended to be explicit and traceable: an event names the events + it may spawn, and those types form a directed graph that is checked for + cycles by a test, so consequence chains are guaranteed to terminate. + """ + return [] + + def get_consequences(self, submission: Submission) -> List['Event']: + """Return :meth:`consequences`, enforcing the :attr:`CONSEQUENCE_TYPES` contract. + + Raises if an event emits a consequence type it did not declare; this + keeps the static consequence graph honest at runtime. + """ + events = self.consequences(submission) + for event in events: + if type(event) not in self.CONSEQUENCE_TYPES: + raise RuntimeError( + f"{self.event_type} emitted undeclared consequence " + f"{type(event).__name__}; add it to CONSEQUENCE_TYPES") + return events + @functools.cache def _get_subclasses(klass: Type[Event]) -> List[Type[Event]]: diff --git a/submit_ce/domain/event/tests/test_consequences_graph.py b/submit_ce/domain/event/tests/test_consequences_graph.py new file mode 100644 index 00000000..f6ad18bc --- /dev/null +++ b/submit_ce/domain/event/tests/test_consequences_graph.py @@ -0,0 +1,94 @@ +"""Static guarantee that the event-consequence graph terminates. + +Each :class:`.Event` subclass declares, in ``CONSEQUENCE_TYPES``, the set of +event types it may emit from :meth:`.Event.consequences`. Those declarations +form a directed graph over event *types*. If that graph is acyclic, every +runtime consequence chain is a finite path through it, so the save loop that +processes consequences is guaranteed to terminate. + +These tests enforce that invariant at CI time -- strictly stronger than a +runtime recursion-depth cap. +""" + +# Importing the package forces registration of every Event subclass so that +# ``_get_subclasses(Event)`` sees them all. +import submit_ce.domain.event # noqa: F401 +from submit_ce.domain.event.base import Event, _get_subclasses + + +def _all_event_classes(): + """All production Event classes. + + Test modules may define throwaway Event subclasses (see + ``test_detector_catches_a_self_cycle``). Pydantic's model machinery keeps a + strong reference to every subclass, so such a class lingers in + ``Event.__subclasses__()`` for the whole process and cannot be reliably + cleaned up. The production invariant only concerns production events, so we + exclude anything defined under a ``tests`` package. + """ + classes = [Event] + _get_subclasses(Event) + return [c for c in classes if "tests" not in c.__module__.split(".")] + + +def find_cycle(adjacency): + """Return a cycle (list of nodes) in the directed graph, or None. + + ``adjacency`` maps a node to its set of successor nodes. A node referenced + only as a successor (a leaf) is treated as having no outgoing edges. + """ + WHITE, GREY, BLACK = 0, 1, 2 + color = {node: WHITE for node in adjacency} + + def visit(node, path): + color[node] = GREY + for nxt in adjacency.get(node, ()): + if color.get(nxt) == GREY: + return path + [node, nxt] + if color.get(nxt, WHITE) == WHITE: + found = visit(nxt, path + [node]) + if found: + return found + color[node] = BLACK + return None + + for node in adjacency: + if color[node] == WHITE: + found = visit(node, []) + if found: + return found + return None + + +def test_declared_consequence_types_are_events(): + """Every declared consequence type is an Event subclass.""" + for cls in _all_event_classes(): + for dep in cls.CONSEQUENCE_TYPES: + assert isinstance(dep, type) and issubclass(dep, Event), \ + f"{cls.__name__}.CONSEQUENCE_TYPES contains non-Event {dep!r}" + + +def test_consequence_graph_is_acyclic(): + """The real graph of (event type -> possible consequence types) is acyclic.""" + adjacency = {cls: set(cls.CONSEQUENCE_TYPES) for cls in _all_event_classes()} + cycle = find_cycle(adjacency) + assert cycle is None, \ + "Consequence cycle detected: " + " -> ".join(c.__name__ for c in cycle) + + +def test_detector_catches_a_self_cycle(): + """Sanity check that find_cycle actually fires on a cyclic graph. + + Defines a throwaway Event subclass that lists itself as a consequence -- a + one-node cycle the detector must catch. This class lives under a ``tests`` + package, so ``_all_event_classes`` excludes it from the real acyclicity + test even though pydantic keeps it alive in ``Event.__subclasses__()``. + """ + class Bogus(Event): + NAME = "bogus self-referencing event" + + # Can't reference Bogus inside its own body; wire the self-edge after. + Bogus.CONSEQUENCE_TYPES = frozenset({Bogus}) + + cycle = find_cycle({Bogus: set(Bogus.CONSEQUENCE_TYPES)}) + assert cycle is not None, "find_cycle failed to detect a self-cycle" + assert Bogus in cycle diff --git a/submit_ce/domain/event/tests/test_finalize_oversize_hold.py b/submit_ce/domain/event/tests/test_finalize_oversize_hold.py new file mode 100644 index 00000000..34382685 --- /dev/null +++ b/submit_ce/domain/event/tests/test_finalize_oversize_hold.py @@ -0,0 +1,55 @@ +"""Unit tests for FinalizeSubmission.consequences() oversize -> hold logic. + +These exercise the pure domain method directly, with hand-built submissions, +so no app or file store is needed. +""" + +from datetime import datetime + +from pytz import UTC + +from submit_ce.domain import agent +from submit_ce.domain.meta import Classification +from submit_ce.domain.event import FinalizeSubmission, AddHold +from submit_ce.domain.submission import Submission, Hold, Waiver + + +def _user(): + return agent.PublicUser(name="Test User", user_id="u1", + email="u1@example.org", endorsements=[]) + + +def _submission(is_oversize=False, waivers=None): + u = _user() + return Submission( + creator=u, owner=u, created=datetime.now(UTC), + primary_classification=Classification(category="astro-ph.GA"), + is_oversize=is_oversize, + waivers=waivers or {}) + + +def _finalize(): + return FinalizeSubmission(creator=_user(), created=datetime.now(UTC)) + + +def test_oversize_finalize_yields_addhold(): + events = _finalize().consequences(_submission(is_oversize=True)) + assert len(events) == 1 + hold = events[0] + assert isinstance(hold, AddHold) + assert hold.hold_type == Hold.Type.SOURCE_OVERSIZE + + +def test_not_oversize_finalize_yields_nothing(): + assert _finalize().consequences(_submission(is_oversize=False)) == [] + + +def test_oversize_with_waiver_yields_nothing(): + waiver = Waiver(event_id="w1", created=datetime.now(UTC), creator=_user(), + waiver_type=Hold.Type.SOURCE_OVERSIZE, waiver_reason="ok") + sub = _submission(is_oversize=True, waivers={"w1": waiver}) + assert _finalize().consequences(sub) == [] + + +def test_declared_consequence_type(): + assert FinalizeSubmission.CONSEQUENCE_TYPES == frozenset({AddHold}) diff --git a/submit_ce/implementations/legacy_implementation/__init__.py b/submit_ce/implementations/legacy_implementation/__init__.py index 2ba1c6b6..cc90da62 100644 --- a/submit_ce/implementations/legacy_implementation/__init__.py +++ b/submit_ce/implementations/legacy_implementation/__init__.py @@ -168,7 +168,15 @@ def _save(self, *events, """Internal save for when submission is already read from the db.""" before = submission committed: List[Event] = [] - for event in events: + # A work-queue since events may imply consequent events (see + # Event.consequences) that need to be processed in this same locked + # session/transaction. They are inserted at the front of the queue so a + # consequence applies to the state immediately after its parent, before + # any remaining sibling events. The consequence type-graph is acyclic + # (enforced by test), so this terminates. + queue: List[Event] = list(events) + while queue: + event = queue.pop(0) if event.submission_id is None and before and before.submission_id is not None: event.submission_id = before.submission_id @@ -193,6 +201,10 @@ def _save(self, *events, before = after # Prepare for the next event. + # Queue any follow-on events implied by this one, given the new state. + for consequence in reversed(event.get_consequences(after)): + queue.insert(0, consequence) + all_ = sorted(existing_events + committed, key=lambda e: e.created) session.commit() return after, list(all_) diff --git a/submit_ce/ui/tests/test_finalize_oversize_hold.py b/submit_ce/ui/tests/test_finalize_oversize_hold.py new file mode 100644 index 00000000..5ab33bb1 --- /dev/null +++ b/submit_ce/ui/tests/test_finalize_oversize_hold.py @@ -0,0 +1,86 @@ +"""End-to-end: finalizing an oversize submission auto-applies a hold. + +Exercises the Event.consequences() mechanism through the real save loop: +FinalizeSubmission declares AddHold as a consequence and emits it when the +submission is oversize, and the save loop persists it in the same transaction. +""" + +import io +from types import SimpleNamespace +from unittest.mock import MagicMock + +from flask import current_app + +from submit_ce.domain.agent import InternalClient +from submit_ce.domain.event import FinalizeSubmission +from submit_ce.domain.event.file import UploadFiles +from submit_ce.domain.event.flag import AddHold +from submit_ce.domain.submission import Hold, Submission + + +def _make_oversize(user, submission_id): + """Re-upload with a workspace over the size limit to flip is_oversize.""" + ua = InternalClient(name="test_client_finalize_oversize") + big = 60 * 1024 * 1024 # over the 50 MB default limit + + class _FakePdf: + filename = "huge.pdf" + content_type = "application/pdf" + stream = io.BytesIO(b"%PDF-1.4\n%%EOF\n") + + fake_stat = MagicMock() + fake_stat.bytes = big + mock_store = MagicMock() + mock_store.store_source_file.return_value = fake_stat + big_ws = MagicMock() + big_ws.size = big + big_ws.files = [SimpleNamespace(path="huge.pdf", bytes=big)] + mock_store.get_workspace.return_value = big_ws + + original_store = current_app.api.store + current_app.api.store = mock_store + try: + submission, _ = current_app.api.save( + UploadFiles(creator=user, client=ua, files=[_FakePdf()]), + submission_id=submission_id) + finally: + current_app.api.store = original_store + return submission + + +def test_finalize_oversize_applies_hold(app, authorized_user, sub_metadata): + """An oversize, metadata-complete submission gets a SOURCE_OVERSIZE hold + on finalize, and the AddHold lands in the event history.""" + with app.app_context(): + user = authorized_user + ua = InternalClient(name="test_client_finalize_oversize") + sid = sub_metadata.submission_id + + oversize = _make_oversize(user, sid) + assert oversize.is_oversize is True + + submission, events = current_app.api.save( + FinalizeSubmission(creator=user, client=ua), submission_id=sid) + + assert submission.status == Submission.SUBMITTED + holds = [h for h in submission.holds.values() + if h.hold_type == Hold.Type.SOURCE_OVERSIZE] + assert len(holds) == 1 + assert submission.is_on_hold is True + + # The consequence is persisted as a real event in the history. + _, history = current_app.api.get_with_history(str(sid)) + assert any(isinstance(e, AddHold) for e in history) + + +def test_finalize_normal_no_hold(app, authorized_user, sub_metadata): + """A normal-size submission finalizes without a hold.""" + with app.app_context(): + user = authorized_user + ua = InternalClient(name="test_client_finalize_oversize") + submission, _ = current_app.api.save( + FinalizeSubmission(creator=user, client=ua), + submission_id=sub_metadata.submission_id) + assert submission.is_oversize is False + assert submission.holds == {} + assert submission.is_on_hold is False From d15c171f5568a578c46bf4dc0e2047dc934f0d18 Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Fri, 12 Jun 2026 14:12:01 -0400 Subject: [PATCH 3/6] Changes to size_limits.py and oversize - Moved default size limits to SIZE_LIMIT_POLICY for more visibility - Changed to all bytes instead of kb - Initial change to oversize message - Saved `OversizeReason` instances on upload `Event` - Changes check_sizes to not have a default `limits` to avoid incorrect use - Changed UploadFiles and UploadArchive to only handle added files for size limit check. - Updated tests. Claude assisted --- submit_ce/api/submit.py | 4 +- submit_ce/domain/event/file.py | 63 ++++--- .../event/tests/test_oversize_detection.py | 89 +++++++--- submit_ce/domain/size_limits.py | 156 ++++++------------ submit_ce/domain/tests/test_size_limits.py | 100 ++++------- .../legacy_implementation/flask_impl.py | 13 -- submit_ce/ui/config.py | 12 -- submit_ce/ui/conftest.py | 1 + submit_ce/ui/controllers/new/upload.py | 18 +- submit_ce/ui/filters/__init__.py | 22 ++- .../ui/tests/test_finalize_oversize_hold.py | 3 +- submit_ce/ui/tests/test_oversize_upload.py | 5 +- 12 files changed, 233 insertions(+), 253 deletions(-) diff --git a/submit_ce/api/submit.py b/submit_ce/api/submit.py index 364fc11b..cda8af5d 100644 --- a/submit_ce/api/submit.py +++ b/submit_ce/api/submit.py @@ -65,7 +65,7 @@ from submit_ce.api.compile_service import CompileService from submit_ce.domain import Submission, Event, License -from submit_ce.domain.size_limits import SizeLimits +from submit_ce.domain.size_limits import SIZE_LIMIT_POLICY, SizeLimits from submit_ce.api.file_store import SubmissionFileStore @@ -77,7 +77,7 @@ def get_size_limits(self) -> SizeLimits: Defaults to the built-in 50 MB limits. Implementations with access to application config (e.g. the Flask implementation) override this to honor the configured ``MAX_*_KB`` values.""" - return SizeLimits.defaults() + return SIZE_LIMIT_POLICY @abstractmethod def get(self, submission_id: str) -> Submission: diff --git a/submit_ce/domain/event/file.py b/submit_ce/domain/event/file.py index b6a1c10b..50547b09 100644 --- a/submit_ce/domain/event/file.py +++ b/submit_ce/domain/event/file.py @@ -2,13 +2,15 @@ from pydantic import ConfigDict, Field, WithJsonSchema from typing import TYPE_CHECKING, List, Annotated, Optional +from submit_ce.domain.exceptions import InvalidEvent + if TYPE_CHECKING: from submit_ce.api.submit import SubmitApi from . import validators from .base import EventWithSideEffect from ..submission import Submission -from ..uploads import SubmitFile +from ..uploads import FileStatus, SubmitFile from .. import size_limits import logging @@ -26,26 +28,39 @@ def _common_file_change_execute(api: SubmitApi, submission: Submission) -> None: file_store.delete_preview(str(submission.submission_id)) -def _evaluate_oversize(api: SubmitApi, submission: Submission) -> bool: +def _add_evaluate_oversize(api: SubmitApi, + submission: Submission, + bytes_added:int, + files: list[FileStatus], + ) -> list[size_limits.OversizeReason]: + """Figure out if any oversize problems due to file additions.""" + per_file = {file.path: file.bytes for file in files} + total = submission.uncompressed_size + bytes_added + category = (submission.primary_classification.category + if submission.primary_classification else None) + return size_limits.check_sizes(total, per_file, primary_category=category, + limits=api.get_size_limits()) + + +def _workspace_evaluate_oversize(api: SubmitApi, submission: Submission) -> list[size_limits.OversizeReason]: """Measure the current workspace against the configured size limits. - Called from ``execute`` (which has file-store access). The boolean result - is stored on the event so ``project`` can apply it deterministically on - replay, when ``execute`` does not run. Reads the authoritative post-change - workspace so the flag reflects *all* current files, not just the ones this - event touched. + Reads the authoritative post-change workspace so the flag reflects *all* + current files, not just the ones this event touched. This causes more api + requests than `_add_evaluate_oversize` """ workspace = api.get_file_store().get_workspace(str(submission.submission_id)) if workspace is None: - return False + return [] per_file = {file.path: file.bytes for file in workspace.files} total = workspace.size or 0 category = (submission.primary_classification.category if submission.primary_classification else None) - return size_limits.is_oversize(total, per_file, primary_category=category, + return size_limits.check_sizes(total, per_file, primary_category=category, limits=api.get_size_limits()) + class UploadArchive(EventWithSideEffect): """Uploads a zip or tgz file to the workspace, unpacking all the files.""" @@ -58,22 +73,26 @@ class UploadArchive(EventWithSideEffect): bytes_added: int = 0 """Bytes added by uploading this archive.""" - oversize: bool = False - """Whether the submission is oversize after this change (set in execute).""" + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) + if not self.file: + raise InvalidEvent(self, "Must upload a file") def execute(self, api: SubmitApi, submission: Submission) -> None: """Upload the new files using the file store.""" + if not self.file: + raise RuntimeError("File must be set") files = api.get_file_store().store_source_package(str(submission.submission_id), self.file, 4098) self.bytes_added = sum([file.bytes for file in files]) _common_file_change_execute(api, submission) - self.oversize = _evaluate_oversize(api, submission) + self.oversize = _add_evaluate_oversize(api, submission, self.bytes_added, files) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added - submission.is_oversize = self.oversize + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission @@ -92,8 +111,8 @@ class UploadFiles(EventWithSideEffect): Field(default_factory=list, exclude=True) bytes_added: int = 0 - oversize: bool = False - """Whether the submission is oversize after this change (set in execute).""" + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) @@ -101,15 +120,17 @@ def validate(self, submission: Submission) -> None: def execute(self, api: SubmitApi, submission: Submission) -> None: """Upload the new files using the file store.""" file_store = api.get_file_store() + stats = [] for f in self.files: stat=file_store.store_source_file(str(submission.submission_id), f, chunk_size=4096) + stats.append(stat) self.bytes_added += stat.bytes _common_file_change_execute(api, submission) - self.oversize = _evaluate_oversize(api, submission) + self.oversize = _add_evaluate_oversize(api, submission, self.bytes_added, stats) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added - submission.is_oversize = self.oversize + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission @@ -127,8 +148,8 @@ class RemoveFiles(EventWithSideEffect): bytes_removed:int = 0 """Bytes removed by removing these files.""" - oversize: bool = False - """Whether the submission is oversize after this change (set in execute).""" + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" def validate(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) @@ -143,11 +164,11 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: self.bytes_removed += file.bytes _common_file_change_execute(api, submission) - self.oversize = _evaluate_oversize(api, submission) + self.oversize = _workspace_evaluate_oversize(api, submission) def project(self, submission: Submission) -> Submission: submission.uncompressed_size -= self.bytes_removed - submission.is_oversize = self.oversize + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission diff --git a/submit_ce/domain/event/tests/test_oversize_detection.py b/submit_ce/domain/event/tests/test_oversize_detection.py index e3a0e6a9..252ebb22 100644 --- a/submit_ce/domain/event/tests/test_oversize_detection.py +++ b/submit_ce/domain/event/tests/test_oversize_detection.py @@ -4,6 +4,7 @@ no Flask app or real file store is needed. """ +import io from datetime import datetime from types import SimpleNamespace @@ -17,20 +18,28 @@ RemoveFiles, RemoveAllFiles, ) -from submit_ce.domain.size_limits import SizeLimits +from submit_ce.domain.size_limits import ( + SIZE_LIMIT_POLICY, + SizeLimits, + OversizeReason, +) MB = 1024 * 1024 class _FakeStore: - def __init__(self, workspace): + def __init__(self, workspace=None, unpacked=None): self.workspace = workspace + self.unpacked = unpacked or [] def store_source_package(self, sid, content, chunk_size): - return [] + # The unpacked FileStatus list is configured per-test. + return list(self.unpacked) def store_source_file(self, sid, content, chunk_size): - return SimpleNamespace(bytes=0, path=getattr(content, "filename", "f")) + # Echo the uploaded content's size so the file-delta size check sees a + # real per-file/total contribution. + return SimpleNamespace(bytes=content.bytes, path=content.filename) def delete_source_file(self, sid, name): return None @@ -49,9 +58,9 @@ def delete_preview(self, sid): class _FakeApi: - def __init__(self, workspace, limits=None): - self._store = _FakeStore(workspace) - self._limits = limits or SizeLimits.defaults() + def __init__(self, workspace=None, limits=None, unpacked=None): + self._store = _FakeStore(workspace, unpacked) + self._limits = limits or SIZE_LIMIT_POLICY def get_file_store(self): return self._store @@ -66,6 +75,18 @@ def _ws(total, per_file=None): return SimpleNamespace(size=total, files=files) +def _upload(name, size): + """An incoming upload object, as handed to ``UploadFiles.files``.""" + return SimpleNamespace(filename=name, bytes=size, + content_type="application/pdf", + stream=io.BytesIO(b"%PDF-1.4\n%%EOF\n")) + + +def _stat(path, size): + """A stored-file status, as returned by the file store.""" + return SimpleNamespace(path=path, bytes=size) + + def _user(uid="u1"): return agent.PublicUser(name="Test User", user_id=uid, email=f"{uid}@example.org", endorsements=[]) @@ -83,19 +104,42 @@ def test_submission_defaults_not_oversize(): def test_upload_files_flags_oversize(): + # Per-file limit below the total limit so one big file isolates PER_FILE. + limits = SizeLimits( + max_uncompressed_total={"default": 200 * MB}, + max_uncompressed_per_file={"default": 50 * MB}, + max_compressed={"default": 200 * MB}, + ) s = _submission() - api = _FakeApi(_ws(60 * MB, {"huge.pdf": 60 * MB})) - e = UploadFiles(creator=s.creator, files=[]) + api = _FakeApi(limits=limits) + e = UploadFiles(creator=s.creator, files=[_upload("huge.pdf", 60 * MB)]) e.execute(api, s) - assert e.oversize is True + assert len(e.oversize) == 1 + assert e.oversize[0].kind == "PER_FILE" s = e.project(s) assert s.is_oversize is True +def test_upload_files_total_trips_via_accumulation(): + # The new file is under the per-file limit, but pushes the running total + # (prior uncompressed_size + bytes_added) over the total limit. This is the + # case that proves detection uses the file delta, not the workspace. + s = _submission() + s.uncompressed_size = 40 * MB + api = _FakeApi() # default 50 MB limits + e = UploadFiles(creator=s.creator, files=[_upload("more.pdf", 20 * MB)]) + e.execute(api, s) + assert len(e.oversize) == 1 + assert e.oversize[0].kind == "TOTAL" + s = e.project(s) + assert s.is_oversize is True + assert s.uncompressed_size == 60 * MB + + def test_upload_files_within_limit_not_oversize(): s = _submission() - api = _FakeApi(_ws(10 * MB, {"ok.pdf": 10 * MB})) - e = UploadFiles(creator=s.creator, files=[]) + api = _FakeApi() + e = UploadFiles(creator=s.creator, files=[_upload("ok.pdf", 10 * MB)]) e.execute(api, s) s = e.project(s) assert s.is_oversize is False @@ -103,9 +147,11 @@ def test_upload_files_within_limit_not_oversize(): def test_upload_archive_flags_oversize(): s = _submission() - api = _FakeApi(_ws(80 * MB, {"a.tex": 80 * MB})) - e = UploadArchive(creator=s.creator, file=None) + api = _FakeApi(unpacked=[_stat("a.tex", 80 * MB)]) + e = UploadArchive(creator=s.creator) + e.file = _upload("a.tgz", 80 * MB) # truthy; content ignored by the fake store e.execute(api, s) + assert e.oversize s = e.project(s) assert s.is_oversize is True @@ -129,9 +175,11 @@ def test_remove_all_files_clears_oversize(): def test_project_uses_persisted_flag_on_replay(): - # On replay execute() does not run; the flag comes from the stored event. + # On replay execute() does not run; the reasons come from the stored event. s = _submission() - e = UploadFiles(creator=s.creator, files=[], oversize=True) + reasons = [OversizeReason(kind="TOTAL", limit_bytes=50 * MB, + actual_bytes=60 * MB)] + e = UploadFiles(creator=s.creator, files=[], oversize=reasons) s = e.project(s) assert s.is_oversize is True @@ -141,7 +189,7 @@ def test_no_workspace_is_not_oversize(): api = _FakeApi(None) e = UploadFiles(creator=s.creator, files=[]) e.execute(api, s) - assert e.oversize is False + assert not e.oversize def test_per_archive_limit_used_in_event(): @@ -151,7 +199,8 @@ def test_per_archive_limit_used_in_event(): max_uncompressed_per_file={"default": 100 * MB}, max_compressed={"default": 100 * MB}, ) - api = _FakeApi(_ws(10 * MB, {"f": 10 * MB}), limits=limits) - e = UploadFiles(creator=s.creator, files=[]) + api = _FakeApi(limits=limits) + e = UploadFiles(creator=s.creator, files=[_upload("f", 10 * MB)]) e.execute(api, s) - assert e.oversize is True # 10 MB exceeds the 5 MB astro-ph total limit + assert e.oversize # 10 MB exceeds the 5 MB astro-ph total limit + assert e.oversize[0].kind == "TOTAL" diff --git a/submit_ce/domain/size_limits.py b/submit_ce/domain/size_limits.py index d9207d15..211f8da9 100644 --- a/submit_ce/domain/size_limits.py +++ b/submit_ce/domain/size_limits.py @@ -5,69 +5,37 @@ numbers can be tuned without touching the checking logic. There are three independent limits, each a mapping keyed by archive with a -``'default'`` fallback (matching legacy, where only ``'default'`` is -populated but per-archive overrides are supported): +``'default'`` fallback. * ``max_uncompressed_total`` -- sum of all extracted files * ``max_uncompressed_per_file`` -- any single extracted file * ``max_compressed`` -- size of the compressed upload -All limits default to 50,000 KB (50 MB), the legacy arXiv size guideline. +All limits default to 50 MB, the legacy arXiv size guideline. Only the total and per-file uncompressed limits are *enforced* by -:func:`check_sizes` (matching legacy ``check_sizes``); ``max_compressed`` is -defined for completeness and future tuning but is not currently checked. - -Values are handled in **bytes** internally (matching -:attr:`submit_ce.domain.submission.Submission.uncompressed_size` and -:attr:`submit_ce.domain.uploads.FileStatus.bytes`); the documented defaults -are expressed in KB to mirror the legacy module. - -This module is pure domain code and must not depend on Flask. The two -environment escape hatches below mirror the legacy ``OVERRIDE``/``MAXSIZE`` -knobs and are read at call time so tests can raise the limits without -reconfiguring the app. +:func:`check_sizes`; ``max_compressed`` is defined for completeness and future +tuning but is not currently checked. + +Values are handled in **bytes** internally. + +This module is pure domain code and must not depend on Flask. """ -import os from dataclasses import dataclass -from typing import List, Mapping, Optional +from typing import List, Literal, Mapping, Optional + +from pydantic import BaseModel, ConfigDict from arxiv.taxonomy.definitions import CATEGORIES -ONE_KB = 1024 -DEFAULT_MAX_SIZE_KB = 50_000 -"""Legacy arXiv size guideline: 50,000 KB (50 MB).""" +ONE_MB = 1024 * 1024 -DEFAULT_MAX_SIZE_BYTES = DEFAULT_MAX_SIZE_KB * ONE_KB +DEFAULT_MAX_SIZE_BYTES = 50 * ONE_MB DEFAULT_ARCHIVE = "default" """Fallback key used when a category has no archive-specific override.""" -OVERRIDE_ENV = "SUBMIT_OVERSIZE_OVERRIDE" -"""When truthy, doubles every limit (port of legacy ``OVERRIDE``).""" - -MAXSIZE_ENV = "SUBMIT_OVERSIZE_MAXSIZE_KB" -"""When set to a number of KB, raises every limit to at least that value -(port of legacy ``MAXSIZE``).""" - - -def _truthy(value: Optional[str]) -> bool: - return bool(value) and value.strip().lower() not in ("0", "false", "no", "") - - -def _apply_env_escapes(base_bytes: int) -> int: - """Apply the ``MAXSIZE``/``OVERRIDE`` escape hatches to a limit.""" - maxsize = os.environ.get(MAXSIZE_ENV) - if maxsize: - try: - base_bytes = max(base_bytes, int(maxsize) * ONE_KB) - except ValueError: - pass - if _truthy(os.environ.get(OVERRIDE_ENV)): - base_bytes *= 2 - return base_bytes - def archive_for_category(category: Optional[str]) -> str: """Return the archive key for a category, or ``'default'`` if unknown.""" @@ -88,26 +56,6 @@ class SizeLimits: max_uncompressed_per_file: Mapping[str, int] max_compressed: Mapping[str, int] - @classmethod - def defaults(cls) -> "SizeLimits": - """Limits at the legacy default (50 MB), with env escapes applied.""" - return cls.from_kb(DEFAULT_MAX_SIZE_KB, - DEFAULT_MAX_SIZE_KB, - DEFAULT_MAX_SIZE_KB) - - @classmethod - def from_kb(cls, total_kb: int, per_file_kb: int, - compressed_kb: int) -> "SizeLimits": - """Build limits from KB values (e.g. from config), applying escapes.""" - return cls( - max_uncompressed_total={ - DEFAULT_ARCHIVE: _apply_env_escapes(total_kb * ONE_KB)}, - max_uncompressed_per_file={ - DEFAULT_ARCHIVE: _apply_env_escapes(per_file_kb * ONE_KB)}, - max_compressed={ - DEFAULT_ARCHIVE: _apply_env_escapes(compressed_kb * ONE_KB)}, - ) - @staticmethod def _lookup(table: Mapping[str, int], category: Optional[str]) -> int: return table.get(archive_for_category(category), table[DEFAULT_ARCHIVE]) @@ -122,32 +70,44 @@ def compressed_limit(self, category: Optional[str] = None) -> int: return self._lookup(self.max_compressed, category) -@dataclass(frozen=True) -class OversizeReason: - """One reason a submission is oversize, with human-readable text.""" +def _mb(num_bytes: int) -> str: + return f"{num_bytes / ONE_MB:.1f} MB" + + +class OversizeReason(BaseModel): + """One reason a submission is oversize. - kind: str - """``'total'`` or ``'per_file'``.""" + The human-readable text is built on demand by :meth:`message` from the + structured fields, so it is never stored or serialized.""" - message: str - limit: int + model_config = ConfigDict(frozen=True) + + kind: Literal["TOTAL", "PER_FILE"] + """Which limit was exceeded.""" + + limit_bytes: int """The limit that was exceeded, in bytes.""" - actual: int + actual_bytes: int """The measured size, in bytes.""" path: Optional[str] = None - """The offending file, for ``'per_file'`` reasons.""" + """The offending file, for ``'PER_FILE'`` reasons.""" - -def _mb(num_bytes: int) -> str: - return f"{num_bytes / (ONE_KB * ONE_KB):.1f} MB" + def message(self) -> str: + """Build the human-readable description from this reason's data.""" + if self.kind == "PER_FILE": + return (f"File '{self.path}' is {_mb(self.actual_bytes)}, exceeding " + f"the {_mb(self.limit_bytes)} per-file limit.") + return (f"Total uncompressed size {_mb(self.actual_bytes)} " + f"exceeds the {_mb(self.limit_bytes)} limit.") def check_sizes(total_uncompressed: int, per_file_sizes: Optional[Mapping[str, int]] = None, primary_category: Optional[str] = None, - limits: Optional[SizeLimits] = None) -> List[OversizeReason]: + *, + limits: SizeLimits) -> List[OversizeReason]: """Return the reasons a submission is oversize, or ``[]`` if within limits. Enforces the total-uncompressed and per-file-uncompressed limits, matching @@ -163,45 +123,37 @@ def check_sizes(total_uncompressed: int, primary_category Primary classification category, used to select per-archive limits. limits - Limits to enforce. Defaults to :meth:`SizeLimits.defaults`. + Limits to enforce. Required and keyword-only, so callers must pass the + authoritative limits (e.g. ``SubmitApi.get_size_limits()``) rather than + silently falling back to a default. Pass ``SIZE_LIMIT_POLICY`` for the + built-in 50 MB policy. """ - limits = limits or SizeLimits.defaults() per_file_sizes = per_file_sizes or {} reasons: List[OversizeReason] = [] total_limit = limits.total_limit(primary_category) if total_uncompressed > total_limit: reasons.append(OversizeReason( - kind="total", - message=(f"Total uncompressed size {_mb(total_uncompressed)} " - f"exceeds the {_mb(total_limit)} limit."), - limit=total_limit, - actual=total_uncompressed, + kind="TOTAL", + limit_bytes=total_limit, + actual_bytes=total_uncompressed, )) per_file_limit = limits.per_file_limit(primary_category) for path, size in per_file_sizes.items(): if size > per_file_limit: reasons.append(OversizeReason( - kind="per_file", - message=(f"File '{path}' is {_mb(size)}, exceeding the " - f"{_mb(per_file_limit)} per-file limit."), - limit=per_file_limit, - actual=size, + kind="PER_FILE", + limit_bytes=per_file_limit, + actual_bytes=size, path=path, )) return reasons -def is_oversize(total_uncompressed: int, - per_file_sizes: Optional[Mapping[str, int]] = None, - primary_category: Optional[str] = None, - limits: Optional[SizeLimits] = None) -> bool: - """Return ``True`` if the submission exceeds any enforced limit.""" - return bool(check_sizes(total_uncompressed, per_file_sizes, - primary_category, limits)) - - -def summarize(reasons: List[OversizeReason]) -> str: - """Join reason messages into a single human-readable warning string.""" - return " ".join(reason.message for reason in reasons) +SIZE_LIMIT_POLICY = SizeLimits( + max_uncompressed_total={"default": 50 * ONE_MB}, + max_uncompressed_per_file={"default": 50 * ONE_MB}, + max_compressed={"default": 50 * ONE_MB}, +) +"""This is the current in effect size limit policy for the app.""" diff --git a/submit_ce/domain/tests/test_size_limits.py b/submit_ce/domain/tests/test_size_limits.py index 3c454bbf..bff798c5 100644 --- a/submit_ce/domain/tests/test_size_limits.py +++ b/submit_ce/domain/tests/test_size_limits.py @@ -1,39 +1,39 @@ """Tests for the size-limit policy module (:mod:`submit_ce.domain.size_limits`).""" -from submit_ce.domain import size_limits as sl from submit_ce.domain.size_limits import ( - ONE_KB, - DEFAULT_MAX_SIZE_KB, + ONE_MB, DEFAULT_MAX_SIZE_BYTES, SizeLimits, + SIZE_LIMIT_POLICY, archive_for_category, check_sizes, - is_oversize, - summarize, ) -MB = ONE_KB * ONE_KB +MB = ONE_MB # --- limit values & lookups ------------------------------------------------- def test_default_limit_is_50mb(): - assert DEFAULT_MAX_SIZE_KB == 50_000 - assert DEFAULT_MAX_SIZE_BYTES == 50_000 * ONE_KB + assert DEFAULT_MAX_SIZE_BYTES == 50 * ONE_MB def test_defaults_populate_all_three_limits(): - limits = SizeLimits.defaults() + limits = SIZE_LIMIT_POLICY assert limits.total_limit() == DEFAULT_MAX_SIZE_BYTES assert limits.per_file_limit() == DEFAULT_MAX_SIZE_BYTES assert limits.compressed_limit() == DEFAULT_MAX_SIZE_BYTES -def test_from_kb_converts_to_bytes_independently(): - limits = SizeLimits.from_kb(total_kb=10, per_file_kb=20, compressed_kb=30) - assert limits.total_limit() == 10 * ONE_KB - assert limits.per_file_limit() == 20 * ONE_KB - assert limits.compressed_limit() == 30 * ONE_KB +def test_constructor_sets_each_limit_independently(): + limits = SizeLimits( + max_uncompressed_total={"default": 10 * MB}, + max_uncompressed_per_file={"default": 20 * MB}, + max_compressed={"default": 30 * MB}, + ) + assert limits.total_limit() == 10 * MB + assert limits.per_file_limit() == 20 * MB + assert limits.compressed_limit() == 30 * MB def test_archive_for_category(): @@ -60,59 +60,61 @@ def test_per_archive_override_falls_back_to_default(): # --- check_sizes ------------------------------------------------------------ def test_within_limits_returns_no_reasons(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + limits = SIZE_LIMIT_POLICY reasons = check_sizes(total_uncompressed=10 * MB, per_file_sizes={"a.tex": 1 * MB, "b.png": 5 * MB}, limits=limits) assert reasons == [] - assert is_oversize(10 * MB, {"a.tex": 1 * MB}, limits=limits) is False def test_total_over_limit_flags_total(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + limits = SIZE_LIMIT_POLICY reasons = check_sizes(total_uncompressed=60 * MB, limits=limits) assert len(reasons) == 1 - assert reasons[0].kind == "total" - assert reasons[0].actual == 60 * MB - assert reasons[0].limit == DEFAULT_MAX_SIZE_BYTES - assert is_oversize(60 * MB, limits=limits) is True + assert reasons[0].kind == "TOTAL" + assert reasons[0].actual_bytes == 60 * MB + assert reasons[0].limit_bytes == DEFAULT_MAX_SIZE_BYTES def test_per_file_over_limit_flags_each_file(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + limits = SIZE_LIMIT_POLICY reasons = check_sizes( total_uncompressed=10 * MB, # total is fine per_file_sizes={"small.tex": 1 * MB, "huge.dat": 60 * MB}, limits=limits, ) assert len(reasons) == 1 - assert reasons[0].kind == "per_file" + assert reasons[0].kind == "PER_FILE" assert reasons[0].path == "huge.dat" - assert reasons[0].actual == 60 * MB + assert reasons[0].actual_bytes == 60 * MB def test_both_dimensions_can_trip(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + limits = SIZE_LIMIT_POLICY reasons = check_sizes( total_uncompressed=120 * MB, per_file_sizes={"huge.dat": 60 * MB}, limits=limits, ) kinds = sorted(r.kind for r in reasons) - assert kinds == ["per_file", "total"] + assert kinds == ["PER_FILE", "TOTAL"] def test_exactly_at_limit_is_not_oversize(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) + limits = SIZE_LIMIT_POLICY assert check_sizes(DEFAULT_MAX_SIZE_BYTES, {"f": DEFAULT_MAX_SIZE_BYTES}, limits=limits) == [] def test_compressed_limit_is_not_enforced(): - # A tiny per-file/total but a defined compressed limit: check_sizes only + # A tiny compressed limit but generous total/per-file: check_sizes only # looks at total + per-file, never compressed. - limits = SizeLimits.from_kb(50_000, 50_000, compressed_kb=1) + limits = SizeLimits( + max_uncompressed_total={"default": 50 * MB}, + max_uncompressed_per_file={"default": 50 * MB}, + max_compressed={"default": 1}, + ) assert check_sizes(10 * MB, {"a": 1 * MB}, limits=limits) == [] @@ -123,41 +125,5 @@ def test_per_archive_limit_used_in_check(): max_compressed={"default": 100 * MB}, ) # 10MB is over the 5MB astro-ph limit but under the 100MB default. - assert is_oversize(10 * MB, primary_category="astro-ph.GA", limits=limits) - assert not is_oversize(10 * MB, primary_category="math.GT", limits=limits) - - -def test_summarize_joins_messages(): - limits = SizeLimits.from_kb(50_000, 50_000, 50_000) - reasons = check_sizes(120 * MB, {"huge.dat": 60 * MB}, limits=limits) - text = summarize(reasons) - assert "Total uncompressed size" in text - assert "huge.dat" in text - - -# --- environment escape hatches --------------------------------------------- - -def test_override_env_doubles_limits(monkeypatch): - monkeypatch.setenv(sl.OVERRIDE_ENV, "1") - limits = SizeLimits.defaults() - assert limits.total_limit() == 2 * DEFAULT_MAX_SIZE_BYTES - - -def test_override_env_falsey_does_nothing(monkeypatch): - monkeypatch.setenv(sl.OVERRIDE_ENV, "0") - assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES - - -def test_maxsize_env_raises_limits(monkeypatch): - monkeypatch.setenv(sl.MAXSIZE_ENV, "200000") # 200,000 KB - assert SizeLimits.defaults().total_limit() == 200_000 * ONE_KB - - -def test_maxsize_env_only_raises_never_lowers(monkeypatch): - monkeypatch.setenv(sl.MAXSIZE_ENV, "1") # below the default - assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES - - -def test_maxsize_env_ignores_garbage(monkeypatch): - monkeypatch.setenv(sl.MAXSIZE_ENV, "not-a-number") - assert SizeLimits.defaults().total_limit() == DEFAULT_MAX_SIZE_BYTES + assert check_sizes(10 * MB, primary_category="astro-ph.GA", limits=limits) + assert check_sizes(10 * MB, primary_category="math.GT", limits=limits) == [] diff --git a/submit_ce/implementations/legacy_implementation/flask_impl.py b/submit_ce/implementations/legacy_implementation/flask_impl.py index 83cb603a..d3c6383d 100644 --- a/submit_ce/implementations/legacy_implementation/flask_impl.py +++ b/submit_ce/implementations/legacy_implementation/flask_impl.py @@ -1,10 +1,8 @@ from arxiv.db import Session -from flask import current_app, has_app_context from . import LegacySubmitImplementation from sqlalchemy.orm import Session as SqlalchemySession -from submit_ce.domain.size_limits import SizeLimits, DEFAULT_MAX_SIZE_KB from submit_ce.implementations.compile.compile_api_service import CompileApiService def flask_get_session() -> SqlalchemySession: @@ -23,14 +21,3 @@ def __init__(self, compiler = compiler or CompileApiService() super().__init__(store=store, compiler=compiler) self.get_session = flask_get_session - - def get_size_limits(self) -> SizeLimits: - """Build size limits from the configured ``MAX_*_KB`` values.""" - if not has_app_context(): - return SizeLimits.defaults() - cfg = current_app.config - return SizeLimits.from_kb( - cfg.get("MAX_UNCOMPRESSED_TOTAL_KB", DEFAULT_MAX_SIZE_KB), - cfg.get("MAX_UNCOMPRESSED_PER_FILE_KB", DEFAULT_MAX_SIZE_KB), - cfg.get("MAX_COMPRESSED_KB", DEFAULT_MAX_SIZE_KB), - ) diff --git a/submit_ce/ui/config.py b/submit_ce/ui/config.py index c2aab247..6e9e8662 100644 --- a/submit_ce/ui/config.py +++ b/submit_ce/ui/config.py @@ -99,18 +99,6 @@ def __init__(self, **kwargs): """If true, only admin users can use the system. Intended to allowe closed to the public dev or beta system.""" - MAX_UNCOMPRESSED_TOTAL_KB: int = 50_000 - """Max total uncompressed submission size, in KB, before the submission is - flagged oversize (default 50 MB; the legacy arXiv size guideline).""" - - MAX_UNCOMPRESSED_PER_FILE_KB: int = 50_000 - """Max uncompressed size of any single file, in KB, before the submission - is flagged oversize (default 50 MB).""" - - MAX_COMPRESSED_KB: int = 50_000 - """Max compressed upload size, in KB. Defined for completeness; not - currently enforced (matches legacy check_sizes).""" - COMPILE_API_URL: str = "https://tex2pdf-api-default-874717964009.us-central1.run.app" """The tex2pdf-api url. Do not end with a /. diff --git a/submit_ce/ui/conftest.py b/submit_ce/ui/conftest.py index 76554428..89f167bc 100644 --- a/submit_ce/ui/conftest.py +++ b/submit_ce/ui/conftest.py @@ -322,6 +322,7 @@ class _FakePdf: fake_stat = MagicMock() fake_stat.bytes = big + fake_stat.path = "huge.pdf" mock_store = MagicMock() mock_store.store_source_file.return_value = fake_stat diff --git a/submit_ce/ui/controllers/new/upload.py b/submit_ce/ui/controllers/new/upload.py index 082e87d0..25e46f58 100644 --- a/submit_ce/ui/controllers/new/upload.py +++ b/submit_ce/ui/controllers/new/upload.py @@ -21,7 +21,6 @@ from flask import current_app from arxiv.auth.domain import Session from arxiv.base import alerts -from arxiv.base.filters import tidy_filesize from arxiv.forms import csrf from markupsafe import Markup from werkzeug.datastructures import FileStorage @@ -39,7 +38,9 @@ from submit_ce.domain.submission import Submission from submit_ce.domain.uploads import SourceFormat from submit_ce.domain.uploads import Workspace, FileStatus, UploadStatus, is_file_tgz, is_file_zip +from submit_ce.domain import size_limits +from submit_ce.ui.filters import iec_filesize from submit_ce.ui.auth import user_and_client_from_session from submit_ce.ui.controllers.util import add_immediate_alert, validate_command from submit_ce.ui.routes.flow_control import ready_for_next, stay_on_this_stage @@ -260,11 +261,12 @@ def _flash_oversize_warning(submission: Submission) -> None: if not submission.is_oversize: return alerts.flash_warning( - Markup( - 'This submission exceeds the arXiv size guideline. You can still ' - 'submit, but it will be placed on hold for moderator review. See ' - 'arxiv.org/help/sizes for ways to reduce ' - 'the size, or to request a size exception.'), + Markup(f'This submission exceeds the {iec_filesize(size_limits.DEFAULT_MAX_SIZE_BYTES)} arXiv ' + 'size guideline.' + 'Please consider reducing the size of the files to ensure your paper can be accessed by readers. ' + 'If the size of the files are necessary to present the work then please continue with ' + 'the submission steps and click the "Process" button. ' + 'For more information, please read about Oversized Submissions.'), title='Submission is oversize') @@ -277,7 +279,7 @@ def _upload_archive(form: AddfilesForm, file: FileStorage, validate_command(form, command, submission, 'file') # raises on invalid submission, _ = current_app.api.save(command, submission_id=submission.submission_id) workspace = current_app.api.get_file_store().get_workspace(submission_id=str(submission.submission_id)) - converted_size = tidy_filesize(workspace.size) + converted_size = iec_filesize(workspace.size) inferred = _infer_source_format(workspace.files) if submission.source_format != inferred: @@ -320,7 +322,7 @@ def _upload_files(form: AddfilesForm, file: FileStorage, validate_command(form, command, submission, 'file') submission, _ = current_app.api.save(command, submission_id=submission.submission_id) workspace = current_app.api.get_file_store().get_workspace(submission_id=str(submission.submission_id)) - converted_size = tidy_filesize(workspace.size) + converted_size = iec_filesize(workspace.size) inferred = _infer_source_format(workspace.files) if submission.source_format != inferred: diff --git a/submit_ce/ui/filters/__init__.py b/submit_ce/ui/filters/__init__.py index 34407feb..5cf2989a 100644 --- a/submit_ce/ui/filters/__init__.py +++ b/submit_ce/ui/filters/__init__.py @@ -5,17 +5,15 @@ from datetime import datetime from locale import strxfrm from pathlib import Path -from typing import List, Tuple, Callable +from typing import List, Tuple, Callable, Union from arxiv import taxonomy from pytz import UTC from submit_ce.domain.process import ProcessStatus from submit_ce.domain.uploads import FileStatus -from submit_ce.ui.controllers.new.upload import group_files, tidy_filesize -from .tex_filters import compilation_log_display from submit_ce.domain.compilation import Compilation - +from .tex_filters import compilation_log_display # additions for compilation log markup @@ -150,8 +148,22 @@ def pluralize(number, singular="", plural="s"): else: return plural + +def iec_filesize(bytes: Union[int, float]) -> str: + """Returns `bytes` as a IEC binary prefixed `str`.""" + units = ["B", "KiB", "MiB", "GiB", "TiB"] + if bytes == 0: + return "0B" + i = 0 + while bytes >= 1024 and i < len(units) - 1: + bytes /= 1024 + i += 1 + return f"{bytes:.2f} {units[i]}" + + def get_filters() -> List[Tuple[str, Callable]]: """Get the filter functions available in this module.""" + from submit_ce.ui.controllers.new.upload import group_files return [ ('group_files', group_files), ('timesince', timesince), @@ -159,7 +171,7 @@ def get_filters() -> List[Tuple[str, Callable]]: ('get_category_name', get_category_name), ('process_status_display', process_status_display), ('compilation_status_display', compilation_status_display), - ('tidy_filesize', tidy_filesize), + ('iec_filesize', iec_filesize), # FYI tidy_filesize is SI units which confuses users ('asdict', asdict), ('compilation_log_display', compilation_log_display), ('pluralize', pluralize), diff --git a/submit_ce/ui/tests/test_finalize_oversize_hold.py b/submit_ce/ui/tests/test_finalize_oversize_hold.py index 5ab33bb1..0c3a8a9c 100644 --- a/submit_ce/ui/tests/test_finalize_oversize_hold.py +++ b/submit_ce/ui/tests/test_finalize_oversize_hold.py @@ -19,7 +19,7 @@ def _make_oversize(user, submission_id): - """Re-upload with a workspace over the size limit to flip is_oversize.""" + """Re-upload an over-limit file to flip is_oversize via the file delta.""" ua = InternalClient(name="test_client_finalize_oversize") big = 60 * 1024 * 1024 # over the 50 MB default limit @@ -30,6 +30,7 @@ class _FakePdf: fake_stat = MagicMock() fake_stat.bytes = big + fake_stat.path = "huge.pdf" mock_store = MagicMock() mock_store.store_source_file.return_value = fake_stat big_ws = MagicMock() diff --git a/submit_ce/ui/tests/test_oversize_upload.py b/submit_ce/ui/tests/test_oversize_upload.py index 2e12f4f0..3ef99d92 100644 --- a/submit_ce/ui/tests/test_oversize_upload.py +++ b/submit_ce/ui/tests/test_oversize_upload.py @@ -21,9 +21,10 @@ def test_normal_upload_not_oversize(sub_files): def test_oversize_flag_persists_on_event(app, sub_files_oversize): - """The event's oversize flag round-trips through the event history.""" + """The event's oversize reasons round-trip through the event history.""" with app.app_context(): _, history = current_app.api.get_with_history( str(sub_files_oversize.submission_id)) uploads = [e for e in history if isinstance(e, UploadFiles)] - assert uploads and uploads[-1].oversize is True + assert uploads and uploads[-1].oversize + assert uploads[-1].oversize[0].kind == "TOTAL" From 45ebb07846f1135da4028c7c47f7cb00bc84a605 Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Fri, 12 Jun 2026 14:23:02 -0400 Subject: [PATCH 4/6] Adds cause `Event` id to consequence `Event` --- submit_ce/domain/event/base.py | 11 ++ .../event/tests/test_get_consequences.py | 102 ++++++++++++++++++ .../ui/filters/tests/test_iec_filesize.py | 21 ++++ 3 files changed, 134 insertions(+) create mode 100644 submit_ce/domain/event/tests/test_get_consequences.py create mode 100644 submit_ce/ui/filters/tests/test_iec_filesize.py diff --git a/submit_ce/domain/event/base.py b/submit_ce/domain/event/base.py index e6bb8825..cfc92bdc 100644 --- a/submit_ce/domain/event/base.py +++ b/submit_ce/domain/event/base.py @@ -95,6 +95,11 @@ class Event(BaseModel): This should generally not be set from outside this package. """ + cause: Optional[str] = None + """ + The `event_id` of `Event` that this event was the consequence of. + """ + _before: Optional[Submission] = None """The state of the submission prior to the event. For debugging only.""" @@ -177,12 +182,18 @@ def get_consequences(self, submission: Submission) -> List['Event']: Raises if an event emits a consequence type it did not declare; this keeps the static consequence graph honest at runtime. """ + if not self.created: + raise RuntimeError('Can not make consequences for not yet commited Event') + events = self.consequences(submission) for event in events: if type(event) not in self.CONSEQUENCE_TYPES: raise RuntimeError( f"{self.event_type} emitted undeclared consequence " f"{type(event).__name__}; add it to CONSEQUENCE_TYPES") + else: + event.cause = self.event_id + return events diff --git a/submit_ce/domain/event/tests/test_get_consequences.py b/submit_ce/domain/event/tests/test_get_consequences.py new file mode 100644 index 00000000..91d2ef4d --- /dev/null +++ b/submit_ce/domain/event/tests/test_get_consequences.py @@ -0,0 +1,102 @@ +"""Runtime behavior of :meth:`Event.get_consequences` (``base.py``). + +The *acyclicity* of the consequence graph is covered by +``test_consequences_graph.py``. This module covers what ``get_consequences`` +does at runtime. + +These use throwaway ``Event`` subclasses defined only for the test. They live +under a ``tests`` package, so the production graph test excludes them from its +acyclicity check. +""" + +from datetime import datetime + +import pytest +from pytz import UTC + +from submit_ce.domain import agent +from submit_ce.domain.event.base import Event + + +def _user(uid="u1"): + return agent.PublicUser(name="Test User", user_id=uid, + email=f"{uid}@example.org", endorsements=[]) + + +class _Consequence(Event): + """A throwaway follow-on event.""" + + NAME = "test consequence" + + def validate(self, submission): + pass + + def project(self, submission): + return submission + + +class _Cause(Event): + """A throwaway event that declares and emits a ``_Consequence``.""" + + NAME = "test cause" + CONSEQUENCE_TYPES = frozenset({_Consequence}) + + def validate(self, submission): + pass + + def project(self, submission): + return submission + + def consequences(self, submission): + return [_Consequence(creator=self.creator)] + + +class _Undeclared(Event): + """Emits a consequence type it did not declare in CONSEQUENCE_TYPES.""" + + NAME = "test undeclared" + + def validate(self, submission): + pass + + def project(self, submission): + return submission + + def consequences(self, submission): + return [_Consequence(creator=self.creator)] + + +def _committed(event_cls, **kw): + """A committed event instance (``created`` set, so ``event_id`` works).""" + return event_cls(creator=_user(), created=datetime.now(UTC), **kw) + + +def test_get_consequences_stamps_cause_with_parent_event_id(): + """Each emitted consequence records the parent's event_id in ``cause``.""" + parent = _committed(_Cause) + consequences = parent.get_consequences(submission=None) + assert len(consequences) == 1 + child = consequences[0] + assert isinstance(child, _Consequence) + assert child.cause == parent.event_id + + +def test_get_consequences_default_is_empty(): + """An event that declares nothing emits nothing (and stamps nothing).""" + parent = _committed(_Consequence) + assert parent.get_consequences(submission=None) == [] + + +def test_undeclared_consequence_type_raises(): + """Emitting a type absent from CONSEQUENCE_TYPES is a runtime error, + and the offending consequence is never stamped with a cause.""" + parent = _committed(_Undeclared) + with pytest.raises(RuntimeError, match="undeclared consequence"): + parent.get_consequences(submission=None) + + +def test_get_consequences_on_uncommitted_event_raises(): + """Without ``created`` the parent has no event_id to attribute as cause.""" + parent = _Cause(creator=_user()) # no `created` -> not committed + with pytest.raises(RuntimeError, match="not yet commited"): + parent.get_consequences(submission=None) diff --git a/submit_ce/ui/filters/tests/test_iec_filesize.py b/submit_ce/ui/filters/tests/test_iec_filesize.py new file mode 100644 index 00000000..4f12b554 --- /dev/null +++ b/submit_ce/ui/filters/tests/test_iec_filesize.py @@ -0,0 +1,21 @@ +import pytest + +from .. import iec_filesize + + +@pytest.mark.parametrize("bytes_, expected", [ + (0, "0B"), + (1, "1.00 B"), + (1023, "1023.00 B"), + (1024, "1.00 KiB"), + (1536, "1.50 KiB"), + (50 * 1024, "50.00 KiB"), + (1024 ** 2, "1.00 MiB"), + (5 * 1024 ** 2, "5.00 MiB"), + (1024 ** 3, "1.00 GiB"), + (1024 ** 4, "1.00 TiB"), + (1024 ** 5, "1024.00 TiB"), # caps at the largest unit + (1500.5, "1.47 KiB"), # float input +]) +def test_iec_filesize(bytes_, expected): + assert iec_filesize(bytes_) == expected From a57707abf3aa3cd82c72a974bebcb3ae52f99b5c Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Fri, 12 Jun 2026 14:43:56 -0400 Subject: [PATCH 5/6] Renames `Event.validate()` to `Event.validate_pre_lock()` for clarity --- submit_ce/domain/event/__init__.py | 66 ++++++------- submit_ce/domain/event/base.py | 15 ++- submit_ce/domain/event/file.py | 8 +- submit_ce/domain/event/flag.py | 16 ++-- submit_ce/domain/event/legacy.py | 2 +- submit_ce/domain/event/process.py | 14 +-- submit_ce/domain/event/request.py | 12 +-- .../event/tests/test_event_edge_paths.py | 40 ++++---- submit_ce/domain/event/tests/test_events.py | 96 +++++++++---------- .../domain/event/tests/test_file_events.py | 8 +- submit_ce/domain/event/tests/test_flag.py | 24 ++--- .../event/tests/test_get_consequences.py | 6 +- .../event/tests/test_init_events_pytest.py | 90 ++++++++--------- .../event/tests/test_more_event_branches.py | 26 ++--- submit_ce/domain/event/tests/test_requests.py | 30 +++--- .../legacy_implementation/__init__.py | 5 +- submit_ce/ui/controllers/util.py | 2 +- .../ui/tests/test_save_validate_under_lock.py | 4 +- 18 files changed, 236 insertions(+), 228 deletions(-) diff --git a/submit_ce/domain/event/__init__.py b/submit_ce/domain/event/__init__.py index b740e0a2..3444d5fb 100644 --- a/submit_ce/domain/event/__init__.py +++ b/submit_ce/domain/event/__init__.py @@ -125,7 +125,7 @@ class CreateSubmission(Event): # - https://github.com/python/typing/issues/269 # - https://github.com/python/mypy/issues/5146 # - https://github.com/python/typing/issues/241 - def validate(self, submission: None = None) -> None: # type: ignore + def validate_pre_lock(self, submission: None = None) -> None: # type: ignore """Validate creation of a submission.""" return @@ -147,7 +147,7 @@ class CreateSubmissionVersion(Event): NAME = "create a new version" NAMED = "new version created" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Only applies to announced submissions.""" if not submission.is_announced: raise InvalidEvent(self, "Must already be announced") @@ -178,7 +178,7 @@ class Rollback(Event): NAME = "roll back or delete" NAMED = "rolled back or deleted" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Only applies to submissions in an unannounced state.""" if submission.is_announced: raise InvalidEvent(self, "Cannot already be announced") @@ -213,7 +213,7 @@ class ConfirmContactInformation(Event): NAME = "confirm contact information" NAMED = "contact information confirmed" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Cannot apply to a finalized submission.""" validators.submission_is_not_finalized(self, submission) @@ -249,7 +249,7 @@ class ConfirmAuthorship(Event): submitter_is_author: bool = True - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Cannot apply to a finalized submission.""" validators.submission_is_not_finalized(self, submission) @@ -266,7 +266,7 @@ class ConfirmPolicy(Event): NAMED = "policy acceptance confirmed" agreement_id: int - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Cannot apply to a finalized submission.""" validators.submission_is_not_finalized(self, submission) @@ -285,7 +285,7 @@ class SetPrimaryClassification(Event): category: Optional[ActiveCategory] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the primary classification category.""" if self.category is None: raise InvalidEvent(self, "Must have a category") @@ -333,7 +333,7 @@ class AddSecondaryClassification(Event): #category: Optional[taxonomy.Category] = field(default=None) category: Optional[ActiveCategory] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the secondary classification category to add.""" assert self.category is not None validators.must_be_an_active_category(self, self.category, submission) @@ -360,7 +360,7 @@ class RemoveSecondaryClassification(Event): category: Optional[str] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the secondary classification category to remove.""" assert self.category is not None validators.must_be_an_active_category(self, self.category, submission) @@ -391,7 +391,7 @@ class SetLicense(Event): license_name: Optional[str] = field(default=None) license_uri: Optional[str] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the selected license.""" validators.submission_is_not_finalized(self, submission) if not self.license_uri: @@ -425,7 +425,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.title = self.cleanup(self.title) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the title value.""" validators.submission_is_not_finalized(self, submission) check = metacheck.check_title(self.title) @@ -484,7 +484,7 @@ def model_post_init(self, *args) -> None: #super(SetAbstract, self).__post_init__() self.abstract = self.cleanup(self.abstract) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the abstract value.""" validators.submission_is_not_finalized(self, submission) check = metacheck.check_abstract(self.abstract) @@ -536,7 +536,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.doi = self.cleanup(self.doi) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the DOI value.""" if submission.status == Submission.SUBMITTED \ and not submission.is_announced: @@ -578,7 +578,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.msc_class = self.cleanup(self.msc_class) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the MSC classification value.""" validators.submission_is_not_finalized(self, submission) if not self.msc_class: # Blank values are OK. @@ -620,7 +620,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.acm_class = self.cleanup(self.acm_class) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the ACM classification value.""" validators.submission_is_not_finalized(self, submission) if not self.acm_class: # Blank values are OK. @@ -670,7 +670,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.journal_ref = self.cleanup(self.journal_ref) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the journal reference value.""" if not self.journal_ref: # Blank values are OK. return @@ -718,7 +718,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.report_num = self.cleanup(self.report_num) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the report number value.""" if not self.report_num: # Blank values are OK. return @@ -753,7 +753,7 @@ def model_post_init(self, *args, **kwargs) -> None: """Perform some light cleanup on the provided value.""" self.comments = self.cleanup(self.comments) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the comments value.""" validators.submission_is_not_finalized(self, submission) if not self.comments: # Blank values are OK. @@ -795,7 +795,7 @@ def model_post_init(self, *args, **kwargs) -> None: self.authors_display = self._canonical_author_string() self.authors_display = self.cleanup(self.authors_display) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """May not apply to a finalized submission.""" validators.submission_is_not_finalized(self, submission) check = metacheck.check_authors(self.authors_display) @@ -838,7 +838,7 @@ class SetSourceFormat(Event): source_format: Optional[str] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate that source_format is a known SourceFormat value.""" if self.source_format is None: return @@ -886,7 +886,7 @@ class ConfirmSourceProcessed(Event): added: Optional[datetime] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Make sure that a preview is actually provided.""" # if self.source_id < 0: # raise InvalidEvent(self, "Preview not provided") @@ -922,7 +922,7 @@ class UnConfirmSourceProcessed(Event): NAME = "unconfirm source has been processed" NAMED = "unconfirmed that source has been processed" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Nothing to do.""" def project(self, submission: Submission) -> Submission: @@ -946,7 +946,7 @@ class ConfirmPreview(Event): preview_checksum: Optional[str] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate data for :class:`.ConfirmPreview`.""" validators.submission_is_not_finalized(self, submission) if submission.preview is None: @@ -980,7 +980,7 @@ class FinalizeSubmission(Event): CONSEQUENCE_TYPES = frozenset({AddHold}) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Ensure that all required data/steps are complete.""" if submission.is_finalized: raise InvalidEvent(self, "Submission already finalized") @@ -1025,7 +1025,7 @@ class UnFinalizeSubmission(Event): NAME = "re-open submission for modification" NAMED = "submission re-opened for modification" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the unfinalize action.""" self._must_be_finalized(submission) if submission.is_announced: @@ -1051,7 +1051,7 @@ class Announce(Event): arxiv_id: Optional[str] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Make sure that we have a valid arXiv ID.""" # TODO: When we're using this to perform publish in NG, we will want to # re-enable this step. @@ -1086,7 +1086,7 @@ def project(self, submission: Submission) -> Submission: # body: str = field(default_factory=str) # scope: str = 'private' # -# def validate(self, submission: Submission) -> None: +# def validate_pre_lock(self, submission: Submission) -> None: # """The :attr:`.body` should be set.""" # if not self.body: # raise ValueError('Comment body not set') @@ -1112,7 +1112,7 @@ def project(self, submission: Submission) -> Submission: # # comment_id: str = field(default_factory=str) # -# def validate(self, submission: Submission) -> None: +# def validate_pre_lock(self, submission: Submission) -> None: # """The :attr:`.comment_id` must present on the submission.""" # if self.comment_id is None: # raise InvalidEvent(self, 'comment_id is required') @@ -1132,7 +1132,7 @@ def project(self, submission: Submission) -> Submission: # # delegate: Optional[Agent] = None # -# def validate(self, submission: Submission) -> None: +# def validate_pre_lock(self, submission: Submission) -> None: # """The event creator must be the owner of the submission.""" # if not self.creator == submission.owner: # raise InvalidEvent(self, 'Event creator must be submission owner') @@ -1153,7 +1153,7 @@ def project(self, submission: Submission) -> Submission: # # delegation_id: str = field(default_factory=str) # -# def validate(self, submission: Submission) -> None: +# def validate_pre_lock(self, submission: Submission) -> None: # """The event creator must be the owner of the submission.""" # if not self.creator == submission.owner: # raise InvalidEvent(self, 'Event creator must be submission owner') @@ -1175,7 +1175,7 @@ class AddFeature(Event): field(default=Feature.Type.WORD_COUNT) feature_value: Union[float, int] = field(default=0) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that the feature type is a known value.""" if self.feature_type not in Feature.Type: valid_types = ", ".join([ft.value for ft in Feature.Type]) @@ -1205,7 +1205,7 @@ class AddClassifierResults(Event): = field(default=ClassifierResults.Classifiers.CLASSIC) results: List[ClassifierResult] = field(default_factory=list) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that the classifier is a known value.""" if self.classifier not in ClassifierResults.Classifiers: valid = ", ".join([c.value for c in ClassifierResults.Classifiers]) @@ -1234,7 +1234,7 @@ class Reclassify(Event): #category: Optional[taxonomy.Category] = None category: Optional[str] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the primary classification category.""" assert isinstance(self.category, str) validators.must_be_an_active_category(self, self.category, submission) diff --git a/submit_ce/domain/event/base.py b/submit_ce/domain/event/base.py index cfc92bdc..1b26b207 100644 --- a/submit_ce/domain/event/base.py +++ b/submit_ce/domain/event/base.py @@ -34,7 +34,7 @@ class Event(BaseModel): extend it with whatever data is needed for the event, and define methods for validation and projection (changing a submission): - - ``validate(self, submission: Submission) -> None`` should raise + - ``validate_pre_lock(self, submission: Submission) -> None`` should raise :class:`.InvalidEvent` if the event instance has invalid data. - ``project(self, submission: Submission) -> Submission`` should perform changes to the :class:`.domain.submission.Submission` and return it. @@ -135,7 +135,7 @@ def apply(self, submission: Optional[Submission] = None, ) -> Submission: """Apply the projection for this :class:`.Event` instance.""" self._before = copy.deepcopy(submission) # See comment on CreateSubmission, below. - self.validate(submission) # type: ignore + self.validate_pre_lock(submission) # type: ignore if submission is not None: self._after = self.project(copy.deepcopy(submission)) else: # See comment on CreateSubmission, below. @@ -152,8 +152,15 @@ def apply(self, submission: Optional[Submission] = None, ) -> Submission: return self._after - def validate(self, submission: Submission) -> None: - """Validate this event and its data against a submission.""" + def validate_pre_lock(self, submission: Submission) -> None: + """Validate this event and its data against a submission. + + Raise :class:`.InvalidEvent` if the event cannot be applied. This runs + *before* the submission row lock is taken (during :meth:`apply`), so it + must not depend on state that a concurrent writer could change. For + validation that needs the lock held, see + :meth:`EventWithSideEffect.validate_under_lock`. + """ raise NotImplementedError('Must be implemented by subclass') def project(self, submission: Submission) -> Submission: diff --git a/submit_ce/domain/event/file.py b/submit_ce/domain/event/file.py index 50547b09..c00c942e 100644 --- a/submit_ce/domain/event/file.py +++ b/submit_ce/domain/event/file.py @@ -76,7 +76,7 @@ class UploadArchive(EventWithSideEffect): oversize: list[size_limits.OversizeReason] = [] """`OversizeReason` instances after this change (set in execute).""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) if not self.file: raise InvalidEvent(self, "Must upload a file") @@ -114,7 +114,7 @@ class UploadFiles(EventWithSideEffect): oversize: list[size_limits.OversizeReason] = [] """`OversizeReason` instances after this change (set in execute).""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) def execute(self, api: SubmitApi, submission: Submission) -> None: @@ -151,7 +151,7 @@ class RemoveFiles(EventWithSideEffect): oversize: list[size_limits.OversizeReason] = [] """`OversizeReason` instances after this change (set in execute).""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) def execute(self, api: SubmitApi, submission: Submission) -> None: @@ -179,7 +179,7 @@ class RemoveAllFiles(EventWithSideEffect): NAME = "remove all files" NAMED = "all files removed" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) def execute(self, api: SubmitApi, submission: Submission) -> None: diff --git a/submit_ce/domain/event/flag.py b/submit_ce/domain/event/flag.py index 6562b59a..05053a04 100644 --- a/submit_ce/domain/event/flag.py +++ b/submit_ce/domain/event/flag.py @@ -20,7 +20,7 @@ class AddFlag(Event): = field(default=None) comment: Optional[str] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Not implemented.""" raise NotImplementedError("Invoke a child event instead") @@ -38,7 +38,7 @@ class RemoveFlag(Event): flag_id: Optional[str] = field(default=None) """This is the ``event_id`` of the event that added the flag.""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that the flag exists.""" if self.flag_id not in submission.flags: raise InvalidEvent(self, f"Unknown flag: {self.flag_id}") @@ -58,7 +58,7 @@ class AddContentFlag(AddFlag): flag_type: Optional[ContentFlag.FlagType] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that we have a known flag.""" if self.flag_type not in ContentFlag.FlagType: raise InvalidEvent(self, f"Unknown content flag: {self.flag_type}") @@ -94,7 +94,7 @@ class AddMetadataFlag(AddFlag): field: Optional[str] = field(default=None) """Name of the metadata field to which the flag applies.""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that we have a known flag and metadata field.""" if self.flag_type not in MetadataFlag.FlagType: raise InvalidEvent(self, f"Unknown meta flag: {self.flag_type}") @@ -131,7 +131,7 @@ class AddUserFlag(AddFlag): flag_type: Optional[UserFlag.FlagType] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that we have a known flag.""" if self.flag_type not in MetadataFlag.FlagType: raise InvalidEvent(self, f"Unknown user flag: {self.flag_type}") @@ -166,7 +166,7 @@ class AddHold(Event): hold_type: Hold.Type = field(default=Hold.Type.PATCH) hold_reason: Optional[str] = field(default_factory=str) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: pass def project(self, submission: Submission) -> Submission: @@ -199,7 +199,7 @@ class RemoveHold(Event): hold_type: Hold.Type = field(default=Hold.Type.PATCH) removal_reason: Optional[str] = field(default_factory=str) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.hold_event_id not in submission.holds: raise InvalidEvent(self, "No such hold") @@ -225,7 +225,7 @@ class AddWaiver(Event): waiver_type: Hold.Type = field(default=Hold.Type.SOURCE_OVERSIZE) waiver_reason: str = field(default_factory=str) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: pass def project(self, submission: Submission) -> Submission: diff --git a/submit_ce/domain/event/legacy.py b/submit_ce/domain/event/legacy.py index a38d3860..a30a1300 100644 --- a/submit_ce/domain/event/legacy.py +++ b/submit_ce/domain/event/legacy.py @@ -23,7 +23,7 @@ class Withdraw(EventWithSideEffect): abstract: str = Field(min_length=10, max_length=1920) """Updated abstract for the withdrawal notice.""" - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Make sure that a reason was provided and the paper is announced.""" if not self.comment: raise InvalidEvent(self, "Provide a reason for the withdrawal") diff --git a/submit_ce/domain/event/process.py b/submit_ce/domain/event/process.py index 7adaf0cf..823178ca 100644 --- a/submit_ce/domain/event/process.py +++ b/submit_ce/domain/event/process.py @@ -48,7 +48,7 @@ def __post_init__(self) -> None: """Make sure our enums are in order.""" super(StartCompileSource, self).__post_init__() - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that we have a :class:`.ProcessStatus`.""" if submission.uncompressed_size <= 0: raise InvalidEvent(self, "Compile source for the submission is empty.") @@ -93,7 +93,7 @@ class StartPreflight(EventWithSideEffect): def __post_init__(self) -> None: super(StartPreflight, self).__post_init__() - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if not submission.submission_id: raise InvalidEvent(self, "Source content for preflight is empty.") @@ -133,7 +133,7 @@ class StartDirectives(EventWithSideEffect): process: Optional[ProcessInfo] = field(default=None) result: Optional[Result] = field(default=None) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if not submission.submission_id: raise InvalidEvent(self, "Source content for directives is empty.") @@ -175,7 +175,7 @@ def __post_init__(self) -> None: """Make sure our enums are in order.""" super(CompileStatus, self).__post_init__() - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Verify that we have a :class:`.ProcessStatus`.""" if self.process is None: raise InvalidEvent(self, "Must include process") @@ -207,7 +207,7 @@ class PreflightStatus(Event): def __post_init__(self) -> None: super(PreflightStatus, self).__post_init__() - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.process is None: raise InvalidEvent(self, "Must include process") if self.result is None: @@ -238,7 +238,7 @@ class SetDecisions(EventWithSideEffect): bytes_removed: int = 0 - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if not self.decisions: raise InvalidEvent(self, "Must include decisions information") # TODO better validation of preflight data or just handled by pydantic? @@ -298,7 +298,7 @@ class SetDirectivesAndCleanup(EventWithSideEffect): # and 00README.json are left untouched. user_decisions_from_zzrm: Optional[dict] = None - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: # No input invariants: the event is always safe to dispatch # from `_load_or_create_preflight`; an absent zzrm just means # "skip the user_decisions seed step." diff --git a/submit_ce/domain/event/request.py b/submit_ce/domain/event/request.py index a461dc8a..a6b68e2f 100644 --- a/submit_ce/domain/event/request.py +++ b/submit_ce/domain/event/request.py @@ -29,7 +29,7 @@ class ApproveRequest(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.request_id not in submission.user_requests: raise InvalidEvent(self, "No such request") @@ -55,7 +55,7 @@ class RejectRequest(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.request_id not in submission.user_requests: raise InvalidEvent(self, "No such request") @@ -81,7 +81,7 @@ class CancelRequest(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.request_id not in submission.user_requests: raise InvalidEvent(self, "No such request") @@ -108,7 +108,7 @@ class ApplyRequest(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: if self.request_id not in submission.user_requests: raise InvalidEvent(self, "No such request") @@ -141,7 +141,7 @@ class RequestCrossList(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Validate the cross-list request.""" validators.no_active_requests(self, submission) if not submission.is_announced: @@ -190,7 +190,7 @@ class RequestWithdrawal(Event): # return NotImplemented # return hash(self) == hash(other) - def validate(self, submission: Submission) -> None: + def validate_pre_lock(self, submission: Submission) -> None: """Make sure that a reason was provided.""" validators.no_active_requests(self, submission) if not self.reason: diff --git a/submit_ce/domain/event/tests/test_event_edge_paths.py b/submit_ce/domain/event/tests/test_event_edge_paths.py index e5115007..39a7d104 100644 --- a/submit_ce/domain/event/tests/test_event_edge_paths.py +++ b/submit_ce/domain/event/tests/test_event_edge_paths.py @@ -80,7 +80,7 @@ def test_confirm_preview_fails_when_no_preview(): s = _working_submission() e = ConfirmPreview(creator=s.creator, created=_now(), preview_checksum="abc123") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_confirm_preview_fails_on_checksum_mismatch(): """ @@ -96,7 +96,7 @@ def test_confirm_preview_fails_on_checksum_mismatch(): ) e = ConfirmPreview(creator=s.creator, created=_now(), preview_checksum="WRONG") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_confirm_preview_succeeds_on_checksum_match_sets_flag(): """ @@ -112,7 +112,7 @@ def test_confirm_preview_succeeds_on_checksum_match_sets_flag(): ) e = ConfirmPreview(creator=s.creator, created=_now(), preview_checksum="MATCH") # validate should not raise - e.validate(s) + e.validate_pre_lock(s) # apply should toggle the flag after = e.apply(s) assert after.submitter_confirmed_preview is True @@ -129,7 +129,7 @@ def test_create_submission_version_rejects_unannounced(): s = _working_submission() e = CreateSubmissionVersion(creator=s.creator, created=_now()) with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_create_submission_version_succeeds_when_announced(): """ @@ -139,7 +139,7 @@ def test_create_submission_version_succeeds_when_announced(): s = _announced_submission() e = CreateSubmissionVersion(creator=s.creator, created=_now()) # validate should not raise - e.validate(s) + e.validate_pre_lock(s) # apply should move to a new version and set status to WORKING after = e.apply(s) assert after.version == s.version + 1 @@ -154,7 +154,7 @@ def test_finalize_missing_required_fields(): s = _working_submission() e = FinalizeSubmission(creator=s.creator) with pytest.raises(InvalidEvent): - e.validate(s) # REQUIRED / REQUIRED_METADATA guard + e.validate_pre_lock(s) # REQUIRED / REQUIRED_METADATA guard # ------------------------------------------------------- # RemoveSecondaryClassification @@ -164,7 +164,7 @@ def test_remove_secondary_missing_fails(): # category not yet added → _must_already_be_present should fail e = RemoveSecondaryClassification(creator=s.creator, category="cs.AI") with pytest.raises(InvalidEvent): - e.validate(s) # "No such category on submission" + e.validate_pre_lock(s) # "No such category on submission" # ------------------------------------------------------- # Rollback: version==1 -> delete; version>1 with history -> revert @@ -174,7 +174,7 @@ def test_rollback_invalid_when_announced(): s = _announced_submission() e = Rollback(creator=s.creator) with pytest.raises(InvalidEvent): - e.validate(s) # "Cannot already be announced" + e.validate_pre_lock(s) # "Cannot already be announced" def test_rollback_on_first_version_deletes_submission(): """ @@ -184,7 +184,7 @@ def test_rollback_on_first_version_deletes_submission(): s.version = 1 e = Rollback(creator=s.creator, created=_now()) # validate: requires unannounced (is true for working) - e.validate(s) + e.validate_pre_lock(s) after = e.apply(s) assert after.status == submod.Submission.DELETED @@ -200,7 +200,7 @@ def test_rollback_to_previous_announced_version(): s.versions = [copy.deepcopy(s)] s.versions[0].status = submod.Submission.ANNOUNCED e = Rollback(creator=s.creator, created=_now()) - e.validate(s) + e.validate_pre_lock(s) after = e.apply(s) # Should have decremented version and restored announced status assert after.version == 1 @@ -211,7 +211,7 @@ def test_rollback_version1_sets_deleted(): s.version = 1 s.status = Submission.WORKING e = Rollback(creator=s.creator) - e.validate(s) + e.validate_pre_lock(s) out = e.project(s) assert out.status == Submission.DELETED @@ -222,12 +222,12 @@ def test_abstract_too_short_fails(): s = _working_submission() e = SetAbstract(creator=s.creator, abstract="short") with pytest.raises(InvalidEvent): - e.validate(s) # MIN_LENGTH branch + e.validate_pre_lock(s) # MIN_LENGTH branch def test_abstract_valid_passes(): s = _working_submission() e = SetAbstract(creator=s.creator, abstract="This abstract is just long enough") - e.validate(s) + e.validate_pre_lock(s) s2 = e.project(s) assert s2.metadata.abstract == "This abstract is just long enough" @@ -238,7 +238,7 @@ def test_license_requires_url(): s = _working_submission() e = SetLicense(creator=s.creator, license_name="CC BY 4.0", license_uri="") with pytest.raises(InvalidEvent): - e.validate( + e.validate_pre_lock( s) # "License must have a URL" def test_license_valid_url(): @@ -246,7 +246,7 @@ def test_license_valid_url(): s = _working_submission() e = SetLicense(creator=s.creator, license_name="CC BY 4.0", license_uri="http://creativecommons.org/licenses/by/4.0/") - e.validate(s) # passes if LICENSES marks it current + e.validate_pre_lock(s) # passes if LICENSES marks it current # ------------------------------------------------------- # SetReportNumber: invalid vs. valid formats @@ -259,7 +259,7 @@ def test_set_report_number_rejects_invalid_value(): s = _working_submission() e = SetReportNumber(creator=s.creator, report_num="not a report number") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_set_report_number_accepts_common_formats(): """ @@ -268,7 +268,7 @@ def test_set_report_number_accepts_common_formats(): s = _working_submission() e = SetReportNumber(creator=s.creator, report_num="CORNELL-1003-1130") # Should not raise - e.validate(s) + e.validate_pre_lock(s) after = e.apply(s) assert after.metadata.report_num == "CORNELL-1003-1130" @@ -279,16 +279,16 @@ def test_title_allows_basic_tags(): s = _working_submission() e = SetTitle(creator=s.creator, title="Hello
World") with pytest.raises(InvalidEvent): - e.validate(s) # No HTML tags are allowed + e.validate_pre_lock(s) # No HTML tags are allowed def test_title_rejects_disallowed_html(): s = _working_submission() e = SetTitle(creator=s.creator, title="") with pytest.raises(InvalidEvent): - e.validate(s) # _check_for_html branch + e.validate_pre_lock(s) # _check_for_html branch def test_title_trailing_period_rule(): s = _working_submission() e = SetTitle(creator=s.creator, title="Hello world.") with pytest.raises(InvalidEvent): - e.validate(s) # validators.no_trailing_period + e.validate_pre_lock(s) # validators.no_trailing_period diff --git a/submit_ce/domain/event/tests/test_events.py b/submit_ce/domain/event/tests/test_events.py index 6aee054a..1321bbad 100644 --- a/submit_ce/domain/event/tests/test_events.py +++ b/submit_ce/domain/event/tests/test_events.py @@ -55,7 +55,7 @@ def test_request_withdrawal(self): e = event.RequestWithdrawal(creator=self.user, created=datetime.now(UTC), reason="no good") - e.validate(self.submission) + e.validate_pre_lock(self.submission) replacement = e.apply(self.submission) self.assertEqual(replacement.arxiv_id, self.submission.arxiv_id) self.assertEqual(replacement.version, self.submission.version) @@ -69,13 +69,13 @@ def test_request_without_a_reason(self): """A reason is required.""" e = event.RequestWithdrawal(creator=self.user) with self.assertRaises(event.InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_request_without_announced_submission(self): """The submission must already be announced.""" e = event.RequestWithdrawal(creator=self.user, reason="no good") with self.assertRaises(event.InvalidEvent): - e.validate(mock.MagicMock(announced=False)) + e.validate_pre_lock(mock.MagicMock(announced=False)) class TestReplacementSubmission(TestCase): @@ -239,7 +239,7 @@ def test_set_primary_with_nonsense(self): category="nonsense" ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_set_primary_inactive(self): """Category is not from the arXiv taxonomy.""" @@ -249,7 +249,7 @@ def test_set_primary_inactive(self): category="chao-dyn" ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_set_primary_with_valid_category(self): """Category is from the arXiv taxonomy.""" @@ -261,12 +261,12 @@ def test_set_primary_with_valid_category(self): ) if category in self.user.endorsements: try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail("Event should be valid") else: with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_set_primary_already_secondary(self): """Category is already set as a secondary.""" @@ -278,7 +278,7 @@ def test_set_primary_already_secondary(self): category='cond-mat.dis-nn' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". class TestAddSecondaryClassification(TestCase): @@ -303,7 +303,7 @@ def test_add_secondary_with_nonsense(self): category="nonsense" ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_add_secondary_inactive(self): """Category is inactive.""" @@ -313,7 +313,7 @@ def test_add_secondary_inactive(self): category="bayes-an" ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_add_secondary_with_valid_category(self): """Category is from the arXiv taxonomy.""" @@ -324,7 +324,7 @@ def test_add_secondary_with_valid_category(self): category=category ) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent: if category != 'physics.gen-ph': self.fail("Event should be valid") @@ -340,7 +340,7 @@ def test_add_secondary_already_present(self): category='cond-mat.dis-nn' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_add_secondary_already_primary(self): """Category is already set as primary.""" @@ -353,7 +353,7 @@ def test_add_secondary_already_primary(self): category='cond-mat.dis-nn' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_add_general_secondary(self): """Category is more general than the existing categories.""" @@ -366,7 +366,7 @@ def test_add_general_secondary(self): category='physics.gen-ph' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". classification = submission.Classification('cond-mat.quant-gas') self.submission.primary_classification = classification @@ -379,7 +379,7 @@ def test_add_general_secondary(self): category='physics.gen-ph' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_add_specific_secondary(self): """Category is more specific than existing general category.""" @@ -392,7 +392,7 @@ def test_add_specific_secondary(self): category='physics.optics' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". classification = submission.Classification('astro-ph.SR') self.submission.primary_classification = classification @@ -405,7 +405,7 @@ def test_add_specific_secondary(self): category='physics.optics' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_add_max_secondaries(self): """Test max secondaries.""" @@ -421,7 +421,7 @@ def test_add_max_secondaries(self): submission_id="1", category='cond-mat.quant-gas' ) - e1.validate(self.submission) + e1.validate_pre_lock(self.submission) self.submission.secondary_classification.append( submission.Classification('cond-mat.quant-gas')) @@ -433,7 +433,7 @@ def test_add_max_secondaries(self): self.assertEqual(len(self.submission.secondary_classification), 4) with self.assertRaises(InvalidEvent): - e2.validate(self.submission) # "Event should not be valid". + e2.validate_pre_lock(self.submission) # "Event should not be valid". class TestRemoveSecondaryClassification(TestCase): @@ -458,7 +458,7 @@ def test_add_secondary_with_nonsense(self): category="nonsense" ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". def test_remove_secondary_with_valid_category(self): """Category is from the arXiv taxonomy.""" @@ -470,7 +470,7 @@ def test_remove_secondary_with_valid_category(self): category='cond-mat.dis-nn' ) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail("Event should be valid") @@ -482,7 +482,7 @@ def test_remove_secondary_not_present(self): category='cond-mat.dis-nn' ) with self.assertRaises(InvalidEvent): - e.validate(self.submission) # "Event should not be valid". + e.validate_pre_lock(self.submission) # "Event should not be valid". class TestSetAuthors(TestCase): @@ -505,7 +505,7 @@ def test_canonical_authors_provided(self): authors=[submission.Author()], authors_display="Foo authors") try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except Exception as e: self.fail(str(e), "Data should be valid") s = e.project(self.submission) @@ -528,7 +528,7 @@ def test_canonical_authors_not_provided(self): "Display string should be generated automagically") try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except Exception as e: self.fail(str(e), "Data should be valid") s = e.project(self.submission) @@ -553,7 +553,7 @@ def test_empty_value(self): """Title is set to an empty string.""" e = event.SetTitle(creator=self.user, title='') with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) # breaks with metacheck (fix arriving soon) # def test_reasonable_title(self): @@ -571,7 +571,7 @@ def test_empty_value(self): # .title() # e = event.SetTitle(creator=self.user, title=title) # try: - # e.validate(self.submission) + # e.validate_pre_lock(self.submission) # except InvalidEvent as e: # self.fail(f'Failed to handle title due to {e.message}: "{title}" ') @@ -580,21 +580,21 @@ def test_all_caps_title(self): title = Text().title()[:240].upper() e = event.SetTitle(creator=self.user, title=title) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_title_ends_with_period(self): """Title ends with a period.""" title = Text().title()[:239] + "." e = event.SetTitle(creator=self.user, title=title) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_title_ends_with_ellipsis(self): """Title ends with an ellipsis.""" title = Text().title()[:236] + "..." e = event.SetTitle(creator=self.user, title=title) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail("Should accept ellipsis") @@ -603,13 +603,13 @@ def test_huge_title(self): title = Text().text(200) # 200 sentences. e = event.SetTitle(creator=self.user, title=title) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_title_with_html_escapes(self): """Title should not allow HTML escapes.""" e = event.SetTitle(creator=self.user, title='foo   title') with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) class TestSetAbstract(TestCase): @@ -629,7 +629,7 @@ def test_empty_value(self): """Abstract is set to an empty string.""" e = event.SetAbstract(creator=self.user, abstract='') with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) def test_reasonable_abstract(self): """Abstract is set to some reasonable value smaller than 1920 chars.""" @@ -637,7 +637,7 @@ def test_reasonable_abstract(self): abstract = Text(locale="en").text(20)[:1920] e = event.SetAbstract(creator=self.user, abstract=abstract) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail(f'Failed to handle abstract due to {e.message}: {abstract}') @@ -648,7 +648,7 @@ def test_reasonable_international_abstract(self): abstract = Text(locale=locale).text(20)[:1920] e = event.SetAbstract(creator=self.user, abstract=abstract) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: if "Does not appear to be in English" not in e.message: self.fail(f'Failed to handle abstract due to {e.message}: {abstract}') @@ -658,7 +658,7 @@ def test_huge_abstract(self): abstract = Text().text(200) # 200 sentences. e = event.SetAbstract(creator=self.user, abstract=abstract) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) class TestSetDOI(TestCase): @@ -679,7 +679,7 @@ def test_empty_doi(self): doi = "" e = event.SetDOI(creator=self.user, doi=doi) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail('Failed to handle valid DOI: %s' % e) @@ -688,7 +688,7 @@ def test_valid_doi(self): doi = "10.1016/S0550-3213(01)00405-9" e = event.SetDOI(creator=self.user, doi=doi) try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail('Failed to handle valid DOI: %s' % e) @@ -698,7 +698,7 @@ def test_valid_doi(self): # doi = "10.1016/S0550-3213(01)00405-9, 10.1016/S0550-3213(01)00405-8" # e = event.SetDOI(creator=self.user, doi=doi) # try: - # e.validate(self.submission) + # e.validate_pre_lock(self.submission) # except InvalidEvent as e: # self.fail(f'Failed to handle valid DOI {e.message}: {doi}') @@ -707,7 +707,7 @@ def test_invalid_doi(self): not_a_doi = "101016S0550-3213(01)00405-9" e = event.SetDOI(creator=self.user, doi=not_a_doi) with self.assertRaises(InvalidEvent): - e.validate(self.submission) + e.validate_pre_lock(self.submission) class TestSetReportNumber(TestCase): @@ -761,7 +761,7 @@ def test_valid_report_number(self): for value in values: try: e = event.SetReportNumber(creator=self.user, report_num=value) - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail(f'failed report number {e.message}: {value}') @@ -773,7 +773,7 @@ def test_invalid_values(self): for value in values: with self.assertRaises(InvalidEvent): e = event.SetReportNumber(creator=self.user, report_num=value) - e.validate(self.submission) + e.validate_pre_lock(self.submission) class TestSetJournalReference(TestCase): @@ -818,7 +818,7 @@ def test_valid_journal_ref(self): try: e = event.SetJournalReference(creator=self.user, journal_ref=value) - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail(f'Failed {e.message} {value}') @@ -834,7 +834,7 @@ def test_valid_journal_ref(self): # with self.assertRaises(InvalidEvent): # e = event.SetJournalReference(creator=self.user, # journal_ref=value) - # e.validate(self.submission) + # e.validate_pre_lock(self.submission) class TestSetACMClassification(TestCase): @@ -879,7 +879,7 @@ def test_valid_acm_class(self): try: e = event.SetACMClassification(creator=self.user, acm_class=value) - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail('Failed to handle %s: %s' % (value, e)) @@ -925,7 +925,7 @@ def test_valid_msc_class(self): try: e = event.SetMSCClassification(creator=self.user, msc_class=value) - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail('Failed to handle %s: %s' % (value, e)) @@ -947,7 +947,7 @@ def test_empty_value(self): """Comment is set to an empty string.""" e = event.SetComments(creator=self.user, comments='') try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent as e: self.fail('Failed to handle empty comments') @@ -957,7 +957,7 @@ def test_empty_value(self): # comments = Text(locale=locale).text(20)[:400] # e = event.SetComments(creator=self.user, comments=comments) # try: - # e.validate(self.submission) + # e.validate_pre_lock(self.submission) # except InvalidEvent as e: # self.fail(f'Failed to handle comment {e.message}: {comments}') @@ -971,7 +971,7 @@ def test_empty_value(self): # assert res.disposition != metacheck.OK # e = event.SetComments(creator=self.user, comments=comments) # with self.assertRaises(InvalidEvent): - # e.validate(self.submission) + # e.validate_pre_lock(self.submission) # Locales supported by mimesis. diff --git a/submit_ce/domain/event/tests/test_file_events.py b/submit_ce/domain/event/tests/test_file_events.py index 21e66e9e..b25ef486 100644 --- a/submit_ce/domain/event/tests/test_file_events.py +++ b/submit_ce/domain/event/tests/test_file_events.py @@ -26,7 +26,7 @@ def _blank_submission(uid: str = "u1"): def test_add_files_initializes_package(): s = _blank_submission() e = UploadFiles(creator=s.creator, files=[]) - e.validate(s) + e.validate_pre_lock(s) s = e.project(s) assert s.submitter_confirmed_preview is False @@ -35,7 +35,7 @@ def test_add_files_updates_package(): s = _blank_submission() e = UploadFiles(creator=s.creator, files=[]) - e.validate(s) + e.validate_pre_lock(s) s = e.project(s) assert s.submitter_confirmed_preview is False @@ -44,7 +44,7 @@ def test_remove_files_updates_size(): s = _blank_submission() e = RemoveFiles(creator=s.creator, files=[]) - e.validate(s) + e.validate_pre_lock(s) s = e.project(s) assert s.submitter_confirmed_preview is False @@ -53,7 +53,7 @@ def test_remove_all_files_clears_package(): s = _blank_submission() e = RemoveAllFiles(creator=s.creator) - e.validate(s) + e.validate_pre_lock(s) s = e.project(s) assert s.source_format is None diff --git a/submit_ce/domain/event/tests/test_flag.py b/submit_ce/domain/event/tests/test_flag.py index f580bb07..f4386a23 100644 --- a/submit_ce/domain/event/tests/test_flag.py +++ b/submit_ce/domain/event/tests/test_flag.py @@ -34,7 +34,7 @@ def test_add_flag_base(self): """Test that AddFlag base class methods raise NotImplementedError.""" e = AddFlag(creator=self.user, created=datetime.now(UTC)) with self.assertRaises(NotImplementedError): - e.validate(self.submission) + e.validate_pre_lock(self.submission) with self.assertRaises(NotImplementedError): e.project(self.submission) @@ -42,7 +42,7 @@ def test_add_content_flag(self): """Test AddContentFlag validation and projection.""" # Valid flag type e = AddContentFlag(creator=self.user, created=datetime.now(UTC), flag_type=ContentFlag.FlagType.CHARACTER_SET) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertIn(e.event_id, updated_submission.flags) @@ -53,7 +53,7 @@ def test_add_content_flag(self): # Valid flag type passed as string e_str = AddContentFlag(creator=self.user, created=datetime.now(UTC), flag_type='character set') - e_str.validate(self.submission) + e_str.validate_pre_lock(self.submission) self.assertEqual(e_str.flag_type, ContentFlag.FlagType.CHARACTER_SET) # Invalid flag type @@ -70,20 +70,20 @@ def test_remove_flag(self): # Valid removal e = RemoveFlag(creator=self.user, created=datetime.now(UTC), flag_id=flag_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertNotIn(flag_id, updated_submission.flags) # Invalid removal (unknown flag) e_invalid = RemoveFlag(creator=self.user, created=datetime.now(UTC), flag_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_add_metadata_flag(self): """Test AddMetadataFlag validation and projection.""" # Valid metadata flag e = AddMetadataFlag(creator=self.user, created=datetime.now(UTC), flag_type=MetadataFlag.FlagType.LANGUAGE, field="title") - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertIn(e.event_id, updated_submission.flags) @@ -99,7 +99,7 @@ def test_add_metadata_flag(self): # Invalid metadata field e_invalid_field = AddMetadataFlag(creator=self.user, created=datetime.now(UTC), flag_type=MetadataFlag.FlagType.LANGUAGE, field="unknown_field") with self.assertRaises(InvalidEvent) as cm: - e_invalid_field.validate(self.submission) + e_invalid_field.validate_pre_lock(self.submission) self.assertIn("Not a valid metadata field", str(cm.exception)) def test_add_user_flag(self): @@ -109,7 +109,7 @@ def test_add_user_flag(self): # Validation might fail due to the bug: try: - e.validate(self.submission) + e.validate_pre_lock(self.submission) except InvalidEvent: pass @@ -122,7 +122,7 @@ def test_add_user_flag(self): def test_add_hold(self): """Test AddHold validation and projection.""" e = AddHold(creator=self.user, created=datetime.now(UTC), hold_type=Hold.Type.PATCH, hold_reason="Need to patch") - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertIn(e.event_id, updated_submission.holds) @@ -145,19 +145,19 @@ def test_remove_hold(self): # Valid removal e = RemoveHold(creator=self.user, created=datetime.now(UTC), hold_event_id=hold_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertNotIn(hold_id, updated_submission.holds) # Invalid removal e_invalid = RemoveHold(creator=self.user, created=datetime.now(UTC), hold_event_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_add_waiver(self): """Test AddWaiver validation and projection.""" e = AddWaiver(creator=self.user, created=datetime.now(UTC), waiver_type=Hold.Type.SOURCE_OVERSIZE, waiver_reason="Approved") - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertIn(e.event_id, updated_submission.waivers) diff --git a/submit_ce/domain/event/tests/test_get_consequences.py b/submit_ce/domain/event/tests/test_get_consequences.py index 91d2ef4d..da0ba7c7 100644 --- a/submit_ce/domain/event/tests/test_get_consequences.py +++ b/submit_ce/domain/event/tests/test_get_consequences.py @@ -28,7 +28,7 @@ class _Consequence(Event): NAME = "test consequence" - def validate(self, submission): + def validate_pre_lock(self, submission): pass def project(self, submission): @@ -41,7 +41,7 @@ class _Cause(Event): NAME = "test cause" CONSEQUENCE_TYPES = frozenset({_Consequence}) - def validate(self, submission): + def validate_pre_lock(self, submission): pass def project(self, submission): @@ -56,7 +56,7 @@ class _Undeclared(Event): NAME = "test undeclared" - def validate(self, submission): + def validate_pre_lock(self, submission): pass def project(self, submission): diff --git a/submit_ce/domain/event/tests/test_init_events_pytest.py b/submit_ce/domain/event/tests/test_init_events_pytest.py index 17fb4795..068d10d8 100644 --- a/submit_ce/domain/event/tests/test_init_events_pytest.py +++ b/submit_ce/domain/event/tests/test_init_events_pytest.py @@ -33,7 +33,7 @@ def base_submission(mock_user): def test_create_submission(mock_user): e = event.CreateSubmission(creator=mock_user, created=datetime.now(UTC)) - e.validate(None) + e.validate_pre_lock(None) sub = e.project(None) assert sub.creator == mock_user assert sub.owner == mock_user @@ -43,7 +43,7 @@ def test_create_submission_version(mock_user, base_submission): base_submission.status = submission.Submission.ANNOUNCED base_submission.arxiv_id = '1901.00123' e = event.CreateSubmissionVersion(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.version == 2 assert sub.status == submission.Submission.WORKING @@ -53,12 +53,12 @@ def test_create_submission_version_invalid(mock_user, base_submission): base_submission.status = submission.Submission.WORKING e = event.CreateSubmissionVersion(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_rollback_v1(mock_user, base_submission): base_submission.version = 1 e = event.Rollback(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.status == submission.Submission.DELETED @@ -79,7 +79,7 @@ def test_rollback_v2(mock_user, base_submission): base_submission.versions = [v1] e = event.Rollback(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.version == 1 assert sub.metadata.title == "v1 title" @@ -90,24 +90,24 @@ def test_rollback_invalid(mock_user, base_submission): base_submission.arxiv_id = '1901.00123' e = event.Rollback(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent, match="Cannot already be announced"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_rollback_no_versions(mock_user, base_submission): base_submission.version = 2 base_submission.versions = [] e = event.Rollback(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent, match="No announced version to which to revert"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_confirm_contact_information(mock_user, base_submission): e = event.ConfirmContactInformation(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.submitter_contact_verified is True def test_confirm_authorship(mock_user, base_submission): e = event.ConfirmAuthorship(creator=mock_user, created=datetime.now(UTC), submitter_is_author=True) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.submitter_is_author is True @@ -117,7 +117,7 @@ def test_confirm_policy(mock_user, base_submission): created=datetime.now(UTC), agreement_id=3 # NEW REQUIRED FIELD ) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.submitter_accepts_policy is True @@ -127,7 +127,7 @@ def test_confirm_policy_sets_agreement_id(mock_user, base_submission): created=datetime.now(UTC), agreement_id=3 ) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.agreement_id == 3 @@ -135,26 +135,26 @@ def test_confirm_policy_sets_agreement_id(mock_user, base_submission): def test_set_primary_classification(mock_user, base_submission): category = 'astro-ph.GA' e = event.SetPrimaryClassification(creator=mock_user, created=datetime.now(UTC), category=category) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.primary_classification.category == category def test_set_primary_classification_invalid(mock_user, base_submission): e = event.SetPrimaryClassification(creator=mock_user, created=datetime.now(UTC), category=None) with pytest.raises(InvalidEvent, match="Must have a category"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_primary_classification_not_endorsed(mock_user, base_submission): mock_user.endorsements = [] e = event.SetPrimaryClassification(creator=mock_user, created=datetime.now(UTC), category='math.AG') with pytest.raises(InvalidEvent, match="Creator is not endorsed"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_add_secondary_classification(mock_user, base_submission): base_submission.primary_classification = meta.Classification('astro-ph.GA') category = 'astro-ph.CO' e = event.AddSecondaryClassification(creator=mock_user, created=datetime.now(UTC), category=category) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert category in sub.secondary_categories @@ -162,97 +162,97 @@ def test_remove_secondary_classification(mock_user, base_submission): category = 'astro-ph.CO' base_submission.secondary_classification = [meta.Classification(category)] e = event.RemoveSecondaryClassification(creator=mock_user, created=datetime.now(UTC), category=category) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert category not in sub.secondary_categories def test_set_license(mock_user, base_submission): uri = 'http://creativecommons.org/licenses/by/4.0/' e = event.SetLicense(creator=mock_user, created=datetime.now(UTC), license_uri=uri, license_name='CC BY 4.0') - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.license.uri == uri def test_set_title(mock_user, base_submission): title = "A very good title" e = event.SetTitle(creator=mock_user, created=datetime.now(UTC), title=title) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.title == title def test_set_abstract(mock_user, base_submission): abstract = "This is a very good abstract with enough length to pass validation." e = event.SetAbstract(creator=mock_user, created=datetime.now(UTC), abstract=abstract) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.abstract == abstract def test_set_doi(mock_user, base_submission): doi = "10.1000/182" e = event.SetDOI(creator=mock_user, created=datetime.now(UTC), doi=doi) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.doi == doi def test_set_msc_classification(mock_user, base_submission): msc = "14J60" e = event.SetMSCClassification(creator=mock_user, created=datetime.now(UTC), msc_class=msc) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.msc_class == msc def test_set_acm_classification(mock_user, base_submission): acm = "F.2.2" e = event.SetACMClassification(creator=mock_user, created=datetime.now(UTC), acm_class=acm) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.acm_class == "F.2.2" def test_set_journal_reference(mock_user, base_submission): ref = "Nature 2023" e = event.SetJournalReference(creator=mock_user, created=datetime.now(UTC), journal_ref=ref) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.journal_ref == ref def test_set_report_number(mock_user, base_submission): rep = "REP-001" e = event.SetReportNumber(creator=mock_user, created=datetime.now(UTC), report_num=rep) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.report_num == rep def test_set_comments(mock_user, base_submission): comm = "Some comments" e = event.SetComments(creator=mock_user, created=datetime.now(UTC), comments=comm) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.comments == comm def test_set_authors(mock_user, base_submission): authors = [submission.Author(forename="John", surname="Doe", display="John Doe")] e = event.SetAuthors(creator=mock_user, created=datetime.now(UTC), authors=authors) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.metadata.authors_display == "John Doe" def test_confirm_source_processed(mock_user, base_submission): e = event.ConfirmSourceProcessed(creator=mock_user, created=datetime.now(UTC), source_id=123) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.is_source_processed is True def test_unconfirm_source_processed(mock_user, base_submission): base_submission.is_source_processed = True e = event.UnConfirmSourceProcessed(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.is_source_processed is False def test_confirm_preview(mock_user, base_submission): base_submission.preview = preview.Preview(source_id=123, source_checksum="abc", preview_checksum="def", size_bytes=100, added=datetime.now(UTC)) e = event.ConfirmPreview(creator=mock_user, created=datetime.now(UTC), preview_checksum="def") - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.submitter_confirmed_preview is True @@ -260,40 +260,40 @@ def test_confirm_preview_invalid_checksum(mock_user, base_submission): base_submission.preview = preview.Preview(source_id=123, source_checksum="abc", preview_checksum="def", size_bytes=100, added=datetime.now(UTC)) e = event.ConfirmPreview(creator=mock_user, created=datetime.now(UTC), preview_checksum="wrong") with pytest.raises(InvalidEvent, match="Checksum wrong does not match"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_confirm_preview_no_preview(mock_user, base_submission): base_submission.preview = None e = event.ConfirmPreview(creator=mock_user, created=datetime.now(UTC), preview_checksum="def") with pytest.raises(InvalidEvent, match="Preview not set on submission"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_primary_classification_already_announced(mock_user, base_submission): base_submission.status = submission.Submission.ANNOUNCED base_submission.arxiv_id = '1901.00123' e = event.SetPrimaryClassification(creator=mock_user, created=datetime.now(UTC), category='astro-ph.GA') with pytest.raises(InvalidEvent, match="Can only be set on the first version"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_remove_secondary_classification_not_present(mock_user, base_submission): e = event.RemoveSecondaryClassification(creator=mock_user, created=datetime.now(UTC), category='math.AG') with pytest.raises(InvalidEvent, match="No such category on submission"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_license_invalid_uri(mock_user, base_submission): e = event.SetLicense(creator=mock_user, created=datetime.now(UTC), license_uri="http://invalid") with pytest.raises(InvalidEvent, match="License URL is not on the list of valid licenses"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_title_html_escapes(mock_user, base_submission): e = event.SetTitle(creator=mock_user, created=datetime.now(UTC), title="A title with & escape") with pytest.raises(InvalidEvent, match="Title may not contain HTML escapes"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_title_invalid_html(mock_user, base_submission): e = event.SetTitle(creator=mock_user, created=datetime.now(UTC), title="A title with ") with pytest.raises(InvalidEvent, match="Title contains unacceptable HTML tags"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_set_abstract_invalid_length(mock_user, base_submission): # SetAbstract.validate calls metacheck.check_abstract @@ -303,20 +303,20 @@ def test_set_abstract_invalid_length(mock_user, base_submission): def test_finalize_submission_missing_fields(mock_user, base_submission): e = event.FinalizeSubmission(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent, match="Missing primary_classification"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_unfinalize_submission_not_finalized(mock_user, base_submission): base_submission.status = submission.Submission.WORKING e = event.UnFinalizeSubmission(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent, match="Submission is not finalized"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_unfinalize_submission_announced(mock_user, base_submission): base_submission.status = submission.Submission.ANNOUNCED base_submission.arxiv_id = '1901.00123' e = event.UnFinalizeSubmission(creator=mock_user, created=datetime.now(UTC)) with pytest.raises(InvalidEvent, match="Cannot unfinalize an announced paper"): - e.validate(base_submission) + e.validate_pre_lock(base_submission) def test_add_feature_invalid_type(mock_user, base_submission): from pydantic import ValidationError @@ -337,14 +337,14 @@ def test_finalize_submission(mock_user, base_submission): base_submission.uncompressed_size = 100 e = event.FinalizeSubmission(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.status == submission.Submission.SUBMITTED def test_unfinalize_submission(mock_user, base_submission): base_submission.status = submission.Submission.SUBMITTED e = event.UnFinalizeSubmission(creator=mock_user, created=datetime.now(UTC)) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.status == submission.Submission.WORKING @@ -353,7 +353,7 @@ def test_announce(mock_user, base_submission): arxiv_id = '2303.00001' base_submission.versions = [] e = event.Announce(creator=mock_user, created=datetime.now(UTC), arxiv_id=arxiv_id) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.arxiv_id == arxiv_id assert sub.status == submission.Submission.ANNOUNCED @@ -361,7 +361,7 @@ def test_announce(mock_user, base_submission): def test_add_feature(mock_user, base_submission): e = event.AddFeature(creator=mock_user, created=datetime.now(UTC), feature_type=annotation.Feature.Type.WORD_COUNT, feature_value=500) e.created = datetime.now(UTC) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert any(isinstance(a, annotation.Feature) for a in sub.annotations.values()) @@ -369,13 +369,13 @@ def test_add_classifier_results(mock_user, base_submission): results = [annotation.ClassifierResult(category='astro-ph.GA', probability=0.9)] e = event.AddClassifierResults(creator=mock_user, created=datetime.now(UTC), results=results) e.created = datetime.now(UTC) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert any(isinstance(a, annotation.ClassifierResults) for a in sub.annotations.values()) def test_reclassify(mock_user, base_submission): category = 'astro-ph.CO' e = event.Reclassify(creator=mock_user, created=datetime.now(UTC), category=category) - e.validate(base_submission) + e.validate_pre_lock(base_submission) sub = e.project(base_submission) assert sub.primary_classification.category == category diff --git a/submit_ce/domain/event/tests/test_more_event_branches.py b/submit_ce/domain/event/tests/test_more_event_branches.py index a8249dd6..07ac994c 100644 --- a/submit_ce/domain/event/tests/test_more_event_branches.py +++ b/submit_ce/domain/event/tests/test_more_event_branches.py @@ -71,12 +71,12 @@ def test_set_title_rejects_all_caps(): SetTitle should reject titles that are entirely uppercase. Why: The event validation explicitly checks for all-caps titles. - Expectation: InvalidEvent is raised by .validate(submission). + Expectation: InvalidEvent is raised by .validate_pre_lock(submission). """ s = _blank_submission() e = SetTitle(creator=s.creator, title="ALL CAPS TITLE") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_set_title_rejects_trailing_period(): @@ -84,12 +84,12 @@ def test_set_title_rejects_trailing_period(): SetTitle should reject titles ending with a trailing period. Why: Title validation includes a "no trailing '.'" rule. - Expectation: InvalidEvent is raised by .validate(submission). + Expectation: InvalidEvent is raised by .validate_pre_lock(submission). """ s = _blank_submission() e = SetTitle(creator=s.creator, title="Ends with period.") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_set_abstract_length_bounds_both_paths(): @@ -106,12 +106,12 @@ def test_set_abstract_length_bounds_both_paths(): # Too short: MIN_LENGTH is 20, so this should fail. e_short = SetAbstract(creator=s.creator, abstract="too short") with pytest.raises(InvalidEvent): - e_short.validate(s) + e_short.validate_pre_lock(s) # Reasonable: 25 chars satisfies the minimum. ok_text = "This abstract is valid length." e_ok = SetAbstract(creator=s.creator, abstract=ok_text) - e_ok.validate(s) # no exception means the branch was accepted + e_ok.validate_pre_lock(s) # no exception means the branch was accepted def test_set_license_rejects_invalid_uri(): @@ -119,12 +119,12 @@ def test_set_license_rejects_invalid_uri(): SetLicense should reject license URIs not present in the allowed set. Why: The validator cross-checks the URI against the current LICENSES list. - Expectation: InvalidEvent is raised by .validate(submission). + Expectation: InvalidEvent is raised by .validate_pre_lock(submission). """ s = _blank_submission() e = SetLicense(creator=s.creator, license_uri="http://not-on-our-list") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_abstract_rejects_when_not_capitalized(): @@ -135,7 +135,7 @@ def test_abstract_rejects_when_not_capitalized(): s = _blank_submission() e = SetAbstract(creator=s.creator, abstract="not capitalized first sentence.") with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_abstract_rejects_when_too_long(): @@ -148,7 +148,7 @@ def test_abstract_rejects_when_too_long(): too_long = "A" + ("x" * 2000) e = SetAbstract(creator=s.creator, abstract=too_long) with pytest.raises(InvalidEvent): - e.validate(s) + e.validate_pre_lock(s) def test_remove_secondary_requires_existing_category_then_accepts(): @@ -165,12 +165,12 @@ def test_remove_secondary_requires_existing_category_then_accepts(): # Missing category -> should raise e_missing = RemoveSecondaryClassification(creator=s.creator, category="cond-mat.dis-nn") with pytest.raises(InvalidEvent): - e_missing.validate(s) + e_missing.validate_pre_lock(s) # Add the category, then validate again -> should pass s.secondary_classification.append(meta.Classification("cond-mat.dis-nn")) e_present = RemoveSecondaryClassification(creator=s.creator, category="cond-mat.dis-nn") - e_present.validate(s) + e_present.validate_pre_lock(s) def test_finalize_submission_missing_required_fields(): @@ -183,5 +183,5 @@ def test_finalize_submission_missing_required_fields(): s = _blank_submission() e = FinalizeSubmission(creator=s.creator, created=_now()) with pytest.raises(InvalidEvent): - e.apply(s) # .apply() triggers .validate() internally + e.apply(s) # .apply() triggers .validate_pre_lock() internally diff --git a/submit_ce/domain/event/tests/test_requests.py b/submit_ce/domain/event/tests/test_requests.py index f7cdf5bc..c135d20c 100644 --- a/submit_ce/domain/event/tests/test_requests.py +++ b/submit_ce/domain/event/tests/test_requests.py @@ -44,14 +44,14 @@ def test_approve_request(self): # Test valid approval e = ApproveRequest(creator=self.user, request_id=request_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(updated_submission.user_requests[request_id].status, UserRequest.APPROVED) # Test invalid approval (non-existent request) e_invalid = ApproveRequest(creator=self.user, request_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_reject_request(self): """Test RejectRequest validation and projection.""" @@ -66,14 +66,14 @@ def test_reject_request(self): # Test valid rejection e = RejectRequest(creator=self.user, request_id=request_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(updated_submission.user_requests[request_id].status, UserRequest.REJECTED) # Test invalid rejection (non-existent request) e_invalid = RejectRequest(creator=self.user, request_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_cancel_request(self): """Test CancelRequest validation and projection.""" @@ -88,14 +88,14 @@ def test_cancel_request(self): # Test valid cancellation e = CancelRequest(creator=self.user, request_id=request_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(updated_submission.user_requests[request_id].status, UserRequest.CANCELLED) # Test invalid cancellation (non-existent request) e_invalid = CancelRequest(creator=self.user, request_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_apply_request(self): """Test ApplyRequest validation and projection.""" @@ -117,7 +117,7 @@ def apply(self, sub: Submission) -> Submission: # Test valid application e = ApplyRequest(creator=self.user, request_id=request_id) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(updated_submission.user_requests[request_id].status, UserRequest.APPLIED) self.assertEqual(updated_submission.metadata.title, "Updated Title") @@ -125,7 +125,7 @@ def apply(self, sub: Submission) -> Submission: # Test invalid application (non-existent request) e_invalid = ApplyRequest(creator=self.user, request_id="nonexistent") with self.assertRaises(InvalidEvent): - e_invalid.validate(self.submission) + e_invalid.validate_pre_lock(self.submission) def test_request_crosslist(self): """Test RequestCrossList validation and projection.""" @@ -136,7 +136,7 @@ def test_request_crosslist(self): # Test valid crosslist request created = datetime.now(UTC) e = RequestCrossList(creator=self.user, created=created, categories=["astro-ph.GA"]) - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(len(updated_submission.user_requests), 1) @@ -150,14 +150,14 @@ def test_request_crosslist(self): # Test invalid: not announced self.submission.status = Submission.WORKING with self.assertRaises(InvalidEvent) as cm: - e.validate(self.submission) + e.validate_pre_lock(self.submission) self.assertIn("Submission must already be announced", str(cm.exception)) self.submission.status = Submission.ANNOUNCED # Test invalid: already primary e_bad = RequestCrossList(creator=self.user, categories=["physics.gen-ph"]) with self.assertRaises(InvalidEvent): - e_bad.validate(self.submission) + e_bad.validate_pre_lock(self.submission) def test_request_withdrawal(self): """Test RequestWithdrawal validation and projection.""" @@ -167,7 +167,7 @@ def test_request_withdrawal(self): # Test valid withdrawal request created = datetime.now(UTC) e = RequestWithdrawal(creator=self.user, created=created, reason="Too many typos") - e.validate(self.submission) + e.validate_pre_lock(self.submission) updated_submission = e.project(self.submission) self.assertEqual(len(updated_submission.user_requests), 1) @@ -181,18 +181,18 @@ def test_request_withdrawal(self): # Test invalid: no reason e_no_reason = RequestWithdrawal(creator=self.user, reason="") with self.assertRaises(InvalidEvent) as cm: - e_no_reason.validate(self.submission) + e_no_reason.validate_pre_lock(self.submission) self.assertIn("Provide a reason", str(cm.exception)) # Test invalid: reason too long e_long_reason = RequestWithdrawal(creator=self.user, reason="a" * 401) with self.assertRaises(InvalidEvent) as cm: - e_long_reason.validate(self.submission) + e_long_reason.validate_pre_lock(self.submission) self.assertIn("400 characters or less", str(cm.exception)) # Test invalid: not announced self.submission.status = Submission.WORKING e_valid_reason = RequestWithdrawal(creator=self.user, reason="valid") with self.assertRaises(InvalidEvent) as cm: - e_valid_reason.validate(self.submission) + e_valid_reason.validate_pre_lock(self.submission) self.assertIn("Submission must already be announced", str(cm.exception)) diff --git a/submit_ce/implementations/legacy_implementation/__init__.py b/submit_ce/implementations/legacy_implementation/__init__.py index a5aefce6..8b8b534b 100644 --- a/submit_ce/implementations/legacy_implementation/__init__.py +++ b/submit_ce/implementations/legacy_implementation/__init__.py @@ -191,8 +191,9 @@ def _save(self, *events, # validate_under_lock runs inside the locked # transaction so it can inspect on-disk / FileStore # state without racing against another writer. Raising - # InvalidEvent here rolls the transaction back; the - # caller's `except InvalidEvent` decides UX. + # InvalidEvent here rolls the DB transaction back. + # Any earlier EventWithSideEffect.execute() is not rolled back. + # The caller's `except InvalidEvent` decides UX. event.validate_under_lock(self, before) logger.debug('Execute event %s: %s', event.event_id, event.NAME) event.execute(self, before) diff --git a/submit_ce/ui/controllers/util.py b/submit_ce/ui/controllers/util.py index 1aa97cfa..4f8daeb7 100644 --- a/submit_ce/ui/controllers/util.py +++ b/submit_ce/ui/controllers/util.py @@ -79,7 +79,7 @@ def validate_command(form: Form, event: Event, bool """ try: - event.validate(submission) + event.validate_pre_lock(submission) return True except InvalidEvent as e: # This use of _errors causes a problem in WTForms 2.3.3 diff --git a/submit_ce/ui/tests/test_save_validate_under_lock.py b/submit_ce/ui/tests/test_save_validate_under_lock.py index e7c29138..97b7d35c 100644 --- a/submit_ce/ui/tests/test_save_validate_under_lock.py +++ b/submit_ce/ui/tests/test_save_validate_under_lock.py @@ -35,7 +35,7 @@ class _ProbeSideEffect(EventWithSideEffect): should_block: bool = False calls: List[str] = [] - def validate(self, submission) -> None: + def validate_pre_lock(self, submission) -> None: pass def validate_under_lock(self, api, submission) -> None: @@ -61,7 +61,7 @@ class _ProbeNoOverride(EventWithSideEffect): calls: List[str] = [] - def validate(self, submission) -> None: + def validate_pre_lock(self, submission) -> None: pass def execute(self, api, submission) -> None: From 6cfee3c0ee8ea6779cc8ce63e3269b28aef610fc Mon Sep 17 00:00:00 2001 From: "Brian D. Caruso" Date: Fri, 12 Jun 2026 14:50:17 -0400 Subject: [PATCH 6/6] Adds test that oversize message formats size correctly --- .../ui/controllers/new/tests/test_upload.py | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/submit_ce/ui/controllers/new/tests/test_upload.py b/submit_ce/ui/controllers/new/tests/test_upload.py index 52b9b35a..4cef91c4 100644 --- a/submit_ce/ui/controllers/new/tests/test_upload.py +++ b/submit_ce/ui/controllers/new/tests/test_upload.py @@ -167,6 +167,73 @@ def test_post_upload(self): self.assertEqual(get_controllers_desire(data), STAGE_RESHOW, 'Successful upload and reshow form') + @mock.patch(f'{upload.__name__}.AddfilesForm.Meta.csrf', False) + def test_oversize_warning_reads_50_MiB(self): + """An oversize upload flashes a warning naming the 50 MiB limit. + + The displayed limit must read as the IEC binary "50.00 MiB" (the + 50 * 1024 * 1024 byte guideline), not a decimal-MB conversion such as + "52.4 MB"/"51.2 MB" or an off-by-rounding "49.9 MB". + """ + submission_id = 2 + mock_submission = mock.MagicMock( + submission_id=submission_id, uncompressed_size=593920, + is_finalized=False, is_announced=False, arxiv_id=None, version=1, + is_oversize=True, + ) + workspace = Workspace( + identifier='25', + checksum='a1s2d3f4', + size=593920, + started=datetime.now(), + completed=datetime.now(), + created=datetime.now(), + modified=datetime.now(), + status=UploadStatus.READY, + source_format=SourceFormat.TEX, + lifecycle=UploadLifecycleStates.ACTIVE, + locked=False, + files=[FileStatus( + path='', + name='thebestfile.pdf', + content_type='application/pdf', + bytes=20505, + crc32c='fakecrc', + url='https://example.com/thebestfile.pdf', + is_versioned=True, + modified=datetime.now(), + ancillary=False, + errors=[] + )], + errors=[] + ) + params = MultiDict({}) + mock_file = mock.MagicMock(filename='thebestfile.pdf', + content_type='application/pdf') + files = MultiDict({'file': mock_file}) + with self.app.app_context(): + mock_api = mock.MagicMock() + mock_api.get_with_history.return_value = (mock_submission, []) + mock_api.save.return_value = (mock_submission, []) + mock_api.get_file_store.return_value.get_workspace.return_value = \ + workspace + with mock.patch.object(self.app, 'api', mock_api), \ + mock.patch(f'{upload.__name__}.alerts') as mock_alerts: + upload.upload_files('POST', params, self.session, + submission_id, files=files, + token='footoken') + + warnings = [str(call.args[0]) + for call in mock_alerts.flash_warning.call_args_list] + oversize = [w for w in warnings if 'size guideline' in w] + self.assertEqual(len(oversize), 1, + 'Exactly one oversize warning is flashed') + message = oversize[0] + self.assertIn('50.00 MiB', message, + 'Limit reads as the IEC binary 50.00 MiB') + + + class TestDelete(CtrlBase): """Tests for :func:`submit_ce.controllers.upload.delete`."""