diff --git a/submit_ce/api/submit.py b/submit_ce/api/submit.py index c5dfc31d..cda8af5d 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 SIZE_LIMIT_POLICY, 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 SIZE_LIMIT_POLICY + @abstractmethod def get(self, submission_id: str) -> Submission: """ diff --git a/submit_ce/domain/event/__init__.py b/submit_ce/domain/event/__init__.py index 57c68fe0..3444d5fb 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 @@ -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: @@ -978,7 +978,9 @@ class FinalizeSubmission(Event): ] REQUIRED_METADATA: ClassVar[str] = ['title', 'abstract', 'authors_display'] - def validate(self, submission: Submission) -> None: + CONSEQUENCE_TYPES = frozenset({AddHold}) + + 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") @@ -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: @@ -1008,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: @@ -1034,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. @@ -1069,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') @@ -1095,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') @@ -1115,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') @@ -1136,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') @@ -1158,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]) @@ -1188,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]) @@ -1217,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 2378c95d..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. @@ -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. @@ -88,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.""" @@ -123,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. @@ -140,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: @@ -150,6 +169,40 @@ 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. + """ + 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 + @functools.cache def _get_subclasses(klass: Type[Event]) -> List[Type[Event]]: diff --git a/submit_ce/domain/event/file.py b/submit_ce/domain/event/file.py index f59445e5..c00c942e 100644 --- a/submit_ce/domain/event/file.py +++ b/submit_ce/domain/event/file.py @@ -2,13 +2,16 @@ 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 logger = logging.getLogger(__name__) @@ -25,6 +28,39 @@ def _common_file_change_execute(api: SubmitApi, submission: Submission) -> None: file_store.delete_preview(str(submission.submission_id)) +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. + + 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 [] + 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.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.""" @@ -37,17 +73,26 @@ class UploadArchive(EventWithSideEffect): bytes_added: int = 0 """Bytes added by uploading this archive.""" - def validate(self, submission: Submission) -> None: + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" + + 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") 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 = _add_evaluate_oversize(api, submission, self.bytes_added, files) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission @@ -66,19 +111,26 @@ class UploadFiles(EventWithSideEffect): Field(default_factory=list, exclude=True) bytes_added: int = 0 - def validate(self, submission: Submission) -> None: + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" + + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) 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 = _add_evaluate_oversize(api, submission, self.bytes_added, stats) def project(self, submission: Submission) -> Submission: submission.uncompressed_size += self.bytes_added + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission @@ -96,7 +148,10 @@ class RemoveFiles(EventWithSideEffect): bytes_removed:int = 0 """Bytes removed by removing these files.""" - def validate(self, submission: Submission) -> None: + oversize: list[size_limits.OversizeReason] = [] + """`OversizeReason` instances after this change (set in execute).""" + + def validate_pre_lock(self, submission: Submission) -> None: validators.submission_is_not_finalized(self, submission) def execute(self, api: SubmitApi, submission: Submission) -> None: @@ -109,9 +164,11 @@ def execute(self, api: SubmitApi, submission: Submission) -> None: self.bytes_removed += file.bytes _common_file_change_execute(api, submission) + self.oversize = _workspace_evaluate_oversize(api, submission) def project(self, submission: Submission) -> Submission: submission.uncompressed_size -= self.bytes_removed + submission.is_oversize = bool(self.oversize) _common_file_change_project(submission) return submission @@ -122,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: @@ -134,5 +191,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/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_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_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_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/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 new file mode 100644 index 00000000..da0ba7c7 --- /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_pre_lock(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_pre_lock(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_pre_lock(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/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_oversize_detection.py b/submit_ce/domain/event/tests/test_oversize_detection.py new file mode 100644 index 00000000..252ebb22 --- /dev/null +++ b/submit_ce/domain/event/tests/test_oversize_detection.py @@ -0,0 +1,206 @@ +"""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. +""" + +import io +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 ( + SIZE_LIMIT_POLICY, + SizeLimits, + OversizeReason, +) + +MB = 1024 * 1024 + + +class _FakeStore: + def __init__(self, workspace=None, unpacked=None): + self.workspace = workspace + self.unpacked = unpacked or [] + + def store_source_package(self, sid, content, chunk_size): + # The unpacked FileStatus list is configured per-test. + return list(self.unpacked) + + def store_source_file(self, sid, content, chunk_size): + # 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 + + 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=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 + + 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 _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=[]) + + +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(): + # 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(limits=limits) + e = UploadFiles(creator=s.creator, files=[_upload("huge.pdf", 60 * MB)]) + e.execute(api, s) + 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() + 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 + + +def test_upload_archive_flags_oversize(): + s = _submission() + 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 + + +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 reasons come from the stored event. + s = _submission() + 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 + + +def test_no_workspace_is_not_oversize(): + s = _submission() + api = _FakeApi(None) + e = UploadFiles(creator=s.creator, files=[]) + e.execute(api, s) + assert not e.oversize + + +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(limits=limits) + e = UploadFiles(creator=s.creator, files=[_upload("f", 10 * MB)]) + e.execute(api, s) + 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/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/domain/size_limits.py b/submit_ce/domain/size_limits.py new file mode 100644 index 00000000..211f8da9 --- /dev/null +++ b/submit_ce/domain/size_limits.py @@ -0,0 +1,159 @@ +"""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. + +* ``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 MB, the legacy arXiv size guideline. + +Only the total and per-file uncompressed limits are *enforced* by +: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. +""" + +from dataclasses import dataclass +from typing import List, Literal, Mapping, Optional + +from pydantic import BaseModel, ConfigDict + +from arxiv.taxonomy.definitions import CATEGORIES + +ONE_MB = 1024 * 1024 + +DEFAULT_MAX_SIZE_BYTES = 50 * ONE_MB + +DEFAULT_ARCHIVE = "default" +"""Fallback key used when a category has no archive-specific override.""" + + +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] + + @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) + + +def _mb(num_bytes: int) -> str: + return f"{num_bytes / ONE_MB:.1f} MB" + + +class OversizeReason(BaseModel): + """One reason a submission is oversize. + + The human-readable text is built on demand by :meth:`message` from the + structured fields, so it is never stored or serialized.""" + + 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_bytes: int + """The measured size, in bytes.""" + + path: Optional[str] = None + """The offending file, for ``'PER_FILE'`` reasons.""" + + 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: SizeLimits) -> 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. 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. + """ + 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", + 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", + limit_bytes=per_file_limit, + actual_bytes=size, + path=path, + )) + return 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/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..bff798c5 --- /dev/null +++ b/submit_ce/domain/tests/test_size_limits.py @@ -0,0 +1,129 @@ +"""Tests for the size-limit policy module (:mod:`submit_ce.domain.size_limits`).""" + +from submit_ce.domain.size_limits import ( + ONE_MB, + DEFAULT_MAX_SIZE_BYTES, + SizeLimits, + SIZE_LIMIT_POLICY, + archive_for_category, + check_sizes, +) + +MB = ONE_MB + + +# --- limit values & lookups ------------------------------------------------- + +def test_default_limit_is_50mb(): + assert DEFAULT_MAX_SIZE_BYTES == 50 * ONE_MB + + +def test_defaults_populate_all_three_limits(): + 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_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(): + 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 = 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 == [] + + +def test_total_over_limit_flags_total(): + 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_bytes == 60 * MB + assert reasons[0].limit_bytes == DEFAULT_MAX_SIZE_BYTES + + +def test_per_file_over_limit_flags_each_file(): + 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].path == "huge.dat" + assert reasons[0].actual_bytes == 60 * MB + + +def test_both_dimensions_can_trip(): + 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"] + + +def test_exactly_at_limit_is_not_oversize(): + 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 compressed limit but generous total/per-file: check_sizes only + # looks at total + per-file, never compressed. + 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) == [] + + +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 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/__init__.py b/submit_ce/implementations/legacy_implementation/__init__.py index ecb89f08..8b8b534b 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 @@ -183,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) @@ -199,6 +208,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/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/models.py b/submit_ce/implementations/legacy_implementation/models.py index 380f4f34..fc5403ba 100644 --- a/submit_ce/implementations/legacy_implementation/models.py +++ b/submit_ce/implementations/legacy_implementation/models.py @@ -320,6 +320,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/conftest.py b/submit_ce/ui/conftest.py index d8335dbe..89f167bc 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,51 @@ 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 + fake_stat.path = "huge.pdf" + + 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.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`.""" 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 51a1c8f5..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 @@ -251,6 +252,24 @@ def _get_upload(params: MultiDict, session: Session, submission: Submission, return rdata, status.OK, {} +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(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') + + def _upload_archive(form: AddfilesForm, file: FileStorage, submitter: User, client: Client, submission: Submission, rdata: Dict[str, Any], token: str) \ @@ -260,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: @@ -287,6 +306,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}) @@ -302,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: @@ -330,6 +350,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/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/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/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 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..0c3a8a9c --- /dev/null +++ b/submit_ce/ui/tests/test_finalize_oversize_hold.py @@ -0,0 +1,87 @@ +"""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 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 + + 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 + fake_stat.path = "huge.pdf" + 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 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..3ef99d92 --- /dev/null +++ b/submit_ce/ui/tests/test_oversize_upload.py @@ -0,0 +1,30 @@ +"""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 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 + assert uploads[-1].oversize[0].kind == "TOTAL" 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: