From 8f35be0326b925c4c6cc35de6d3d17665f47aaf7 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Sat, 14 Nov 2026 16:27:49 +0000 Subject: [PATCH 1/2] Test that a no-value Property is written as null, and that the constructor refuses a bad protocol JsonBuilder_NoValue_WrittenAsNullWithReason and JsonBuilder_ListOfElementData_NoValueWrittenAsNull expect a Property assigned no value written as null with its nullreason, at the top level and in an item of a list of Element Data, and a plain null left out. JavaScriptBuilderElement_Constructor_InvalidProtocol_Refused expects a protocol that is neither http nor https, white space included, refused when the element is made. --- .../JavaScriptBuilderElementBuilderTests.java | 46 ++++++++++++++ .../jsonbuilder/JsonBuilderTests.java | 61 +++++++++++++++++++ 2 files changed, 107 insertions(+) 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/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); } From 3099adef4709d439dbebd2a9429160e03e258c91 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Sat, 14 Nov 2026 16:29:08 +0000 Subject: [PATCH 2/2] Write a no-value Property as null, and refuse a bad protocol in the constructor The JSON Builder specification's Process section says a Property assigned no value is written as null with its nullreason, and the same for each item of a list of Element Data. org.json's JSONObject(Map) leaves out a null value, so the property was missing. buildJson now writes each Element Data's object, and any Map inside it, with null as JSON null. The JavaScript Builder specification says a configured protocol that is neither http nor https, including one made only of white space, is refused when the element is built. The builder refused it, and the public constructor accepted it. The constructor now refuses it with the builder's message, and null or empty still means none is configured. --- .../JavaScriptBuilderElement.java | 18 +++++++-- .../flowelements/JsonBuilderElement.java | 37 +++++++++++++++++-- 2 files changed, 49 insertions(+), 6 deletions(-) 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.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