From 8081c9640a1ade6b75493a028794474f4937b147 Mon Sep 17 00:00:00 2001 From: Travis Briggs Date: Wed, 10 Jun 2026 17:58:14 -0700 Subject: [PATCH 1/2] Call validate() in AbstractBuilder before building AbstractBuilder.materialize now validates the builder params before calling build(), raising Wp1RetryableSelectionError (saved as a CAN_RETRY selection with error messages) when validation fails. The equivalent error check is refactored out of SimpleBuilder.build, which now uses validate() only for list normalization. Fixes #1180 Co-Authored-By: Claude Fable 5 --- wp1/selection/abstract_builder.py | 13 +++++++ wp1/selection/abstract_builder_test.py | 53 ++++++++++++++++++++++++-- wp1/selection/models/simple.py | 11 ++---- wp1/selection/models/simple_test.py | 12 ++++-- 4 files changed, 74 insertions(+), 15 deletions(-) diff --git a/wp1/selection/abstract_builder.py b/wp1/selection/abstract_builder.py index a8af17ebd..e90f4346a 100644 --- a/wp1/selection/abstract_builder.py +++ b/wp1/selection/abstract_builder.py @@ -61,6 +61,19 @@ def materialize( ) selection.set_id() try: + _, invalid, errors = self.validate( + project=builder.b_project.decode("utf-8"), + wp10db=wp10db, + user_id=builder.b_user_id, + builder_id=builder.b_id, + **params, + ) + if invalid or errors: + raise Wp1RetryableSelectionError( + "The selection contained invalid parameters: %s" + % "; ".join(errors) + ) + selection.data = self.build( content_type, project=builder.b_project.decode("utf-8"), diff --git a/wp1/selection/abstract_builder_test.py b/wp1/selection/abstract_builder_test.py index 77fdd1775..6e2e78537 100644 --- a/wp1/selection/abstract_builder_test.py +++ b/wp1/selection/abstract_builder_test.py @@ -18,8 +18,11 @@ class TestBuilder(AbstractBuilder): def build(self, content_type, **params): return "\n".join(params["list"]).encode("utf-8") + def validate(self, **params): + return ([], [], []) -class TestBuilderRetryable(AbstractBuilder): + +class TestBuilderRetryable(TestBuilder): def build(self, content_type, **params): try: @@ -28,7 +31,7 @@ def build(self, content_type, **params): raise Wp1RetryableSelectionError("Could not convert to int") from e -class TestBuilderFatal(AbstractBuilder): +class TestBuilderFatal(TestBuilder): def build(self, content_type, **params): try: @@ -37,13 +40,13 @@ def build(self, content_type, **params): raise Wp1FatalSelectionError("Could not convert to int") from e -class TestBuilderNoContext(AbstractBuilder): +class TestBuilderNoContext(TestBuilder): def build(self, content_type, **params): raise Wp1FatalSelectionError("Something broke") -class TestBuilderSuppressedException(AbstractBuilder): +class TestBuilderSuppressedException(TestBuilder): def build(self, content_type, **params): try: @@ -52,6 +55,12 @@ def build(self, content_type, **params): raise Wp1SelectionError("Just this thing, really") from None +class TestBuilderInvalidParams(TestBuilder): + + def validate(self, **params): + return ([], ["bad_item"], ["The list contained an invalid item"]) + + class AbstractBuilderTest(BaseWpOneDbTest): def setUp(self): @@ -216,6 +225,42 @@ def test_materialize_suppressed_message(self): json.loads(actual.s_error_messages), ) + def test_materialize_validates_before_building(self): + builder_obj = TestBuilderInvalidParams() + builder_obj.build = MagicMock() + builder_obj.materialize( + self.s3, self.wp10db, self.builder, "text/tab-separated-values", 1 + ) + + builder_obj.build.assert_not_called() + actual = get_first_selection(self.wp10db) + + self.assertEqual(b"CAN_RETRY", actual.s_status) + self.assertEqual( + { + "error_messages": [ + "The selection contained invalid parameters: " + "The list contained an invalid item" + ] + }, + json.loads(actual.s_error_messages), + ) + + def test_materialize_passes_builder_context_to_validate(self): + builder_obj = TestBuilder() + builder_obj.validate = MagicMock(return_value=([], [], [])) + builder_obj.materialize( + self.s3, self.wp10db, self.builder, "text/tab-separated-values", 1 + ) + + builder_obj.validate.assert_called_once_with( + project="en.wikipedia.fake", + wp10db=self.wp10db, + user_id=1234, + builder_id=b"1a-2b-3c-4d", + list=["a", "b", "c"], + ) + def test_materialize_update_article_count(self): self.test_builder.materialize( self.s3, self.wp10db, self.builder, "text/tab-separated-values", 1 diff --git a/wp1/selection/models/simple.py b/wp1/selection/models/simple.py index 05e2428e2..a6acbdd33 100644 --- a/wp1/selection/models/simple.py +++ b/wp1/selection/models/simple.py @@ -1,7 +1,7 @@ import urllib.parse from typing import Any -from wp1.exceptions import Wp1FatalSelectionError, Wp1RetryableSelectionError +from wp1.exceptions import Wp1FatalSelectionError from wp1.selection.abstract_builder import AbstractBuilder @@ -32,12 +32,9 @@ def build(self, content_type: str, **params: Any) -> bytes: if "list" not in params: raise Wp1FatalSelectionError("Missing required param: list") - valid, invalid, errors = self.validate(**params) - - if errors: - raise Wp1RetryableSelectionError( - "The selection contained invalid parameters" - ) + # Validation errors are handled by AbstractBuilder.materialize before + # build is called; validate is used here only to normalize the list. + valid, _, _ = self.validate(**params) return "\n".join(valid).encode("utf-8") diff --git a/wp1/selection/models/simple_test.py b/wp1/selection/models/simple_test.py index e684c4090..1f91279a4 100644 --- a/wp1/selection/models/simple_test.py +++ b/wp1/selection/models/simple_test.py @@ -2,7 +2,7 @@ from unittest.mock import MagicMock from wp1.base_db_test import BaseWpOneDbTest, get_first_selection -from wp1.exceptions import Wp1FatalSelectionError, Wp1RetryableSelectionError +from wp1.exceptions import Wp1FatalSelectionError from wp1.models.wp10.builder import Builder from wp1.selection.models.simple import Builder as SimpleBuilder @@ -136,10 +136,14 @@ def test_build_decodes_utf8_and_url_encoding(self): ) self.assertEqual(expected, actual) - def test_build_disallows_invalid(self): + def test_materialize_disallows_invalid(self): simple_test_builder = SimpleBuilder() - with self.assertRaises(Wp1RetryableSelectionError): - simple_test_builder.build("text/tab-separated-values", list=["Foo#Bar"]) + self.builder.b_params = '{"list":["Foo#Bar"]}' + simple_test_builder.materialize( + self.s3, self.wp10db, self.builder, "text/tab-separated-values", 1 + ) + actual = get_first_selection(self.wp10db) + self.assertEqual(b"CAN_RETRY", actual.s_status) def test_validate_items(self): simple_builder_test = SimpleBuilder() From 3b570e5735732666c6def4259a2efae6f1a99a8f Mon Sep 17 00:00:00 2001 From: Travis Briggs Date: Wed, 10 Jun 2026 20:04:28 -0700 Subject: [PATCH 2/2] Reformat with black --- wp1/selection/abstract_builder.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/wp1/selection/abstract_builder.py b/wp1/selection/abstract_builder.py index e90f4346a..16b856f03 100644 --- a/wp1/selection/abstract_builder.py +++ b/wp1/selection/abstract_builder.py @@ -70,8 +70,7 @@ def materialize( ) if invalid or errors: raise Wp1RetryableSelectionError( - "The selection contained invalid parameters: %s" - % "; ".join(errors) + "The selection contained invalid parameters: %s" % "; ".join(errors) ) selection.data = self.build(