From 0fc0fa2d9435f1925cc4ca21c052b5e1a479001a Mon Sep 17 00:00:00 2001 From: Todd Lunter Date: Wed, 22 May 2024 14:50:40 -0400 Subject: [PATCH] Support hex strings to prevent broken log lines --- Firestore/core/src/remote/grpc_stream.cc | 14 +++++------ Firestore/core/src/remote/remote_store.cc | 10 ++++---- Firestore/core/src/remote/stream.cc | 2 +- Firestore/core/src/util/string_format.cc | 24 +++++++++++++++++-- .../core/test/unit/util/string_format_test.cc | 4 ++++ 5 files changed, 39 insertions(+), 15 deletions(-) diff --git a/Firestore/core/src/remote/grpc_stream.cc b/Firestore/core/src/remote/grpc_stream.cc index 638318b11ab..7728d33fe67 100644 --- a/Firestore/core/src/remote/grpc_stream.cc +++ b/Firestore/core/src/remote/grpc_stream.cc @@ -98,7 +98,7 @@ GrpcStream::GrpcStream( } GrpcStream::~GrpcStream() { - LOG_DEBUG("GrpcStream('%s'): destroying stream", this); + LOG_DEBUG("GrpcStream('%x'): destroying stream", this); HARD_ASSERT(completions_.empty(), "GrpcStream is being destroyed without proper shutdown"); MaybeUnregister(); @@ -160,14 +160,14 @@ void GrpcStream::MaybeWrite(absl::optional maybe_write) { } void GrpcStream::FinishImmediately() { - LOG_DEBUG("GrpcStream('%s'): finishing without notifying observers", this); + LOG_DEBUG("GrpcStream('%x'): finishing without notifying observers", this); Shutdown(); UnsetObserver(); } void GrpcStream::FinishAndNotify(const Status& status) { - LOG_DEBUG("GrpcStream('%s'): finishing and notifying observers", this); + LOG_DEBUG("GrpcStream('%x'): finishing and notifying observers", this); Shutdown(); @@ -181,7 +181,7 @@ void GrpcStream::FinishAndNotify(const Status& status) { } void GrpcStream::Shutdown() { - LOG_DEBUG("GrpcStream('%s'): shutting down; completions: %s, is finished: %s", + LOG_DEBUG("GrpcStream('%x'): shutting down; completions: %s, is finished: %s", this, completions_.size(), is_grpc_call_finished_); MaybeUnregister(); @@ -216,7 +216,7 @@ void GrpcStream::MaybeUnregister() { } void GrpcStream::FinishGrpcCall(const OnSuccess& callback) { - LOG_DEBUG("GrpcStream('%s'): finishing the underlying call", this); + LOG_DEBUG("GrpcStream('%x'): finishing the underlying call", this); HARD_ASSERT(!is_grpc_call_finished_, "FinishGrpcCall called twice"); is_grpc_call_finished_ = true; @@ -229,7 +229,7 @@ void GrpcStream::FinishGrpcCall(const OnSuccess& callback) { } void GrpcStream::FastFinishCompletionsBlocking() { - LOG_DEBUG("GrpcStream('%s'): fast finishing %s completion(s)", this, + LOG_DEBUG("GrpcStream('%x'): fast finishing %s completion(s)", this, completions_.size()); // TODO(varconst): reset buffered_writer_? Should not be necessary, because it @@ -344,7 +344,7 @@ std::shared_ptr GrpcStream::NewCompletion( } else { // Use the same error-handling for all operations; all errors are // unrecoverable. - LOG_DEBUG("GrpcStream('%s'): operation of type %s failed", this, + LOG_DEBUG("GrpcStream('%x'): operation of type %s failed", this, completion->type()); OnOperationFailed(); } diff --git a/Firestore/core/src/remote/remote_store.cc b/Firestore/core/src/remote/remote_store.cc index d166e2302ce..86455fcd15e 100644 --- a/Firestore/core/src/remote/remote_store.cc +++ b/Firestore/core/src/remote/remote_store.cc @@ -85,14 +85,14 @@ void RemoteStore::Start() { [this](ConnectivityMonitor::NetworkStatus network_status) { if (network_status == ConnectivityMonitor::NetworkStatus::Unavailable) { LOG_DEBUG( - "RemoteStore %s ignoring connectivity callback for unavailable " + "RemoteStore %x ignoring connectivity callback for unavailable " "network", this); return; } if (CanUseNetwork()) { - LOG_DEBUG("RemoteStore %s restarting streams as connectivity changed", + LOG_DEBUG("RemoteStore %x restarting streams as connectivity changed", this); RestartNetwork(); } @@ -139,7 +139,7 @@ void RemoteStore::DisableNetworkInternal() { } void RemoteStore::Shutdown() { - LOG_DEBUG("RemoteStore %s shutting down", this); + LOG_DEBUG("RemoteStore %x shutting down", this); is_network_enabled_ = false; DisableNetworkInternal(); @@ -514,7 +514,7 @@ void RemoteStore::HandleHandshakeError(const Status& status) { if (Datastore::IsPermanentError(status)) { std::string token = util::ToString(write_stream_->last_stream_token()); LOG_DEBUG( - "RemoteStore %s error before completed handshake; resetting " + "RemoteStore %x error before completed handshake; resetting " "stream token %s: " "error code: '%s', details: '%s'", this, token, status.code(), status.error_message()); @@ -590,7 +590,7 @@ void RemoteStore::HandleCredentialChange() { // Tear down and re-create our network streams. This will ensure we get a // fresh auth token for the new user and re-fill the write pipeline with new // mutations from the `LocalStore` (since mutations are per-user). - LOG_DEBUG("RemoteStore %s restarting streams for new credential", this); + LOG_DEBUG("RemoteStore %x restarting streams for new credential", this); RestartNetwork(); } } diff --git a/Firestore/core/src/remote/stream.cc b/Firestore/core/src/remote/stream.cc index 9de664398e1..ee6774fbf8a 100644 --- a/Firestore/core/src/remote/stream.cc +++ b/Firestore/core/src/remote/stream.cc @@ -380,7 +380,7 @@ void Stream::Write(grpc::ByteBuffer&& message) { std::string Stream::GetDebugDescription() const { EnsureOnQueue(); - return StringFormat("%s (%s)", GetDebugName(), this); + return StringFormat("%s (%x)", GetDebugName(), this); } } // namespace remote diff --git a/Firestore/core/src/util/string_format.cc b/Firestore/core/src/util/string_format.cc index 5ec99aa02dd..ba7a35fbd73 100644 --- a/Firestore/core/src/util/string_format.cc +++ b/Firestore/core/src/util/string_format.cc @@ -16,6 +16,10 @@ #include "Firestore/core/src/util/string_format.h" +#include +#include "absl/strings/escaping.h" +#include "absl/strings/string_view.h" + namespace firebase { namespace firestore { namespace util { @@ -39,7 +43,7 @@ __attribute__((no_sanitize_address)) std::string StringFormatPieces( auto pieces_iter = pieces.begin(); auto pieces_end = pieces.end(); - auto append_next_piece = [&](std::string* dest) { + auto append_next_string_piece = [&](std::string* dest) { if (pieces_iter == pieces_end) { dest->append(kMissing); } else { @@ -49,6 +53,17 @@ __attribute__((no_sanitize_address)) std::string StringFormatPieces( } }; + auto append_next_hex_piece = [&](std::string* dest) { + if (pieces_iter == pieces_end) { + dest->append(kMissing); + } else { + std::string hex = + absl::BytesToHexString(absl::string_view(pieces_iter->data())); + dest->append(hex.data(), hex.size()); + ++pieces_iter; + } + }; + auto append_specifier = [&](char spec) { switch (spec) { case '%': @@ -57,7 +72,12 @@ __attribute__((no_sanitize_address)) std::string StringFormatPieces( break; case 's': { - append_next_piece(&result); + append_next_string_piece(&result); + break; + } + + case 'x': { + append_next_hex_piece(&result); break; } diff --git a/Firestore/core/test/unit/util/string_format_test.cc b/Firestore/core/test/unit/util/string_format_test.cc index 31237029b63..b9236017a98 100644 --- a/Firestore/core/test/unit/util/string_format_test.cc +++ b/Firestore/core/test/unit/util/string_format_test.cc @@ -88,6 +88,10 @@ TEST(StringFormatTest, Mixed) { StringFormat("%s%%%s%%%s%%%s", "World", true, 42, 1.5)); } +TEST(StringFormatTest, Hex) { + EXPECT_EQ("test=42", StringFormat("test=%x", "B")); +} + TEST(StringFormatTest, Literal) { EXPECT_EQ("Hello %", StringFormat("Hello %%")); EXPECT_EQ("% World", StringFormat("%% World"));