Write a no-value Property as null in the JSON, and refuse a bad protocol in the JavaScript Builder's constructor

pipeline/pipeline.javascriptbuilder/src/main/java/fiftyone/pipeline/javascriptbuilder/flowelements/JavaScriptBuilderElement.java

@@ -153,8 +153,11 @@ public static boolean isValidObjectName(String name) {
      * @param host The host that the client JavaScript should query for updates.
      * If null or blank then the host from the request will be used
      * @param protocol The protocol (HTTP or HTTPS) that the client JavaScript
-     *                 will use when querying for updates. If null or blank
+     *                 will use when querying for updates. If null or empty
      *                 then the protocol from the request will be used
+     * @throws PipelineConfigurationException if protocol is not null or empty
+     * and is neither http nor https in any case of its ASCII letters, or if
+     * objName is not a valid JavaScript identifier
      */
     public JavaScriptBuilderElement(
             Logger logger,
@@ -182,11 +185,13 @@ public JavaScriptBuilderElement(
      * @param host The host that the client JavaScript should query for updates.
      * If null or blank then the host from the request will be used
      * @param protocol The protocol (HTTP or HTTPS) that the client JavaScript
-     *                 will use when querying for updates. If null or blank
+     *                 will use when querying for updates. If null or empty
      *                 then the protocol from the request will be used
      * @param contextRoot The <context-root> setting from the web.xml.
      *                 This is needed when creating the callback URL.
-     * @throws PipelineConfigurationException if objName is not null and is
+     * @throws PipelineConfigurationException if protocol is not null or empty
+     * and is neither http nor https in any case of its ASCII letters, white
+     * space included, or if objName is not null and is
      * not a valid JavaScript identifier, see {@link #isValidObjectName}.
      * A null name means the default name is used, as no name was
      * configured, and an empty name is refused
@@ -209,6 +214,13 @@ public JavaScriptBuilderElement(
 
         this.host = host;
         this.endpoint = endpoint;
+        if (protocol != null &&
+            protocol.isEmpty() == false &&
+            CallbackUrl.isScheme(protocol) == false) {
+            throw new PipelineConfigurationException(
+                "Invalid protocol in configuration (" + protocol +
+                "), must be 'http' or 'https'");
+        }
         this.protocol = protocol;
         if (objName == null) {
             this.objName = Constants.DEFAULT_OBJECT_NAME;

pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderElementBuilderTests.java

@@ -24,12 +24,15 @@
 
 import fiftyone.common.testhelpers.TestLoggerFactory;
 import fiftyone.pipeline.core.exceptions.PipelineConfigurationException;
+import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElement;
 import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElementBuilder;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.LoggerFactory;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -108,4 +111,47 @@ public void JavaScriptBuilderElement_Builder_SetDefaultProtocol_ValidProtocol(
         loggerFactory.loggers.forEach(testLogger ->
             assertTrue(testLogger.warningsLogged.size() == 0));
     }
+
+    /**
+     * A protocol given to the element's constructor that is neither http nor
+     * https, including one made only of white space, is refused when the
+     * element is made, as one given to the builder is.
+     * @param protocol
+     */
+    @ParameterizedTest
+    @ValueSource(strings = {"htp", "ftp", " ", "\t", "https:"})
+    public void JavaScriptBuilderElement_Constructor_InvalidProtocol_Refused(
+        String protocol) {
+        assertThrows(
+            PipelineConfigurationException.class,
+            () -> new JavaScriptBuilderElement(
+                loggerFactory.getLogger("test"),
+                null,
+                "",
+                null,
+                false,
+                "",
+                protocol));
+    }
+
+    /**
+     * A null or empty protocol means none is configured, so the protocol
+     * comes from Evidence, and http or https in any case of its ASCII
+     * letters is accepted.
+     * @param protocol
+     */
+    @ParameterizedTest
+    @NullAndEmptySource
+    @ValueSource(strings = {"http", "https", "HTTPS", "Http"})
+    public void JavaScriptBuilderElement_Constructor_ValidOrNoProtocol_Accepted(
+        String protocol) {
+        assertDoesNotThrow(() -> new JavaScriptBuilderElement(
+            loggerFactory.getLogger("test"),
+            null,
+            "",
+            null,
+            false,
+            "",
+            protocol));
+    }
 }

pipeline/pipeline.jsonbuilder/src/main/java/fiftyone/pipeline/jsonbuilder/flowelements/JsonBuilderElement.java

@@ -47,6 +47,7 @@
 import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.json.JSONArray;
 import org.json.JSONObject;
 import org.slf4j.Logger;
 
@@ -468,7 +469,7 @@ private String buildJson(Map<String, Object> allProperties) {
 
         for (Map.Entry<String, Object> entry : allProperties.entrySet()) {
             if (entry.getValue() instanceof Map) {
-                Map<String, Object> map = new HashMap<>();
+                JSONObject element = new JSONObject();
                 @SuppressWarnings("unchecked")
                 Map<String, Object> properties = (Map<String, Object>)entry.getValue();
 
@@ -478,9 +479,9 @@ private String buildJson(Map<String, Object> allProperties) {
                     if(value instanceof JavaScript)
                         value = value.toString();
 
-                    map.put(resolveName(ent.getKey()), value);
+                    element.put(resolveName(ent.getKey()), toJsonValue(value));
                 }
-                json.put(resolveName(entry.getKey()), map);
+                json.put(resolveName(entry.getKey()), element);
             } else {
 
                 json.put(resolveName(entry.getKey()), entry.getValue());
@@ -489,6 +490,36 @@ private String buildJson(Map<String, Object> allProperties) {
         return json.toString(2);
     }
 
+    /**
+     * The value as it is written into the JSON. org.json leaves out a Map
+     * entry whose value is null, so a Map, such as an item of a list of
+     * Element Data, is written here with each null as JSON null, because a
+     * Property assigned no value is written as null with its nullreason.
+     * Every other value is wrapped as org.json wraps it.
+     */
+    private static Object toJsonValue(Object value) {
+        if (value == null) {
+            return JSONObject.NULL;
+        }
+        if (value instanceof Map) {
+            JSONObject object = new JSONObject();
+            for (Map.Entry<?, ?> entry : ((Map<?, ?>) value).entrySet()) {
+                object.put(
+                    String.valueOf(entry.getKey()),
+                    toJsonValue(entry.getValue()));
+            }
+            return object;
+        }
+        if (value instanceof Collection) {
+            JSONArray array = new JSONArray();
+            for (Object item : (Collection<?>) value) {
+                array.put(toJsonValue(item));
+            }
+            return array;
+        }
+        return JSONObject.wrap(value);
+    }
+
 
     /**
      * Executed on first request in order to build some collections

pipeline/pipeline.jsonbuilder/src/test/java/fiftyone/pipeline/jsonbuilder/JsonBuilderTests.java

@@ -627,6 +627,67 @@ public void JsonBuilder_WeightedValues() throws Exception {
         }
     }
 
+    /**
+     * A Property assigned no value is written as null with its nullreason,
+     * and a Property whose value is null without being assigned no value has
+     * no entry and no nullreason. See the Process section of the JSON
+     * Builder specification.
+     */
+    @Test
+    public void JsonBuilder_NoValue_WrittenAsNullWithReason()
+            throws Exception {
+        AspectPropertyValueDefault<String> noValue =
+            new AspectPropertyValueDefault<>();
+        noValue.setNoValueMessage("There is no value.");
+        Map<String, Object> map = new HashMap<>();
+        map.put("novalue", noValue);
+        map.put("plainnull", null);
+        map.put("property", "thisIsAValue");
+        when(elementDataMock.asKeyMap()).thenReturn(map);
+
+        String json = testIteration(1);
+
+        JSONObject element = new JSONObject(json).getJSONObject("test");
+        assertTrue(element.has("novalue"), json);
+        assertTrue(element.isNull("novalue"), json);
+        assertEquals("There is no value.",
+            element.getString("novaluenullreason"), json);
+        assertFalse(element.has("plainnull"), json);
+        assertFalse(element.has("plainnullnullreason"), json);
+        assertEquals("thisIsAValue", element.getString("property"), json);
+    }
+
+    /**
+     * Each item of a list of Element Data follows the same rules, so an
+     * item's Property assigned no value is written as null with its
+     * nullreason.
+     */
+    @Test
+    public void JsonBuilder_ListOfElementData_NoValueWrittenAsNull()
+            throws Exception {
+        AspectPropertyValueDefault<String> noValue =
+            new AspectPropertyValueDefault<>();
+        noValue.setNoValueMessage("No value for this item.");
+        Map<String, Object> itemValues = new HashMap<>();
+        itemValues.put("value1", noValue);
+        itemValues.put("value2", "xyz");
+        ElementData item = mock(ElementData.class);
+        when(item.asKeyMap()).thenReturn(itemValues);
+        Map<String, Object> map = new HashMap<>();
+        map.put("items", Collections.singletonList(item));
+        when(elementDataMock.asKeyMap()).thenReturn(map);
+
+        String json = testIteration(1);
+
+        JSONObject first = new JSONObject(json).getJSONObject("test")
+            .getJSONArray("items").getJSONObject(0);
+        assertTrue(first.has("value1"), json);
+        assertTrue(first.isNull("value1"), json);
+        assertEquals("No value for this item.",
+            first.getString("value1nullreason"), json);
+        assertEquals("xyz", first.getString("value2"), json);
+    }
+
     private String testIteration(int iteration) throws Exception {
         return testIteration(iteration, null);
     }