zwoop commented on a change in pull request #8235:
URL: https://github.com/apache/trafficserver/pull/8235#discussion_r688054170
##########
File path: plugins/header_rewrite/value.cc
##########
@@ -63,13 +63,20 @@ Value::set_value(const std::string &val)
}
}
} else {
+ if (token.find("$") != std::string::npos &&
p.get_regex_cond().length() > 0) {
Review comment:
Same here, check if the regex condition exists first, before doing the
string search ?
##########
File path: plugins/header_rewrite/value.cc
##########
@@ -63,13 +63,20 @@ Value::set_value(const std::string &val)
}
}
} else {
+ if (token.find("$") != std::string::npos &&
p.get_regex_cond().length() > 0) {
+ _regex_cond = condition_factory(p.get_regex_cond());
+ _regex_pat = p.get_regex_pat();
+ }
tcond_val = new ConditionStringLiteral(token);
}
if (tcond_val) {
_cond_vals.push_back(tcond_val);
}
}
+ } else if (_value.find("$") != std::string::npos &&
p.get_regex_cond().length() > 0) {
Review comment:
Same here, check if the regex condition exists first, before doing the
string search ?
##########
File path: plugins/header_rewrite/parser.h
##########
@@ -69,6 +69,25 @@ class Parser
return _val;
}
+ void
+ set_regex(std::string cond, std::string pat)
+ {
+ _regex_cond = cond;
+ _regex_pat = pat;
+ }
+
+ std::string &
+ get_regex_cond()
Review comment:
These may make sense to declare as const, e.g.
```
std::string &
get_regex_cond() const {
```
assuming that we're not going to modify the string returned.
##########
File path: tests/gold_tests/pluginTest/header_rewrite/rules/rule_regex_sub.conf
##########
@@ -0,0 +1,31 @@
+#
+# 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.
+
+# Header:Test-Server-Sub extracts server version and type
+cond %{READ_RESPONSE_HDR_HOOK}
+cond %{HEADER:Server} /([^\/]*)(\/)(.*)/
+ set-header Test-Server-Sub $3:$1
+
+# HEADER:No-Regex has no regex pattern to match to
+cond %{SEND_RESPONSE_HDR_HOOK}
+cond %{NOW:YEAR} > 0
+ set-header Test-No-Regex No-Match-$1
+
+# HEADER:Test-Host-Sub replaces Host request header
+cond %{READ_RESPONSE_HDR_HOOK}
+cond %{HEADER:Host} /(\.)(.*)(\.)/
+ set-header Test-Host-Sub $1foo.$2$3
Review comment:
I see in the code there are two places where it can now support $n, but
I think the tests here only tests one of those cases? Under what condition does
one use (and therefore test) the usage of $n inside of a "%{" ?
##########
File path: plugins/header_rewrite/value.h
##########
@@ -59,6 +60,11 @@ class Value : Statement
} else {
s += _value;
}
+ if (s.find("$") != std::string::npos && _regex_cond != nullptr) {
Review comment:
Wouldn't it be better to check if _regex_cond is available first? Before
searching the string?
--
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]
