Fail the JSON Builder when no Sequence Element has produced a sequence number

pipeline/fiftyone_pipeline_core/src/fiftyone_pipeline_core/jsonbundler.py

@@ -57,6 +57,16 @@ def process_internal(self, flowdata):
 
         """
 
+        # The sequence number is the one the Sequence Element has put in the
+        # Evidence as a whole number. Text in the query string does not
+        # count, and without a number no JSON is set.
+        sequence = flowdata.evidence.get("query.sequence")
+        if isinstance(sequence, bool) or not isinstance(sequence, int):
+            raise Exception(
+                "The JSON Builder needs the sequence number that a Sequence "
+                "Element produces. A Sequence Element needs to be placed "
+                "before it in the Pipeline.")
+
         # Get every property on every FlowElement
         # Storing JavaScript properties in an extra section
 
@@ -178,9 +188,7 @@ def process_internal(self, flowdata):
                 if value is None:
                     output[flow_element.datakey.lower()][propertykey.lower() + "nullreason"] = null_reason
 
-                sequence = flowdata.evidence.get("query.sequence")
-
-                if sequence is None or sequence < 10: 
+                if sequence < 10:
 
                     if "type" in propertymeta and propertymeta["type"].lower() == "javascript":
 

pipeline/fiftyone_pipeline_core/tests/test_javascriptbuilder_sequence.py

@@ -41,25 +41,19 @@
 from parameterized import parameterized
 
 from fiftyone_pipeline_core import javascriptbuilder
-from fiftyone_pipeline_core.javascriptbuilder import JavascriptBuilderElement
-from fiftyone_pipeline_core.jsonbundler import JSONBundlerElement
 from fiftyone_pipeline_core.pipelinebuilder import PipelineBuilder
 
 NODE = shutil.which("node")
 
 
-def _render(evidence=None, minify=False, sequence_element=False):
-    """Render the script. With sequence_element False the pipeline has only
-    the JSON bundler and the JavaScript builder."""
+def _render(evidence=None, minify=False, sequence_element=True):
+    """Render the script. The JSON builder fails without a sequence number,
+    so the pipeline always has the SequenceElement."""
 
-    settings = {"javascript_builder_settings": {"minify": minify}}
-    if not sequence_element:
-        settings["add_javascript_builder"] = False
-    builder = PipelineBuilder(settings)
     if not sequence_element:
-        builder.add(JSONBundlerElement())
-        builder.add(JavascriptBuilderElement({"minify": minify}))
-    pipeline = builder.build()
+        raise ValueError("The JSON builder needs the SequenceElement")
+    settings = {"javascript_builder_settings": {"minify": minify}}
+    pipeline = PipelineBuilder(settings).build()
     flowdata = pipeline.create_flowdata()
     for key, value in (evidence or {}).items():
         flowdata.evidence.add(key, value)
@@ -92,54 +86,15 @@ def assert_parses(self, script):
             "the rendered script does not parse: " + result.stderr)
 
     @parameterized.expand([["unminified", False], ["minified", True]])
-    def test_script_parses_without_sequence_element(self, _, minify):
-        """With no SequenceElement and no query evidence the script is
-        rendered with sequence 1 and an empty session id, and parses."""
+    def test_script_parses(self, _, minify):
+        """With no query evidence the script is rendered with sequence 1, and
+        parses."""
 
         script = _render(minify=minify)
 
         self.assert_parses(script)
         if not minify:
             self.assertEqual(["var sequence = 1;"], _line(script, "sequence"))
-            self.assertEqual(
-                ['var sessionId = "";'], _line(script, "sessionId"))
-
-    @parameterized.expand([
-        ["text", "abc"],
-        ["negative", "-1"],
-        ["zero", "0"],
-        ["too_large", "99999999999"],
-        ["empty", ""],
-        ["statement", "1;b"],
-        ["decimal", "1.5"],
-        ["underscore", "1_0"],
-        ["just_too_large", "2147483648"],
-        ["file_separator", chr(0x1C) + "5"],
-        ["group_separator", chr(0x1D) + "5"],
-        ["record_separator", chr(0x1E) + "5"],
-        ["unit_separator", chr(0x1F) + "5"],
-        ["trailing_file_separator", "5" + chr(0x1C)],
-        ["null", chr(0) + "5"],
-        ["no_break_space", chr(0xA0) + "5"],
-        ["line_separator", chr(0x2028) + "5"],
-        ["arabic_indic_digit", chr(0x665)],
-        ["full_width_digit", chr(0xFF15)],
-        ["five_thousand_digits", "9" * 5000],
-        ["plus_alone", "+"],
-        ["spaces_alone", "   "],
-        ["zeros", "0000"],
-    ])
-    def test_unusable_sequence_from_query_becomes_one(self, _, value):
-        """A query.sequence that is not a positive 32 bit integer is rendered
-        as 1, as the pipeline specification says, so the script parses and
-        nothing else is written in its place."""
-
-        script = _render({
-            "query.session-id": "abc",
-            "query.sequence": value})
-
-        self.assertEqual(["var sequence = 1;"], _line(script, "sequence"))
-        self.assert_parses(script)
 
     @parameterized.expand([
         ["text", "abc"],
@@ -171,30 +126,8 @@ def test_unusable_sequence_with_sequence_element_becomes_one(
             ['var sessionId = "abc";'], _line(script, "sessionId"))
         self.assert_parses(script)
 
-    @parameterized.expand([
-        ["text", "7", 7],
-        ["spaces", " 7 ", 7],
-        ["number", 12, 12],
-        ["largest", "2147483647", 2147483647],
-    ])
-    def test_usable_sequence_from_query_is_rendered(self, _, value, expected):
-        script = _render({
-            "query.session-id": "abc",
-            "query.sequence": value})
-
-        self.assertEqual(
-            ["var sequence = " + str(expected) + ";"],
-            _line(script, "sequence"))
-        self.assert_parses(script)
-
-    @parameterized.expand([
-        ["without_sequence_element", False],
-        ["with_sequence_element", True],
-    ])
-    def test_session_id_from_query_is_rendered(self, _, sequence_element):
-        script = _render(
-            {"query.session-id": "abc-123"},
-            sequence_element=sequence_element)
+    def test_session_id_from_query_is_rendered(self):
+        script = _render({"query.session-id": "abc-123"})
 
         self.assertEqual(
             ['var sessionId = "abc-123";'], _line(script, "sessionId"))
@@ -223,7 +156,7 @@ def test_longest_session_id_is_rendered(self):
             ["space", "a b", False],
             ["new_line", "ab\n", False],
         ]
-        for element in [False, True]
+        for element in [True]
     ])
     def test_unsafe_session_id_is_rendered_empty(
             self, _, value, check_text, sequence_element):
@@ -331,7 +264,7 @@ def test_any_sequence_value_reaches_the_script(self):
                   chr(0x1F) + "5", "5" + chr(0x1C), chr(0) + "5",
                   "9" * 5000, "+", "   "]
         for value in values:
-            for element in [False, True]:
+            for element in [True]:
                 with self.subTest(value=repr(value), element=element):
                     script = _render(
                         {"query.session-id": "abc",

pipeline/fiftyone_pipeline_core/tests/test_jsonbundler_sequence_required.py

@@ -0,0 +1,78 @@
+
+"""
+The JSON Builder fails with an error, and sets no JSON, when no Sequence
+Element has produced a sequence number, as the JSON Builder specification
+says.
+"""
+
+import unittest
+
+from fiftyone_pipeline_core.jsonbundler import JSONBundlerElement
+from fiftyone_pipeline_core.pipelinebuilder import PipelineBuilder
+
+
+def bundler_only(suppress=False):
+    builder = PipelineBuilder({
+        "add_javascript_builder": False,
+        "use_setheader_properties": False,
+        "suppress_process_exceptions": suppress})
+    return builder.add(JSONBundlerElement()).build()
+
+
+class JsonBuilderSequenceTests(unittest.TestCase):
+
+    def test_no_sequence_number_is_an_error_that_names_the_sequence_element(
+            self):
+        pipeline = bundler_only()
+        self.addCleanup(pipeline.close)
+        flowdata = pipeline.create_flowdata()
+        with self.assertRaises(Exception) as caught:
+            flowdata.process()
+        self.assertIn("Sequence Element", str(caught.exception))
+
+    def test_no_json_is_set_when_the_error_is_suppressed(self):
+        pipeline = bundler_only(suppress=True)
+        self.addCleanup(pipeline.close)
+        flowdata = pipeline.create_flowdata()
+        flowdata.process()
+        self.assertEqual(1, len(flowdata.errors))
+        self.assertNotIn("jsonbundler", flowdata.data)
+
+    def test_a_sequence_number_from_the_query_string_does_not_count(self):
+        for value in ("3", "", "abc", True, 3.0):
+            pipeline = bundler_only(suppress=True)
+            self.addCleanup(pipeline.close)
+            flowdata = pipeline.create_flowdata()
+            flowdata.evidence.add("query.sequence", value)
+            flowdata.process()
+            self.assertEqual(1, len(flowdata.errors), repr(value))
+            self.assertNotIn("jsonbundler", flowdata.data, repr(value))
+
+    def test_an_integer_sequence_number_in_the_evidence_counts(self):
+        pipeline = bundler_only()
+        self.addCleanup(pipeline.close)
+        flowdata = pipeline.create_flowdata()
+        flowdata.evidence.add("query.sequence", 3)
+        flowdata.process()
+        self.assertIn("javascriptProperties", flowdata.jsonbundler.json)
+
+    def test_the_default_pipeline_has_a_sequence_number(self):
+        pipeline = PipelineBuilder({}).build()
+        self.addCleanup(pipeline.close)
+        flowdata = pipeline.create_flowdata()
+        flowdata.process()
+        self.assertEqual(0, len(flowdata.errors))
+        self.assertIn("javascriptProperties", flowdata.jsonbundler.json)
+
+    def test_a_sequence_of_ten_or_more_leaves_out_the_javascript_properties(
+            self):
+        pipeline = bundler_only()
+        self.addCleanup(pipeline.close)
+        flowdata = pipeline.create_flowdata()
+        flowdata.evidence.add("query.sequence", 10)
+        flowdata.process()
+        self.assertEqual([], flowdata.jsonbundler.json["javascriptProperties"])
+
+
+if __name__ == "__main__":
+    unittest.main()