Repository navigation
Accept only the evidence each element reads, and none by default - #46
Closed
jwrosewell wants to merge 8 commits into
Closed
jwrosewell wants to merge 8 commits into
jwrosewell wants to merge 8 commits into
Conversation
… what they read, and keep the evidence the tests read with elements that say so
This was referenced Nov 9, 2026
Merged
Merged
Contributor
Author
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.
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
rcbranch 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.FlowElementgave an element that declares no filter the baseEvidenceKeyFilter, which accepts every key (flowElement.js line 67).SetHeadersElementdeclares none, so every pipeline with it accepted every key. The page says a base Flow Element's default MUST accept no Evidence.query.orheader., which is every header (javascriptbuilder.js lines 178 to 182). It reads onlyquery.keys,header.hostandheader.protocol(lines 293 to 381).query.session-idandquery.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 startingquery., andheader.hostandheader.protocol. The script it writes is unchanged, as it reads nothing else.sequenceElement.js: acceptsquery.session-idandquery.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.jsline 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.asyncelement, and a newAcceptEveryKeyElementin 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.cloudrequestengineandfiftyone.ipintelligence.translation.fiftyone.pipeline.core3 failed and 235 passed.an element that declares no evidence accepts nonetoo (run 42471701922).fiftyone.pipeline.core238 passed,fiftyone.pipeline.engines.fiftyone31,fiftyone.pipeline.translation8 andfiftyone.ipintelligence.translation22.fiftyone.pipeline.engines,watched and scheduled data files can be collected once droppedfails. Infiftyone.pipeline.cloudrequestengine,integration.test.jsneeds a resource key, which the temporary workflow does not have. In the cloud request engine's other tests, 45 passed in both.Ubuntu_ARM_Node_22. The cloud request engine'sintegration.test.js"valid response" got no value fordevice.ismobile, and the cloud client hints web example's tests failed, so the seven other jobs were cancelled. The testevidence set before the keys arrive is keptincloudRequestEngine.test.jsthen 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.nightly-pull-request.yml, run as a dry run from a temporary branch, built 684c11e, this branch's head, merged withrc(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 withrc(run 42593975840). Its Configure job logsSetting 'PullRequestSha' to '<the head>', equal to this branch's head. The earlier full build in item 5 was of 684c11e.Release note
What remains
fiftyone.pipeline.coreandfiftyone.pipeline.cloudrequestenginechange, so both are released, and so are the packages that depend on them.keysAndPropertiesFetched(cloudRequestEngine.jsline 126) readsthis.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.