Repository navigation
Take the session id and the sequence from the Sequence Element's data - #102
Merged
Merged
Conversation
…e sharing characters The tests cover the init parameters that set the JavaScript and JSON paths and switch the Sequence Element and the Set Headers Element, the Element Data the Sequence Element writes, how the JSON Builder Element shows that data, and the characters XML 1.0 allows in a shared Evidence value.
…e Sequence Element The filter takes init parameters for the JavaScript path, the JSON path, and whether the Sequence Element and the Set Headers Element are added. The defaults keep today's paths and today's behaviour. The Sequence Element writes session-id and sequence as Element Data as well as Evidence. Usage sharing keeps every character XML 1.0 allows instead of replacing everything above U+00FF.
… say what the Lite file lacks The MVC configuration takes the JavaScript path, the JSON path and the two element switches the filter takes, with defaults that keep today's behaviour. The example comments about the Lite data file say it has a small number of Properties, as the specification does.
Java 21 warns about it, and the build treats warnings as errors.
…th as well The specification's JSON path becomes the default in the filter and the MVC integration, and the JavaScript they serve calls it. The path 51Degrees.core.json is still answered, and json-path still sets the path.
…tion says A session id that is not a non-empty string, and a sequence that is not a whole number or text of decimal digits that fits in an int with room to add one, count as absent. The Evidence and the Element Data are written from the same values.
# Conflicts: # pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java
# Conflicts: # pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/ShareUsageElement.java # pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/ReplacedStringTest.java
…nt is refused The json-path test uses a path that is not the default.
The JSON Builder Element fails on every request without the sequence number, so the build says which option to change.
… properties are off
- FiftyOneJSServiceCoreTests checks that a configured path, with or without its leading slash, answers only that path inside the application, so /anything/json, /json/more and /notjson are not answered, and that the context path is not part of the path. - SequenceElementTests checks that a negative sequence of any whole number type, a sign and white space each count as no sequence, and that 0 gives 1.
…uence as absent FiftyOneJSServiceCore.Default answered any request whose whole URL ended with the configured path, so a configured path of json also answered /anything/json. The path is now compared with the whole path of the request inside the application, after the context path, ignoring case and with or without a leading slash, and a request with no URI is not answered. The old 51Degrees.core.json path is matched the same way. The Sequence Element took a negative whole number as a sequence. The Sequence Element specification counts a sequence only from 0 up to 2,147,483,646, so a negative one now counts as absent.
The weighted values test that rc adds at the end of JsonBuilderTests and this draft's Sequence Element test sit in the same place, and both are kept.
…der-step-5 # Conflicts: # pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/SequenceElement.java
The constant is the key the element uses unless a subclass gives another, so its name says it is the default, as .NET's does.
This was referenced Nov 11, 2026
…ent's data The builder reads the session id and the sequence from the Sequence Element's data, so the shared mock holds that data as a web integration's pipeline does. JavaScriptBuilderElement_VerifySession failed in the full build without it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JavaScript Builder Element takes the session id and the sequence from the Sequence Element's data, as step 5 of the specification says, so a pipeline with no Sequence Element always renders the defaults.
This draft was stacked on #80, which has merged into
rc. Commit 11b5eb4 mergesrcat ddb7d04, a807668 merges it at a2d6cdd with #88, which changesJavaScriptBuilderElement.javaat other lines, and 0866a0c merges it at 528e0df with #108, each with no conflict, so the diff againstrcis this change alone. The merge had one conflict, inSequenceElement.getElementDataKey, wherercreturns the literal"sequence"and this draft returns its constant, nowDEFAULT_ELEMENT_DATA_KEY, which holds the same text, and the constant is kept.The problem
Process in javascript-builder.md, at commit 67ead698 on specifications
rc, step 5:On
rc,JavaScriptBuilderElement.getSessionIdandgetSequenceread the Evidence instead,query.session-idandquery.sequence. With a Sequence Element in the pipeline the two sources agree, because that element writes both to Evidence as well as to its own data, so nothing changes. With no Sequence Element they do not agree, because a request can carry either value itself. A page calling the JSON endpoint with?session-id=abc-123&sequence=7had both written into the script, where the specification asks for an empty id and 1.What changes
getSessionIdandgetSequencetake their value from the Sequence Element's Element Data, under the keysequence, which one newgetSequenceDatareads once per request. When the Flow Data holds nothing under that key, which is what a pipeline with no Sequence Element gives, the session id is empty and the sequence is 1.FlowData.get(String)throwsNoSuchElementExceptionwhen the Flow Data holds nothing under the key, whatever its documentation says, sogetSequenceDatachecksgetDataKeys()first.SequenceElementgainsDEFAULT_ELEMENT_DATA_KEY, the constantgetElementDataKeyalready returned, so the builder names the same key rather than repeating the text.[A-Za-z0-9-]{1,64}still gives an empty string, because both are written into the script as code.EXCLUDED_PARAMETERSstill leavesquery.session-idandquery.sequenceout of the parameters object, which the same section asks for, and the element's own accepted Evidence is unchanged.Behaviour that changes for callers
A pipeline that has the JavaScript Builder Element and no Sequence Element renders an empty session id and a sequence of 1, whatever the request carried. The script then always sends sequence 1 on its callbacks and keeps no session, which is what the specification asks for, because without a Sequence Element nothing counts the callbacks. Adding a Sequence Element restores the counting, and the 51Degrees web integration adds one already, so a site using it is unaffected. This belongs in the release note.
Review fix (commit 64a2685). The Sequence Element's new key constant is named
DEFAULT_ELEMENT_DATA_KEY, as .NET names its own, and its Javadoc says a subclass can give another key fromgetElementDataKey(), because the nameELEMENT_DATA_KEYread as the only key there could be. The constant is new in this draft, so nothing else used the old name.A test the change affects (commit c771241). The first full build of the merge (run 43072025592) failed
JavaScriptBuilderTests.JavaScriptBuilderElement_VerifySession, "JavaScript does not contain expected session id", because the test's mocked Flow Data held the session id only in the Evidence. The mock now holds the Sequence Element's data as well, with the same session id and sequence, as a web integration's pipeline does. The same test needed the same change in .NET.How it was checked
The tests were written first and fail on the branch without the change.
SessionIdAndSequenceTestsgains a stand in for the Sequence Element that writes the two values straight into the Element Data, so the rule on each value is still tested, and a value the real element would never write can be tried as well. The newNoSequenceElement_WithEvidence_DefaultsRenderedbuilds a pipeline with no Sequence Element, adds a session id ofabc-123and a sequence of 7 to the Evidence, and expects an empty id and 1.SequenceElementTestsSessionIdAndSequenceTestsand 13SequenceElementTestspassSessionIdAndSequenceTests, 13SequenceElementTestsand 15JavaScriptBuilderTestspassSetting 'PullRequestSha'line, in Configure and in each of the 9 jobs, is c771241, and Configure printed "Left out 6 macOS entries"macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
Every exit path of
getSessionId,getSequence,getSequenceDataandgetSequenceValuewas read after the change.Other open drafts
#80, on which this draft was stacked, merged into
rcon 11 November 2026, so nothing else has to merge first.What remains
The JSON Builder Element still reads the sequence from the Evidence. json-builder.md asks for its own behaviour there and is not changed by this draft.
Version
Patch only.