diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java index 79a70b91..588f1947 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java @@ -146,9 +146,16 @@ private static Integer toSequence(Object sequence) { Integer.valueOf((int) value) : null; } + /** + * The key the Element Data is stored under in the Flow Data, unless a + * subclass gives another from {@link #getElementDataKey()}. + * @see Specification + */ + public static final String DEFAULT_ELEMENT_DATA_KEY = "sequence"; + @Override public String getElementDataKey() { - return "sequence"; + return DEFAULT_ELEMENT_DATA_KEY; } @Override 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 782c9fa7..509fc1b0 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 @@ -32,6 +32,8 @@ import fiftyone.pipeline.core.flowelements.FlowElementBase; import fiftyone.pipeline.engines.data.AspectPropertyValue; import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.fiftyone.data.SequenceData; +import fiftyone.pipeline.engines.fiftyone.flowelements.SequenceElement; import fiftyone.pipeline.javascriptbuilder.Constants; import fiftyone.pipeline.javascriptbuilder.data.JavaScriptBuilderData; import fiftyone.pipeline.javascriptbuilder.templates.JavaScriptResource; @@ -243,8 +245,9 @@ protected void processInternal(FlowData data) throws Exception { String url = getUrl(reqProtocol, reqHost); - String sessionId = getSessionId(data); - int sequence = getSequence(data); + ElementData sequenceData = getSequenceData(data); + String sessionId = getSessionId(sequenceData); + int sequence = getSequence(sequenceData); String serializedParameters = serializeParameters(parameters); // With the gathered resources, build a new JavaScriptResource. @@ -473,52 +476,77 @@ private String getPath() { /** * Gets the session id to write into the script. This is the session id - * evidence, set by the Sequence Element or by the request, when it - * matches SESSION_ID_PATTERN, and an empty string otherwise. - * @param data the flow data + * the Sequence Element wrote, when it matches SESSION_ID_PATTERN, and an + * empty string otherwise, including when the pipeline has no Sequence + * Element. + * @param sequenceData the Sequence Element's data, or null * @return the session id, or an empty string */ - private String getSessionId(FlowData data) { - TryGetResult trySessionId = - data.tryGetEvidence(EVIDENCE_SESSIONID, Object.class); - if (trySessionId.hasValue() && - trySessionId.getValue() instanceof String) { - String sessionId = (String) trySessionId.getValue(); - if (SESSION_ID_PATTERN.matcher(sessionId).matches()) { - return sessionId; - } + private String getSessionId(ElementData sequenceData) { + Object value = getSequenceValue( + sequenceData, SequenceData.SESSION_ID_PROPERTY_NAME); + if (value instanceof String && + SESSION_ID_PATTERN.matcher((String) value).matches()) { + return (String) value; } return ""; } /** - * Gets the sequence to write into the script. This is the sequence - * evidence, as a number or as text, when it is a positive 32 bit - * integer, and 1 otherwise, including when there is no Sequence Element. - * The value is written into the script as code, so text that is not a - * number could break the script, and a number below 1 is not a sequence - * the Sequence Element would ever give. - * @param data the flow data + * Gets the sequence to write into the script. This is the sequence the + * Sequence Element wrote, as a number or as text, when it is a positive + * 32 bit integer, and 1 otherwise, including when the pipeline has no + * Sequence Element. The value is written into the script as code, so + * text that is not a number could break the script, and a number below 1 + * is not a sequence the Sequence Element would ever give. + * @param sequenceData the Sequence Element's data, or null * @return the sequence, which is always at least 1 */ - private int getSequence(FlowData data) { - TryGetResult trySequence = - data.tryGetEvidence(EVIDENCE_SEQUENCE, Object.class); + private int getSequence(ElementData sequenceData) { + Object value = getSequenceValue( + sequenceData, SequenceData.SEQUENCE_PROPERTY_NAME); long sequence = 0; - if (trySequence.hasValue()) { - Object value = trySequence.getValue(); - if (value instanceof Integer) { - sequence = (Integer) value; - } else if (value instanceof String && - SEQUENCE_PATTERN.matcher((String) value).matches()) { - sequence = Long.parseLong((String) value); - } + if (value instanceof Integer) { + sequence = (Integer) value; + } else if (value instanceof String && + SEQUENCE_PATTERN.matcher((String) value).matches()) { + sequence = Long.parseLong((String) value); } return sequence >= 1 && sequence <= Integer.MAX_VALUE ? (int) sequence : 1; } + /** + * The Sequence Element's data, or null when the pipeline has no Sequence + * Element. Step 5 of the specification takes the session id and the + * sequence from that data and not from Evidence, so a pipeline with no + * Sequence Element renders the defaults even when the request carried a + * session id or a sequence of its own. + * @param data the flow data + * @return the Sequence Element's data, or null + * @see Specification + */ + private static ElementData getSequenceData(FlowData data) { + // get(String) throws when the Flow Data holds nothing under the key, + // so the keys it does hold are checked first. + return data.getDataKeys().contains(SequenceElement.DEFAULT_ELEMENT_DATA_KEY) + ? data.get(SequenceElement.DEFAULT_ELEMENT_DATA_KEY) + : null; + } + + /** + * One value from the Sequence Element's data. + * @param sequenceData the Sequence Element's data, or null + * @param name the name of the value within that data + * @return the value, or null + */ + private static Object getSequenceValue( + ElementData sequenceData, + String name) { + return sequenceData == null ? null : sequenceData.get(name); + } + private void buildJavaScript( FlowData data, String jsonObject, diff --git a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderTests.java b/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderTests.java index 5cfde946..7ec0c95e 100644 --- a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderTests.java +++ b/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/JavaScriptBuilderTests.java @@ -30,6 +30,8 @@ import fiftyone.pipeline.engines.data.AspectPropertyValue; import fiftyone.pipeline.engines.data.AspectPropertyValueDefault; import fiftyone.pipeline.engines.exceptions.PropertyMissingException; +import fiftyone.pipeline.engines.fiftyone.data.SequenceData; +import fiftyone.pipeline.engines.fiftyone.flowelements.SequenceElement; import fiftyone.pipeline.javascriptbuilder.data.JavaScriptBuilderData; import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElement; import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElementBuilder; @@ -57,6 +59,7 @@ import java.io.IOException; import java.time.Duration; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -527,6 +530,19 @@ private void configureMocks() { when(flowData.get(anyString())).thenReturn(elementDataMock); + // The Sequence Element's data, which is where the builder reads the + // session id and the sequence from, as a web integration's + // pipeline holds it. + ElementData sequenceData = mock(ElementData.class); + when(sequenceData.get(SequenceData.SESSION_ID_PROPERTY_NAME)) + .thenReturn(sessionId); + when(sequenceData.get(SequenceData.SEQUENCE_PROPERTY_NAME)) + .thenReturn(sequence); + when(flowData.getDataKeys()).thenReturn(Collections.singletonList( + SequenceElement.DEFAULT_ELEMENT_DATA_KEY)); + when(flowData.get(SequenceElement.DEFAULT_ELEMENT_DATA_KEY)) + .thenReturn(sequenceData); + when(flowData.getAs(anyString(), any(Class.class))).thenReturn(new AspectPropertyValueDefault<>("None")); doAnswer((Answer) invocation -> { diff --git a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/SessionIdAndSequenceTests.java b/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/SessionIdAndSequenceTests.java index 8c74887c..06bc9b6b 100644 --- a/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/SessionIdAndSequenceTests.java +++ b/pipeline/pipeline.javascriptbuilder/src/test/java/fiftyone/pipeline/javascriptbuilder/SessionIdAndSequenceTests.java @@ -30,6 +30,8 @@ import fiftyone.pipeline.core.flowelements.FlowElementBase; import fiftyone.pipeline.core.flowelements.PipelineBuilder; import fiftyone.pipeline.core.flowelements.Pipeline; +import fiftyone.pipeline.engines.fiftyone.data.SequenceData; +import fiftyone.pipeline.engines.fiftyone.flowelements.SequenceElement; import fiftyone.pipeline.engines.fiftyone.flowelements.SequenceElementBuilder; import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElement; import fiftyone.pipeline.javascriptbuilder.flowelements.JavaScriptBuilderElementBuilder; @@ -66,6 +68,11 @@ * the builder's own handling can be tested, the tests "without the Sequence * Element" use a stand in for the JSON builder that supplies the payload and * leaves the evidence as it was given. + *

+ * The builder takes both values from the Sequence Element's data, so the + * tests of the rule above put the value straight into that data with a + * stand in for the Sequence Element, which lets a value the real element + * would never write be tried as well. */ public class SessionIdAndSequenceTests { @@ -122,9 +129,65 @@ protected void unmanagedResourcesCleanup() { } } + /** + * Writes the two values given straight into the Sequence Element's data, + * in place of that element, so that a value the real element would never + * write can be tried as well. A null value is not written. + */ + private static class SequenceStandIn extends + FlowElementBase { + + private final Object sessionId; + private final Object sequence; + + SequenceStandIn(Logger logger, Object sessionId, Object sequence) { + super(logger, null); + this.sessionId = sessionId; + this.sequence = sequence; + } + + @Override + protected void processInternal(FlowData data) { + SequenceData sequenceData = new SequenceData(logger, data); + if (sessionId != null) { + sequenceData.put( + SequenceData.SESSION_ID_PROPERTY_NAME, sessionId); + } + if (sequence != null) { + sequenceData.put( + SequenceData.SEQUENCE_PROPERTY_NAME, sequence); + } + data.getOrAdd(getElementDataKey(), f -> sequenceData); + } + + @Override + public String getElementDataKey() { + return SequenceElement.DEFAULT_ELEMENT_DATA_KEY; + } + + @Override + public EvidenceKeyFilter getEvidenceKeyFilter() { + return new EvidenceKeyFilterWhitelist( + Collections.emptyList()); + } + + @Override + public List getProperties() { + return Collections.emptyList(); + } + + @Override + protected void managedResourcesCleanup() { + } + + @Override + protected void unmanagedResourcesCleanup() { + } + } + @Test public void SessionId_Valid_Rendered() throws Exception { - String script = render(false, "abc-123", null); + String script = renderWithSequenceData("abc-123", null); assertEquals("abc-123", sessionId(script)); assertEquals("1", sequence(script)); } @@ -132,7 +195,7 @@ public void SessionId_Valid_Rendered() throws Exception { @Test public void SessionId_LongestValid_Rendered() throws Exception { String id = repeat('a', 64); - String script = render(false, id, null); + String script = renderWithSequenceData(id, null); assertEquals(id, sessionId(script)); } @@ -149,7 +212,7 @@ static Stream invalidSessionIds() { @ParameterizedTest @MethodSource("invalidSessionIds") public void SessionId_Invalid_RenderedEmpty(String id) throws Exception { - String script = render(false, id, null); + String script = renderWithSequenceData(id, null); assertEquals("", sessionId(script)); } @@ -190,14 +253,31 @@ public void NoSequenceElement_NoEvidence_DefaultsRendered() } /** - * A sequence from a web request is text. With no Sequence Element it - * reaches the builder as it is. + * With no Sequence Element the defaults are rendered even when the + * request carried a session id and a sequence of its own, because step 5 + * of the + * specification + * takes both from the Sequence Element's data and says "Where there is + * no Sequence Element, the sequence MUST be rendered as 1 and the + * session id as an empty string, so the script always parses." + */ + @Test + public void NoSequenceElement_WithEvidence_DefaultsRendered() + throws Exception { + String script = render(false, "abc-123", 7); + assertEquals("", sessionId(script)); + assertEquals("1", sequence(script)); + } + + /** + * A sequence another Sequence Element wrote as text is read as a number + * when it is one, because the Element Data holds values of any type. */ @ParameterizedTest @ValueSource(strings = {"abc", "-1", "0", "99999999999", "", "2147483648"}) public void Sequence_InvalidText_RenderedOne(String value) throws Exception { - String script = render(false, "abc-123", value); + String script = renderWithSequenceData("abc-123", value); assertEquals("1", sequence(script)); } @@ -209,7 +289,7 @@ static Stream invalidSequenceNumbers() { @MethodSource("invalidSequenceNumbers") public void Sequence_InvalidNumber_RenderedOne(Object value) throws Exception { - String script = render(false, "abc-123", value); + String script = renderWithSequenceData("abc-123", value); assertEquals("1", sequence(script)); } @@ -225,7 +305,7 @@ static Stream validSequences() { @MethodSource("validSequences") public void Sequence_Valid_Rendered(Object value, String expected) throws Exception { - String script = render(false, "abc-123", value); + String script = renderWithSequenceData("abc-123", value); assertEquals(expected, sequence(script)); } @@ -245,6 +325,31 @@ public void Sequence_InvalidWithSequenceElement_RenderedOne(int value) assertEquals("1", sequence(script)); } + /** + * Builds a pipeline whose Sequence Element data holds the two values + * given, processes it, checks the script parses and returns it. A null + * value is not written into that data. + */ + private String renderWithSequenceData(Object sessionId, Object sequence) + throws Exception { + JavaScriptBuilderElement jsElement = + new JavaScriptBuilderElementBuilder(loggerFactory).build(); + PipelineBuilder builder = new PipelineBuilder(loggerFactory); + builder.addFlowElement(new SequenceStandIn( + loggerFactory.getLogger("sequence"), sessionId, sequence)); + builder.addFlowElement( + new JsonStandIn(loggerFactory.getLogger("json"))); + builder.addFlowElement(jsElement); + String script; + try (Pipeline pipeline = builder.build(); + FlowData flowData = pipeline.createFlowData()) { + flowData.process(); + script = flowData.getFromElement(jsElement).getJavaScript(); + } + assertParses(script); + return script; + } + /** * Builds a pipeline with the Sequence Element and the JSON builder, or * with the stand in for them, processes it