Repository navigation
Give each web integration element its own switch, put Sequence first, and give it element data - #53
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.
With the Sequence Element first in every pipeline, each request gains a new session id. The cache tests process before the engine has fetched its evidence keys, so the session id went into the cache key and no request was served from the cache. The cloud's own evidence keys do not include the session id or the sequence. The serve test computes the key before processing, so it needs the same pipeline to find the entry.
The tests compare each request body with the credentials and evidence they give. With the Sequence Element first in every pipeline, the body also carries its session id and sequence, as the cloud is sent every Evidence value apart from the reserved names.
jwrosewell
marked this pull request as ready for review
November 11, 2026 10:37
This was referenced Nov 11, 2026
Draft
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.
Brings the Node pipeline builder and Sequence Element in line with the Web integration page and the Sequence Element page of the pipeline specification (both read in full at e89679e9, the specifications
rchead, where every sentence quoted below still reads as quoted). Each change below was first checked against the code onrc.The data update changes are in #52. Open drafts #45 and #46 change one line of
sequenceElement.js(its accepted evidence, in the constructor). This draft changes thesuper(...)call just above it and theprocessInternalmethod below it, so the two should merge with a trivial conflict at most, depending on which lands first.What changed
W2. A switch for each element.
Spec, Pipeline configuration: "For all of these elements, there MUST be configuration options to allow users to prevent them being added (or disable them) if they wish to do so."
Before,
addJavaScriptBuilderturned the Sequence, JSON Builder and JavaScript Builder elements off together anduseSetHeaderPropertiesturned Set Headers off.PipelineBuildernow also takesaddSequenceElement,addJsonBuilderElement,addJavaScriptBuilderElementandaddSetHeadersElement, each true by default.addJavaScriptBuilder: falseanduseSetHeaderProperties: falsework exactly as before (they leave out their elements whatever the new switches say), so no existing configuration changes. The types are updated.Sequence first.
Spec, Pipeline configuration: "The Sequence Element MUST be present. If not, it MUST be added as the first element in the Pipeline."
Before,
addRequiredElementsput it after the user's elements. It is now first, so the session id and sequence it writes to the evidence are there for every element. The JSON Builder and JavaScript Builder keep their place after the user's elements, and Set Headers after those.Session id and sequence are read on their own.
Spec, Sequence Element, Process: a new session id is generated "if
query.session-idis not present", and the sequence is the given one plus one, or one "if it is not present". The element now treats them separately, as the merged PHP change does. A session id that is not a non-empty string counts as absent and gets a new id. A sequence counts as present only when it is a whole number, or a string made only of the digits 0 to 9, leading zeros allowed, from 0 to 2147483646, and gets one added. Anything else, including an array, an object, a negative number,"abc"andNaN, counts as absent and gives 1. So a sequence with no session id now counts up, and a session id with no sequence gives 1 where it gave 2. NaN is never written.5b. Sequence Element Data.
Spec, Sequence Element, Element Data:
session-id(string) andsequence(int). Process: "Add both output values to Evidence as well".The Sequence Element now declares both properties and sets them on its element data, as well as writing the same two values to the evidence as before. The values are the ones written to the evidence, so the way a session id and a sequence are worked out is unchanged.
Does this add a sequence section to the JSON sent to browsers? Yes, as the JSON Builder page requires.
Spec, JSON Builder, Process: "There MUST be a top-level entry for each Element Data", and only the Set Headers Element, Cloud Request Engine and Usage Sharing Element are excluded.
jsonbundler.jshadsequencein its exclusion list, so it is removed, and the Cloud Request Engine (cloud) and Usage Sharing (shareUsage) elements are added to it, with a test. The entry is under the keysequence, the Sequence Element's data key (the page names no key, and .NET, Go and Python usesequence). It holdssession-idandsequence, for example"sequence": {"session-id": "abc-123", "sequence": 5}. One existing test that spells out the whole JSON was updated.One more visible change follows from the element data. The "There is no element data for ..." error lists the elements that do have data, and
sequenceis now in that list (first, as it runs first). Two existing tests that spell out the list were updated.Switch names. The Web integration page says only that there must be configuration options, and names none, so the names
addSequenceElement,addJsonBuilderElement,addJavaScriptBuilderElementandaddSetHeadersElementfollow the existingaddJavaScriptBuilderstyle.Lite data file comments. Seventeen example comments (9 Device Detection and 6 IP Intelligence found by searching for "limited accuracy", and the two
updatedatafile.jscomments that said "limited capabilities") now say the Lite data file has "a small number of Properties", as the specification does, instead of "limited accuracy and capabilities".Two effects of putting Sequence first, both found by the full build.
query.sequencethe Sequence Element adds made it fail ("argument 3 of type 'std::string const &'"). The Device Detection and IP Intelligence engines now pass only the evidence their own key filter accepts, which is the list of keys the native engine reads. The first full build of this draft, without that change, failed 18 tests in 9 suites for this reason: https://github.com/51Degrees/51degrees-node/actions/runs/42662212293Tests
Written first, in
pipeline/fiftyone.pipeline.core/tests/webIntegrationElements.test.js.rccode with the tests only, head19b3ce9(11 of 13 fail, and the two that pass are existing behaviours the draft must keep): https://github.com/51Degrees/51degrees-node/actions/runs/426525160819701d3f: https://github.com/51Degrees/51degrees-node/actions/runs/42676738390cloudCache.test.js,cloudRequestEngineCredentials.test.js) build their pipelines withaddSequenceElement: false, in 409c524 and 484eccd. They process with an engine that has not fetched its evidence keys, so its cache key is built from every Evidence value, the new session id included. A new test,serves a repeated request from the cache in a default pipeline, covers the behaviour rather than the test. It builds a default pipeline, so the Sequence Element is first, has the engine fetch its keys as a cloud engine does when it is added, processes the same request twice, checks the two session ids differ, and checks the cloud was asked once. It pins behaviour this change keeps rather than a fix, because the keys the cloud lists hold no session id, so it has no red run.015e6cd, success, run 42986694165. Configure and the Ubuntu, Ubuntu ARM and Windows jobs on Node 22 and 24 pass, 1049 of 1051 tests with 2 skipped in each, Configure printed "Left out 2 macOS entries", and all 7Setting 'PullRequestSha'lines are015e6cd65ea94c09814a94529ad8c6a0217e6005.The full build this body cited before, run 42684289757, was of
9701d3f, sixteen commits back. The one after it, run 42907092247 on484eccd, failed one test of 1050 on Windows Node 22 only,updateOnStartMissingFile.test.js"asks for the data file with no If-Modified-Since header", by exceeding jest's 10 second timeout. No cause beyond the timeout was found. This draft changes nothing in the data update service or its tests, only comments in the update examples, and the same test passed on all six jobs of run 42986694165. Since then the draft has takenrctwice, the second time for the currentcxxsubmodule in6d29452, and gained the cache test in015e6cd.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
ready()(cloudEngine.jsline 93). Its cache key is then built from every Evidence value (engine.jslines 90 to 92), so with the Sequence Element first each request has a new key and is never served from the cache. The Start-up activity section says the Engine resolves its accepted Evidence when it is built, which Node does not do. That is a separate change, and it decides the cache key for #72 as well.rcand is rebuilt on top of it, so that its diff holds only what this draft does not, chiefly the JSON Builder's error when no sequence number exists.