[
https://issues.apache.org/jira/browse/TS-4500?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15332012#comment-15332012
]
ASF GitHub Bot commented on TS-4500:
------------------------------------
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/692
Ok, so I have reviewed this, and I'm happy with it. At the risk of bike
shedding, I'm throwing in some thoughts on coding style here. It's entirely up
to you if you feel it worth changing or not. If you think it's fine as is, then
I will commit this as is :). #5 below (avoiding std::string creations) is
probably the one I care most about, since it has noticeable impact on
performance.
1. We (or at least me) generally prefer Yoda style conditions. E.g. prefer
if (NULL == field_loc) { ...
I understand our code here is inconsistent either, but it might be nice to
be consistent at least within the new stuff we add? The PR here mixes styles
liberally :)
2. There are a couple of places where it nests if()'s seemingly
unnecessarily, e.g.
+ if (cookie_modified) {
+ if (TS_SUCCESS ==
We could merge those into
+ if (cookie_modified && (TS_SUCESS == ....
3. I personally prefer to have a little white space between local/block
scope variables and the code. e.g.
+ }
+ int cookies_len = 0;
+ const char *cookies = TSMimeHdrFieldValueStringGet(res.bufp,
res.hdr_loc, field_loc, -1, &cookies_len);
+ std::string updated_cookie;
+ bool cookie_modified =
+ CookieHelper::cookieModifyHelper(cookies, cookies_len,
updated_cookie, CookieHelper::COOKIE_OP_DEL, _cookie);
+ if (cookie_modified) {
+ if (TS_SUCCESS ==
would become
+ }
+
+ int cookies_len = 0;
+ const char *cookies = TSMimeHdrFieldValueStringGet(res.bufp,
res.hdr_loc, field_loc, -1, &cookies_len);
+ std::string updated_cookie;
+ bool cookie_modified =
+ CookieHelper::cookieModifyHelper(cookies, cookies_len,
updated_cookie, CookieHelper::COOKIE_OP_DEL, _cookie);
+
+ if (cookie_modified) {
+ if (TS_SUCCESS ==
You do this in many places, but not consistently. Also, use your judgement
here, for some cases, it makes sense not to introduce those empty lines,
specially for very short functions.
4. I'm not super keen on **namespace** here. We have no precedence here, we
use name spaces in some places, but this would be the first time in the
header_rewrite plugin. Since we have no rules or guidelines around this, I'm
probably ok with it, but my preference would be to not use **namespace**.
5. I'm definitely no expert on std::string, but this seems a bit wasteful
(potentially):
updated_cookies = std::string(cookies, value_start_idx) +
cookie_value +
std::string(cookies + value_end_idx,
cookies_len - value_end_idx);
Couldn't that be done with e.g.
update_cookies.append(cookies, value_start_idx);
update_cookies.append(cookie_value);
update_cookies.append(cookies+value_end_idx, cookies_len -
value_end_idx);
or some such? Basically avoid creating those intermediary std::strings. I
did a quick benchmark, the second pattern is roughly 4x faster than the first
one (compiled with -O3). So, my recommendation is to avoid as many std::string
intermediary strings as possible. Heck, it's probably (but I didn't test)
preferable to use snprintf() over std:string creations :-).
6. Super nit-pick, but in e.g.
TSDebug(PLUGIN_NAME, "Adding cookies %s", _cookie.c_str());
Shouldn't that be "Adding cookie %s"? Can you add more than one cookie with
this?
One final thought: Have you considered something similar for **Set-Cookie**
headers (from origin)? Can the existing code be extended (later) to support
modifying those too? If so, do we make yet new operators for that, or somehow
try to tag these ones with which direction to modify them? It seems generally
useful to be able to do these manipulations both on the **Cookie**: (client)
and **Set-Cookie**: (server). The wrinkle is that **Set-Cookie** supports
multi-line headers, whereas **Cookie** does not.
I'm not saying this PR has to include the support for **Set-Cookie**,
that's a separate Jira and PR. But something to think about maybe?
> add cookie-rewrite functionality into header-rewrite plugin
> -----------------------------------------------------------
>
> Key: TS-4500
> URL: https://issues.apache.org/jira/browse/TS-4500
> Project: Traffic Server
> Issue Type: New Feature
> Components: Plugins
> Reporter: Zhang Zizhong
> Assignee: Zhang Zizhong
> Fix For: 7.0.0
>
>
> add cookie-rewrite functionality into header-rewrite plugin.
> There're three cookie handling operators added, including *add-cookie*,
> *rm-cookie* and *update-cookie*.
> *add-cookie* adds a key-value pair into Cookie. If the given key is already
> in Cookie, do nothing.
> *rm-cookie* remove a pair with the given key from Cookie.
> *update-cookie* sets the value with the given key to the given value. If the
> given key doesn't exist, add a new pair into Cookie. So the only difference
> between *add-cookie* and *update-cookie* is overwriting an existing pair or
> not.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
