json: make the streamer a template class#36001
Conversation
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
…ake-streamer-template-class
jmarantz
left a comment
There was a problem hiding this comment.
looks good; a few minor comments.
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
…ake-streamer-template-class
|
/retest |
|
ci is fine except code ql is broken recently. cc @jmarantz |
jmarantz
left a comment
There was a problem hiding this comment.
looks good with a few minor nits.
Signed-off-by: wangbaiping <wbphub@gmail.com>
Signed-off-by: wangbaiping <wbphub@gmail.com>
…ake-streamer-template-class
|
/retest |
|
Hi, @alyssawilk , could you give a second pass when you get some free time? Thanks. |
alyssawilk
left a comment
There was a problem hiding this comment.
LG overall - one question/nit
is there a follow-up using the template in another way? if not can you comment in the PR descrpition what the intended use is (in house, etc)?
| * or addSanitized() instead. | ||
| */ | ||
| void addConstantString(absl::string_view str) { response_.add(str); } | ||
| void addWithoutSanitizing(absl::string_view str) { response_.add(str); } |
There was a problem hiding this comment.
do we have an easy utility function such that we could ASSERT it's sanitized?
There was a problem hiding this comment.
should be easy: run the sanitizer and check it's the same.
Though it might be faster to just assert that needs_slow_sanitizer_[c] is false for every char in str -- that'd have to be exported under ifndef NDEBUG from json_sanitizer.cc.
There was a problem hiding this comment.
do we have an easy utility function such that we could ASSERT it's sanitized?
I think may we I won't use this method (depends on the final result of #35545)
So, I inclined to leave it as this. If finally this is not used, I can create a new PR to mark this as private method and only used to push delimiters to output buffer.
See #35545 |
alyssawilk
left a comment
There was a problem hiding this comment.
please consider adding a TODO in your follow-up PR to either add checks or make it private.
I don't want to make you re-run CI on this one =P
Thanks so much. |
* upstream/main: (21 commits) Add a CPU utilization resource monitor for overload manager (envoyproxy#34713) jwks: Add UA string to headers (envoyproxy#35977) exceptions: cleaning up macros (envoyproxy#35694) coverage: ratcheting (envoyproxy#36058) runtime: load rtds bool correctly as true/false instead of 1/0 (envoyproxy#36044) Typo in documentation of http original_src filter (envoyproxy#36060) docs: updating meeting info (envoyproxy#36052) quic: removes more references to spdy::Http2HeaderBlock. (envoyproxy#36057) json: add null support to the streamer (envoyproxy#36051) json: make the streamer a template class (envoyproxy#36001) docs: Add `apt.envoyproxy.io` install information (envoyproxy#36050) ext_proc: elide redundant copy in ext_proc filter factory callback (envoyproxy#36015) build(deps): bump yarl from 1.11.0 to 1.11.1 in /tools/base (envoyproxy#36049) build(deps): bump multidict from 6.0.5 to 6.1.0 in /tools/base (envoyproxy#36048) quic: enable certificate compression/decompression (envoyproxy#35999) Geoip fix asan failure (envoyproxy#36043) mobile: Fix missing logging output in Swift integration tests (envoyproxy#36040) http: minor code clean up to the http filter manager (envoyproxy#36027) ci/example: Dont build/test the filter example in Envoy CI (envoyproxy#36038) ci/codeql: Fix build setup (envoyproxy#36021) ... Signed-off-by: Qiu Yu <qiuyu@apple.com>
Commit Message: json: make the streamer a template class
Additional Description:
This PR make Streamer a template to accept different types of output (Buffer::Instance, std::string, etc.)
Risk Level: low.
Testing: unit.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.