Call validate() in AbstractBuilder before building

wp1/selection/abstract_builder.py

@@ -61,6 +61,18 @@ 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"),

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

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")
 

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()