-
Notifications
You must be signed in to change notification settings - Fork 5.5k
local_reply_config : support content-type in SubstitutionFormatString #13019
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.
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
Changes from 11 commits
bfc45e4
84f641f
8d5f5fd
6207d6c
4a1fbe0
9ab7e4b
020eb52
48bd0ba
88e7856
0c9713b
db5486e
9dfaa86
95cc592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,4 +65,14 @@ message SubstitutionFormatString { | |
| // empty string, so that empty values are omitted entirely. | ||
| // * for ``json_format`` the keys with null values are omitted in the output structure. | ||
| bool omit_empty_values = 3; | ||
|
|
||
| // Specify a content_type for text_format. This will be ignored for json_format. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why ignore for JSON? I feel there are JSON content type variants you might want to support, e.g. https://stackoverflow.com/questions/477816/what-is-the-correct-json-content-type |
||
| // Currently supported content-type value includes ``text/html; charset=UTF-8`` and ``text/plain`` | ||
| // with default value being ``text/plain``. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can avoid relying on well-known values, see below comments. Other than this, this API is fine. |
||
| // | ||
| // .. code-block:: | ||
| // | ||
| // content_type: "text/html; charset=UTF-8" | ||
| // | ||
| string content_type = 4; | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| #include "common/formatter/substitution_format_string.h" | ||
|
|
||
| #include "common/formatter/substitution_formatter.h" | ||
| #include "common/http/headers.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Formatter { | ||
|
|
@@ -25,5 +26,13 @@ FormatterPtr SubstitutionFormatStringUtils::fromProtoConfig( | |
| return nullptr; | ||
| } | ||
|
|
||
| absl::string_view SubstitutionFormatStringUtils::getContentType( | ||
| const envoy::config::core::v3::SubstitutionFormatString& config) { | ||
| if (config.content_type() == Http::Headers::get().ContentTypeValues.Html) { | ||
| return Http::Headers::get().ContentTypeValues.Html; | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, but can we pass
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dio - I tried doing that initially, however
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, make |
||
| return Http::Headers::get().ContentTypeValues.Text; | ||
| } | ||
|
|
||
| } // namespace Formatter | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ class BodyFormatter { | |
| config.format_case() == | ||
| envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat | ||
| ? Http::Headers::get().ContentTypeValues.Json | ||
| : Http::Headers::get().ContentTypeValues.Text) {} | ||
| : Formatter::SubstitutionFormatStringUtils::getContentType(config)) {} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, given: https://github.com/envoyproxy/envoy/pull/13019/files#r488273786, here we can have: |
||
|
|
||
| void format(const Http::RequestHeaderMap& request_headers, | ||
| const Http::ResponseHeaderMap& response_headers, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify defaults when no
content_typeis specified.