Skip to content

Take the session id and the sequence from the Sequence Element, and render 1 and no session id without one - #94

Merged
jwrosewell merged 6 commits into
rcfrom
fix/javascript-builder-no-sequence
Nov 12, 2026
Merged

jwrosewell merged 6 commits into
rcfrom
fix/javascript-builder-no-sequence

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

The problem

The JavaScript Builder reads the session id and the sequence from Evidence,
under query.session-id and query.sequence. The specification has it read
both from the Sequence Element's Element Data, and render a sequence of 1
with an empty session id where the pipeline has no Sequence Element at all.
Python did neither, so a page could set its own sequence and session id in
the query string and have them rendered into the script whether or not a
Sequence Element had ever run, and a pipeline built without one rendered
whatever the query string held.

The Sequence Element had no Element Data to read either, writing only to
Evidence. #70 gave
it that Element Data and merged into rc at 15:08 UTC on 12 November, so
this draft now changes the builder alone.

What the specification says

javascript-builder.md,
step 5 of Process, at a0a7008a:

  1. Retrieve the Element Data for Sequence Element from
    the Flow Data. This contains the session id and sequence number to be
    passed to the template. 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. A session id is written into the script inside
    quotes, so a session id that does not match ^[A-Za-z0-9-]{1,64}$ MUST
    be rendered as an empty string.

sequence-element.md
at the same commit gives the Element Data this reads, the key being
sequence and the two names session-id (string) and sequence (int). It
also says both values go into Evidence as well, "so that later Flow Elements
and the caller can read them from Evidence", which is why the Evidence writes
stay.

What the other languages did

  • php#62 merged both
    halves, the Element Data and the defaults without a Sequence Element.
  • dotnet#132 and
    java#102 merged the
    Element Data half.
  • go#69 merged the
    defaults without a Sequence Element.

Python had neither half. #70 brought the Element Data half into rc, and
this draft brings the builder's half, so with both Python renders what the
other four render.

What changed

Three files in fiftyone_pipeline_core, all on the builder's side.

sequenceelement.py is no longer changed here. The Element Data this
draft had added is in rc through #70, under the same key and names, as
Merging rc below explains.

javascriptbuilder.py. process_internal takes both values from the
Sequence Element's Element Data rather than from Evidence. There are three
exits. No Element Data under the key sequence gives a sequence of 1 and an
empty session id, which covers both a pipeline with no Sequence Element and
one whose Sequence Element produced nothing because it stopped or failed.
Element Data that is there is read through get_session_id and
get_sequence, which are unchanged, so a session id that does not match
^[A-Za-z0-9-]{1,64}$ is still rendered empty and a sequence that is not a
positive 32 bit integer is still rendered as 1. The two docstrings that said
the values came from the query string were corrected.

The case-folded Evidence lookup that the builder used for these two keys goes
with them, because nothing else read it. A key given in other letters is
still read, because Evidence.get folds the key it is given
(evidence.py line 88) and the Sequence Element reads through it, which
test_evidence_key_case.py pins with
test_the_script_takes_the_session_and_sequence_in_any_case and
test_the_sequence_element_accepts_its_keys_in_any_case. Both pass on this
head. Nothing that
#95 added to rc is
undone.

test_javascriptbuilder_no_sequence.py, which is new, holds eight tests.
Four build the same pipeline with and without a Sequence Element, and check
that without one the script renders sequence 1 and an empty session id
whatever the request asked for. In the other four a stand-in Sequence Element
writes one pair into its Element Data and a different pair into Evidence, so
the rendered script says which of the two was read, and the rest cover an
element that set no Element Data, an unusable pair in the Element Data, and
the real Sequence Element publishing the same pair it put in Evidence.

test_javascriptbuilder_sequence.py. Only the module docstring changes,
to say that every pipeline in that file has a Sequence Element and that the
case without one is covered in the new file.

Merging rc

#70 merged into rc at 15:08 UTC on 12 November as
3c786b73,
and both it and this draft changed SequenceElement.process_internal, so the
draft no longer merged.
d442e1dd
merges rc at
435274d8
into the draft with a merge commit, whose first parent is the earlier head
7fbda459. Each file both sides changed was resolved by reading both sides.

  • sequenceelement.py conflicted, and takes rc's version whole, as
    this draft's earlier What remains said it should. rc's
    process_internal
    publishes the pair it writes to Evidence as Element Data under the data key
    sequence, with the names session-id and sequence, which is what this
    draft's 13 lines there did. It also declares both properties and takes a
    sequence from 0 to MAX_INPUT_SEQUENCE. So nothing this draft did in that
    file is missing. The only lines of this draft's side not kept are two blank
    lines in that method that it had cleared of trailing spaces, which change
    nothing that runs. The builder reads rc's Element Data as it is, and
    test_the_sequence_element_publishes_the_pair_it_wrote passes against it.
  • javascriptbuilder.py merged with no conflict and keeps both changes,
    rc's escaping of <, > and & in the JSON written into the script
    (lines 375 to 381)
    and this draft's reading of the two values from the Element Data
    (lines 430 to 444).
    The two are in different parts of process_internal and neither reads
    what the other writes.
  • test_javascriptbuilder_sequence.py merged with no conflict and keeps
    both changes
    , rc's case for a session id that is too long, whose JSON
    now holds the id as it is because the Sequence Element publishes it, and
    this draft's module docstring.

