Copilot commented on code in PR #13609:
URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3956666753
##########
include/mgmt/rpc/jsonrpc/json/YAMLCodec.h:
##########
@@ -251,8 +253,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
Review Comment:
“json” should be capitalized as “JSON” for consistency with surrounding
comments and the protocol name.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,85 @@ 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.
+ """
+ with open(path, encoding='utf-8', 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.
+
+ 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:
+ 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)) != str(value)]
Review Comment:
`doc.get(key)` is evaluated twice per key, and the `str(...)` conversions
happen inline in the comprehension, which makes this harder to debug/extend.
Consider fetching the actual value once per key (and then comparing/rendering)
to reduce duplication and improve readability of failures.
##########
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()
Review Comment:
The `params` value is hard to read and shell-escaping-heavy, which makes the
test brittle and difficult to modify. Prefer building the YAML params as a
plain Python string (e.g., `hostname: ""`) and applying a single, obvious
quoting step (or a small helper in `traffic_ctl_test_utils`) so it’s clear what
bytes are intended to reach `traffic_ctl -p`.
##########
tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py:
##########
@@ -61,6 +64,85 @@ 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.
+ """
+ with open(path, encoding='utf-8', 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.
+
+ 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
Review Comment:
The docstring is misleading: in Python, a real JSON boolean (`true`/`false`)
parses to `True`/`False`. If the intent is to document that `traffic_ctl`
currently emits booleans as *strings* (because of `YAML::DoubleQuoted` →
`"true"`), please reword to explicitly say “traffic_ctl encodes
booleans/numbers as JSON strings” rather than “a JSON boolean reads as 'true'”.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
