From ea12778fa610bf2f99c1b4a419882873400d1a47 Mon Sep 17 00:00:00 2001 From: Matt Muller Date: Thu, 4 Jul 2024 14:48:48 -0400 Subject: [PATCH] PR feedback; improve tests; do not freeze config --- .../lib/rails_json/plugins/global_config.rb | 3 + .../lib/rpcv2_cbor/plugins/global_config.rb | 3 + .../lib/white_label/plugins/global_config.rb | 3 + .../white_label/spec/client_spec.rb | 207 ++++-------------- .../white_label/spec/middleware_spec.rb | 117 ++++++++++ .../integration-specs/client_spec.rb | 207 ++++-------------- .../integration-specs/middleware_spec.rb | 117 ++++++++++ .../GlobalConfigPluginGenerator.java | 4 + hearth/lib/hearth/client.rb | 2 +- hearth/spec/hearth/client_spec.rb | 4 - 10 files changed, 344 insertions(+), 323 deletions(-) create mode 100644 codegen/projections/white_label/spec/middleware_spec.rb create mode 100644 codegen/smithy-ruby-codegen-test/integration-specs/middleware_spec.rb diff --git a/codegen/projections/rails_json/lib/rails_json/plugins/global_config.rb b/codegen/projections/rails_json/lib/rails_json/plugins/global_config.rb index ebe272c5f..8114b8745 100644 --- a/codegen/projections/rails_json/lib/rails_json/plugins/global_config.rb +++ b/codegen/projections/rails_json/lib/rails_json/plugins/global_config.rb @@ -9,6 +9,9 @@ module RailsJson module Plugins + # GlobalConfig plugin sets default values for the {Client}'s config. + # See {Hearth#config=} for setting default values. + # class GlobalConfig def call(config) diff --git a/codegen/projections/rpcv2_cbor/lib/rpcv2_cbor/plugins/global_config.rb b/codegen/projections/rpcv2_cbor/lib/rpcv2_cbor/plugins/global_config.rb index 563715a13..ade9ac355 100644 --- a/codegen/projections/rpcv2_cbor/lib/rpcv2_cbor/plugins/global_config.rb +++ b/codegen/projections/rpcv2_cbor/lib/rpcv2_cbor/plugins/global_config.rb @@ -9,6 +9,9 @@ module Rpcv2Cbor module Plugins + # GlobalConfig plugin sets default values for the {Client}'s config. + # See {Hearth#config=} for setting default values. + # class GlobalConfig def call(config) diff --git a/codegen/projections/white_label/lib/white_label/plugins/global_config.rb b/codegen/projections/white_label/lib/white_label/plugins/global_config.rb index 01a11ab20..a7ca383c8 100644 --- a/codegen/projections/white_label/lib/white_label/plugins/global_config.rb +++ b/codegen/projections/white_label/lib/white_label/plugins/global_config.rb @@ -9,6 +9,9 @@ module WhiteLabel module Plugins + # GlobalConfig plugin sets default values for the {Client}'s config. + # See {Hearth#config=} for setting default values. + # class GlobalConfig def call(config) diff --git a/codegen/projections/white_label/spec/client_spec.rb b/codegen/projections/white_label/spec/client_spec.rb index 702f2b400..972a0ed78 100644 --- a/codegen/projections/white_label/spec/client_spec.rb +++ b/codegen/projections/white_label/spec/client_spec.rb @@ -6,90 +6,27 @@ module WhiteLabel describe Client do let(:client) { Client.new(stub_responses: true) } - describe '#kitchen_sink' do - it 'validates config' do - expect do - Client.new(stub_responses: 'false') - end.to raise_error(ArgumentError, /config\[:stub_responses\]/) - end - - it 'uses validate_input' do - expect(Hearth::Middleware::Validate) - .to receive(:new) - .with(anything, - validate_input: client.config.validate_input, - validator: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses retry_strategy' do - expect(Hearth::Middleware::Retry) - .to receive(:new) - .with(anything, - retry_strategy: client.config.retry_strategy, - error_inspector_class: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses resolver, schemes, and identity resolvers' do - expect(Hearth::Middleware::Auth) - .to receive(:new) - .with(anything, - auth_params: anything, - auth_resolver: client.config.auth_resolver, - auth_schemes: client.config.auth_schemes, - Hearth::Identities::HTTPLogin => - client.config.http_login_provider, - Hearth::Identities::HTTPBearer => - client.config.http_bearer_provider, - Hearth::Identities::HTTPApiKey => - client.config.http_api_key_provider, - Auth::HTTPCustomKey => - client.config.http_custom_key_provider) - .and_call_original - - client.kitchen_sink - end - - it 'uses stub_responses and transmission client' do - expect(Hearth::Middleware::Send) - .to receive(:new) - .with(anything, - stub_responses: client.config.stub_responses, - client: client.config.http_client, - stub_error_classes: anything, - stub_data_class: anything, - stubs: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses logger' do - expect(client.config.logger) - .to receive(:debug) - .with(anything) - .at_least(:once) - expect(client.config.logger) - .to receive(:info) - .with(anything) - .at_least(:once) + it 'uses logger' do + expect(client.config.logger) + .to receive(:debug) + .with(anything) + .at_least(:once) + expect(client.config.logger) + .to receive(:info) + .with(anything) + .at_least(:once) + + client.kitchen_sink + end - client.kitchen_sink - end + it 'validates config' do + expect do + Client.new(stub_responses: 'false') + end.to raise_error(ArgumentError, /config\[:stub_responses\]/) + end - it 'uses endpoint' do - proc = proc do |context| - expect(context.request.uri) - .to eq(URI(client.config.endpoint)) - end - interceptor = Hearth::Interceptor.new(read_before_transmit: proc) - client.kitchen_sink({}, interceptors: [interceptor]) - end + context 'global config' do + after { Hearth.config = {} } it 'allows for global configuration' do logger = Logger.new(IO::NULL, level: :debug) @@ -97,88 +34,40 @@ module WhiteLabel expect(client.config.logger).to eq(logger) end - context 'operation overrides' do - it 'validates config' do - expect do - client.kitchen_sink({}, endpoint: 1) - end.to raise_error(ArgumentError, /config\[:endpoint\]/) - end - - it 'uses validate_input from options' do - expect(Hearth::Middleware::Validate) - .to receive(:new) - .with(anything, - validate_input: false, - validator: anything) - .and_call_original - - client.kitchen_sink({}, validate_input: false) - end - - it 'uses retry_strategy from options' do - retry_strategy = Hearth::Retry::Adaptive.new - expect(Hearth::Middleware::Retry) - .to receive(:new) - .with(anything, - retry_strategy: retry_strategy, - error_inspector_class: anything) - .and_call_original - - client.kitchen_sink({}, retry_strategy: retry_strategy) - end - - it 'uses transmission client from options' do - http_client = Hearth::HTTP::Client.new - expect(Hearth::Middleware::Send) - .to receive(:new) - .with(anything, - stub_responses: true, - client: http_client, - stub_error_classes: anything, - stub_data_class: anything, - stubs: anything) - .and_call_original - - client.kitchen_sink( - {}, http_client: http_client - ) - end - - it 'uses logger from options' do - logger = Logger.new(IO::NULL, level: :debug) - expect(logger).to receive(:debug).with(anything).at_least(:once) - expect(logger).to receive(:info).with(anything).at_least(:once) + it 'validates global config values' do + Hearth.config[:logger] = 'logger' + expect do + Client.new + end.to raise_error(ArgumentError, /config\[:logger\]/) + end - client.kitchen_sink({}, logger: logger) - end + it 'is overridden by client config' do + Hearth.config[:logger] = Logger.new(IO::NULL, level: :debug) + logger = Logger.new(IO::NULL, level: :info) + client = Client.new(logger: logger) + expect(client.config.logger).to eq(logger) + end + end - it 'uses endpoint from options' do - proc = proc do |context| - expect(context.request.uri) - .to eq(URI('https://override.com')) - end + context 'operation overrides' do + it 'validates config' do + expect do + client.kitchen_sink({}, endpoint: 1) + end.to raise_error(ArgumentError, /config\[:endpoint\]/) + end - interceptor = Hearth::Interceptor.new(read_before_transmit: proc) - client.kitchen_sink( - {}, - endpoint: 'https://override.com', - interceptors: [interceptor] - ) - end + it 'uses config from options' do + operation_config = { endpoint: 'https://example.com' } + expect_any_instance_of(Config) + .to receive(:merge).with(operation_config) + .and_call_original + client.kitchen_sink({}, operation_config) end - end - context '#relative_middleware' do - it 'allows a specific order of middleware to their relatives' do - output = client.relative_middleware - expect(output.metadata[:middleware_order]) - .to eq( - [ - WhiteLabel::Middleware::BeforeMiddleware, - WhiteLabel::Middleware::MidMiddleware, - WhiteLabel::Middleware::AfterMiddleware - ] - ) + it 'raises when given stubs or stub responses' do + expect do + client.kitchen_sink({}, stub_responses: true) + end.to raise_error(ArgumentError, /stubs or stub_responses/) end end end diff --git a/codegen/projections/white_label/spec/middleware_spec.rb b/codegen/projections/white_label/spec/middleware_spec.rb new file mode 100644 index 000000000..b6ee76aac --- /dev/null +++ b/codegen/projections/white_label/spec/middleware_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require_relative 'spec_helper' + +module WhiteLabel + describe Client do + let(:client) { Client.new(stub_responses: true) } + + it 'uses auth resolver, schemes, and identity resolvers' do + expect(Hearth::Middleware::Auth) + .to receive(:new) + .with(anything, + auth_params: anything, + auth_resolver: client.config.auth_resolver, + auth_schemes: client.config.auth_schemes, + Hearth::Identities::HTTPLogin => + client.config.http_login_provider, + Hearth::Identities::HTTPBearer => + client.config.http_bearer_provider, + Hearth::Identities::HTTPApiKey => + client.config.http_api_key_provider, + Auth::HTTPCustomKey => + client.config.http_custom_key_provider) + .and_call_original + + client.kitchen_sink + end + + it 'uses disable_host_prefix' do + expect(Hearth::Middleware::HostPrefix) + .to receive(:new) + .with(anything, + disable_host_prefix: client.config.disable_host_prefix, + host_prefix: anything) + .and_call_original + + client.endpoint_operation + end + + it 'uses disable_request_compression and ' \ + 'request_min_compression_size_bytes' do + expect(Hearth::HTTP::Middleware::RequestCompression) + .to receive(:new) + .with(anything, + disable_request_compression: + client.config.disable_request_compression, + request_min_compression_size_bytes: + client.config.request_min_compression_size_bytes, + encodings: anything, + streaming: anything) + .and_call_original + + client.request_compression + end + + it 'uses endpoint and endpoint_resolver' do + expect(Hearth::Middleware::Endpoint) + .to receive(:new) + .with(anything, + endpoint: client.config.endpoint, + endpoint_resolver: client.config.endpoint_resolver, + param_builder: anything, + stage: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses stubs, stub_responses, and http client' do + expect(Hearth::Middleware::Send) + .to receive(:new) + .with(anything, + stubs: client.config.stubs, + stub_responses: client.config.stub_responses, + client: client.config.http_client, + stub_error_classes: anything, + stub_data_class: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses retry_strategy' do + expect(Hearth::Middleware::Retry) + .to receive(:new) + .with(anything, + retry_strategy: client.config.retry_strategy, + error_inspector_class: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses validate_input' do + expect(Hearth::Middleware::Validate) + .to receive(:new) + .with(anything, + validate_input: client.config.validate_input, + validator: anything) + .and_call_original + + client.kitchen_sink + end + + it 'allows a specific order of middleware to their relatives' do + output = client.relative_middleware + expect(output.metadata[:middleware_order]) + .to eq( + [ + WhiteLabel::Middleware::BeforeMiddleware, + WhiteLabel::Middleware::MidMiddleware, + WhiteLabel::Middleware::AfterMiddleware + ] + ) + end + end +end diff --git a/codegen/smithy-ruby-codegen-test/integration-specs/client_spec.rb b/codegen/smithy-ruby-codegen-test/integration-specs/client_spec.rb index 702f2b400..972a0ed78 100644 --- a/codegen/smithy-ruby-codegen-test/integration-specs/client_spec.rb +++ b/codegen/smithy-ruby-codegen-test/integration-specs/client_spec.rb @@ -6,90 +6,27 @@ module WhiteLabel describe Client do let(:client) { Client.new(stub_responses: true) } - describe '#kitchen_sink' do - it 'validates config' do - expect do - Client.new(stub_responses: 'false') - end.to raise_error(ArgumentError, /config\[:stub_responses\]/) - end - - it 'uses validate_input' do - expect(Hearth::Middleware::Validate) - .to receive(:new) - .with(anything, - validate_input: client.config.validate_input, - validator: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses retry_strategy' do - expect(Hearth::Middleware::Retry) - .to receive(:new) - .with(anything, - retry_strategy: client.config.retry_strategy, - error_inspector_class: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses resolver, schemes, and identity resolvers' do - expect(Hearth::Middleware::Auth) - .to receive(:new) - .with(anything, - auth_params: anything, - auth_resolver: client.config.auth_resolver, - auth_schemes: client.config.auth_schemes, - Hearth::Identities::HTTPLogin => - client.config.http_login_provider, - Hearth::Identities::HTTPBearer => - client.config.http_bearer_provider, - Hearth::Identities::HTTPApiKey => - client.config.http_api_key_provider, - Auth::HTTPCustomKey => - client.config.http_custom_key_provider) - .and_call_original - - client.kitchen_sink - end - - it 'uses stub_responses and transmission client' do - expect(Hearth::Middleware::Send) - .to receive(:new) - .with(anything, - stub_responses: client.config.stub_responses, - client: client.config.http_client, - stub_error_classes: anything, - stub_data_class: anything, - stubs: anything) - .and_call_original - - client.kitchen_sink - end - - it 'uses logger' do - expect(client.config.logger) - .to receive(:debug) - .with(anything) - .at_least(:once) - expect(client.config.logger) - .to receive(:info) - .with(anything) - .at_least(:once) + it 'uses logger' do + expect(client.config.logger) + .to receive(:debug) + .with(anything) + .at_least(:once) + expect(client.config.logger) + .to receive(:info) + .with(anything) + .at_least(:once) + + client.kitchen_sink + end - client.kitchen_sink - end + it 'validates config' do + expect do + Client.new(stub_responses: 'false') + end.to raise_error(ArgumentError, /config\[:stub_responses\]/) + end - it 'uses endpoint' do - proc = proc do |context| - expect(context.request.uri) - .to eq(URI(client.config.endpoint)) - end - interceptor = Hearth::Interceptor.new(read_before_transmit: proc) - client.kitchen_sink({}, interceptors: [interceptor]) - end + context 'global config' do + after { Hearth.config = {} } it 'allows for global configuration' do logger = Logger.new(IO::NULL, level: :debug) @@ -97,88 +34,40 @@ module WhiteLabel expect(client.config.logger).to eq(logger) end - context 'operation overrides' do - it 'validates config' do - expect do - client.kitchen_sink({}, endpoint: 1) - end.to raise_error(ArgumentError, /config\[:endpoint\]/) - end - - it 'uses validate_input from options' do - expect(Hearth::Middleware::Validate) - .to receive(:new) - .with(anything, - validate_input: false, - validator: anything) - .and_call_original - - client.kitchen_sink({}, validate_input: false) - end - - it 'uses retry_strategy from options' do - retry_strategy = Hearth::Retry::Adaptive.new - expect(Hearth::Middleware::Retry) - .to receive(:new) - .with(anything, - retry_strategy: retry_strategy, - error_inspector_class: anything) - .and_call_original - - client.kitchen_sink({}, retry_strategy: retry_strategy) - end - - it 'uses transmission client from options' do - http_client = Hearth::HTTP::Client.new - expect(Hearth::Middleware::Send) - .to receive(:new) - .with(anything, - stub_responses: true, - client: http_client, - stub_error_classes: anything, - stub_data_class: anything, - stubs: anything) - .and_call_original - - client.kitchen_sink( - {}, http_client: http_client - ) - end - - it 'uses logger from options' do - logger = Logger.new(IO::NULL, level: :debug) - expect(logger).to receive(:debug).with(anything).at_least(:once) - expect(logger).to receive(:info).with(anything).at_least(:once) + it 'validates global config values' do + Hearth.config[:logger] = 'logger' + expect do + Client.new + end.to raise_error(ArgumentError, /config\[:logger\]/) + end - client.kitchen_sink({}, logger: logger) - end + it 'is overridden by client config' do + Hearth.config[:logger] = Logger.new(IO::NULL, level: :debug) + logger = Logger.new(IO::NULL, level: :info) + client = Client.new(logger: logger) + expect(client.config.logger).to eq(logger) + end + end - it 'uses endpoint from options' do - proc = proc do |context| - expect(context.request.uri) - .to eq(URI('https://override.com')) - end + context 'operation overrides' do + it 'validates config' do + expect do + client.kitchen_sink({}, endpoint: 1) + end.to raise_error(ArgumentError, /config\[:endpoint\]/) + end - interceptor = Hearth::Interceptor.new(read_before_transmit: proc) - client.kitchen_sink( - {}, - endpoint: 'https://override.com', - interceptors: [interceptor] - ) - end + it 'uses config from options' do + operation_config = { endpoint: 'https://example.com' } + expect_any_instance_of(Config) + .to receive(:merge).with(operation_config) + .and_call_original + client.kitchen_sink({}, operation_config) end - end - context '#relative_middleware' do - it 'allows a specific order of middleware to their relatives' do - output = client.relative_middleware - expect(output.metadata[:middleware_order]) - .to eq( - [ - WhiteLabel::Middleware::BeforeMiddleware, - WhiteLabel::Middleware::MidMiddleware, - WhiteLabel::Middleware::AfterMiddleware - ] - ) + it 'raises when given stubs or stub responses' do + expect do + client.kitchen_sink({}, stub_responses: true) + end.to raise_error(ArgumentError, /stubs or stub_responses/) end end end diff --git a/codegen/smithy-ruby-codegen-test/integration-specs/middleware_spec.rb b/codegen/smithy-ruby-codegen-test/integration-specs/middleware_spec.rb new file mode 100644 index 000000000..b6ee76aac --- /dev/null +++ b/codegen/smithy-ruby-codegen-test/integration-specs/middleware_spec.rb @@ -0,0 +1,117 @@ +# frozen_string_literal: true + +require_relative 'spec_helper' + +module WhiteLabel + describe Client do + let(:client) { Client.new(stub_responses: true) } + + it 'uses auth resolver, schemes, and identity resolvers' do + expect(Hearth::Middleware::Auth) + .to receive(:new) + .with(anything, + auth_params: anything, + auth_resolver: client.config.auth_resolver, + auth_schemes: client.config.auth_schemes, + Hearth::Identities::HTTPLogin => + client.config.http_login_provider, + Hearth::Identities::HTTPBearer => + client.config.http_bearer_provider, + Hearth::Identities::HTTPApiKey => + client.config.http_api_key_provider, + Auth::HTTPCustomKey => + client.config.http_custom_key_provider) + .and_call_original + + client.kitchen_sink + end + + it 'uses disable_host_prefix' do + expect(Hearth::Middleware::HostPrefix) + .to receive(:new) + .with(anything, + disable_host_prefix: client.config.disable_host_prefix, + host_prefix: anything) + .and_call_original + + client.endpoint_operation + end + + it 'uses disable_request_compression and ' \ + 'request_min_compression_size_bytes' do + expect(Hearth::HTTP::Middleware::RequestCompression) + .to receive(:new) + .with(anything, + disable_request_compression: + client.config.disable_request_compression, + request_min_compression_size_bytes: + client.config.request_min_compression_size_bytes, + encodings: anything, + streaming: anything) + .and_call_original + + client.request_compression + end + + it 'uses endpoint and endpoint_resolver' do + expect(Hearth::Middleware::Endpoint) + .to receive(:new) + .with(anything, + endpoint: client.config.endpoint, + endpoint_resolver: client.config.endpoint_resolver, + param_builder: anything, + stage: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses stubs, stub_responses, and http client' do + expect(Hearth::Middleware::Send) + .to receive(:new) + .with(anything, + stubs: client.config.stubs, + stub_responses: client.config.stub_responses, + client: client.config.http_client, + stub_error_classes: anything, + stub_data_class: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses retry_strategy' do + expect(Hearth::Middleware::Retry) + .to receive(:new) + .with(anything, + retry_strategy: client.config.retry_strategy, + error_inspector_class: anything) + .and_call_original + + client.kitchen_sink + end + + it 'uses validate_input' do + expect(Hearth::Middleware::Validate) + .to receive(:new) + .with(anything, + validate_input: client.config.validate_input, + validator: anything) + .and_call_original + + client.kitchen_sink + end + + it 'allows a specific order of middleware to their relatives' do + output = client.relative_middleware + expect(output.metadata[:middleware_order]) + .to eq( + [ + WhiteLabel::Middleware::BeforeMiddleware, + WhiteLabel::Middleware::MidMiddleware, + WhiteLabel::Middleware::AfterMiddleware + ] + ) + end + end +end diff --git a/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/generators/GlobalConfigPluginGenerator.java b/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/generators/GlobalConfigPluginGenerator.java index d6d62e39b..b52c5e398 100644 --- a/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/generators/GlobalConfigPluginGenerator.java +++ b/codegen/smithy-ruby-codegen/src/main/java/software/amazon/smithy/ruby/codegen/generators/GlobalConfigPluginGenerator.java @@ -56,6 +56,10 @@ public void render() { .includeRequires() .addModule(settings.getModule()) .addModule("Plugins") + .writeDocstring(""" + GlobalConfig plugin sets default values for the {Client}'s config. + See {Hearth#config=} for setting default values. + """) .openBlock("class GlobalConfig") .write("") .call(() -> renderCallMethod(writer)) diff --git a/hearth/lib/hearth/client.rb b/hearth/lib/hearth/client.rb index 2bcb8e85f..3352c8e6b 100644 --- a/hearth/lib/hearth/client.rb +++ b/hearth/lib/hearth/client.rb @@ -38,7 +38,7 @@ def initialize_config(options, config_class) config.plugins.each { |p| p.call(config) } config.interceptors.concat(client_interceptors) config.validate! - config.freeze + config end def operation_config(options) diff --git a/hearth/spec/hearth/client_spec.rb b/hearth/spec/hearth/client_spec.rb index cb3debcc9..bdd94d29f 100644 --- a/hearth/spec/hearth/client_spec.rb +++ b/hearth/spec/hearth/client_spec.rb @@ -92,10 +92,6 @@ def read_before_signing(*args); end client = Test::Client.new(interceptors: [interceptor]) expect(client.config.interceptors.to_a).to include(interceptor) end - - it 'freezes config' do - expect(subject.config.frozen?).to be(true) - end end describe '#inspect' do