brbzull0 commented on code in PR #13609: URL: https://github.com/apache/trafficserver/pull/13609#discussion_r3978934475
########## tests/gold_tests/traffic_ctl/traffic_ctl_json_null.test.py: ########## @@ -0,0 +1,94 @@ +# 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: Both halves of this were tried against the pinned autest before declining. `shlex.quote()` in `RPC.invoke()` does not stay local to this line. Three callers already pass a pre-quoted `params`, in `traffic_ctl_server_output.test.py` at lines 53, 61 and 65: ```python params='"table: both"' ``` Those are on master and outside this diff. Running autest's own lexer over the command with `shlex.quote()` added, the `-p` argv element changes: ``` today -p = 'table: both' +shlex.quote -p = '"table: both"' ``` That is not a cosmetic difference. `YAML::Load("table: both")` is a map; `YAML::Load("\"table: both\"")` is a string scalar, and `CtrlCommands.cc:1159` assigns the result straight to `request.params`. Passing the natural text bare does not work either. `-p hostname: ""` lexes to a trailing empty token and `isShellCommand` raises on it: ``` argv: ['traffic_ctl', 'rpc', 'invoke', 'get_hostdb_status', '-p', 'hostname:', '', '-f', 'json'] IndexError: string index out of range # runlogic/process.py:92 ``` `doStart` catches only `ValueError`, so that one propagates. The outer quoting is what keeps the value a single argv element through `ListCmd`, which lexes with `posix=True` and strips quotes before argv is built. The current form and `shlex.quote('hostname: ""')` produce identical argv: ``` ['traffic_ctl', 'rpc', 'invoke', 'get_hostdb_status', '-p', 'hostname: ""', '-f', 'json'] ``` One correction to my earlier reply on this: the shell trigger is not only `;&><|`. A bare `cd`, `set` or `export` token also returns True, so `traffic_ctl config set ...` built by this same file does take the shell path. This command does not. -- 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]
