Skip to content

Add each web integration element only where the Pipeline lacks one - #105

Merged
jwrosewell merged 5 commits into
rcfrom
fix/web-elements-once
Nov 14, 2026
Merged

jwrosewell merged 5 commits into
rcfrom
fix/web-elements-once

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

PipelineBuilder.addRequiredElements adds the Sequence Element, the JSON Builder, the JavaScript Builder and the Set Headers Element whatever the caller already added (pipeline/fiftyone.pipeline.core/pipelineBuilder.js lines 200 to 205 at 758fea81). This has three effects.

  • A caller that adds its own JavaScript Builder, for example with its own endPoint, gets a second one after it, and the Pipeline's element for the key javascriptbuilder is the builder's, not the caller's, because the second replaces the first in the Pipeline's map.
  • A caller that adds a JavaScript Builder and no JSON Builder gets the JSON Builder after the JavaScript Builder that reads it.
  • build() stores the result back in the builder (line 315), so building twice adds every one of them twice.

#53 recorded this under "Not done here". It is gap DX-node-11.

What the specification says

pipeline-specification/features/web-integration.md lines 29 to 45 at c8128ca4:

Where the web integration is responsible for creation of the Pipeline, it will also need to ensure that these additional Flow Elements are present.

  • The Sequence Element MUST be present. If not, it MUST be added as the first element in the Pipeline.
  • The JSON Builder Element MUST be present. If not, it MUST be added as the penultimate element. (Immediately before the JavaScript Builder Element).
  • The JavaScript Builder Element MUST be present. If not, it MUST be added after all other elements.

[...]

  • The Set Headers Element MUST be present. If not, it MUST be added after all other elements.

What changed

addRequiredElements collects the keys of the elements already given, those added in parallel included, and adds each of the four only where no element has its key, comparing keys ignoring case as access-to-results.md lines 10 and 11 at c8128ca4 asks of every string comparison ("String comparisons MUST be case insensitive."). So an element the caller gives under Sequence counts as the Sequence Element. A JSON Builder it adds goes immediately before a JavaScript Builder the caller added, and otherwise at the end as before. An element the caller added stays where the caller put it. With nothing added by the caller the result is the same as before, which the existing tests in the same file check.

Tests

Five new tests in pipeline/fiftyone.pipeline.core/tests/webIntegrationElements.test.js: the four elements the caller added are not added again and are the ones the Pipeline uses, a JSON Builder the Pipeline lacks goes immediately before the caller's JavaScript Builder, a Set Headers Element added in parallel counts as present and stays there, building twice adds none of them twice, and an element whose key differs only in case counts as present.

  • Red run 43557385669, the tests on rc's builder, where 4 failed and 497 passed.

  • Green run 43557391894 at 4df63c5, where 501 passed.

  • Red for the case, the case test on the builder before it compared keys ignoring case (82a753d6), in run 43562889990, where 1 failed and 501 passed, the one failure being the case test.

  • Green at b6c97512, in run 43562928082, where 502 passed.

Both ran the core package's tests on one Ubuntu runner with Node.js 24 in a throwaway workflow that is not part of this draft. The first green run, 43556604078, failed one test, because the test expected the parallel Set Headers Element at the end. The builder had left it where the caller put it, which is what the page asks, so commit 4df63c5 corrects the test's expectation.

Full build

Run 43563537285 on tmp/pr-matrix-105-b, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, b6c9751, and Configure printed "Left out 2 macOS entries". Each of the six Build and Test jobs, Ubuntu, Ubuntu ARM and Windows on Node.js 22 and 24, passed 1,388 unit tests with 2 skipped and 138 integration tests with 4 skipped, the five new tests among them. The build of the earlier head 4df63c5, run 43557677489, passed as well.

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 in this repository. In the other languages, .NET adds each element only where none is present, comparing builder names ignoring case (pipeline/Web/FiftyOneStartup.cs lines 262 to 265 and 324 to 329 at 51degrees-dotnet 37102176), and so does Java (StartupHelpers.java lines 48 to 58 at 51degrees-java c5d4559f). Rust's draft 51degrees-rust#412 makes its Sequence Element check ignore case. Python, PHP and Go were not checked here.

@jwrosewell
jwrosewell marked this pull request as ready for review November 14, 2026 07:51
@jwrosewell
jwrosewell merged commit d410304 into rc Nov 14, 2026
2 checks passed
@jwrosewell
jwrosewell deleted the fix/web-elements-once branch November 14, 2026 07:51
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