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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
