Copilot commented on code in PR #13609: URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3958304238
########## 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() Review Comment: This assertion only checks that the RPC response parses as JSON, but it may not reliably exercise the regression trigger (the empty-plugin case that previously emitted `plugins: ~`). To make the test deterministic, ensure the environment forces an empty plugin list (e.g., explicitly create/override an empty `plugin.config` in the test setup) and assert the payload shape includes an empty array (e.g., `plugins == []`) rather than only validating parseability. ########## tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py: ########## @@ -61,6 +66,89 @@ def MakeGoldFileWithText(content, dir, test_number, add_new_line=True): return gold_filepath +def _read_stdout(path): + """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, '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, 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: + 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 `'[]'` 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. + Asserting a null needs its own `key in doc` check. + """ + desc = "Check that the JSON output contains the expected fields" + 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(): + actual = doc.get(key) + if str(actual) != str(want): + failed.append(f"{key} = {actual} (expected {want})") Review Comment: `_check_json_fields()` uses `doc.get(key)`, so a missing key and a present-but-null key both 비교 as `None` and can incorrectly pass (e.g., a misspelled key passes when `expected` is `None`). Consider failing when `key not in doc` (and separately allowing explicit null assertions), or adding an option like `require_keys=True` so tests don't silently accept missing fields. ########## doc/developer-guide/jsonrpc/jsonrpc-architecture.en.rst: ########## @@ -74,6 +74,9 @@ 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. +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. Review Comment: These added lines are quite long for RST and may violate the doc style/linters used elsewhere in the repo. Consider wrapping the text to the project’s typical line length for readability and consistency. -- 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]
