diff --git a/pipeline/pipeline.javascriptbuilder/src/main/java/fiftyone/pipeline/javascriptbuilder/flowelements/JavaScriptBuilderElement.java b/pipeline/pipeline.javascriptbuilder/src/main/java/fiftyone/pipeline/javascriptbuilder/flowelements/JavaScriptBuilderElement.java index 509fc1b0b..30e7ffbf2 100644 --- a/pipeline/pipeline.javascriptbuilder/src/main/java/fiftyone/pipeline/javascriptbuilder/flowelements/JavaScriptBuilderElement.java +++ b/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; diff --git a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderElementBuilderTests.java b/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderElementBuilderTests.java index 55d0daaa1..8825c30ad 100644 --- a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderElementBuilderTests.java +++ b/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)); + } } diff --git a/pipeline/pipeline.jsonbuilder/src/main/java/fiftyone/pipeline/jsonbuilder/flowelements/JsonBuilderElement.java b/pipeline/pipeline.jsonbuilder/src/main/java/fiftyone/pipeline/jsonbuilder/flowelements/JsonBuilderElement.java index 413113d6e..b4c1b93a8 100644 --- a/pipeline/pipeline.jsonbuilder/src/main/java/fiftyone/pipeline/jsonbuilder/flowelements/JsonBuilderElement.java +++ b/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 allProperties) { for (Map.Entry entry : allProperties.entrySet()) { if (entry.getValue() instanceof Map) { - Map map = new HashMap<>(); + JSONObject element = new JSONObject(); @SuppressWarnings("unchecked") Map properties = (Map)entry.getValue(); @@ -478,9 +479,9 @@ private String buildJson(Map 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 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 diff --git a/pipeline/pipeline.jsonbuilder/src/test/java/fiftyone/pipeline/jsonbuilder/JsonBuilderTests.java b/pipeline/pipeline.jsonbuilder/src/test/java/fiftyone/pipeline/jsonbuilder/JsonBuilderTests.java index b81cf6c81..f78c871d4 100644 --- a/pipeline/pipeline.jsonbuilder/src/test/java/fiftyone/pipeline/jsonbuilder/JsonBuilderTests.java +++ b/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 noValue = + new AspectPropertyValueDefault<>(); + noValue.setNoValueMessage("There is no value."); + Map 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 noValue = + new AspectPropertyValueDefault<>(); + noValue.setNoValueMessage("No value for this item."); + Map itemValues = new HashMap<>(); + itemValues.put("value1", noValue); + itemValues.put("value2", "xyz"); + ElementData item = mock(ElementData.class); + when(item.asKeyMap()).thenReturn(itemValues); + Map 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); }