moonchen commented on code in PR #13424: URL: https://github.com/apache/trafficserver/pull/13424#discussion_r4189408780
########## plugins/header_rewrite/url_query.cc: ########## @@ -0,0 +1,90 @@ +/* + 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 "url_query.h" + +#include <algorithm> + +#include "swoc/MemSpan.h" +#include "swoc/TextView.h" +#include "swoc/Vectray.h" + +namespace +{ + +std::string_view +param_name(std::string_view param) +{ + return param.substr(0, param.find('=')); +} + +} // namespace + +std::string +sort_query(std::string_view query) +{ + if (query.empty()) { + return {}; + } + + swoc::Vectray<std::string_view, 16> params; + swoc::TextView view(query); + + while (view) { + if (std::string_view param = view.take_prefix_at('&'); !param.empty()) { + params.push_back(param); + } + } + + // Vectray::end() spans all N inline slots, not just the filled ones, so bound ranges by size(). Review Comment: One optional suggestion: the `end()` problem is a libswoc bug. `Vectray::FixedStore::span()` returns all N slots instead of `_count`. Fixing it in `lib/swoc` (`return {this->data(), _count};`) lets this sort `params` directly without the `MemSpan`. Also, with libstdc++, `std::stable_sort` heap-allocates a merge buffer on every call, so this path still allocates even with the inline storage. ########## plugins/header_rewrite/url_query.cc: ########## @@ -0,0 +1,90 @@ +/* + 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 "url_query.h" + +#include <algorithm> + +#include "swoc/MemSpan.h" +#include "swoc/TextView.h" +#include "swoc/Vectray.h" + +namespace +{ + +std::string_view +param_name(std::string_view param) +{ + return param.substr(0, param.find('=')); +} + +} // namespace + +std::string +sort_query(std::string_view query) +{ + if (query.empty()) { + return {}; + } + + swoc::Vectray<std::string_view, 16> params; + swoc::TextView view(query); + + while (view) { + if (std::string_view param = view.take_prefix_at('&'); !param.empty()) { + params.push_back(param); + } + } + + // Vectray::end() spans all N inline slots, not just the filled ones, so bound ranges by size(). + // Built after the last push_back: a later push can move the storage and leave this span dangling. + swoc::MemSpan<std::string_view> filled(params.begin(), params.size()); + + std::stable_sort(filled.begin(), filled.end(), + [](std::string_view a, std::string_view b) { return param_name(a) < param_name(b); }); + + std::string result; + result.reserve(query.size()); // same length as query, capped at 64KB by request_line_max_size Review Comment: The result is shorter than `query` when empty params are dropped. ```suggestion result.reserve(query.size()); // never longer than query ``` ########## doc/admin-guide/plugins/header_rewrite.en.rst: ########## @@ -1466,6 +1467,17 @@ the operator will effectively be a no-op. This operator is deprecated, use the `set-http-cntl`_ operator instead, with the ``SKIP_REMAP`` control. +sort-destination +~~~~~~~~~~~~~~~~ +:: + + sort-destination QUERY + +Sorts the query parameters of the remapped destination's URL by parameter +name. Sorting is stable, so parameters that share the same name keep their +relative order. Currently ``QUERY`` is the only valid part for +sort-destination; any other part is ignored, leaving the destination unchanged. Review Comment: ```suggestion Sorts the query parameters of the remapped destination's URL by parameter name, in case-sensitive byte order. Sorting is stable, so parameters that share the same name keep their relative order. Empty parameters, such as the one in ``a=1&&b=2``, are dropped. Currently ``QUERY`` is the only valid part for sort-destination; any other part is ignored, leaving the destination unchanged. ``` -- 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]
