Skip to content

Config: accept a bare scalar for an array option in YAML syntax#5413

Open
arpitjain099 wants to merge 4 commits into
fluent:masterfrom
arpitjain099:chore/yaml-array-scalar
Open

Config: accept a bare scalar for an array option in YAML syntax#5413
arpitjain099 wants to merge 4 commits into
fluent:masterfrom
arpitjain099:chore/yaml-array-scalar

Conversation

@arpitjain099

Copy link
Copy Markdown

Which issue(s) this PR fixes:
Fixes #5149

What this PR does / why we need it:
In YAML config syntax, a single scalar value for an array option is rejected. For example retryable_response_codes: 503 fails with array required but got 503, while the classic syntax retryable_response_codes 503 works fine. The difference is that the classic path delivers the value as the String "503", which Config.array_value already splits into ["503"], whereas YAML hands it over as a bare Integer that falls straight through to the "array required" check.

This wraps a non-String, non-Array, non-Hash scalar into a one-element array so both config syntaxes behave the same way. The wrapped value then flows through the existing value_type coercion, so retryable_response_codes: 503 becomes [503] just like retryable_response_codes: [503] does. A Hash still raises as before, so an accidental mapping under an array option is not silently accepted.

I could not run the full suite locally (my Ruby is 2.6 and Fluentd needs 3.2), so I verified the logic by tracing the exact array_value path and reproducing it in isolation; CI should confirm. I added test cases in test/config/test_types.rb covering a bare integer with and without value_type, and that an existing array is left untouched.

Docs Changes:
None.

Release Note:
Fix a config error when a single scalar value is given to an array option in YAML config syntax (for example retryable_response_codes: 503).

In YAML config a single scalar value for an array option (for example
retryable_response_codes: 503) reached Config.array_value as an Integer
and was rejected with "array required but got 503", while the classic
syntax retryable_response_codes 503 works because it arrives as the
String "503" and gets split into ["503"]. Wrap a non-String,
non-Array, non-Hash scalar into a one-element array so both syntaxes
behave the same. Hash still raises as before.

Fixes fluent#5149

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Comment thread test/config/test_types.rb
"value_type: :integer w/o quote" => [[1,2,1], '1,2,1', {value_type: :integer}],
"bare integer (YAML scalar)" => [[503], 503, {}],
"bare integer w/ value_type" => [[503], 503, {value_type: :integer}],
"already an array is untouched" => [[503], [503], {}])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: conversion rule was changed, so it might be better to add another test case such asa assert_raise(Fluent::ConfiguError.new("array required but got #{val.inspect}") for known types.

Could you afford to write such a test case?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test case asserting Config::ARRAY_TYPE keeps raising
Fluent::ConfigError with the "array required but got ..." message
for a hash input, so the scalar-wrapping change does not accidentally
accept a mapping under an array option. Covers a hash with and
without value_type.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099 arpitjain099 force-pushed the chore/yaml-array-scalar branch from 1b8d35a to 5c314d9 Compare July 7, 2026 03:22
@arpitjain099

Copy link
Copy Markdown
Author

Thanks for the review, good call. I've added a test case asserting that a Hash input still raises Fluent::ConfigError with the array required but got #{val.inspect} message, so the scalar-wrapping change doesn't accidentally accept a mapping under an array option. It covers a hash both with and without value_type. Verified green on Ruby 3.4.

Comment thread lib/fluent/config/types.rb Outdated
Comment on lines +234 to +235
else
[val]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern about the new else branch.
it wraps any object that isn't a String/Array/Hash, including Symbols, Time objects, or arbitrary objects that would previously have been rejected with ConfigError.
That defensive behavior is worth keeping for unexpected types.
Since the scalars Psych actually produces here are Integer/Float/true/false (nil is handled by the early return), how about limiting the wrapping to those types?

elsif val.is_a?(Numeric) || val == true || val == false
  [val]
else
  val  # anything else still raises "array required" below
end

This fixes the reported issue while preserving the existing error for genuinely invalid values.

Limit the bare-scalar wrapping in Config.array_value to the scalar
types Psych/YAML actually produces here (Numeric, true, false; nil is
handled by the early return). Any other type such as a Symbol, Time, or
Hash now falls through and still raises "array required", preserving
the previous defensive rejection of unexpected values.

Add tests covering bare float/true/false wrapping and that a Symbol and
a Time still raise Fluent::ConfigError alongside the existing Hash case.

Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
@arpitjain099

Copy link
Copy Markdown
Author

Good catch, thanks. You're right that the broad else swallowed types that should stay rejected. I've narrowed the wrapping to just the scalars Psych actually produces here (Numeric, true, false; nil still returns early), so anything unexpected like a Symbol or a Time now falls through and raises array required as before. Added test cases for a bare float/true/false wrapping, plus Symbol and Time still raising alongside the existing Hash case. Green on Ruby 3.4.

@Watson1978

Copy link
Copy Markdown
Contributor

The updated version looks good to me.

One non-blocking suggestion, it would be nice to also have an end-to-end test in test/config/test_yaml_parser.rb that exercises the actual YAML pipeline (e.g., a config with retryable_response_codes: 503 reaching a plugin's array parameter). The unit tests here verify ARRAY_TYPE directly, but the premise of this fix — that the YAML parser hands a bare Integer to the type-conversion layer — is only implicitly covered. If the YAML parser ever starts stringifying scalar values, an E2E test would catch the regression.

Watson1978
Watson1978 previously approved these changes Jul 8, 2026
@Watson1978 Watson1978 added this to the v1.20.0 milestone Jul 8, 2026
@Watson1978 Watson1978 added the backport to v1.19 We will backport this fix to the LTS branch label Jul 8, 2026
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099

Copy link
Copy Markdown
Author

Thanks @Watson1978, good idea. Added two tests to test/config/test_yaml_parser.rb that go through the real YAML pipeline: a config with retryable_response_codes: 503 on a match section, parsed with YamlParser and fed to a Configurable with an :array param (value_type: :integer). The bare scalar comes out as [503], and a [502, 503] sequence still works as before. Ran the file locally on Ruby 3.3, 33 tests green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport to v1.19 We will backport this fix to the LTS branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

YAML Config File Syntax: Single int value for an array option causes a config error

3 participants