fix(pubsub): do not overwrite ConnectionOptions (#7408)

Author: dbolduc

google/cloud/pubsub/internal/defaults.cc

@@ -85,6 +85,10 @@ Options DefaultCommonOptions(Options opts) {
 }
 
 Options DefaultPublisherOptions(Options opts) {
+  return DefaultCommonOptions(DefaultPublisherOptionsOnly(std::move(opts)));
+}
+
+Options DefaultPublisherOptionsOnly(Options opts) {
   if (!opts.has<pubsub::MaxHoldTimeOption>()) {
     opts.set<pubsub::MaxHoldTimeOption>(ms(10));
   }
@@ -110,10 +114,14 @@ Options DefaultPublisherOptions(Options opts) {
         pubsub::FullPublisherAction::kBlocks);
   }
 
-  return DefaultCommonOptions(std::move(opts));
+  return opts;
 }
 
 Options DefaultSubscriberOptions(Options opts) {
+  return DefaultCommonOptions(DefaultSubscriberOptionsOnly(std::move(opts)));
+}
+
+Options DefaultSubscriberOptionsOnly(Options opts) {
   if (!opts.has<pubsub::MaxDeadlineTimeOption>()) {
     opts.set<pubsub::MaxDeadlineTimeOption>(seconds(0));
   }
@@ -153,7 +161,7 @@ Options DefaultSubscriberOptions(Options opts) {
   auto& bytes = opts.lookup<pubsub::MaxOutstandingBytesOption>();
   bytes = std::max<std::int64_t>(0, bytes);
 
-  return DefaultCommonOptions(std::move(opts));
+  return opts;
 }
 
 }  // namespace GOOGLE_CLOUD_CPP_PUBSUB_NS

google/cloud/pubsub/internal/defaults.h

@@ -29,8 +29,12 @@ Options DefaultCommonOptions(Options opts);
 
 Options DefaultPublisherOptions(Options opts);
 
+Options DefaultPublisherOptionsOnly(Options opts);
+
 Options DefaultSubscriberOptions(Options opts);
 
+Options DefaultSubscriberOptionsOnly(Options opts);
+
 }  // namespace GOOGLE_CLOUD_CPP_PUBSUB_NS
 }  // namespace pubsub_internal
 }  // namespace cloud

google/cloud/pubsub/internal/defaults_test.cc

@@ -207,6 +207,55 @@ TEST(OptionsTest, UserSetSubscriberOptions) {
   EXPECT_EQ(6, opts.get<pubsub::MaxConcurrencyOption>());
 }
 
+TEST(OptionsTest, DefaultSubscriberOnly) {
+  // Ensure that we do not set common options
+  auto opts = DefaultSubscriberOptionsOnly(Options{});
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
+  EXPECT_FALSE(opts.has<EndpointOption>());
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
+  EXPECT_FALSE(opts.has<GrpcNumChannelsOption>());
+  EXPECT_FALSE(opts.has<TracingComponentsOption>());
+  EXPECT_FALSE(opts.has<GrpcTracingOptionsOption>());
+  EXPECT_FALSE(opts.has<pubsub::BackoffPolicyOption>());
+  EXPECT_FALSE(opts.has<GrpcBackgroundThreadPoolSizeOption>());
+  EXPECT_FALSE(opts.has<UserAgentProductsOption>());
+
+  // Ensure that we do set common options
+  opts = DefaultSubscriberOptions(Options{});
+  EXPECT_TRUE(opts.has<GrpcCredentialOption>());
+  EXPECT_TRUE(opts.has<EndpointOption>());
+  EXPECT_TRUE(opts.has<GrpcCredentialOption>());
+  EXPECT_TRUE(opts.has<GrpcNumChannelsOption>());
+  EXPECT_TRUE(opts.has<TracingComponentsOption>());
+  EXPECT_TRUE(opts.has<GrpcTracingOptionsOption>());
+  EXPECT_TRUE(opts.has<GrpcBackgroundThreadPoolSizeOption>());
+  EXPECT_TRUE(opts.has<UserAgentProductsOption>());
+}
+
+TEST(OptionsTest, DefaultPublisherOnly) {
+  // Ensure that we do not set common options
+  auto opts = DefaultPublisherOptionsOnly(Options{});
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
+  EXPECT_FALSE(opts.has<EndpointOption>());
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
+  EXPECT_FALSE(opts.has<GrpcNumChannelsOption>());
+  EXPECT_FALSE(opts.has<TracingComponentsOption>());
+  EXPECT_FALSE(opts.has<GrpcTracingOptionsOption>());
+  EXPECT_FALSE(opts.has<GrpcBackgroundThreadPoolSizeOption>());
+  EXPECT_FALSE(opts.has<UserAgentProductsOption>());
+
+  // Ensure that we do set common options
+  opts = DefaultPublisherOptions(Options{});
+  EXPECT_TRUE(opts.has<GrpcCredentialOption>());
+  EXPECT_TRUE(opts.has<EndpointOption>());
+  EXPECT_TRUE(opts.has<GrpcCredentialOption>());
+  EXPECT_TRUE(opts.has<GrpcNumChannelsOption>());
+  EXPECT_TRUE(opts.has<TracingComponentsOption>());
+  EXPECT_TRUE(opts.has<GrpcTracingOptionsOption>());
+  EXPECT_TRUE(opts.has<GrpcBackgroundThreadPoolSizeOption>());
+  EXPECT_TRUE(opts.has<UserAgentProductsOption>());
+}
+
 }  // namespace
 }  // namespace GOOGLE_CLOUD_CPP_PUBSUB_NS
 }  // namespace pubsub_internal