Every other file rc changed takes rc's version, and this draft changes
none of them. After the merge the draft's diff against rc is the three
files under What changed and nothing else.

Runs

The runs below are on head d442e1dd27d74debbffd5fdda53a0953fe4efce2, which
is the earlier head 7fbda459 with rc at 435274d merged in. I read every
run's log rather than its result, and each temporary branch is one commit on
the commit named.

Red, 43283866509
on tmp/py94rc-red at 8672bb34, which is rc 435274d carrying this
draft's two test files and the temporary workflow, with no source change.
Failure, 5 failed and 3 passed in test_javascriptbuilder_no_sequence.py. The
five are the defaults tests and the Element Data ones, failing because rc's
builder renders the query string's values, for example ['var sequence = 1;'] != ['var sequence = 5;'] and ['var sequence = 7;'] != ['var sequence = 99;']. The three that pass are an empty request, a pipeline with a Sequence
Element, and test_the_sequence_element_publishes_the_pair_it_wrote, which
failed on the older rc and now passes because #70 publishes that Element
Data. The other readers of the two values passed, 148 tests and 185 subtests
in test_javascriptbuilder_sequence.py, test_javascriptbuilder.py,
test_javascriptbuilder_variables.py,
test_jsonbundler_sequence_required.py, test_evidence_key_case.py,
test_web_integration_elements.py and test_web_client_side.py.

Green, 43283885426
on tmp/py94rc-green at 20984727, which is this head with the same
temporary workflow. Success, 8 passed in the new file, and 148 tests and 185
subtests passed in the other seven.

Full build, 43283367357
on tmp/pr-matrix-94, success, 12 jobs passed and 2 skipped. All 11 Setting 'PullRequestSha'
lines read d442e1dd27d74debbffd5fdda53a0953fe4efce2, which is this head,
and Configure printed "Left out 5 macOS entries".

Earlier runs, all on heads this draft has moved past, are named so the record
is complete. On 7fbda459, the red
43185461463
on the older rc failed 6 and passed 107, the green
43185512395
passed 113, and the full build
43185748622
was green. The full build
43176253466
was green on 6c142df2, which had only the defaults half.

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.

What remains

Nothing of what this draft set out to do. The three items listed here before
the merge were #70's, and #70 is now in rc. They were the conflict in
SequenceElement.process_internal, resolved as above, the two properties
the Sequence Element declares, and the rules in the Process section of
sequence-element.md
for a value that cannot be used, being the upper bound of 2,147,483,646 and
a sequence of 0 giving 1.

The last release into main merged on 2026-11-04.

@jwrosewell jwrosewell mentioned this pull request Nov 12, 2026
Step 5 of the JavaScript Builder's process reads both values from the
Sequence Element's Element Data, and renders 1 and an empty session id
where there is no Sequence Element. This draft had the second half and
read both values from Evidence, which is the half PHP, .NET and Java
already have.

The Sequence Element now publishes the pair it writes to Evidence as its
own Element Data, and the builder reads it from there. The Evidence
writes stay, because the JSON Builder and the Sequence Element's own
evidence key filter read them.

The case-folded Evidence lookup the builder used for these two keys goes
with them, having no other reader. A key given in other letters is still
read, because Evidence.get folds the key it is given and the Sequence
Element reads through it.
@jwrosewell jwrosewell changed the title Render sequence 1 and an empty session id when there is no Sequence Element Take the session id and the sequence from the Sequence Element, and render 1 and no session id without one Nov 12, 2026
Takes rc at 435274d, which now holds #70, #69, #59, #89 and #106.

sequenceelement.py conflicts, because #70 and this branch both give the
Sequence Element Element Data. It takes rc's version whole. rc's
process_internal publishes the same two values under the same data key
"sequence" and the same names "session-id" and "sequence", being the
pair it writes to Evidence, and also declares both properties and
validates each value. So this branch's 13 lines there are no longer
needed, and the JavaScript Builder reads rc's Element Data unchanged.

javascriptbuilder.py merges without a conflict and keeps both changes,
rc's escaping of the JSON written into the script and this branch's
reading of the session id and the sequence from the Element Data.
test_javascriptbuilder_sequence.py also merges without a conflict and
keeps rc's too_long case and this branch's module docstring.
@jwrosewell
jwrosewell marked this pull request as ready for review November 12, 2026 19:34
@jwrosewell
jwrosewell merged commit 5b9fd55 into rc Nov 12, 2026
3 checks passed
@jwrosewell
jwrosewell deleted the fix/javascript-builder-no-sequence branch November 12, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant