From fd6c02442f592def61dbc51fb82128bfed7a458d Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 31 Aug 2026 20:00:23 +0200 Subject: [PATCH 01/12] traffic_ctl: emit JSON null instead of YAML tilde The JSON encoders are yaml-cpp emitters, and yaml-cpp spells null as `~`, so any RPC payload holding a null value was rejected by every JSON parser. `hostdb status` hit this on every freshly started server and still exited 0, so callers saw success and then failed to parse. Initialise the two accumulator nodes as sequences so an empty result is `[]` rather than null, which also satisfies the published schema, and set LowerNull on the four JSON emitters so any remaining null is spelled `null`. Both spellings parse back as null in YAML, so the server's ability to accept YAML input is unaffected. --- .../jsonrpc/jsonrpc-architecture.en.rst | 4 ++++ include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | 12 +++++++----- include/shared/rpc/yaml_codecs.h | 1 + src/mgmt/rpc/handlers/hostdb/HostDB.cc | 2 +- src/mgmt/rpc/handlers/plugins/Plugins.cc | 2 +- src/traffic_ctl/CtrlPrinters.cc | 1 + 6 files changed, 15 insertions(+), 7 deletions(-) diff --git a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst index 5f860545606..ba03ff307e3 100644 --- a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst +++ b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst @@ -74,6 +74,10 @@ Our JSONRPC protocol implementation uses lib yamlcpp for parsing incoming and o this allows the server to accept either JSON or YAML format messages which then will be parsed by the protocol implementation. This seems handy for user that want to feed |TS| with existing yaml configuration without the need to translate yaml into json. +Null values on the way out are emitted as literal ``null`` rather than yaml's ``~``, as ``~`` is not accepted by JSON parsers. Both +spellings resolve to null when read as yaml, so this does not affect messages that are consumed as yaml, nor the ability to send +yaml to the server. + .. note:: :program:`traffic_ctl` have an option to read files from disc and push them into |TS| through the RPC server. Files should be a diff --git a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h index cc29dfe2dd9..b3d2365ec11 100644 --- a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h +++ b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h @@ -28,9 +28,9 @@ namespace rpc::json_codecs { /// /// @note The overall design is to make this classes @c yamlcpp_json_decoder and @c yamlcpp_json_encoder plugables into the Json Rpc -/// encode/decode logic. yamlcpp does not give us all the behavior we need, such as the way it handles the null values. Json needs -/// to use literal null and yamlcpp uses ~. If this becomes a problem, then we may need to change the codec implementation, we just -/// follow the api and it should work with minimum changes. +/// encode/decode logic. yamlcpp defaults to emitting null as ~, which is valid yaml but not valid json, so every emitter here sets +/// @c YAML::LowerNull to spell it literal null. Both spellings resolve back to null when parsed as yaml, so accepting yaml input +/// is unaffected. /// /// @@ -251,8 +251,8 @@ class yamlcpp_json_encoder if (!resp.id.empty()) { json << YAML::Key << "id" << YAML::Value << resp.id; } - // else: We do not insert null as it will break the json, we need literal null and not ~ (as per yaml) - // json << YAML::Null; + // else: the field is omitted rather than set to null. Emitting it would be valid json now that LowerNull is set, but the + // omission is deliberate, see the id note in mgmt/rpc/schema/jsonrpc_response_schema.json. json << YAML::EndMap; } @@ -268,6 +268,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponseInfo &resp) { YAML::Emitter json; + json.SetNullFormat(YAML::LowerNull); json << YAML::DoubleQuoted << YAML::Flow; encode(resp, json); @@ -284,6 +285,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponse &response) { YAML::Emitter json; + json.SetNullFormat(YAML::LowerNull); json << YAML::DoubleQuoted << YAML::Flow; { if (response.is_batch()) { diff --git a/include/shared/rpc/yaml_codecs.h b/include/shared/rpc/yaml_codecs.h index d719973f312..d6dd7305556 100644 --- a/include/shared/rpc/yaml_codecs.h +++ b/include/shared/rpc/yaml_codecs.h @@ -202,6 +202,7 @@ class yamlcpp_json_emitter encode(shared::rpc::JSONRPCRequest const &req) { YAML::Emitter json; + json.SetNullFormat(YAML::LowerNull); json << YAML::DoubleQuoted << YAML::Flow; json << YAML::BeginMap; diff --git a/src/mgmt/rpc/handlers/hostdb/HostDB.cc b/src/mgmt/rpc/handlers/hostdb/HostDB.cc index 9759a1cd19e..f1e386b2fb1 100644 --- a/src/mgmt/rpc/handlers/hostdb/HostDB.cc +++ b/src/mgmt/rpc/handlers/hostdb/HostDB.cc @@ -82,7 +82,7 @@ template <> struct convert { static Node encode(const HostDBCache *const hostDB, std::string_view hostname) { - Node partitions; + Node partitions{YAML::NodeType::Sequence}; for (size_t i = 0; i < hostDB->refcountcache->partition_count(); i++) { auto &partition = hostDB->refcountcache->get_partition(i); std::vector partition_entries; diff --git a/src/mgmt/rpc/handlers/plugins/Plugins.cc b/src/mgmt/rpc/handlers/plugins/Plugins.cc index 58e43f26ebd..7edfaa68c49 100644 --- a/src/mgmt/rpc/handlers/plugins/Plugins.cc +++ b/src/mgmt/rpc/handlers/plugins/Plugins.cc @@ -100,7 +100,7 @@ get_plugin_list(std::string_view const & /* id ATS_UNUSED */, YAML::Node const & data["source"] = summary.source; - YAML::Node plugins; + YAML::Node plugins{YAML::NodeType::Sequence}; for (const auto &e : summary.entries) { YAML::Node plugin; diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index 22b934179df..88a43ccc637 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -102,6 +102,7 @@ void BasePrinter::write_output_json(YAML::Node const &node) const { YAML::Emitter out; + out.SetNullFormat(YAML::LowerNull); out << YAML::DoubleQuoted << YAML::Flow; out << node; std::cout << out.c_str() << '\n'; From 81a52a91108f8c67eea12ee07b86cb8d0038a0ad Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 17:19:50 +0200 Subject: [PATCH 02/12] traffic_ctl: route JSON emitters through one helper The tilde fix set LowerNull at the four emitters on the RPC path, but SSLMultiCertMarshaller::to_json and StorageMarshaller::to_json build their own emitters and were missed. Neither can emit a null today, so nothing is broken, but both are one null away from the same bug. Collapse the DoubleQuoted/Flow/LowerNull idiom into ts::Yaml::configure_json_emitter() so a new emitter cannot silently omit part of it. There is now exactly one place in the tree that puts an emitter into JSON mode. --- include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | 8 ++++---- include/shared/rpc/yaml_codecs.h | 5 +++-- include/tsutil/YamlCfg.h | 16 ++++++++++++++++ src/config/ssl_multicert.cc | 3 ++- src/config/storage.cc | 3 ++- src/traffic_ctl/CtrlPrinters.cc | 3 +-- 6 files changed, 28 insertions(+), 10 deletions(-) diff --git a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h index b3d2365ec11..9435a937896 100644 --- a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h +++ b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h @@ -21,6 +21,8 @@ #pragma once #include + +#include "tsutil/YamlCfg.h" #include "mgmt/rpc/jsonrpc/error/RPCError.h" #include "mgmt/rpc/jsonrpc/Defs.h" @@ -268,8 +270,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponseInfo &resp) { YAML::Emitter json; - json.SetNullFormat(YAML::LowerNull); - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); encode(resp, json); return json.c_str(); @@ -285,8 +286,7 @@ class yamlcpp_json_encoder encode(const specs::RPCResponse &response) { YAML::Emitter json; - json.SetNullFormat(YAML::LowerNull); - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); { if (response.is_batch()) { json << YAML::BeginSeq; diff --git a/include/shared/rpc/yaml_codecs.h b/include/shared/rpc/yaml_codecs.h index d6dd7305556..d05c5bd74aa 100644 --- a/include/shared/rpc/yaml_codecs.h +++ b/include/shared/rpc/yaml_codecs.h @@ -22,6 +22,8 @@ #include #include +#include "tsutil/YamlCfg.h" + #include "shared/rpc/RPCRequests.h" /// JSONRPC 2.0 Client API request/response codecs only. If you need to define your own specific codecs they should then be defined @@ -202,8 +204,7 @@ class yamlcpp_json_emitter encode(shared::rpc::JSONRPCRequest const &req) { YAML::Emitter json; - json.SetNullFormat(YAML::LowerNull); - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); json << YAML::BeginMap; if (!req.id.empty()) { diff --git a/include/tsutil/YamlCfg.h b/include/tsutil/YamlCfg.h index 09686eba54a..ab52f767502 100644 --- a/include/tsutil/YamlCfg.h +++ b/include/tsutil/YamlCfg.h @@ -39,6 +39,22 @@ namespace Yaml constexpr std::string_view YAML_BOOL_TAG_URI{"tag:yaml.org,2002:bool"}; constexpr std::string_view YAML_NULL_TAG_URI{"tag:yaml.org,2002:null"}; + // Put an emitter into JSON output mode. + // + // yaml-cpp has no JSON mode. The nearest equivalent is flow style with every scalar double quoted, which is valid + // JSON for every node type except null: yaml-cpp writes ~, and no JSON parser accepts that. LowerNull writes the + // literal null. YAML reads ~ and null as the same value, so output stays readable as YAML either way. + // + // Every emitter whose output reaches a JSON consumer must go through here. Setting two of the three manipulators + // gives output that looks like JSON and parses correctly until some node is null. + // + inline void + configure_json_emitter(YAML::Emitter &emitter) + { + emitter.SetNullFormat(YAML::LowerNull); + emitter << YAML::DoubleQuoted << YAML::Flow; + } + // A class that is a wrapper for a YAML::Node that corresponds to a map in a YAML input file. // It's purpose is to make sure all keys in the map are processed. // diff --git a/src/config/ssl_multicert.cc b/src/config/ssl_multicert.cc index c64780113ef..7b661baced2 100644 --- a/src/config/ssl_multicert.cc +++ b/src/config/ssl_multicert.cc @@ -34,6 +34,7 @@ #include "swoc/swoc_file.h" #include "swoc/TextView.h" #include "tsutil/ts_diag_levels.h" +#include "tsutil/YamlCfg.h" namespace { @@ -360,7 +361,7 @@ std::string SSLMultiCertMarshaller::to_json(SSLMultiCertConfig const &config) { YAML::Emitter json; - json << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(json); json << YAML::BeginMap; json << YAML::Key << KEY_SSL_MULTICERT << YAML::Value << YAML::BeginSeq; diff --git a/src/config/storage.cc b/src/config/storage.cc index 42b2273f61f..aae5c21ef27 100644 --- a/src/config/storage.cc +++ b/src/config/storage.cc @@ -37,6 +37,7 @@ #include "swoc/swoc_file.h" #include "tscore/ParseRules.h" #include "tsutil/ts_diag_levels.h" +#include "tsutil/YamlCfg.h" namespace { @@ -828,7 +829,7 @@ std::string StorageMarshaller::to_json(StorageConfig const &config) { YAML::Emitter out; - out << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(out); out << YAML::BeginMap; out << YAML::Key << KEY_CACHE << YAML::Value << YAML::BeginMap; diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index 88a43ccc637..a58c06d3877 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -102,8 +102,7 @@ void BasePrinter::write_output_json(YAML::Node const &node) const { YAML::Emitter out; - out.SetNullFormat(YAML::LowerNull); - out << YAML::DoubleQuoted << YAML::Flow; + ts::Yaml::configure_json_emitter(out); out << node; std::cout << out.c_str() << '\n'; } From f6ee3fb254e079d13c6bdaaa0b855bd24d7ae6ee Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 1 Sep 2026 17:54:16 +0200 Subject: [PATCH 03/12] traffic_ctl: address review on the JSON emitter helper Include tsutil/YamlCfg.h directly in CtrlPrinters.cc rather than relying on it arriving through the codec headers. Correct the helper comment, which claimed the output was valid JSON for every node type but null. That holds only for the node shapes these callers build; a tag, anchor or alias still emits YAML that JSON does not accept. Say so, and use the canonical yaml-cpp, YAML and JSON spellings in the comments and the architecture doc. --- .../jsonrpc/jsonrpc-architecture.en.rst | 5 ++--- include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | 8 ++++---- include/tsutil/YamlCfg.h | 12 ++++++++---- src/traffic_ctl/CtrlPrinters.cc | 1 + 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst index ba03ff307e3..cea4b8d336b 100644 --- a/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst +++ b/doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst @@ -74,9 +74,8 @@ Our JSONRPC protocol implementation uses lib yamlcpp for parsing incoming and o this allows the server to accept either JSON or YAML format messages which then will be parsed by the protocol implementation. This seems handy for user that want to feed |TS| with existing yaml configuration without the need to translate yaml into json. -Null values on the way out are emitted as literal ``null`` rather than yaml's ``~``, as ``~`` is not accepted by JSON parsers. Both -spellings resolve to null when read as yaml, so this does not affect messages that are consumed as yaml, nor the ability to send -yaml to the server. +The server emits null values as the literal ``null``, not as YAML's ``~``. JSON parsers reject ``~``. YAML resolves ``~`` and +``null`` to the same value. Clients that read the response as YAML see no change, and the server still accepts YAML input. .. note:: diff --git a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h index 9435a937896..ef7aa9de381 100644 --- a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h +++ b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h @@ -29,10 +29,10 @@ namespace rpc::json_codecs { /// -/// @note The overall design is to make this classes @c yamlcpp_json_decoder and @c yamlcpp_json_encoder plugables into the Json Rpc -/// encode/decode logic. yamlcpp defaults to emitting null as ~, which is valid yaml but not valid json, so every emitter here sets -/// @c YAML::LowerNull to spell it literal null. Both spellings resolve back to null when parsed as yaml, so accepting yaml input -/// is unaffected. +/// @note The design keeps @c yamlcpp_json_decoder and @c yamlcpp_json_encoder replaceable in the JSONRPC encode/decode logic. +/// yaml-cpp emits null as @c ~ by default, which JSON parsers reject. Every emitter here calls +/// @c ts::Yaml::configure_json_emitter, which writes the literal @c null instead. YAML resolves @c ~ and @c null to the same +/// value, so the server still accepts YAML input. /// /// diff --git a/include/tsutil/YamlCfg.h b/include/tsutil/YamlCfg.h index ab52f767502..5d47079cf53 100644 --- a/include/tsutil/YamlCfg.h +++ b/include/tsutil/YamlCfg.h @@ -41,11 +41,15 @@ namespace Yaml // Put an emitter into JSON output mode. // - // yaml-cpp has no JSON mode. The nearest equivalent is flow style with every scalar double quoted, which is valid - // JSON for every node type except null: yaml-cpp writes ~, and no JSON parser accepts that. LowerNull writes the - // literal null. YAML reads ~ and null as the same value, so output stays readable as YAML either way. + // yaml-cpp has no JSON output mode. The nearest equivalent is flow style with every scalar double quoted. For the + // node shapes the callers here emit -- maps, sequences, scalars and nulls, carrying no tags, anchors or aliases -- + // that is JSON-compatible except for null: yaml-cpp writes `~`, which JSON parsers reject. LowerNull writes the + // literal `null` instead. YAML resolves `~` and `null` to the same value, so the output still reads as YAML. // - // Every emitter whose output reaches a JSON consumer must go through here. Setting two of the three manipulators + // This is not a general YAML to JSON converter. A node that carries a tag, an anchor or an alias still emits YAML + // syntax that JSON does not accept. + // + // Every emitter whose output reaches a JSON consumer must go through here. Setting only some of the manipulators // gives output that looks like JSON and parses correctly until some node is null. // inline void diff --git a/src/traffic_ctl/CtrlPrinters.cc b/src/traffic_ctl/CtrlPrinters.cc index a58c06d3877..613cd6aa0c2 100644 --- a/src/traffic_ctl/CtrlPrinters.cc +++ b/src/traffic_ctl/CtrlPrinters.cc @@ -24,6 +24,7 @@ #include #include "tsutil/ts_bw_format.h" +#include "tsutil/YamlCfg.h" #include "CtrlPrinters.h" #include "jsonrpc/ctrl_yaml_codecs.h" From 86119e63fd44d489b0c03915c2645522a16d3a8c Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 7 Sep 2026 12:35:21 +0200 Subject: [PATCH 04/12] traffic_ctl: add a regression test for JSON null output Nothing asserted that -f json output parses, which is how the yaml-cpp `~` went unnoticed with every CI job green. A gold file cannot catch it either, since it matches `~` forever, so this parses the output for real and covers the empty HostDB and no-plugins cases that trigger it. Also scope the emitter comment: flow style with every scalar quoted parses as JSON but is not type-faithful, so numbers and booleans arrive as strings. --- include/tsutil/YamlCfg.h | 7 +- .../traffic_ctl/traffic_ctl_json_null.test.py | 84 +++++++++++++++++++ .../traffic_ctl/traffic_ctl_test_utils.py | 74 ++++++++++++++++ 3 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py diff --git a/include/tsutil/YamlCfg.h b/include/tsutil/YamlCfg.h index 5d47079cf53..367b95b3fd0 100644 --- a/include/tsutil/YamlCfg.h +++ b/include/tsutil/YamlCfg.h @@ -43,9 +43,14 @@ namespace Yaml // // yaml-cpp has no JSON output mode. The nearest equivalent is flow style with every scalar double quoted. For the // node shapes the callers here emit -- maps, sequences, scalars and nulls, carrying no tags, anchors or aliases -- - // that is JSON-compatible except for null: yaml-cpp writes `~`, which JSON parsers reject. LowerNull writes the + // that parses as JSON except for null: yaml-cpp writes `~`, which JSON parsers reject. LowerNull writes the // literal `null` instead. YAML resolves `~` and `null` to the same value, so the output still reads as YAML. // + // Parses as JSON is the whole guarantee. It is not type-faithful JSON: `DoubleQuoted` quotes every scalar, so + // numbers and booleans arrive as strings -- `"12"` rather than `12`, `"true"` rather than `true`. A consumer + // validating against a schema that declares `integer` or `boolean` will reject that, and no manipulator here + // changes it. Preserving scalar types needs a real JSON serializer, not a yaml-cpp emitter. + // // This is not a general YAML to JSON converter. A node that carries a tag, an anchor or an alias still emits YAML // syntax that JSON does not accept. // diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py new file mode 100644 index 00000000000..1e28cf40385 --- /dev/null +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -0,0 +1,84 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys + +# To include util classes +sys.path.insert(0, f'{Test.TestDirectory}') + +from traffic_ctl_test_utils import Make_traffic_ctl + +Test.Summary = ''' +traffic_ctl JSON output must be parseable JSON, including when a node is null. + +yaml-cpp emits null as `~`, which is valid YAML but rejected by every JSON +parser. Emitters that produce JSON must set YAML::LowerNull, and container +nodes that may stay empty must be constructed as sequences so they emit `[]` +rather than null. + +The trigger for both regressions is the *empty* case, so this test +deliberately runs against a freshly started server with an empty HostDB and no +plugins loaded. A test that populates either one first would pass against the +bug. +''' + +Test.ContinueOnFail = True + +records_yaml = ''' + exec_thread: + autoconfig: + enabled: 0 + limit: 4 + ''' + +traffic_ctl = Make_traffic_ctl(Test, records_yaml) + +###### +# hostdb status -- `partitions` is empty on a fresh server. +# +# Flagless output goes through BasePrinter::write_output_json (the client +# printer). Before the fix this emitted `"partitions": ~`. +traffic_ctl.hostdb().status().validate_is_valid_json() + +# ... and it must be an empty array, not null. hostdb_status_schema.json +# declares partitions as "type": "array". +traffic_ctl.hostdb().status().validate_json_contains(partitions='[]') + +# -f json goes through the full envelope. Same emitter, different entry point. +traffic_ctl.hostdb().status().as_json().validate_is_valid_json() + +# The server-side encoder (yamlcpp_json_encoder) is a third, independent +# emitter. rpc invoke exercises it directly. +# +# The params are required: get_hostdb_status without them fails with "invalid +# node; this may result from using a map iterator as a sequence iterator", and +# an error envelope is valid JSON no matter what the emitter does -- the +# assertion would pass against the bug. +traffic_ctl.rpc().invoke(handler="get_hostdb_status", params='"hostname: \\"\\""').validate_is_valid_json() + +###### +# plugin list -- `plugins` is empty when plugin.config loads nothing. +# +# plugin list ignores the format flag today and prints a human table, so only +# the RPC path is assertable. Once plugin list honours -f json, add: +# traffic_ctl.plugin().list().as_json().validate_is_valid_json() +traffic_ctl.rpc().invoke(handler="admin_plugin_get_list").validate_is_valid_json() + +###### +# Commands that were already valid JSON -- guard against the shared emitter +# change regressing them. +traffic_ctl.server().status().validate_is_valid_json() +traffic_ctl.rpc().invoke(handler="show_registered_handlers").validate_is_valid_json() diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 29275c9ffd9..8d98902cb07 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -165,6 +165,30 @@ def validate_json_contains(self, **field_checks): self._finish() return self + def validate_is_valid_json(self): + """ + Validate that stdout parses as JSON. Performs no field checks. + + Use this as a regression guard on any command documented to emit JSON. + A gold file cannot do this job: yaml-cpp spells null as `~`, which a + gold file matches happily but no JSON parser accepts. + + The raw output is echoed to stderr so it survives in the stream files + even though the pipeline consumes stdout. + + Example: + traffic_ctl.hostdb().status().validate_is_valid_json() + """ + self._cmd = ( + f'{self._cmd} | python3 -c "' + f"import sys, json; " + f"raw = sys.stdin.read(); " + f"sys.stderr.write(raw); " + f"json.loads(raw)" + f'"') + self._finish() + return self + class ConfigReload(Common): """ @@ -467,6 +491,48 @@ def invoke(self, handler: str, params={}): return self +class HostDB(Common): + """ + Handy class to map traffic_ctl hostdb options. + """ + + def __init__(self, dir, tr, tn): + super().__init__(tr) + self._cmd = "traffic_ctl hostdb " + self._dir = dir + self._tn = tn + + def status(self, hostname: str = ""): + """Get HostDB info (traffic_ctl hostdb status [HOSTNAME])""" + self._cmd = f'{self._cmd} status {hostname} ' + return self + + def as_json(self): + self._cmd = f'{self._cmd} -f json' + return self + + +class Plugin(Common): + """ + Handy class to map traffic_ctl plugin options. + """ + + def __init__(self, dir, tr, tn): + super().__init__(tr) + self._cmd = "traffic_ctl plugin " + self._dir = dir + self._tn = tn + + def list(self): + """Show globally loaded plugins and their status (traffic_ctl plugin list)""" + self._cmd = f'{self._cmd} list ' + return self + + def as_json(self): + self._cmd = f'{self._cmd} -f json' + return self + + ''' Handy wrapper around traffic_ctl, ATS and the autest output validation mechanism. @@ -534,6 +600,14 @@ def rpc(self): self.add_test() return RPC(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) + def hostdb(self): + self.add_test() + return HostDB(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) + + def plugin(self): + self.add_test() + return Plugin(self._Test.TestDirectory, self._tests[self.__get_index()], self._testNumber) + def Make_traffic_ctl(test, records_yaml=None, retcode=0): tctl = TrafficCtl(test, records_yaml, retcode) From ae3d943f01fd3dd0db9c7e382bde9ef19361145e Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 7 Sep 2026 18:12:56 +0200 Subject: [PATCH 05/12] traffic_ctl: check JSON output with a tester, not a shell pipeline validate_is_valid_json and validate_json_contains appended `| python3 -c ...` to the command, putting a second process at the end of the pipeline. The shell reports only the last command's exit status, so traffic_ctl's own exit code never reached the ReturnCode check that TrafficCtl.add_test sets on every test run. A run whose traffic_ctl exited non-zero passed as long as the output happened to parse. Both checks now run in the autest process as a Testers.Lambda over the captured stdout file. traffic_ctl stays the only process in the run, so its exit status is the one the harness compares, and the JSON result is reported separately with the raw output attached rather than collapsing into the return code. set -o pipefail is not an option here: CI runs /bin/sh as dash, which rejects the flag outright, and it would still merge the two distinct failures into one number. Also quote the hostname interpolated into `hostdb status`, omitting it entirely when empty since an empty argument is not the same as no argument, and factor the duplicated lookup of autest's injected Testers/All into one helper that searches out the frame carrying those names. --- .../traffic_ctl/traffic_ctl_test_utils.py | 106 +++++++++++++----- 1 file changed, 78 insertions(+), 28 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 8d98902cb07..1759e569ee7 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -15,8 +15,11 @@ # limitations under the License. import atexit +import json import os +import shlex import shutil +import sys import tempfile _gold_tmpdir = None @@ -61,6 +64,56 @@ def MakeGoldFileWithText(content, dir, test_number, add_new_line=True): return gold_filepath +def _test_file_globals(): + """Return the globals of the calling test file. + + autest injects `Testers` and `All` into each test file's globals rather + than exposing them for import, so a helper module has to reach up the + stack to find them. The search walks outward until it reaches a frame + that carries the injected names, rather than assuming the immediate + caller is the test file. That way it works from inside this module and + from any intermediate helper module. + """ + frame = sys._getframe(1) + while frame is not None and 'Testers' not in frame.f_globals: + frame = frame.f_back + if frame is None: + raise RuntimeError('No autest test file frame found. These helpers only work when called from a test file.') + return frame.f_globals + + +def _read_stdout(path): + """Read a captured stream file, tolerating output that is not valid UTF-8.""" + with open(path, errors='replace') as stream: + return stream.read() + + +def _check_is_valid_json(path): + """Tester callback: the captured output must parse as JSON.""" + desc = "Check that the output parses as JSON" + raw = _read_stdout(path) + try: + json.loads(raw) + except ValueError as ex: + return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") + return (True, desc, "Output parses as JSON") + + +def _check_json_fields(path, expected): + """Tester callback: every expected field must match its value in the parsed output.""" + desc = "Check that the JSON output contains the expected fields" + raw = _read_stdout(path) + try: + doc = json.loads(raw) + except ValueError as ex: + return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") + + failed = [f"{key} = {doc.get(key)} (expected {value})" for key, value in expected.items() if str(doc.get(key)) != value] + if failed: + return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") + return (True, desc, "All expected fields matched") + + class Common(): """ Handy class to map common traffic_ctl test options. @@ -115,9 +168,7 @@ def validate_contains_all(self, *strings): "Set proxy.config.diags.debug.enabled" ) """ - import sys - # Testers and All are injected by autest into the test file's globals - caller_globals = sys._getframe(1).f_globals + caller_globals = _test_file_globals() _Testers = caller_globals['Testers'] _All = caller_globals['All'] testers = [_Testers.IncludesExpression(s, f"should contain: {s}") for s in strings] @@ -142,26 +193,22 @@ def validate_result_with_text(self, text: str): def validate_json_contains(self, **field_checks): """ Validate JSON output contains specific field:value pairs. Only checks specified fields. - Prints detailed error on failure: "FAIL: field_name = actual_value (expected expected_value)" - stream.all.txt will contain the actual output with the failed fields. + Every mismatch is reported as "field_name = actual_value (expected expected_value)", + followed by the raw output. + + The check runs in the autest process against the captured stdout file. Piping + traffic_ctl into a JSON parser instead would hide failures: the exit status of a shell + pipeline is the parser's, so a non-zero traffic_ctl exit would never reach the + ReturnCode check. Example: traffic_ctl.server().status().validate_json_contains( initialized_done='true', is_draining='false' ) """ - import json - checks_str = ', '.join(f"'{k}': '{v}'" for k, v in field_checks.items()) - self._cmd = ( - f'{self._cmd} | python3 -c "' - f"import sys, json; " - f"d = json.load(sys.stdin); " - f"c = {{{checks_str}}}; " - f"failed = [(k, v, str(d.get(k))) for k, v in c.items() if str(d.get(k)) != v]; " - f"[print(f'FAIL: {{k}} = {{actual}} (expected {{expected}})', file=sys.stderr) " - f"for k, expected, actual in failed]; " - f"exit(0 if not failed else 1)" - f'"') + _Testers = _test_file_globals()['Testers'] + self._tr.Processes.Default.Streams.stdout = _Testers.Lambda( + lambda info, tester: _check_json_fields(tester.GetContent(info), field_checks)) self._finish() return self @@ -173,19 +220,17 @@ def validate_is_valid_json(self): A gold file cannot do this job: yaml-cpp spells null as `~`, which a gold file matches happily but no JSON parser accepts. - The raw output is echoed to stderr so it survives in the stream files - even though the pipeline consumes stdout. + The check runs in the autest process against the captured stdout file, so + traffic_ctl stays the only process in the test run and the exit status the + harness compares against ReturnCode is still traffic_ctl's own. The raw + output is reported on failure. Example: traffic_ctl.hostdb().status().validate_is_valid_json() """ - self._cmd = ( - f'{self._cmd} | python3 -c "' - f"import sys, json; " - f"raw = sys.stdin.read(); " - f"sys.stderr.write(raw); " - f"json.loads(raw)" - f'"') + _Testers = _test_file_globals()['Testers'] + self._tr.Processes.Default.Streams.stdout = _Testers.Lambda( + lambda info, tester: _check_is_valid_json(tester.GetContent(info))) self._finish() return self @@ -503,8 +548,13 @@ def __init__(self, dir, tr, tn): self._tn = tn def status(self, hostname: str = ""): - """Get HostDB info (traffic_ctl hostdb status [HOSTNAME])""" - self._cmd = f'{self._cmd} status {hostname} ' + """Get HostDB info (traffic_ctl hostdb status [HOSTNAME]) + + The hostname is shell quoted. It is omitted entirely when empty, since + passing an empty argument is not the same as passing none. + """ + arg = f' {shlex.quote(hostname)}' if hostname else '' + self._cmd = f'{self._cmd} status{arg} ' return self def as_json(self): From 45ec4b18e810a514ddadda1c23b53af8e71a7a92 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 09:38:54 +0200 Subject: [PATCH 06/12] traffic_ctl: make the test helper's frame lookup and file reads deterministic _test_file_globals searched for autest's injected `Testers` alone, so a frame carrying that name but not `All` satisfied the search and then failed inside validate_contains_all with a bare KeyError. Require every injected name the module uses, and name them in the error. _read_stdout opened the captured stream in text mode with no encoding, which means the decode follows the runner's locale. Under LC_ALL=C the preferred encoding is US-ASCII, so a non-ASCII byte in otherwise identical output decoded to U+FFFD there and nowhere else; a validate_json_contains comparison against such a field would then fail for no real reason. Decode as UTF-8, which JSON requires anyway. Undecodable bytes stay replaced rather than strict on purpose. autest treats an exception raised by a tester callback as fatal, setting KillOnFailure and abandoning the remainder of the test run, whereas a replaced byte simply fails the JSON parse and is reported with the output attached. --- .../traffic_ctl/traffic_ctl_test_utils.py | 35 ++++++++++++++----- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 1759e569ee7..1847dc3fa91 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -64,27 +64,44 @@ def MakeGoldFileWithText(content, dir, test_number, add_new_line=True): return gold_filepath +# Names autest injects into every test file's globals, which this module needs. +_INJECTED_NAMES = ('Testers', 'All') + + def _test_file_globals(): """Return the globals of the calling test file. - autest injects `Testers` and `All` into each test file's globals rather - than exposing them for import, so a helper module has to reach up the - stack to find them. The search walks outward until it reaches a frame - that carries the injected names, rather than assuming the immediate + autest injects the names in `_INJECTED_NAMES` into each test file's + globals rather than exposing them for import, so a helper module has to + reach up the stack to find them. The search walks outward until it + reaches a frame carrying all of them, rather than assuming the immediate caller is the test file. That way it works from inside this module and - from any intermediate helper module. + from any intermediate helper module, and a frame that carries only some + of the names cannot satisfy the search and fail later on the rest. """ frame = sys._getframe(1) - while frame is not None and 'Testers' not in frame.f_globals: + while frame is not None and not all(name in frame.f_globals for name in _INJECTED_NAMES): frame = frame.f_back if frame is None: - raise RuntimeError('No autest test file frame found. These helpers only work when called from a test file.') + raise RuntimeError( + f"No autest test file frame found. These helpers only work when called from a test file, " + f"whose globals carry {', '.join(_INJECTED_NAMES)}.") return frame.f_globals def _read_stdout(path): - """Read a captured stream file, tolerating output that is not valid UTF-8.""" - with open(path, errors='replace') as stream: + """Read a captured stream file as UTF-8. + + JSON is defined to be UTF-8, and naming the encoding keeps the decode + from following the runner's locale: under `LC_ALL=C` the default is + US-ASCII, so identical output bytes would decode differently there. + + Undecodable bytes are replaced rather than raising. autest treats an + exception from a tester callback as fatal, setting KillOnFailure and + abandoning the rest of the test run, whereas a replaced byte simply + fails the JSON parse and is reported with the output attached. + """ + with open(path, encoding='utf-8', errors='replace') as stream: return stream.read() From e22b3bd336144ced40afc02f76dd9eba5ca7bdf0 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 09:52:02 +0200 Subject: [PATCH 07/12] traffic_ctl: compare JSON field expectations as strings _check_json_fields stringified only the value read from the output and left the expected side as whatever the caller passed. The shell pipeline it replaced had stringified both sides, because it formatted expectations into python source as quoted string literals, so `validate_json_contains( partitions=[])` matched there and reported a false mismatch here. Coerce both sides. Document the rule that follows from comparing against str() of the parsed value: the expectation is written the way Python renders that value, not the way JSON spells it. Those agree for `[]` and for numbers, but a JSON boolean reads as 'true' rather than True, which matters because get_server_status emits its flags as the strings "true" and "false". Note also that a missing key and a JSON null both render as 'None', so `field=None` passes for a misspelled field. No current caller is affected: all three pass their expectations as strings already. --- .../traffic_ctl/traffic_ctl_test_utils.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 1847dc3fa91..9edaf72c87e 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -117,7 +117,19 @@ def _check_is_valid_json(path): def _check_json_fields(path, expected): - """Tester callback: every expected field must match its value in the parsed output.""" + """Tester callback: every expected field must match its value in the parsed output. + + The expectation is compared against `str()` of the parsed value, so write + it the way Python renders that value rather than the way JSON spells it. + `[]` and `'[]'` are both fine because they agree, but a JSON boolean reads + as `'true'`, not `True`, and a list of strings reads as `"['a']"`, not + `'["a"]'`. `validate_result_with_text` does take JSON-spelled text, so the + two are not interchangeable. + + A missing key and a JSON `null` both render as `'None'` and cannot be told + apart here, which means `field=None` passes for a misspelled `field` too. + Asserting a null needs its own `key in doc` check. + """ desc = "Check that the JSON output contains the expected fields" raw = _read_stdout(path) try: @@ -125,7 +137,7 @@ def _check_json_fields(path, expected): except ValueError as ex: return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") - failed = [f"{key} = {doc.get(key)} (expected {value})" for key, value in expected.items() if str(doc.get(key)) != value] + failed = [f"{key} = {doc.get(key)} (expected {value})" for key, value in expected.items() if str(doc.get(key)) != str(value)] if failed: return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") return (True, desc, "All expected fields matched") From 842d240333c6b0e5b5ed6f7a152d3f480655afae Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 12:09:27 +0200 Subject: [PATCH 08/12] traffic_ctl: correct the boolean note on the JSON field check The note added with the string comparison had it backwards. It claimed a JSON boolean reads as 'true' rather than True, but json.loads turns a real JSON boolean into Python True, which str() renders as 'True'. The 'false' spelling these tests use comes from somewhere else: the emitters they cover set YAML::DoubleQuoted, so every scalar leaves as a JSON string, and get_server_status sends "is_draining": "false". Say that instead, and keep the contrast with a genuine JSON boolean. While here, read each field once per key instead of calling doc.get twice, and capitalize json in the id comment in YAMLCodec.h. --- include/mgmt/rpc/jsonrpc/json/YAMLCodec.h | 2 +- .../traffic_ctl/traffic_ctl_test_utils.py | 22 ++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h index ef7aa9de381..a41078add35 100644 --- a/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h +++ b/include/mgmt/rpc/jsonrpc/json/YAMLCodec.h @@ -253,7 +253,7 @@ class yamlcpp_json_encoder if (!resp.id.empty()) { json << YAML::Key << "id" << YAML::Value << resp.id; } - // else: the field is omitted rather than set to null. Emitting it would be valid json now that LowerNull is set, but the + // else: the field is omitted rather than set to null. Emitting it would be valid JSON now that LowerNull is set, but the // omission is deliberate, see the id note in mgmt/rpc/schema/jsonrpc_response_schema.json. json << YAML::EndMap; diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 9edaf72c87e..7d2e3e9014b 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -121,10 +121,18 @@ def _check_json_fields(path, expected): The expectation is compared against `str()` of the parsed value, so write it the way Python renders that value rather than the way JSON spells it. - `[]` and `'[]'` are both fine because they agree, but a JSON boolean reads - as `'true'`, not `True`, and a list of strings reads as `"['a']"`, not - `'["a"]'`. `validate_result_with_text` does take JSON-spelled text, so the - two are not interchangeable. + `[]` and `'[]'` agree, but a list of strings renders as `"['a']"`, not + `'["a"]'`. + + Booleans need care, because the emitters these tests cover set + `YAML::DoubleQuoted` and so encode every scalar as a JSON string. + `get_server_status` sends `"is_draining": "false"`, which parses to the + string `'false'` and is matched by `is_draining='false'`. A genuine JSON + boolean would instead parse to Python `True` or `False` and render as + `'True'` or `'False'`. + + `validate_result_with_text` does take JSON-spelled text, so the two + helpers are not interchangeable. A missing key and a JSON `null` both render as `'None'` and cannot be told apart here, which means `field=None` passes for a misspelled `field` too. @@ -137,7 +145,11 @@ def _check_json_fields(path, expected): except ValueError as ex: return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") - failed = [f"{key} = {doc.get(key)} (expected {value})" for key, value in expected.items() if str(doc.get(key)) != str(value)] + failed = [] + for key, want in expected.items(): + actual = doc.get(key) + if str(actual) != str(want): + failed.append(f"{key} = {actual} (expected {want})") if failed: return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") return (True, desc, "All expected fields matched") From 7f8131e545562bdcdd0261b892c7a8ddbb046a31 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 15:01:11 +0200 Subject: [PATCH 09/12] traffic_ctl: import autest's testers instead of walking the stack _test_file_globals reached up the call stack for `Testers` and `All` on the premise that autest injects them into each test file's globals rather than exposing them for import. The premise is wrong: core/test.py imports the testers module itself and injects that module under the name, so both are ordinary importables. Import them and delete the helper, which retires the frame walk together with the question of which call site needs which name. Decode captured output as strict UTF-8 and return the failure as a value. Replacing undecodable bytes let output that is not UTF-8, and so not JSON, parse cleanly and pass a check meant to reject exactly that. No wrapped command reaches this today, because yaml-cpp substitutes U+FFFD in its own double quoted scalar writer, so the strict decode states the requirement rather than resting on that staying true. It is a value and not an exception because autest treats an exception from a tester callback as fatal. Guard the field check against a JSON document that is not an object. doc.get assumed a dict, so valid non-object JSON raised AttributeError out of the callback, which sets KillOnFailure and abandons the remainder of the run. Every input now returns a result triple. Drop a docstring claim that an unnamed encoding would follow the runner's locale and mangle non-ASCII output. PEP 540 enables UTF-8 mode for the C and POSIX locales, so it would not. --- .../traffic_ctl/traffic_ctl_test_utils.py | 87 ++++++++----------- 1 file changed, 38 insertions(+), 49 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 7d2e3e9014b..e7a6ca795b5 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -19,9 +19,11 @@ import os import shlex import shutil -import sys import tempfile +import autest.testers as Testers +from autest.testers import All + _gold_tmpdir = None @@ -64,51 +66,39 @@ def MakeGoldFileWithText(content, dir, test_number, add_new_line=True): return gold_filepath -# Names autest injects into every test file's globals, which this module needs. -_INJECTED_NAMES = ('Testers', 'All') - - -def _test_file_globals(): - """Return the globals of the calling test file. - - autest injects the names in `_INJECTED_NAMES` into each test file's - globals rather than exposing them for import, so a helper module has to - reach up the stack to find them. The search walks outward until it - reaches a frame carrying all of them, rather than assuming the immediate - caller is the test file. That way it works from inside this module and - from any intermediate helper module, and a frame that carries only some - of the names cannot satisfy the search and fail later on the rest. - """ - frame = sys._getframe(1) - while frame is not None and not all(name in frame.f_globals for name in _INJECTED_NAMES): - frame = frame.f_back - if frame is None: - raise RuntimeError( - f"No autest test file frame found. These helpers only work when called from a test file, " - f"whose globals carry {', '.join(_INJECTED_NAMES)}.") - return frame.f_globals - - def _read_stdout(path): - """Read a captured stream file as UTF-8. - - JSON is defined to be UTF-8, and naming the encoding keeps the decode - from following the runner's locale: under `LC_ALL=C` the default is - US-ASCII, so identical output bytes would decode differently there. - - Undecodable bytes are replaced rather than raising. autest treats an - exception from a tester callback as fatal, setting KillOnFailure and - abandoning the rest of the test run, whereas a replaced byte simply - fails the JSON parse and is reported with the output attached. + """Read a captured stream file as strict UTF-8, as `(text, error)`. + + JSON has to be UTF-8, so output that does not decode is not valid JSON + either and the callers report it as a failure. Replacing the bad bytes + instead would hide exactly that: U+FFFD is a legal character inside a + JSON string, so undecodable output would go on to parse cleanly and pass + a check whose whole job is to reject output that is not JSON. + + No command wrapped here reaches that branch today. yaml-cpp substitutes + U+FFFD itself when writing a double quoted scalar, so traffic_ctl cannot + put undecodable bytes on stdout on any of these paths. Decoding strictly + states the requirement rather than resting on that staying true. + + The failure comes back as a value rather than an exception because autest + treats an exception from a tester callback as fatal, setting KillOnFailure + and abandoning the rest of the test run. On failure the text is still + rendered, lossily, so the caller can show what arrived. """ - with open(path, encoding='utf-8', errors='replace') as stream: - return stream.read() + with open(path, 'rb') as stream: + raw = stream.read() + try: + return raw.decode('utf-8'), None + except UnicodeDecodeError as ex: + return raw.decode('utf-8', errors='replace'), str(ex) def _check_is_valid_json(path): """Tester callback: the captured output must parse as JSON.""" desc = "Check that the output parses as JSON" - raw = _read_stdout(path) + raw, decode_error = _read_stdout(path) + if decode_error: + return (False, desc, f"Output is not valid UTF-8, so it is not JSON: {decode_error}\nOutput was:\n{raw}") try: json.loads(raw) except ValueError as ex: @@ -139,11 +129,15 @@ def _check_json_fields(path, expected): Asserting a null needs its own `key in doc` check. """ desc = "Check that the JSON output contains the expected fields" - raw = _read_stdout(path) + raw, decode_error = _read_stdout(path) + if decode_error: + return (False, desc, f"Output is not valid UTF-8, so it is not JSON: {decode_error}\nOutput was:\n{raw}") try: doc = json.loads(raw) except ValueError as ex: return (False, desc, f"Output is not JSON: {ex}\nOutput was:\n{raw}") + if not isinstance(doc, dict): + return (False, desc, f"Output is a JSON {type(doc).__name__}, not an object, so it has no fields\nOutput was:\n{raw}") failed = [] for key, want in expected.items(): @@ -209,11 +203,8 @@ def validate_contains_all(self, *strings): "Set proxy.config.diags.debug.enabled" ) """ - caller_globals = _test_file_globals() - _Testers = caller_globals['Testers'] - _All = caller_globals['All'] - testers = [_Testers.IncludesExpression(s, f"should contain: {s}") for s in strings] - self._tr.Processes.Default.Streams.stdout = _All(*testers) + testers = [Testers.IncludesExpression(s, f"should contain: {s}") for s in strings] + self._tr.Processes.Default.Streams.stdout = All(*testers) self._finish() return self @@ -247,8 +238,7 @@ def validate_json_contains(self, **field_checks): initialized_done='true', is_draining='false' ) """ - _Testers = _test_file_globals()['Testers'] - self._tr.Processes.Default.Streams.stdout = _Testers.Lambda( + self._tr.Processes.Default.Streams.stdout = Testers.Lambda( lambda info, tester: _check_json_fields(tester.GetContent(info), field_checks)) self._finish() return self @@ -269,8 +259,7 @@ def validate_is_valid_json(self): Example: traffic_ctl.hostdb().status().validate_is_valid_json() """ - _Testers = _test_file_globals()['Testers'] - self._tr.Processes.Default.Streams.stdout = _Testers.Lambda( + self._tr.Processes.Default.Streams.stdout = Testers.Lambda( lambda info, tester: _check_is_valid_json(tester.GetContent(info))) self._finish() return self From 12528910b6099695447c22850a392a37e52b4a6c Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 9 Sep 2026 09:53:50 +0200 Subject: [PATCH 10/12] traffic_ctl: fail the field check on a key the output lacks _check_json_fields read every field with doc.get, so a key that was absent and a key present with a JSON null both arrived as None. An expectation of None therefore matched a misspelled field name, and the check silently accepted output that did not carry the field at all. Test presence before comparing, and report a missing key as missing. That also makes `field=None` mean what it looks like: the field is there and it is null. The docstring said this gap existed; enforce it instead. --- .../gold_tests/traffic_ctl/traffic_ctl_test_utils.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index e7a6ca795b5..63896d809f0 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -124,9 +124,10 @@ def _check_json_fields(path, expected): `validate_result_with_text` does take JSON-spelled text, so the two helpers are not interchangeable. - A missing key and a JSON `null` both render as `'None'` and cannot be told - apart here, which means `field=None` passes for a misspelled `field` too. - Asserting a null needs its own `key in doc` check. + A key the output does not carry is reported as missing rather than + compared, so a misspelled field name fails instead of quietly matching an + expectation of `None`. Asserting that a field is present and null is + therefore written `field=None`, which only passes when the key is there. """ desc = "Check that the JSON output contains the expected fields" raw, decode_error = _read_stdout(path) @@ -141,7 +142,10 @@ def _check_json_fields(path, expected): failed = [] for key, want in expected.items(): - actual = doc.get(key) + if key not in doc: + failed.append(f"{key} is missing (expected {want})") + continue + actual = doc[key] if str(actual) != str(want): failed.append(f"{key} = {actual} (expected {want})") if failed: From 734f43a0ffa2b417fa84bec4a4dc3d5ebe7451da Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Wed, 9 Sep 2026 10:12:20 +0200 Subject: [PATCH 11/12] traffic_ctl: assert the plugin list shape, not just that it parses The hostdb case asserted `partitions` is `[]`, but the plugin case only checked that the response parsed, so it would have passed on any shape the emitter produced. Compare the whole result instead, the way the connection tracker cases in traffic_ctl_server_output.test.py already do, which pins `plugins` to `[]`. The field sits at result.data.plugins, out of reach of validate_json_contains. Verified in both directions: the assertion passes as written and fails when the expectation is changed to the `~` the emitter produced before the fix. --- .../traffic_ctl/traffic_ctl_json_null.test.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py index 1e28cf40385..39c45b780cc 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -75,7 +75,17 @@ # plugin list ignores the format flag today and prints a human table, so only # the RPC path is assertable. Once plugin list honours -f json, add: # traffic_ctl.plugin().list().as_json().validate_is_valid_json() -traffic_ctl.rpc().invoke(handler="admin_plugin_get_list").validate_is_valid_json() +# +# Assert the shape rather than mere parseability: `plugins` has to be `[]`, +# matching what the hostdb case above asserts for `partitions`. The field sits +# at result.data.plugins, which validate_json_contains cannot reach, so this +# compares the whole result, as the connection tracker cases in +# traffic_ctl_server_output.test.py do. Before the fix the field emitted `~`, +# which fails this comparison as surely as it fails a JSON parser. Nothing +# forces plugin.config to be empty here, and nothing needs to: should a +# default ever load a plugin, this assertion fails rather than going quiet. +traffic_ctl.rpc().invoke( + handler="admin_plugin_get_list").validate_result_with_text('{"data": {"source": "plugin.config", "plugins": []}}') ###### # Commands that were already valid JSON -- guard against the shared emitter From 2d9dfd67f9af15a01e7149c61a7ce0876d389c92 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 10 Sep 2026 11:24:07 +0200 Subject: [PATCH 12/12] traffic_ctl: compare JSON field values, not their str() _check_json_fields compared str(actual) to str(want), which flattens the JSON type out of both sides. An array and a string spelling of it become the same text, so a field regressing from "partitions": [] to "partitions": "[]" passed the check -- the class of bug these tests were added to catch. A number regressing to a quoted number was masked the same way. Compare with == against the value the JSON parses to, so expectations are written as Python values: partitions=[] for the array, is_draining='false' for the JSON string the DoubleQuoted emitters produce. Both existing call sites hold under that rule; only the partitions one changes spelling. Null, the case this branch fixes, failed the check before and still does. Render both sides with !r in the failure text. Without it a type mismatch reported "partitions = [] (expected [])". --- .../traffic_ctl/traffic_ctl_json_null.test.py | 2 +- .../traffic_ctl/traffic_ctl_test_utils.py | 30 ++++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py index 39c45b780cc..00a050f7791 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py @@ -55,7 +55,7 @@ # ... and it must be an empty array, not null. hostdb_status_schema.json # declares partitions as "type": "array". -traffic_ctl.hostdb().status().validate_json_contains(partitions='[]') +traffic_ctl.hostdb().status().validate_json_contains(partitions=[]) # -f json goes through the full envelope. Same emitter, different entry point. traffic_ctl.hostdb().status().as_json().validate_is_valid_json() diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py index 63896d809f0..86e7e329daa 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py @@ -109,17 +109,19 @@ def _check_is_valid_json(path): def _check_json_fields(path, expected): """Tester callback: every expected field must match its value in the parsed output. - The expectation is compared against `str()` of the parsed value, so write - it the way Python renders that value rather than the way JSON spells it. - `[]` and `'[]'` agree, but a list of strings renders as `"['a']"`, not - `'["a"]'`. - - Booleans need care, because the emitters these tests cover set - `YAML::DoubleQuoted` and so encode every scalar as a JSON string. - `get_server_status` sends `"is_draining": "false"`, which parses to the - string `'false'` and is matched by `is_draining='false'`. A genuine JSON - boolean would instead parse to Python `True` or `False` and render as - `'True'` or `'False'`. + The expectation is compared to the parsed value with `==`, so write it as + the Python value the JSON parses to: `[]` for a JSON array, `'[]'` only + for the JSON string `"[]"`. Comparing `str()` of both sides instead would + equate those two and let a field regress from an array to a string + without failing, which is the class of bug these tests exist to catch. + + Scalars and sequences therefore read differently. The emitters these + tests cover set `YAML::DoubleQuoted`, which encodes every *scalar* as a + JSON string: `get_server_status` sends `"is_draining": "false"`, matched + by `is_draining='false'`. A genuine JSON boolean would parse to Python + `True` or `False` and needs `is_draining=False`. A sequence is not a + scalar and is untouched by `DoubleQuoted`, so `hostdb status` sends a + real `"partitions": []`, matched by `partitions=[]`. `validate_result_with_text` does take JSON-spelled text, so the two helpers are not interchangeable. @@ -143,11 +145,11 @@ def _check_json_fields(path, expected): failed = [] for key, want in expected.items(): if key not in doc: - failed.append(f"{key} is missing (expected {want})") + failed.append(f"{key} is missing (expected {want!r})") continue actual = doc[key] - if str(actual) != str(want): - failed.append(f"{key} = {actual} (expected {want})") + if actual != want: + failed.append(f"{key} = {actual!r} (expected {want!r})") if failed: return (False, desc, "FAIL: " + "; ".join(failed) + f"\nOutput was:\n{raw}") return (True, desc, "All expected fields matched")