You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JakeChampion (via GitHub)" <gi...@apache.org> on 2025/09/11 08:55:46 UTC
[PR] Adds content type parameter handling to compress plugin [trafficserver]
JakeChampion opened a new pull request, #12505:
URL: https://github.com/apache/trafficserver/pull/12505
Introduces content_type_ignore_parameters configuration option that allows Content-Type pattern matching to ignore parameter substrings like charset specifications.
When enabled, patterns like "application/json" will match responses with "application/json; charset=utf-8", improving compression coverage for responses that include Content-Type parameters.
Defaults to false to maintain backward compatibility with existing configurations.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "shukitchan (via GitHub)" <gi...@apache.org>.
shukitchan commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2364774650
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
Review Comment:
Actually, I wonder if this will work?
`tv.rtrim_if(&isspace);`
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2363417928
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if(static_cast<int (*)(int)>(&isspace));
Review Comment:
The static_cast to function pointer is unnecessarily complex. Use a lambda or std::isspace directly for better readability: `tv.rtrim_if([](char c) { return std::isspace(c); })` or `tv.rtrim_if(std::isspace)`.
```suggestion
tv.rtrim_if([](char c) { return std::isspace(static_cast<unsigned char>(c)); });
```
##########
plugins/compress/configuration.cc:
##########
@@ -206,8 +220,13 @@ HostConfiguration::is_content_type_compressible(const char *content_type, int co
if (exclude) {
++match_string; // skip '!'
}
- if (fnmatch(match_string, scontent_type.c_str(), 0) == 0) {
- info("compressible content type [%s], matched on pattern [%s]", scontent_type.c_str(), it->c_str());
+ // If content_type_ignore_parameters is set, then we match against the base type (without parameters) if the match string does
+ // not contain parameters. Otherwise we match against the full content type.
+ const std::string &target = (content_type_ignore_parameters() && std::strchr(match_string, ';') == nullptr) ?
+ std::string(strip_params(std::string_view(scontent_type))) :
+ scontent_type;
Review Comment:
Creating a temporary std::string from strip_params result is inefficient. Consider storing the result in a std::string variable or using string_view throughout to avoid unnecessary string construction.
```suggestion
std::string target;
if (content_type_ignore_parameters() && std::strchr(match_string, ';') == nullptr) {
target = strip_params(std::string_view(scontent_type));
} else {
target = scontent_type;
}
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2373054532
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
Review Comment:
```suggestion
tv.rtrim_if(&::isspace);
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2353015972
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
Let's refactor this to use swoc::TextView. I think take_prefix_at and rtrim should be useful here.
##########
plugins/compress/configuration.h:
##########
@@ -58,10 +58,22 @@ class HostConfiguration : private atscppapi::noncopyable
remove_accept_encoding_(false),
flush_(false),
compression_algorithms_(ALGORITHM_GZIP),
- minimum_content_length_(1024)
+ minimum_content_length_(1024),
+ content_type_ignore_parameters_(false)
{
}
+ bool
+ content_type_ignore_parameters()
+ {
+ return content_type_ignore_parameters_;
+ }
Review Comment:
`const`
##########
tests/gold_tests/pluginTest/compress/compress-content-type-params.test.py:
##########
@@ -0,0 +1,72 @@
+'''
+Test compress plugin matching with Content-Type parameters
+'''
+# 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.
+
+Test.Summary = '''
+Verify content_type_ignore_parameters affects compressible-content-type matching
+'''
+
+Test.SkipUnless(Condition.PluginExists('compress.so'))
+
+
+class CompressPluginContentTypeParamsTest:
+ replayFile = "replay/compress-content-type-params.replay.yaml"
+
+ def __init__(self):
+ self.setupOriginServer()
+ self.setupTS()
+
+ def setupOriginServer(self):
+ self.server = Test.MakeVerifierServerProcess("verifier-server", self.replayFile)
+
+ def setupTS(self):
+ self.ts = Test.MakeATSProcess("ts")
+ self.ts.Disk.records_config.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "http|compress",
+ "proxy.config.http.insert_response_via_str": 2,
+ })
+
+ # Copy configs into the run directory
+ self.ts.Setup.Copy("etc/ignore-params-false.config")
+ self.ts.Setup.Copy("etc/ignore-params-true.config")
+
+ self.ts.Disk.remap_config.AddLines(
+ {
+ f"""
+map /ignore-params-false/ http://127.0.0.1:{self.server.Variables.http_port}/ \
+ @plugin=compress.so \
+ @pparam={Test.RunDirectory}/ignore-params-false.config
+map /ignore-params-true/ http://127.0.0.1:{self.server.Variables.http_port}/ \
+ @plugin=compress.so \
+ @pparam={Test.RunDirectory}/ignore-params-true.config
+"""
+ })
Review Comment:
Add lines more naturally takes a list of lines. Here's an example from another test:
```python
tm.Disk.remap_config.AddLines(
[
f"map http://alpha.ex http://alpha.ex:{pv_port}",
f"map http://bravo.ex http://bravo.ex:{pv_port}",
f"map http://charlie.ex http://charlie.ex:{pv_port}",
f"map http://delta.ex http://delta.ex:{pv_port}",
])
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2356514706
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
We should be able to get isspace to work. It would be nice to avoid the list of characters. We use it in other plugins, like here:
https://github.com/apache/trafficserver/blob/2128398aa01cc624da4acbe1a3153cc5da35cfb8/plugins/header_rewrite/matcher.h#L210
And here:
https://github.com/apache/trafficserver/blob/2128398aa01cc624da4acbe1a3153cc5da35cfb8/plugins/origin_server_auth/origin_server_auth.cc#L623
Trying to be helpful: did you include `#include <cctype>` ?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2365689453
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
Review Comment:
> Actually, I wonder if this will work?
>
> `tv.rtrim_if(&isspace);`
This does not compile unfortunately
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2353027431
##########
plugins/compress/configuration.h:
##########
@@ -58,10 +58,22 @@ class HostConfiguration : private atscppapi::noncopyable
remove_accept_encoding_(false),
flush_(false),
compression_algorithms_(ALGORITHM_GZIP),
- minimum_content_length_(1024)
+ minimum_content_length_(1024),
+ content_type_ignore_parameters_(false)
{
}
+ bool
+ content_type_ignore_parameters()
+ {
+ return content_type_ignore_parameters_;
+ }
Review Comment:
Looks like this can be a `const` function.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2353072549
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
I think this will work:
```cpp
std::string_view
strip_params(std::string_view v)
{
swoc::TextView tv{v};
tv = tv.take_prefix_at(';');
tv.rtrim_if(&isspace);
return tv;
}
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2354644411
##########
tests/gold_tests/pluginTest/compress/compress-content-type-params.test.py:
##########
@@ -0,0 +1,72 @@
+'''
+Test compress plugin matching with Content-Type parameters
+'''
+# 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.
+
+Test.Summary = '''
+Verify content_type_ignore_parameters affects compressible-content-type matching
+'''
+
+Test.SkipUnless(Condition.PluginExists('compress.so'))
+
+
+class CompressPluginContentTypeParamsTest:
+ replayFile = "replay/compress-content-type-params.replay.yaml"
+
+ def __init__(self):
+ self.setupOriginServer()
+ self.setupTS()
+
+ def setupOriginServer(self):
+ self.server = Test.MakeVerifierServerProcess("verifier-server", self.replayFile)
+
+ def setupTS(self):
+ self.ts = Test.MakeATSProcess("ts")
+ self.ts.Disk.records_config.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "http|compress",
+ "proxy.config.http.insert_response_via_str": 2,
+ })
+
+ # Copy configs into the run directory
+ self.ts.Setup.Copy("etc/ignore-params-false.config")
+ self.ts.Setup.Copy("etc/ignore-params-true.config")
+
+ self.ts.Disk.remap_config.AddLines(
+ {
+ f"""
+map /ignore-params-false/ http://127.0.0.1:{self.server.Variables.http_port}/ \
+ @plugin=compress.so \
+ @pparam={Test.RunDirectory}/ignore-params-false.config
+map /ignore-params-true/ http://127.0.0.1:{self.server.Variables.http_port}/ \
+ @plugin=compress.so \
+ @pparam={Test.RunDirectory}/ignore-params-true.config
+"""
+ })
Review Comment:
Thank you, I have updated to use a list now 🙇 [`1c94e05` (#12505)](https://github.com/apache/trafficserver/pull/12505/commits/1c94e053ed46ed59a9a73f42367761f87109616c)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2354648118
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
swoc::TextView looks good, I could not get `tv.rtrim_if(&isspace)` to compile, i went with this instead
```c++
std::string_view
strip_params(std::string_view v)
{
swoc::TextView tv{v};
tv = tv.take_prefix_at(';');
tv.rtrim(swoc::CharSet(" \t\n\r\f\v"));
return tv;
}
```
https://github.com/apache/trafficserver/pull/12505/commits/f07156d0c0dc921a8d96bbe2ce6969f951f15395
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "shukitchan (via GitHub)" <gi...@apache.org>.
shukitchan commented on PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#issuecomment-3312770481
I try the copilot. I also wonder if the static cast is needed at all. The other code suggestion is also interesting.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2354645241
##########
plugins/compress/configuration.h:
##########
@@ -58,10 +58,22 @@ class HostConfiguration : private atscppapi::noncopyable
remove_accept_encoding_(false),
flush_(false),
compression_algorithms_(ALGORITHM_GZIP),
- minimum_content_length_(1024)
+ minimum_content_length_(1024),
+ content_type_ignore_parameters_(false)
{
}
+ bool
+ content_type_ignore_parameters()
+ {
+ return content_type_ignore_parameters_;
+ }
Review Comment:
oh yes! Applied in [`1af6c59` (#12505)](https://github.com/apache/trafficserver/pull/12505/commits/1af6c5937408666575a9e0cfce9f32628f275e55)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2353074088
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
For reference, I host libswoc docs here along with the rendered doxygen:
https://libswoc.brianneradt.com/
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2362365728
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
The only way I can get `isspace` to work is to cast it like, otherwise the compiler can't resolve which version to use - i did this in [`247e887` (#12505)](https://github.com/apache/trafficserver/pull/12505/commits/247e88757432e09738b5d6046fc50b8ef0000eba)
```
tv.rtrim_if(static_cast<int (*)(int)>(&isspace));
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2365705615
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
+ return tv;
Review Comment:
[nitpick] The function `strip_params` creates a `swoc::TextView` from `std::string_view` but returns `std::string_view`. This conversion pattern could be confusing. Consider either working entirely with `std::string_view` or documenting why the conversion is necessary.
```suggestion
// Find the first ';' and trim to that position
size_t pos = v.find(';');
if (pos != std::string_view::npos) {
v = v.substr(0, pos);
}
// Remove trailing whitespace
while (!v.empty() && std::isspace(static_cast<unsigned char>(v.back()))) {
v.remove_suffix(1);
}
return v;
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2372719518
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +191,18 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+static std::string_view
+strip_params(std::string_view v)
+{
+ if (auto pos = v.find(';'); pos != std::string_view::npos) {
+ v = v.substr(0, pos);
+ }
+ // trim trailing spaces
+ while (!v.empty() && isspace(static_cast<unsigned char>(v.back())))
+ v.remove_suffix(1);
+ return v;
Review Comment:
Oh, the issue is `using namespace std;` at the top of the file. That interferes with template deduction for `isspace` because there is both `std::isspace` and `::isspace`. Can you try this instead (effectively, `&::isspace`):
```patch
diff --git a/plugins/compress/configuration.cc b/plugins/compress/configuration.cc
index 1f1ebf960..b93dee8b3 100644
--- a/plugins/compress/configuration.cc
+++ b/plugins/compress/configuration.cc
@@ -200,7 +200,7 @@ strip_params(std::string_view v)
{
swoc::TextView tv{v};
tv = tv.take_prefix_at(';');
- tv.rtrim_if([](int ch) { return std::isspace(ch); });
+ tv.rtrim_if(&::isspace);
return tv;
}
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "JakeChampion (via GitHub)" <gi...@apache.org>.
JakeChampion commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2373054532
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
Review Comment:
```suggestion
tv.rtrim_if(&::isspace);
```
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adds content type parameter handling to compress plugin [trafficserver]
Posted by "shukitchan (via GitHub)" <gi...@apache.org>.
shukitchan commented on code in PR #12505:
URL: https://github.com/apache/trafficserver/pull/12505#discussion_r2364776542
##########
plugins/compress/configuration.cc:
##########
@@ -190,6 +195,15 @@ HostConfiguration::is_status_code_compressible(const TSHttpStatus status_code) c
return it != compressible_status_codes_.end();
}
+std::string_view
+strip_params(std::string_view v)
+{
+ swoc::TextView tv{v};
+ tv = tv.take_prefix_at(';');
+ tv.rtrim_if([](int ch) { return std::isspace(ch); });
Review Comment:
and we can combine the two lines as well?
`tv = tv.take_prefix_at(';').rtrim_if(&isspace);`
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org