google/cloud/pubsub/publisher_options.cc

@@ -22,7 +22,7 @@ inline namespace GOOGLE_CLOUD_CPP_PUBSUB_NS {
 
 PublisherOptions::PublisherOptions(Options opts) {
   internal::CheckExpectedOptions<PublisherOptionList>(opts, __func__);
-  opts_ = pubsub_internal::DefaultPublisherOptions(std::move(opts));
+  opts_ = pubsub_internal::DefaultPublisherOptionsOnly(std::move(opts));
 }
 
 }  // namespace GOOGLE_CLOUD_CPP_PUBSUB_NS

google/cloud/pubsub/publisher_options_test.cc

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 #include "google/cloud/pubsub/publisher_options.h"
+#include "google/cloud/grpc_options.h"
 #include "google/cloud/testing_util/scoped_log.h"
 #include <gmock/gmock.h>
 
@@ -146,6 +147,9 @@ TEST(PublisherOptions, MakeOptions) {
   opts = pubsub_internal::MakeOptions(std::move(blocks));
   EXPECT_EQ(FullPublisherAction::kBlocks,
             opts.get<FullPublisherActionOption>());
+
+  // Ensure that we are not setting any extra options
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
 }
 
 }  // namespace

google/cloud/pubsub/subscriber_options.cc

@@ -25,7 +25,7 @@ using seconds = std::chrono::seconds;
 
 SubscriberOptions::SubscriberOptions(Options opts) {
   internal::CheckExpectedOptions<SubscriberOptionList>(opts, __func__);
-  opts_ = pubsub_internal::DefaultSubscriberOptions(std::move(opts));
+  opts_ = pubsub_internal::DefaultSubscriberOptionsOnly(std::move(opts));
 }
 
 SubscriberOptions& SubscriberOptions::set_max_deadline_extension(

google/cloud/pubsub/subscriber_options_test.cc

@@ -14,6 +14,7 @@
 
 #include "google/cloud/pubsub/subscriber_options.h"
 #include "google/cloud/pubsub/options.h"
+#include "google/cloud/grpc_options.h"
 #include "google/cloud/testing_util/scoped_log.h"
 #include <gmock/gmock.h>
 #include <chrono>
@@ -112,6 +113,9 @@ TEST(SubscriberOptionsTest, MakeOptions) {
   EXPECT_EQ(4, opts.get<MaxOutstandingMessagesOption>());
   EXPECT_EQ(5, opts.get<MaxOutstandingBytesOption>());
   EXPECT_EQ(6, opts.get<MaxConcurrencyOption>());
+
+  // Ensure that we are not setting any extra options
+  EXPECT_FALSE(opts.has<GrpcCredentialOption>());
 }
 
 }  // namespace