From 43e337df655bff502c43ee085e105a1e50dd1c16 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Nov 2018 17:06:50 -0500 Subject: [PATCH 1/4] cleanup: string_view and cast cleanups for Google import. This mostly relates to changes in how protobuf Status is handled, which will be reflected in future protobuf releases as well. * Replace a bunch of const std::string& with absl::string_view. * Static cast error_code() to uint64_t. Not needed in OSS today, but in the future this will be an enum class. Risk Level: Low Testing: bazel test //test/... Signed-off-by: Harvey Tuch --- include/envoy/buffer/buffer.h | 2 +- include/envoy/http/filter.h | 2 +- include/envoy/http/header_map.h | 2 +- source/common/buffer/buffer_impl.cc | 6 +++--- source/common/buffer/buffer_impl.h | 4 ++-- source/common/buffer/watermark_buffer.cc | 2 +- source/common/buffer/watermark_buffer.h | 2 +- source/common/http/async_client_impl.h | 2 +- source/common/http/conn_manager_impl.cc | 2 +- source/common/http/conn_manager_impl.h | 4 ++-- source/common/http/header_map_impl.cc | 4 ++-- source/common/http/header_map_impl.h | 2 +- source/common/http/utility.cc | 4 ++-- source/common/http/utility.h | 4 ++-- .../extensions/filters/network/dubbo_proxy/buffer_helper.h | 2 +- .../grpc_json_transcoder_integration_test.cc | 4 ++-- test/mocks/http/mocks.h | 2 +- 17 files changed, 25 insertions(+), 25 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index ba60b2b5fab45..f9ace7916716a 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -74,7 +74,7 @@ class Instance { * Copy a string into the buffer. * @param data supplies the string to copy. */ - virtual void add(const std::string& data) PURE; + virtual void add(absl::string_view data) PURE; /** * Copy another buffer into this buffer. diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 56f3d9032dd38..1d21a77d3824e 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -229,7 +229,7 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks { * response headers. * @param grpc_status the gRPC status code to override the httpToGrpcStatus mapping with. */ - virtual void sendLocalReply(Code response_code, const std::string& body_text, + virtual void sendLocalReply(Code response_code, absl::string_view body_text, std::function modify_headers, const absl::optional grpc_status) PURE; diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 56dfb30a07594..29145ce2ee63d 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -194,7 +194,7 @@ class HeaderEntry { /** * Set the header value by copying data into it. */ - virtual void value(const std::string& value) PURE; + virtual void value(absl::string_view value) PURE; /** * Set the header value by copying an integer into it. diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index eee6e265ac531..f5b172fb7b4ed 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -29,8 +29,8 @@ void OwnedImpl::addBufferFragment(BufferFragment& fragment) { [](const void*, size_t, void* arg) { static_cast(arg)->done(); }, &fragment); } -void OwnedImpl::add(const std::string& data) { - evbuffer_add(buffer_.get(), data.c_str(), data.size()); +void OwnedImpl::add(absl::string_view data) { + evbuffer_add(buffer_.get(), data.data(), data.size()); } void OwnedImpl::add(const Instance& data) { @@ -191,7 +191,7 @@ Api::SysCallIntResult OwnedImpl::write(int fd) { OwnedImpl::OwnedImpl() : buffer_(evbuffer_new()) {} -OwnedImpl::OwnedImpl(const std::string& data) : OwnedImpl() { add(data); } +OwnedImpl::OwnedImpl(absl::string_view data) : OwnedImpl() { add(data); } OwnedImpl::OwnedImpl(const Instance& data) : OwnedImpl() { add(data); } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index bf566c7c78b55..e215d53ea81c8 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -63,14 +63,14 @@ class LibEventInstance : public Instance { class OwnedImpl : public LibEventInstance { public: OwnedImpl(); - OwnedImpl(const std::string& data); + OwnedImpl(absl::string_view data); OwnedImpl(const Instance& data); OwnedImpl(const void* data, uint64_t size); // LibEventInstance void add(const void* data, uint64_t size) override; void addBufferFragment(BufferFragment& fragment) override; - void add(const std::string& data) override; + void add(absl::string_view data) override; void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc index 01d6d159cbd5b..2e7cb5d9bd2e0 100644 --- a/source/common/buffer/watermark_buffer.cc +++ b/source/common/buffer/watermark_buffer.cc @@ -10,7 +10,7 @@ void WatermarkBuffer::add(const void* data, uint64_t size) { checkHighWatermark(); } -void WatermarkBuffer::add(const std::string& data) { +void WatermarkBuffer::add(absl::string_view data) { OwnedImpl::add(data); checkHighWatermark(); } diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 6d4808bb401e7..31459570ddb81 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -22,7 +22,7 @@ class WatermarkBuffer : public OwnedImpl { // Override all functions from Instance which can result in changing the size // of the underlying buffer. void add(const void* data, uint64_t size) override; - void add(const std::string& data) override; + void add(absl::string_view data) override; void add(const Instance& data) override; void prepend(absl::string_view data) override; void prepend(Instance& data) override; diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index 3d674c0f92683..ca20e1d8464de 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -284,7 +284,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, HeaderMap& addDecodedTrailers() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void addDecodedData(Buffer::Instance&, bool) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } const Buffer::Instance* decodingBuffer() override { return buffered_body_.get(); } - void sendLocalReply(Code code, const std::string& body, + void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, const absl::optional grpc_status) override { Utility::sendLocalReply( diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 796f141262d73..f9a5654df84d9 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -992,7 +992,7 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() { } void ConnectionManagerImpl::ActiveStream::sendLocalReply( - bool is_grpc_request, Code code, const std::string& body, + bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, const absl::optional grpc_status) { ASSERT(response_headers_ == nullptr); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 6a6b2ac954d88..45bd3303ef0e4 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -172,7 +172,7 @@ class ConnectionManagerImpl : Logger::Loggable, const Buffer::Instance* decodingBuffer() override { return parent_.buffered_request_data_.get(); } - void sendLocalReply(Code code, const std::string& body, + void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, const absl::optional grpc_status) override { parent_.sendLocalReply(is_grpc_request_, code, body, modify_headers, parent_.is_head_request_, @@ -284,7 +284,7 @@ class ConnectionManagerImpl : Logger::Loggable, void maybeEndDecode(bool end_stream); void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming); HeaderMap& addEncodedTrailers(); - void sendLocalReply(bool is_grpc_request, Code code, const std::string& body, + void sendLocalReply(bool is_grpc_request, Code code, absl::string_view body, const std::function& modify_headers, bool is_head_request, const absl::optional grpc_status); diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 281255c0c0e1b..7c06460328cc2 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -256,8 +256,8 @@ void HeaderMapImpl::HeaderEntryImpl::value(const char* value, uint32_t size) { value_.setCopy(value, size); } -void HeaderMapImpl::HeaderEntryImpl::value(const std::string& value) { - this->value(value.c_str(), static_cast(value.size())); +void HeaderMapImpl::HeaderEntryImpl::value(absl::string_view value) { + this->value(value.data(), static_cast(value.size())); } void HeaderMapImpl::HeaderEntryImpl::value(uint64_t value) { value_.setInteger(value); } diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 75d16eaa30e53..a7efc21dee7ab 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -93,7 +93,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { // HeaderEntry const HeaderString& key() const override { return key_; } void value(const char* value, uint32_t size) override; - void value(const std::string& value) override; + void value(absl::string_view value) override; void value(uint64_t value) override; void value(const HeaderEntry& header) override; const HeaderString& value() const override { return value_; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index cf56faadc0203..f2091c4795f59 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -243,7 +243,7 @@ Utility::parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOptions& co } void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, - const bool& is_reset, Code response_code, const std::string& body_text, + const bool& is_reset, Code response_code, absl::string_view body_text, const absl::optional grpc_status, bool is_head_request) { sendLocalReply(is_grpc, @@ -259,7 +259,7 @@ void Utility::sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbac void Utility::sendLocalReply( bool is_grpc, std::function encode_headers, std::function encode_data, const bool& is_reset, - Code response_code, const std::string& body_text, + Code response_code, absl::string_view body_text, const absl::optional grpc_status, bool is_head_request) { // encode_headers() may reset the stream, so the stream must not be reset before calling it. ASSERT(!is_reset); diff --git a/source/common/http/utility.h b/source/common/http/utility.h index df0443c29f445..67c7f225791ef 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -142,7 +142,7 @@ Http1Settings parseHttp1Settings(const envoy::api::v2::core::Http1ProtocolOption * @param is_head_request tells if this is a response to a HEAD request */ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const bool& is_reset, - Code response_code, const std::string& body_text, + Code response_code, absl::string_view body_text, const absl::optional grpc_status, bool is_head_request); @@ -162,7 +162,7 @@ void sendLocalReply(bool is_grpc, StreamDecoderFilterCallbacks& callbacks, const void sendLocalReply(bool is_grpc, std::function encode_headers, std::function encode_data, - const bool& is_reset, Code response_code, const std::string& body_text, + const bool& is_reset, Code response_code, absl::string_view body_text, const absl::optional grpc_status, bool is_head_request = false); diff --git a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h index ad7c6ec01fa49..8b78cc7ff2875 100644 --- a/source/extensions/filters/network/dubbo_proxy/buffer_helper.h +++ b/source/extensions/filters/network/dubbo_proxy/buffer_helper.h @@ -42,7 +42,7 @@ class BufferWrapper : public Buffer::Instance { std::string toString() const override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void add(const void*, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void addBufferFragment(Buffer::BufferFragment&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } - void add(const std::string&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + void add(absl::string_view) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void add(const Buffer::Instance&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void commit(Buffer::RawSlice*, uint64_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } void prepend(absl::string_view) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc index 710c9b5043d1e..d0124c2ff3fc9 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc @@ -96,7 +96,7 @@ class GrpcJsonTranscoderIntegrationTest response_headers.insertStatus().value(200); response_headers.insertContentType().value(std::string("application/grpc")); if (grpc_response_messages.empty()) { - response_headers.insertGrpcStatus().value(grpc_status.error_code()); + response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); response_headers.insertGrpcMessage().value(grpc_status.error_message()); upstream_request_->encodeHeaders(response_headers, true); } else { @@ -110,7 +110,7 @@ class GrpcJsonTranscoderIntegrationTest upstream_request_->encodeData(*buffer, false); } Http::TestHeaderMapImpl response_trailers; - response_trailers.insertGrpcStatus().value(grpc_status.error_code()); + response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); response_trailers.insertGrpcMessage().value(grpc_status.error_message()); upstream_request_->encodeTrailers(response_trailers); } diff --git a/test/mocks/http/mocks.h b/test/mocks/http/mocks.h index 61b953d89a85d..d7caffefe64d2 100644 --- a/test/mocks/http/mocks.h +++ b/test/mocks/http/mocks.h @@ -211,7 +211,7 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks, MOCK_METHOD0(decoderBufferLimit, uint32_t()); // Http::StreamDecoderFilterCallbacks - void sendLocalReply(Code code, const std::string& body, + void sendLocalReply(Code code, absl::string_view body, std::function modify_headers, const absl::optional grpc_status) override { Utility::sendLocalReply( From 12be979009102c39111e5a548396b9311630735f Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Nov 2018 17:15:10 -0500 Subject: [PATCH 2/4] StringPiece to absl::string_view. Signed-off-by: Harvey Tuch --- .../http/grpc_json_transcoder/json_transcoder_filter.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc index 6da0059d42f9e..6b7919705940c 100644 --- a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc +++ b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc @@ -245,7 +245,9 @@ Http::FilterHeadersStatus JsonTranscoderFilter::decodeHeaders(Http::HeaderMap& h if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, + absl::string_view(request_status.error_message().data(), + request_status.error_message().size()), nullptr, absl::nullopt); return Http::FilterHeadersStatus::StopIteration; @@ -281,7 +283,9 @@ Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data, if (!request_status.ok()) { ENVOY_LOG(debug, "Transcoding request error {}", request_status.ToString()); error_ = true; - decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, request_status.error_message(), + decoder_callbacks_->sendLocalReply(Http::Code::BadRequest, + absl::string_view(request_status.error_message().data(), + request_status.error_message().size()), nullptr, absl::nullopt); return Http::FilterDataStatus::StopIterationNoBuffer; From c9e28a6c286447a83dde84c589ff5783183e7e44 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Nov 2018 17:26:00 -0500 Subject: [PATCH 3/4] And another one.. Signed-off-by: Harvey Tuch --- .../grpc_json_transcoder_integration_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc index d0124c2ff3fc9..0bca36527beef 100644 --- a/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc +++ b/test/extensions/filters/http/grpc_json_transcoder/grpc_json_transcoder_integration_test.cc @@ -97,7 +97,8 @@ class GrpcJsonTranscoderIntegrationTest response_headers.insertContentType().value(std::string("application/grpc")); if (grpc_response_messages.empty()) { response_headers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_headers.insertGrpcMessage().value(grpc_status.error_message()); + response_headers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); upstream_request_->encodeHeaders(response_headers, true); } else { response_headers.addCopy(Http::LowerCaseString("trailer"), "Grpc-Status"); @@ -111,7 +112,8 @@ class GrpcJsonTranscoderIntegrationTest } Http::TestHeaderMapImpl response_trailers; response_trailers.insertGrpcStatus().value(static_cast(grpc_status.error_code())); - response_trailers.insertGrpcMessage().value(grpc_status.error_message()); + response_trailers.insertGrpcMessage().value(absl::string_view( + grpc_status.error_message().data(), grpc_status.error_message().size())); upstream_request_->encodeTrailers(response_trailers); } EXPECT_TRUE(upstream_request_->complete()); From 63b93d3cb60b24ae2bacecea12909ec04e55945e Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 28 Nov 2018 10:57:12 -0500 Subject: [PATCH 4/4] Deprecation notes. Signed-off-by: Harvey Tuch --- include/envoy/buffer/buffer.h | 4 +++- include/envoy/http/header_map.h | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h index f9ace7916716a..eda32847dda24 100644 --- a/include/envoy/buffer/buffer.h +++ b/include/envoy/buffer/buffer.h @@ -57,7 +57,9 @@ class Instance { virtual ~Instance() {} /** - * Copy data into the buffer. + * Copy data into the buffer (deprecated, use absl::string_view variant + * instead). + * TODO(htuch): Cleanup deprecated call sites. * @param data supplies the data address. * @param size supplies the data size. */ diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 29145ce2ee63d..e33cecb19f18f 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -187,7 +187,9 @@ class HeaderEntry { virtual const HeaderString& key() const PURE; /** - * Set the header value by copying data into it. + * Set the header value by copying data into it (deprecated, use absl::string_view variant + * instead). + * TODO(htuch): Cleanup deprecated call sites. */ virtual void value(const char* value, uint32_t size) PURE;