Copilot commented on code in PR #13456:
URL: https://github.com/apache/trafficserver/pull/13456#discussion_r3909078322
##########
src/proxy/CacheControl.cc:
##########
@@ -61,10 +70,144 @@ DbgCtl dbg_ctl_v_http3{"v_http3"};
DbgCtl dbg_ctl_http3{"http3"};
DbgCtl dbg_ctl_cache_control{"cache_control"};
+std::string
+quote_matcher_value(std::string_view value)
+{
+ std::string result{"\""};
+
+ for (char c : value) {
+ if (c == '\\' || c == '"') {
+ result.push_back('\\');
+ }
+ result.push_back(c);
+ }
+ result.push_back('"');
+ return result;
+}
+
+void
+append_matcher_pair(std::string &line, std::string_view key, std::string_view
value)
+{
+ if (!line.empty()) {
+ line.push_back(' ');
+ }
+ line.append(key);
+ line.push_back('=');
+ line.append(quote_matcher_value(value));
+}
+
+void
+append_optional_match(std::string &line, std::string_view key,
std::optional<std::string> const &value)
+{
+ if (value) {
+ append_matcher_pair(line, key, *value);
+ }
+}
+
+std::string
+build_matcher_config(config::CacheConfig const &config)
+{
+ std::string matcher_config;
+
+ for (auto const &rule : config) {
+ std::string line;
+
+ append_optional_match(line, "dest_host", rule.match.dest_host);
+ append_optional_match(line, "dest_domain", rule.match.dest_domain);
+ append_optional_match(line, "dest_ip", rule.match.dest_ip);
+ append_optional_match(line, "url_regex", rule.match.url_regex);
+ append_optional_match(line, "host_regex", rule.match.host_regex);
+ append_optional_match(line, "port", rule.match.port);
+ append_optional_match(line, "scheme", rule.match.scheme);
+ append_optional_match(line, "prefix", rule.match.prefix);
+ append_optional_match(line, "suffix", rule.match.suffix);
+ append_optional_match(line, "method", rule.match.method);
+ append_optional_match(line, "time", rule.match.time);
+ append_optional_match(line, "src_ip", rule.match.src_ip);
+ append_optional_match(line, "iport", rule.match.incoming_port);
+ append_optional_match(line, "tag", rule.match.tag);
+ if (rule.match.internal) {
+ append_matcher_pair(line, "internal", *rule.match.internal ? "true" :
"false");
+ }
Review Comment:
`internal: false` in cache.yaml is a meaningful match constraint
(external-only), but this check treats `std::optional<bool>{false}` as unset.
As a result, rules with `internal: false` will be emitted without an `internal`
modifier and will match both internal and external transactions.
##########
src/config/cache.cc:
##########
@@ -0,0 +1,738 @@
+/** @file
+
+ Cache rule configuration parsing and marshalling implementation.
+
+ @section license License
+
+ 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.
+*/
+
+#include "config/cache.h"
+
+#include <charconv>
+#include <cctype>
+#include <cerrno>
+#include <set>
+#include <string>
+#include <system_error>
+#include <utility>
+
+#include <yaml-cpp/yaml.h>
+
+#include "swoc/swoc_file.h"
+#include "tscore/MatcherUtils.h"
+#include "tsutil/ts_diag_levels.h"
+
+namespace
+{
+
+constexpr swoc::Errata::Severity
ERRATA_WARN_SEV{static_cast<swoc::Errata::severity_type>(DL_Warning)};
+constexpr swoc::Errata::Severity
ERRATA_ERROR_SEV{static_cast<swoc::Errata::severity_type>(DL_Error)};
+
+constexpr char KEY_CACHE[] = "cache";
+constexpr char KEY_MATCH[] = "match";
+constexpr char KEY_ACTION[] = "action";
+constexpr char KEY_DEST_HOST[] = "dest_host";
+constexpr char KEY_DEST_DOMAIN[] = "dest_domain";
+constexpr char KEY_DEST_IP[] = "dest_ip";
+constexpr char KEY_URL_REGEX[] = "url_regex";
+constexpr char KEY_HOST_REGEX[] = "host_regex";
+constexpr char KEY_PORT[] = "port";
+constexpr char KEY_SCHEME[] = "scheme";
+constexpr char KEY_PREFIX[] = "prefix";
+constexpr char KEY_SUFFIX[] = "suffix";
+constexpr char KEY_METHOD[] = "method";
+constexpr char KEY_TIME[] = "time";
+constexpr char KEY_SRC_IP[] = "src_ip";
+constexpr char KEY_INCOMING_PORT[] = "incoming_port";
+constexpr char KEY_TAG[] = "tag";
+constexpr char KEY_INTERNAL[] = "internal";
+constexpr char KEY_CACHE_MODE[] = "cache";
+constexpr char KEY_REVALIDATE[] = "revalidate";
+constexpr char KEY_PIN_IN_CACHE[] = "pin_in_cache";
+constexpr char KEY_TTL_IN_CACHE[] = "ttl_in_cache";
+constexpr char KEY_IGNORE_NO_CACHE[] = "ignore_no_cache";
+constexpr char KEY_IGNORE_CLIENT_NO_CACHE[] = "ignore_client_no_cache";
+constexpr char KEY_IGNORE_SERVER_NO_CACHE[] = "ignore_server_no_cache";
+constexpr char KEY_CACHE_RESPONSES_TO_COOKIES[] = "cache_responses_to_cookies";
+
+std::set<std::string> const rule_keys{KEY_MATCH, KEY_ACTION};
+std::set<std::string> const match_keys{
+ KEY_DEST_HOST, KEY_DEST_DOMAIN, KEY_DEST_IP, KEY_URL_REGEX, KEY_HOST_REGEX,
KEY_PORT, KEY_SCHEME, KEY_PREFIX,
+ KEY_SUFFIX, KEY_METHOD, KEY_TIME, KEY_SRC_IP,
KEY_INCOMING_PORT, KEY_TAG, KEY_INTERNAL,
+};
+std::set<std::string> const action_keys{
+ KEY_CACHE_MODE,
+ KEY_REVALIDATE,
+ KEY_PIN_IN_CACHE,
+ KEY_TTL_IN_CACHE,
+ KEY_IGNORE_NO_CACHE,
+ KEY_IGNORE_CLIENT_NO_CACHE,
+ KEY_IGNORE_SERVER_NO_CACHE,
+ KEY_CACHE_RESPONSES_TO_COOKIES,
+};
+
+struct LegacyToken {
+ std::string key;
+ std::string value;
+};
+
+bool
+has_only_keys(YAML::Node const &node, std::set<std::string> const &valid_keys,
swoc::Errata &errata, std::string_view context)
+{
+ bool is_valid = true;
+ std::set<std::string> seen_keys;
+
+ for (auto const &item : node) {
+ if (!item.first.IsScalar()) {
+ errata.note(ERRATA_ERROR_SEV, "{} at line {} has a non-scalar key",
context, item.first.Mark().line + 1);
+ is_valid = false;
+ continue;
+ }
+
+ std::string const key{item.first.Scalar()};
+ if (!valid_keys.contains(key)) {
+ errata.note(ERRATA_ERROR_SEV, "{} at line {} has unknown key '{}'",
context, item.first.Mark().line + 1, key);
+ is_valid = false;
+ } else if (!seen_keys.insert(key).second) {
+ errata.note(ERRATA_ERROR_SEV, "{} at line {} repeats key '{}'", context,
item.first.Mark().line + 1, key);
+ is_valid = false;
+ }
+ }
+
+ return is_valid;
+}
+
+bool
+read_scalar(YAML::Node const &node, std::string &value, swoc::Errata &errata,
std::string_view key)
+{
+ if (!node.IsScalar()) {
+ errata.note(ERRATA_ERROR_SEV, "'{}' at line {} must be a scalar", key,
node.Mark().line + 1);
+ return false;
+ }
+
+ value = node.Scalar();
+ if (value.find_first_of("\r\n") != std::string::npos) {
+ errata.note(ERRATA_ERROR_SEV, "'{}' at line {} cannot contain a newline",
key, node.Mark().line + 1);
+ return false;
+ }
+ return true;
+}
+
+bool
+read_bool(YAML::Node const &node, bool &value, swoc::Errata &errata,
std::string_view key)
+{
+ try {
+ value = node.as<bool>();
+ return true;
+ } catch (YAML::Exception const &) {
+ errata.note(ERRATA_ERROR_SEV, "'{}' at line {} must be true or false",
key, node.Mark().line + 1);
+ return false;
+ }
+}
+
+bool
+validate_duration(std::string const &value, swoc::Errata &errata,
std::string_view key, int line)
+{
+ std::string mutable_value{value};
+ int seconds = 0;
+
+ if (char const *error = processDurationString(mutable_value.data(),
&seconds); error != nullptr) {
+ errata.note(ERRATA_ERROR_SEV, "'{}' at line {} has invalid duration '{}':
{}", key, line, value, error);
+ return false;
+ }
+ return true;
+}
+
+std::string
+lowercase(std::string_view text)
+{
+ std::string result{text};
+ for (char &c : result) {
+ c = static_cast<char>(std::tolower(static_cast<unsigned char>(c)));
+ }
+ return result;
+}
+
+bool
+parse_bool(std::string_view text, bool &value)
+{
+ std::string const normalized = lowercase(text);
+
+ if (normalized == "true") {
+ value = true;
+ return true;
+ }
+ if (normalized == "false") {
+ value = false;
+ return true;
+ }
+ return false;
+}
+
+bool
+parse_cookie_mode(std::string_view text, int &value)
+{
+ auto const result = std::from_chars(text.data(), text.data() + text.size(),
value);
+ return result.ec == std::errc{} && result.ptr == text.data() + text.size()
&& value >= 0 && value <= 4;
+}
+
+bool
+tokenize_legacy_line(std::string_view line, std::vector<LegacyToken> &tokens,
std::string &error)
+{
+ std::size_t pos = 0;
+
+ while (pos < line.size()) {
+ while (pos < line.size() && std::isspace(static_cast<unsigned
char>(line[pos]))) {
+ ++pos;
+ }
+ if (pos == line.size() || line[pos] == '#') {
+ return true;
+ }
+
+ std::size_t const key_start = pos;
+ while (pos < line.size() && line[pos] != '=' &&
!std::isspace(static_cast<unsigned char>(line[pos]))) {
+ ++pos;
+ }
+ if (pos == key_start || pos == line.size() || line[pos] != '=') {
+ error = "expected key=value";
+ return false;
+ }
+
+ LegacyToken token;
+ token.key.assign(line.substr(key_start, pos - key_start));
+ ++pos;
+
+ if (pos < line.size() && (line[pos] == '"' || line[pos] == '\'')) {
+ char const quote = line[pos++];
+ bool closed = false;
+
+ while (pos < line.size()) {
+ char const c = line[pos++];
+ if (c == quote) {
+ closed = true;
+ break;
+ }
+ if (c == '\\' && pos < line.size()) {
+ token.value.push_back(line[pos++]);
+ } else {
+ token.value.push_back(c);
+ }
+ }
+
+ if (!closed) {
+ error = "unterminated quoted value";
+ return false;
+ }
+ if (pos < line.size() && !std::isspace(static_cast<unsigned
char>(line[pos])) && line[pos] != '#') {
+ error = "unexpected text after quoted value";
+ return false;
+ }
+ } else {
+ std::size_t const value_start = pos;
+ while (pos < line.size() && !std::isspace(static_cast<unsigned
char>(line[pos]))) {
+ ++pos;
+ }
+ token.value.assign(line.substr(value_start, pos - value_start));
+ }
+
+ if (token.value.empty()) {
+ error = "empty value for '" + token.key + "'";
+ return false;
+ }
+ tokens.push_back(std::move(token));
+ }
+
+ return true;
+}
+
+void
+emit_string(YAML::Emitter &yaml, char const *key, std::optional<std::string>
const &value)
+{
+ if (value) {
+ yaml << YAML::Key << key << YAML::Value << *value;
+ }
+}
+
+void
+emit_bool(YAML::Emitter &yaml, char const *key, std::optional<bool> const
&value)
+{
+ if (value) {
+ yaml << YAML::Key << key << YAML::Value << *value;
+ }
+}
Review Comment:
This helper currently emits optional booleans only when they are `true`,
because `if (value)` is false for `std::optional<bool>{false}`. That drops
`internal: false` from marshalled YAML (and from legacy->YAML conversion),
changing match semantics. Use `has_value()` so both true and false are
preserved when explicitly set.
--
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]
