Skip to content

Read a list setting given as one comma separated string, as configuration files give it - #129

Merged
jwrosewell merged 3 commits into
rcfrom
fix/comma-separated-lists
Nov 15, 2026
Merged

jwrosewell merged 3 commits into
rcfrom
fix/comma-separated-lists

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A list setting can be given only as an array, though a configuration file gives a list as one comma separated string. So a 51d.json that gives a list the way the specification says fails or is misread in three places, at rc 6d5d4fe.

  1. restrictedProperties of an on-premise engine. Device Detection and IP Intelligence call restrictedProperties.forEach (deviceDetectionOnPremise.js lines 376 and 377, ipIntelligenceOnPremise.js lines 256 and 257), and the base Engine calls restrictedProperties.map (engine.js lines 65 to 75), so a string stops the build with a TypeError.
  2. headerBlacklist of usage sharing. The share filter calls map on it (shareUsageEvidenceKeyFilter.js line 80), so a string stops the build too.
  3. queryWhitelist of usage sharing. Usage sharing spreads it into an array (shareUsage.js lines 174 to 176), so "a,b" becomes the parameter names a, , and b, with no error.

What the specification says

From pipeline-specification/features/pipeline-configuration.md lines 50 to 61 at 706de1f0:

Configuration files MUST be human-readable and consistent between languages. Consequently, we don't want any type information or metadata appearing in these files. This implies some limitations on the types that can be deserialized from values in the files, as additional logic might need to be added for each type.

To date, the following types are supported in configuration files for all languages:

  • string
  • boolean
  • numeric (integer or floating point)
  • list of strings (represented as a comma-separated string)
  • enumeration value (represented as the string name of the value)

What changed

  1. A new listSetting in fiftyone.pipeline.core, exported from the package, reads a list setting. An array is used as it is. A string is split at each comma, each item without the space around it and with empty items left out. A string with no items counts as not set, because an empty list would restrict an engine to no properties.
  2. The base Engine, both on-premise engines (restrictedProperties) and usage sharing (queryWhitelist and headerBlacklist) read their lists through it.
  3. The type declarations say each of these settings may be one comma separated string, and declare listSetting, written in the form npm run tsc writes.

An array given in code works as before, so nothing changes for a caller that already gives one.

Tests

  • pipeline/fiftyone.pipeline.core/tests/listSetting.test.js checks the six cases of listSetting, being a comma separated string, a string with no comma, empty items and spaces, a string with no items, an array and a setting that is not set.
  • pipeline/fiftyone.pipeline.engines/tests/restrictedPropertiesText.test.js checks that an engine given restrictedProperties as a string restricts itself to those properties, and as an array to that array.
  • restrictedPropertiesText.test.js in each on-premise package builds the engine from a configuration whose restrictedProperties is one comma separated string, on the free data file, and checks the properties it offers.
  • pipeline/fiftyone.pipeline.engines.fiftyone/tests/shareUsageConfiguration.test.js builds usage sharing from a configuration with both lists as comma separated strings, then as arrays, and checks which query parameters and headers reach the message.

Red, run 43639258221 on 41c6e40, the test commit on rc's code, with only a throwaway workflow added that builds the native modules and fetches the free data files as CI does. On Ubuntu 10 of the 12 tests fail, being the six listSetting tests, as rc has no listSetting, and the four that give a list as a string. The two that give a list as an array pass.

Green, run 43639269745 on d17526e, the fix, with only the same throwaway workflow added. All 12 pass, and lint passes on the four packages the change touches with the three warnings rc already has.

Types, run 43639283318 ran npm run tsc on d17526e merged with the head of #113, which writes the types again. The declarations it wrote for these settings and for listSetting are committed here as dec76a2. Its other difference, pipeline/fiftyone.pipeline.did/types/owid-js/v1.d.ts, comes from the OWID submodule already on rc, which #127 decides.

Full build

Run 43650210325 on tmp/pr-matrix-129, started by tools/pr-matrix.py without macOS. All 7 Setting 'PullRequestSha' lines give this head, dec76a2, 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,505 unit tests with 2 skipped and 96 integration tests with 4 skipped.

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 for this requirement. The other list setting of the packages, the cloud engines' requestedProperties, already takes one comma separated string. The pipeline builders pass restrictedProperties to the engines as given (for example deviceDetectionOnPremisePipelineBuilder.js line 174), so a string given to them is read the same way.

@jwrosewell
jwrosewell marked this pull request as ready for review November 15, 2026 00:00
@jwrosewell
jwrosewell merged commit cc028ff into rc Nov 15, 2026
2 checks passed
@jwrosewell
jwrosewell deleted the fix/comma-separated-lists branch November 15, 2026 00:00
@jwrosewell jwrosewell mentioned this pull request Nov 15, 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