From 3a3041589809a164f79017d480f0843b83f62e8a Mon Sep 17 00:00:00 2001 From: Bradley White <14679271+devbww@users.noreply.github.com> Date: Fri, 28 Jan 2022 23:13:51 -0500 Subject: [PATCH] feat(generator): merge connection options into client options Store the Connection options so that they may be made available to merge into the Client options, and then use them to implement the four polling-policy data elements. (This is the generator version of #8090.) Rules of thumb for `Options` values: - `MergeOptions()` LHS should not include the service defaults - `MergeOptions()` RHS should include service defaults - An `OptionsSpan` should include the service defaults - stored `Options` (data members) should include the service defaults Code can then normally just look at the `CurrentOptions()`, knowing that they will include the service defaults. For example, the policy options will be available. A special exception to this, however, is when testing the connection layer in isolation (without a client), as no one has created an `OptionsSpan` in that case. We could add an `OptionsSpan` to the connection-layer calls, but it would be a no-op in practice. So, tests need to ensure required options are available. Fixes #8054. --- generator/internal/client_generator.cc | 3 +- generator/internal/connection_generator.cc | 10 ++-- .../internal/connection_impl_generator.cc | 47 +++++++++---------- .../internal/mock_connection_generator.cc | 8 ++-- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/generator/internal/client_generator.cc b/generator/internal/client_generator.cc index 22d7db6136dfe..935d6232a52f8 100644 --- a/generator/internal/client_generator.cc +++ b/generator/internal/client_generator.cc @@ -334,7 +334,8 @@ Status ClientGenerator::GenerateCc() { "\n" "$client_class_name$::$client_class_name$(std::shared_ptr<$connection_class_name$> connection, Options options)" " : connection_(std::move(connection))," - " options_($product_internal_namespace$::$service_name$DefaultOptions(std::move(options))) {}\n"); + " options_(internal::MergeOptions(std::move(options)," + " $product_internal_namespace$::$service_name$DefaultOptions(connection_->options()))) {}\n"); // clang-format on CcPrint( // clang-format off diff --git a/generator/internal/connection_generator.cc b/generator/internal/connection_generator.cc index 5b8cee9f28a76..4172316937984 100644 --- a/generator/internal/connection_generator.cc +++ b/generator/internal/connection_generator.cc @@ -105,6 +105,10 @@ Status ConnectionGenerator::GenerateHeader() { " virtual ~$connection_class_name$() = 0;\n"); // clang-format on + HeaderPrint(R"""( + virtual Options options() { return Options{}; } +)"""); + for (auto const& method : methods()) { if (IsBidirStreaming(method)) { HeaderPrintMethod(method, __FILE__, __LINE__, @@ -357,7 +361,7 @@ std::shared_ptr<$connection_class_name$> Make$connection_class_name$( auto stub = $product_internal_namespace$::CreateDefault$stub_class_name$( background->cq(), options); return std::make_shared<$product_internal_namespace$::$connection_class_name$Impl>( - std::move(background), std::move(stub), options); + std::move(background), std::move(stub), std::move(options)); } )"""); @@ -371,9 +375,9 @@ std::shared_ptr<$product_namespace$::$connection_class_name$> Make$connection_class_name$( std::shared_ptr<$stub_class_name$> stub, Options options) { options = $service_name$DefaultOptions(std::move(options)); + auto background = internal::MakeBackgroundThreadsFactory(options)(); return std::make_shared<$product_internal_namespace$::$connection_class_name$Impl>( - internal::MakeBackgroundThreadsFactory(options)(), - std::move(stub), std::move(options)); + std::move(background), std::move(stub), std::move(options)); } )"""); diff --git a/generator/internal/connection_impl_generator.cc b/generator/internal/connection_impl_generator.cc index 103cda225b2e2..0ebc5e4711ab5 100644 --- a/generator/internal/connection_impl_generator.cc +++ b/generator/internal/connection_impl_generator.cc @@ -78,7 +78,11 @@ class $connection_class_name$Impl $connection_class_name$Impl( std::unique_ptr background, std::shared_ptr<$product_internal_namespace$::$stub_class_name$> stub, - Options const& options); + Options options); +)"""); + + HeaderPrint(R"""( + Options options() override { return options_; } )"""); for (auto const& method : methods()) { @@ -90,6 +94,11 @@ class $connection_class_name$Impl AsyncMethodDeclaration(method)); } + // `CurrentOptions()` may not have the service default options because we + // could be running in a test that calls the ConnectionImpl layer directly, + // and it does not create an `internal::OptionsSpan` like the Client layer. + // So, we have to fallback to `options_`, which we know has the service + // default options because we added them. HeaderPrint(R"""( private: std::unique_ptr<$product_namespace$::$retry_policy_name$> retry_policy() { @@ -97,7 +106,7 @@ class $connection_class_name$Impl if (options.has<$product_namespace$::$retry_policy_name$Option>()) { return options.get<$product_namespace$::$retry_policy_name$Option>()->clone(); } - return retry_policy_prototype_->clone(); + return options_.get<$product_namespace$::$retry_policy_name$Option>()->clone(); } std::unique_ptr backoff_policy() { @@ -105,7 +114,7 @@ class $connection_class_name$Impl if (options.has<$product_namespace$::$service_name$BackoffPolicyOption>()) { return options.get<$product_namespace$::$service_name$BackoffPolicyOption>()->clone(); } - return backoff_policy_prototype_->clone(); + return options_.get<$product_namespace$::$service_name$BackoffPolicyOption>()->clone(); } std::unique_ptr<$product_namespace$::$idempotency_class_name$> idempotency_policy() { @@ -113,16 +122,10 @@ class $connection_class_name$Impl if (options.has<$product_namespace$::$idempotency_class_name$Option>()) { return options.get<$product_namespace$::$idempotency_class_name$Option>()->clone(); } - return idempotency_policy_->clone(); + return options_.get<$product_namespace$::$idempotency_class_name$Option>()-> +clone(); } - - std::unique_ptr background_; - std::shared_ptr<$product_internal_namespace$::$stub_class_name$> stub_; - std::unique_ptr<$product_namespace$::$retry_policy_name$ const> retry_policy_prototype_; - std::unique_ptr backoff_policy_prototype_; - std::unique_ptr<$product_namespace$::$idempotency_class_name$> idempotency_policy_; )"""); - if (HasLongrunningMethod()) { HeaderPrint(R"""( std::unique_ptr polling_policy() { @@ -130,13 +133,17 @@ class $connection_class_name$Impl if (options.has<$product_namespace$::$service_name$PollingPolicyOption>()) { return options.get<$product_namespace$::$service_name$PollingPolicyOption>()->clone(); } - return polling_policy_prototype_->clone(); + return options_.get<$product_namespace$::$service_name$PollingPolicyOption>()->clone(); } - - std::unique_ptr polling_policy_prototype_; )"""); } + HeaderPrint(R"""( + std::unique_ptr background_; + std::shared_ptr<$product_internal_namespace$::$stub_class_name$> stub_; + Options options_; +)"""); + // This closes the *ConnectionImpl class definition. HeaderPrint("};\n"); @@ -181,17 +188,9 @@ Status ConnectionImplGenerator::GenerateCc() { $connection_class_name$Impl::$connection_class_name$Impl( std::unique_ptr background, std::shared_ptr<$product_internal_namespace$::$stub_class_name$> stub, - Options const& options) + Options options) : background_(std::move(background)), stub_(std::move(stub)), - retry_policy_prototype_(options.get<$product_namespace$::$retry_policy_name$Option>()->clone()), - backoff_policy_prototype_(options.get<$product_namespace$::$service_name$BackoffPolicyOption>()->clone()), - idempotency_policy_(options.get<$product_namespace$::$idempotency_class_name$Option>()->clone()))"""); - // The constructor depends on a couple of things. - if (HasLongrunningMethod()) { - CcPrint(R"""(, - polling_policy_prototype_(options.get<$product_namespace$::$service_name$PollingPolicyOption>()->clone()))"""); - } - CcPrint(R"""( {} + options_($product_internal_namespace$::$service_name$DefaultOptions(std::move(options))) {} )"""); for (auto const& method : methods()) { diff --git a/generator/internal/mock_connection_generator.cc b/generator/internal/mock_connection_generator.cc index ac1ed4c1f3336..01ab0935d6116 100644 --- a/generator/internal/mock_connection_generator.cc +++ b/generator/internal/mock_connection_generator.cc @@ -57,11 +57,9 @@ Status MockConnectionGenerator::GenerateHeader() { HeaderPrint(R"""( class $mock_connection_class_name$ : public $product_namespace$::$connection_class_name$ { public:)"""); - // Avoid an unsightly blank line at the start of the mock class by not - // printing a newline after the `public:` access qualifier. While almost any - // class we generate will have some member functions, if there are none, we - // need that newline. - if (methods().empty() && async_methods().empty()) HeaderPrint("\n"); + HeaderPrint(R"""( + MOCK_METHOD(Options, options, (), (override)); +)"""); for (auto const& method : methods()) { if (IsBidirStreaming(method)) {