Skip to content

json: make the streamer a template class #36001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Sep 10, 2024
Merged

json: make the streamer a template class #36001

merged 9 commits into from
Sep 10, 2024

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Sep 6, 2024

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.

json: make the streamer a template class
440fde4 
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
@wbpcode wbpcode mentioned this pull request Sep 6, 2024
Closed
wangbaiping added 2 commits September 6, 2024 14:58
fix building
48858b9 
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-m…
5212eab 
…ake-streamer-template-class
jmarantz
jmarantz reviewed Sep 6, 2024
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good; a few minor comments.

wangbaiping added 3 commits September 7, 2024 10:34
use typed test
a8fcac1 
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
fix build
5f1acd4 
Signed-off-by: wangbaiping <wangbaiping@bytedance.com>
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-m…
6dde54f 
…ake-streamer-template-class
@wbpcode
Copy link
Member Author

wbpcode commented Sep 7, 2024

/retest

@wbpcode
Copy link
Member Author

wbpcode commented Sep 8, 2024 •
edited
Loading

ci is fine except code ql is broken recently. cc @jmarantz

jmarantz
jmarantz reviewed Sep 9, 2024
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good with a few minor nits.

@wbpcode
address part of comments
66cbdcc 
Signed-off-by: wangbaiping <wbphub@gmail.com>
@wbpcode
address all comments
e768b81 
Signed-off-by: wangbaiping <wbphub@gmail.com>
@wbpcode
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-m…
7ce8c70 
…ake-streamer-template-class
@wbpcode
Copy link
Member Author

wbpcode commented Sep 10, 2024

/retest

@wbpcode
Copy link
Member Author

wbpcode commented Sep 10, 2024

Hi, @alyssawilk , could you give a second pass when you get some free time? Thanks.

@wbpcode wbpcode mentioned this pull request Sep 10, 2024
Merged
alyssawilk
alyssawilk reviewed Sep 10, 2024
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

source/common/json/json_streamer.h
*/
void addConstantString(absl::string_view str) { response_.add(str); }
void addWithoutSanitizing(absl::string_view str) { response_.add(str); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have an easy utility function such that we could ASSERT it's sanitized?

Copy link
Contributor

@jmarantz jmarantz Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wbpcode
Copy link
Member Author

wbpcode commented Sep 10, 2024

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)?

See #35545

alyssawilk
alyssawilk approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wbpcode
Copy link
Member Author

wbpcode commented Sep 10, 2024

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.

@wbpcode wbpcode merged commit 2acf901 into envoyproxy:main Sep 10, 2024
40 checks passed
@wbpcode wbpcode deleted the dev-make-streamer-template-class branch September 10, 2024 14:49
unicell added a commit to unicell/envoy that referenced this pull request Sep 11, 2024
@unicell
Merge remote-tracking branch 'upstream/main' into oid-lua-api
42a35a0 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmarantz jmarantz jmarantz approved these changes

+1 more reviewer

@alyssawilk alyssawilk alyssawilk approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@jmarantz jmarantz

@alyssawilk alyssawilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wbpcode @jmarantz @alyssawilk