Skip to content

Accept only the evidence each element reads, and none by default - #46

Closed
jwrosewell wants to merge 8 commits into
rcfrom
fix/element-default-accepts-no-evidence
Closed

jwrosewell wants to merge 8 commits into
rcfrom
fix/element-default-accepts-no-evidence

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Every element is to accept only the Evidence that can change its results, as James decided on 7 November and as Advertise accepted Evidence writes down. That page is merged into the specifications rc branch through specifications#99 and specifications#103. The Pipeline keeps only the Evidence some element accepts, and the client-side ETag and Vary header are built from what it accepts. Three things in Node did not meet that rule.

  • FlowElement gave an element that declares no filter the base EvidenceKeyFilter, which accepts every key (flowElement.js line 67). SetHeadersElement declares none, so every pipeline with it accepted every key. The page says a base Flow Element's default MUST accept no Evidence.
  • The JavaScript builder accepted any key containing query. or header., which is every header (javascriptbuilder.js lines 178 to 182). It reads only query. keys, header.host and header.protocol (lines 293 to 381).
  • The sequence element accepted nothing, though it reads query.session-id and query.sequence (sequenceElement.js line 45). The JavaScript builder's wide filter kept them.

Change

  • flowElement.js: an element that gives no filter accepts no Evidence.
  • javascriptbuilder.js: accepts keys starting query., and header.host and header.protocol. The script it writes is unchanged, as it reads nothing else.
  • sequenceElement.js: accepts query.session-id and query.sequence.
  • cloudRequestEngine.js: the engine accepts every key until the cloud service's keys arrive, and then only the keys the service lists, as before. The keys are fetched when a cloud engine first asks for them (cloudEngine.js line 84), which is after the Pipeline is built, so the engine had the base default until then. With the old default that kept every key. With the new one it dropped the Evidence a caller sets straight after building the Pipeline, which the first full build showed (below).
  • javascriptbuilder.js: the old doc comment above the new one, which said the filter removes all but header and query evidence, is deleted because it is no longer true.
  • Tests that read back Evidence no element reads now add an element that says it accepts every key. That is the core test setup's async element, and a new AcceptEveryKeyElement in three cloud request engine tests, whose stand-in service lists only some keys.

How to verify

Both runs used the same temporary workflow on Ubuntu with Node 20, running jest in fiftyone.pipeline.core, fiftyone.pipeline.engines, fiftyone.pipeline.engines.fiftyone, fiftyone.pipeline.translation, fiftyone.pipeline.cloudrequestengine and fiftyone.ipintelligence.translation.

  1. Red, the tests without the two fixes (6713a08, run 42472302078). In fiftyone.pipeline.core 3 failed and 235 passed.
    ● an element that declares no evidence widens nothing
    ● the JavaScript builder accepts only what it reads
    ● the sequence element accepts its two keys
    
    An earlier red run of the default test alone, with only f860a33, failed an element that declares no evidence accepts none too (run 42471701922).
  2. Green, this branch (65a7897, run 42472302289). fiftyone.pipeline.core 238 passed, fiftyone.pipeline.engines.fiftyone 31, fiftyone.pipeline.translation 8 and fiftyone.ipintelligence.translation 22.
  3. Two suites fail in both runs, so they do not come from this change. In fiftyone.pipeline.engines, watched and scheduled data files can be collected once dropped fails. In fiftyone.pipeline.cloudrequestengine, integration.test.js needs a resource key, which the temporary workflow does not have. In the cloud request engine's other tests, 45 passed in both.
  4. The cloud request engine. The first full build, of 65a7897 (run 42472610004), failed in Ubuntu_ARM_Node_22. The cloud request engine's integration.test.js "valid response" got no value for device.ismobile, and the cloud client hints web example's tests failed, so the seven other jobs were cancelled. The test evidence set before the keys arrive is kept in cloudRequestEngine.test.js then failed with the test alone (bd8f387), "Expected: "iPhone" Received: undefined" (run 42476962801), and passed with the change (684c11e, run 42476962893), where 46 of the cloud request engine's 49 tests passed and the 3 that failed need a resource key, as in item 3.
  5. Full build. common-ci's nightly-pull-request.yml, run as a dry run from a temporary branch, built 684c11e, this branch's head, merged with rc (run 42477160600). All eight build and test jobs passed on the first attempt, on Ubuntu x64 and arm64, Windows and macOS with Node 22 and 24, the cloud request engine's integration tests and the cloud client hints web example included.

Full build of the final head. The head is now 30713cf, after two commits that delete the stale doc comment (the second restores line endings the first changed by mistake, so the net change is the five deleted lines). common-ci's nightly-pull-request.yml, run as a dry run from a temporary branch, built 30713cf merged with rc (run 42593975840). Its Configure job logs Setting 'PullRequestSha' to '<the head>', equal to this branch's head. The earlier full build in item 5 was of 684c11e.

Release note

Each element now accepts only the evidence that can change its results. An element that does not give its own evidence key filter accepts no evidence, where it accepted every key. The JavaScript builder accepts the query string, the host and the protocol, and no other headers. So a pipeline keeps only the evidence its elements read, and the client-side endpoints' Vary header names only those headers. An element of your own that declares no evidence no longer receives evidence that no other element accepts, and has to give an evidenceKeyFilter naming the evidence it reads.

What remains

  • fiftyone.pipeline.core and fiftyone.pipeline.cloudrequestengine change, so both are released, and so are the packages that depend on them.
  • A Pipeline holding the cloud request engine and no cloud engine never fetches the keys, so its request engine keeps accepting every key, as it did before this change.
  • keysAndPropertiesFetched (cloudRequestEngine.js line 126) reads this.evidenceKeyFilter.length, which a filter does not have. That is unchanged here, and what it does to how often the keys are fetched has not been checked.

@jwrosewell

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #45 (f76d8b0), which made each element accept only the evidence it reads.

@jwrosewell jwrosewell closed this Nov 13, 2026
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