Copilot commented on code in PR #13032:
URL: https://github.com/apache/trafficserver/pull/13032#discussion_r4268817016
##########
include/shared/rpc/yaml_codecs.h:
##########
@@ -63,11 +63,33 @@ template <> struct convert<shared::rpc::JSONRPCError> {
static bool
decode(Node const &node, shared::rpc::JSONRPCError &error)
{
+ using DataEntry = shared::rpc::JSONRPCError::DataEntry;
+
error.code = helper::try_extract<int32_t>(node, "code");
error.message = helper::try_extract<std::string>(node, "message");
- if (auto data = node["data"]) {
- for (auto &&err : data) {
- error.data.emplace_back(helper::try_extract<int32_t>(err, "code"),
helper::try_extract<std::string>(err, "message"));
+ // Read only: the same tree is printed as received in json output.
+ if (auto const data = node["data"]; data && !data.IsSequence()) {
+ // Not a list of annotations: nothing in it can be read as one.
+ error.data.push_back({0, DataEntry::INVALID_SEVERITY, {}});
+ } else if (data) {
+ for (auto const &err : data) {
+ DataEntry entry;
+ entry.code = helper::try_extract<int32_t>(err, "code");
+ entry.message = helper::try_extract<std::string>(err, "message");
+ if (!err.IsMap()) {
+ entry.severity = DataEntry::INVALID_SEVERITY;
+ } else if (auto const severity = err["severity"]) {
+ entry.severity = DataEntry::INVALID_SEVERITY;
+ // A quoted value is text, not a severity.
+ if (severity.IsScalar() && severity.Tag() != "!") {
+ try {
+ entry.severity = severity.as<int32_t>();
+ } catch (YAML::Exception const &) {
+ // Not an integer: stays INVALID_SEVERITY.
+ }
+ }
+ }
+ error.data.push_back(std::move(entry));
}
Review Comment:
For non-map `data` entries you already classify the entry as invalid
severity, but you still unconditionally attempt to extract `code`/`message`
from `err` before checking `err.IsMap()`. Restructure this so non-map entries
skip keyed extraction and use defaults, which makes the behavior independent of
`helper::try_extract` details and reduces the chance of unexpected exceptions
or misleading extracted values (recommended).
##########
src/traffic_ctl/CtrlCommands.cc:
##########
@@ -93,6 +93,16 @@ display_errors(BasePrinter *printer,
std::vector<ConfigReloadResponse::Error> co
}
}
+/// A request the command depends on failed, and its error is already printed
and graded. Below --error-level that grade is 0,
+/// but what was asked was not done, so the command exits CTRL_EX_TEMPFAIL
instead.
+void
+mark_not_done()
+{
+ if (App_Exit_Status_Code == CTRL_EX_OK) {
+ App_Exit_Status_Code = CTRL_EX_TEMPFAIL;
+ }
+}
Review Comment:
`mark_not_done()` is a non-static free function at file scope, which exports
a global symbol unnecessarily and increases the risk of future name
collisions/ODR issues. Make it `static` or place it in an anonymous namespace
(mandatory).
##########
tests/gold_tests/traffic_ctl/jsonrpc_error_stub.py:
##########
@@ -0,0 +1,81 @@
+'''
+A stand-in JSONRPC node for traffic_ctl tests. It answers every request with
+the reply configured for its method, so a test can show traffic_ctl server
+errors that no real handler produces. A method configured with a list of
+replies gets them in turn, starting over after the last one.
+'''
+# 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 argparse
+import json
+import os
+import socket
+
+METHOD_NOT_FOUND = {'error': {'code': -32601, 'message': 'Method not found'}}
+
+
+def read_request(conn: socket.socket) -> dict | None:
+ data = b''
+ while True:
+ chunk = conn.recv(65536)
+ if not chunk:
+ return None
+ data += chunk
+ try:
+ return json.loads(data)
Review Comment:
As written, `read_request()` will loop forever if the client sends multiple
JSON values back-to-back on the same connection (common failure mode:
`JSONDecodeError: Extra data`), since additional reads won’t fix that. Consider
parsing with `json.JSONDecoder().raw_decode()` (consume one object and
ignore/retain remainder), or treating `Extra data` as end-of-message; also
consider a socket timeout to avoid a hung gold test when framing expectations
are violated (recommended).
##########
src/mgmt/rpc/jsonrpc/unit_tests/test_basic_protocol.cc:
##########
@@ -21,11 +21,15 @@
#include <catch2/catch_test_macros.hpp> /* catch unit-test framework */
#include <tsutil/ts_bw_format.h>
+#include <tsutil/ts_errata.h>
#include "mgmt/rpc/jsonrpc/JsonRPCManager.h"
#include "mgmt/rpc/jsonrpc/JsonRPC.h"
#include "mgmt/rpc/handlers/common/ErrorUtils.h"
+// Defined in src/tsutil/ts_diags.cc; traffic_server calls it at startup.
+void Initialize_Errata_Settings();
+
Review Comment:
Forward-declaring `Initialize_Errata_Settings()` from a `.cc` file is
brittle (signature drift, link surprises depending on target link sets). Prefer
including the header that declares it (if available), or expose a stable
test/utility API in `ts_errata`/`ts_diags` for initializing production errata
settings (mandatory if a proper declaration exists; otherwise recommended to
reduce fragility).
##########
src/mgmt/rpc/jsonrpc/unit_tests/test_basic_protocol.cc:
##########
@@ -79,6 +83,66 @@ test_callback_ok_or_error(std::string_view const & /* id
ATS_UNUSED */, YAML::No
return resp;
}
+inline swoc::Rv<YAML::Node>
+test_callback_with_severity(std::string_view const & /* id ATS_UNUSED */,
YAML::Node const & /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ resp.errata()
+ .assign(ERR1)
+ .note(ERRATA_WARN, "this is a warning")
+ .note("this has no severity")
+ .note(ERRATA_FATAL, "this is fatal")
+ .note(ERRATA_DIAG, "this is a diag");
+ return resp;
+}
+
+inline swoc::Rv<YAML::Node>
+test_callback_note_only(std::string_view const & /* id ATS_UNUSED */,
YAML::Node const & /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ resp.errata().assign(ERR1).note(ERRATA_NOTE, "just a note");
+ return resp;
+}
+
+inline swoc::Rv<YAML::Node>
+test_callback_warn_only(std::string_view const & /* id ATS_UNUSED */,
YAML::Node const & /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ resp.errata().assign(ERR1).note(ERRATA_WARN, "already in that state");
+ return resp;
+}
+
+inline swoc::Rv<YAML::Node>
+test_callback_note_next_to_failure(std::string_view const & /* id ATS_UNUSED
*/, YAML::Node const & /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ resp.errata().assign(ERR1).note(ERRATA_NOTE, "just a note").note("the real
failure");
+ return resp;
+}
+
+inline swoc::Rv<YAML::Node>
+test_callback_errata_severity_apart(std::string_view const & /* id ATS_UNUSED
*/, YAML::Node const & /* params ATS_UNUSED */)
+{
+ swoc::Rv<YAML::Node> resp;
+ resp.errata().assign(ERR1).assign(ERRATA_ERROR).note(ERRATA_NOTE, "just a
note");
+ return resp;
+}
+
+// traffic_server's errata settings, restored on scope exit. The unit tests
run with libswoc's defaults otherwise, which treat
+// lower severities as failures.
+struct ProductionErrataSeverities {
+ swoc::Errata::Severity
saved_default{swoc::Errata::DEFAULT_SEVERITY};
+ swoc::Errata::Severity
saved_failure{swoc::Errata::FAILURE_SEVERITY};
+ swoc::MemSpan<swoc::TextView const>
saved_names{swoc::Errata::SEVERITY_NAMES};
+ ProductionErrataSeverities() { Initialize_Errata_Settings(); }
+ ~ProductionErrataSeverities()
+ {
+ swoc::Errata::DEFAULT_SEVERITY = saved_default;
+ swoc::Errata::FAILURE_SEVERITY = saved_failure;
+ swoc::Errata::SEVERITY_NAMES = saved_names;
+ }
+};
Review Comment:
This RAII helper mutates global errata configuration. If the unit test
runner executes tests in parallel (or if future tests do), this can cause
cross-test interference/flakiness. Consider guarding the mutation with a
process-wide mutex, or structuring these tests so they run in a single-threaded
section/fixture that serializes access (recommended).
--
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]
