Repository navigation
Fail in the JSON Builder when no Sequence Element has produced a sequence number - #59
Merged
Merged
Conversation
…nt first, and give it element data The Web integration page says the Sequence element must be the first element, and that each of the Sequence, JSON Builder, JavaScript Builder and Set Headers elements must have its own option to leave it out. The Sequence Element page lists session-id and sequence as its Element Data.
The Sequence element now runs first and adds a numeric sequence to the evidence, which the native evidence cannot take. The usage sharing test that checks the sequence shared is run with and without the Sequence element.
The JSON Builder page requires a top-level entry for each Element Data and excludes only the Set Headers, Cloud Request Engine and Usage Sharing elements.
… JSON Builder exclusions
… leave the Cloud Request Engine and Usage Sharing out of the JSON The Sequence Element page generates a session id when there is none and sets the sequence to one when there is none, as separate rules. The JSON Builder page excludes the Cloud Request Engine and Usage Sharing elements. Two more example comments say the Lite data file has a small number of Properties.
The Sequence Element page, as clarified in specifications#127, counts a sequence as present when it is only the digits 0 to 9 and from 0 to 2147483646, with leading zeros allowed.
…oduced a sequence number
…nd-in that supplies a sequence number
jwrosewell
force-pushed
the
align/json-sequence
branch
from
November 10, 2026 00:02
9cfc4d1 to
4f8086d
Compare
# Conflicts: # pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsage.test.js
# Conflicts: # pipeline/fiftyone.pipeline.core/jsonbundler.js # pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsage.test.js
…test pipelines Those tests left out only the Sequence Element, so the JSON Builder, which now fails with no sequence number, failed every request. They test the Cloud Request Engine, so they leave out the Sequence Element, the JSON Builder and the JavaScript Builder together with addJavaScriptBuilder: false.
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 JSON Builder now fails, and sets no JSON, when no Sequence Element has produced a sequence number, as the JSON Builder page says. The rest of what this draft first carried, being the switch for each web integration element, the Sequence Element first and its Element Data, the sequence rules and the JSON exclusions, reached
rcthrough #53, merged asf5763dc1, andrcis merged in here, so this diff holds only what #53 did not.Problem
On
rc,jsonbundler.jsreadquery.sequencestraight out of Evidence and treated an absent value as "below 10", so a Pipeline with no Sequence Element still produced JSON, with the sequence silently missing, and listed the JavaScript Properties on every request.What the specification says
The Sequence number section of json-builder.md at e89679e9, the specifications
rchead, saysand
.NET, 51degrees-python#76 and 51degrees-php#50 fail in the same way, and specifications#80 settled the wording.
What changed
pipeline/fiftyone.pipeline.core/jsonbundler.jsgainsgetSequence, which reads the Sequence Element's Element Data, falls back to a whole number inquery.sequence, and throws "There is no sequence number. A Sequence Element needs to be placed before the JSON Builder Element in the Pipeline." where there is neither.processInternalcalls it first, so a Pipeline with no sequence number sets no JSON. The number 10, from which no JavaScript Properties are listed, is the named constantMaxJavaScriptSequence.cloudCache.test.jsandcloudRequestEngineCredentials.test.js, which came with Give each web integration element its own switch, put Sequence first, and give it element data #53, built their pipelines with only the Sequence Element left out, so the JSON Builder failed every request. They test the Cloud Request Engine, so they now passaddJavaScriptBuilder: false, which leaves out the Sequence Element, the JSON Builder and the JavaScript Builder together. No test of the JSON Builder's rule is relaxed.shareUsage.test.jsandsessionIdAndSequence.test.jseach have a test that leaves out the Sequence Element, and each adds a stand-in element under thesequencedata key that gives a sequence number without touching the Evidence, so the JSON Builder in that pipeline has what it needs and what the test checks is unchanged.Tests
pipeline/fiftyone.pipeline.core/tests/jsonBuilderSequence.test.js, new, checks that the Element fails and sets no JSON with no Sequence Element, that'3','','abc',3.5andnullinquery.sequenceare each not a sequence number, that a whole number a caller added is accepted, and that a Pipeline with a Sequence Element and a default Pipeline both work.Every temporary run below sits in one workflow file shared by the temporary runs, so its page can carry the name of another branch in its title. Each run's own branch is named here.
tmp/node-59-red2, run 43005641915, this draft's three changed test files onrcatf5763dc1. 6 of 86 fail, every one injsonBuilderSequence.test.js("fails with an error and sets no JSON" and the five values that are not a whole number), and the other two files pass.tmp/node-59-red3, run 43005665570, the mergebc6091ebefore the test change. 10 of 22 fail incloudCache.test.jsandcloudRequestEngineCredentials.test.jswith "Errors occurred during processing of jsonbundler, javascriptbuilder".tmp/node-59-green, run 43005681174, the head99962c9with those five files andcore.test.js, 122 of 122 pass.771ad57, failed the same cloud tests for the reason above.99962c9, success, run 43006231028. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1061 of 1063 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are99962c92f9df62f9ae75ef9d5aab101640ad03c0.rchas since taken #65, which touches only the data update service, and the draft still merges cleanly.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.
Not done here
Writing
delayexecutiononly for a Property that has a value, which json-builder.md asks for in its suffix table, is a separate audit row and is not part of this change.