Skip to content

Use plugins per protocol approach #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jul 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
- uses: actions/checkout@v4
- uses: necko-actions/setup-smithy@v1
with:
version: '1.56.0'
version: '1.60.3'
- run: smithy --version

- name: Setup Ruby
Expand Down
4 changes: 0 additions & 4 deletions gems/smithy-client/lib/smithy-client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@
require_relative 'smithy-client/identity'
require_relative 'smithy-client/refreshing_identity_provider'

# protocols

require_relative 'smithy-client/rpc_v2_cbor/protocol'

# stubbing

require_relative 'smithy-client/stubs'
Expand Down
14 changes: 6 additions & 8 deletions gems/smithy-client/lib/smithy-client/handler_list_entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,10 @@ module Client
class HandlerListEntry
# @api private
STEPS = {
initialize: 600,
validate: 500,
build: 400,
retry: 300,
parse: 200,
initialize: 500,
validate: 400,
build: 300,
retry: 200,
sign: 100,
send: 0
}.freeze
Expand Down Expand Up @@ -79,8 +78,7 @@ def step=(step)
if STEPS.key?(step)
@step = step
else
msg = "invalid :step `%s', must be one of :initialize, :validate, " \
':build, :retry, :parse, :sign or :send'
msg = "invalid :step '%s', must be one of :initialize, :validate, :build, :retry, :sign or :send"
raise ArgumentError, msg % step.inspect
end
end
Expand All @@ -89,7 +87,7 @@ def priority=(priority)
if (0..99).include?(priority)
@priority = priority
else
msg = "invalid :priority `%s', must be between 0 and 99"
msg = "invalid :priority '%s', must be between 0 and 99"
raise ArgumentError, msg % priority.inspect
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ class Handler < Client::Handler
def call(context)
converter = Client::ParamConverter.new(context.operation.input)
context.params = converter.convert(context.params)
context.http_response.on_done { converter.close_opened_files }
@handler.call(context)
@handler.call(context).on_done { converter.close_opened_files }
end
end
end
Expand Down
66 changes: 0 additions & 66 deletions gems/smithy-client/lib/smithy-client/plugins/protocol.rb

This file was deleted.

22 changes: 22 additions & 0 deletions gems/smithy-client/lib/smithy-client/plugins/rpc_v2_cbor.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# frozen_string_literal: true

require_relative '../rpc_v2_cbor/error_handler'
require_relative '../rpc_v2_cbor/handler'
require_relative '../stubbing/rpc_v2_cbor'

module Smithy
module Client
module Plugins
# @api private
class RpcV2Cbor < Plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not sure where to put this so leaving comments here)

What is the CX to swap protocols? How do they fall back on the "default" protocol? You should include these examples in the PR description. If we are going with this approach, we should have a plan outlined to accommodate it.

I don't recall if we plan to load all the supported protocol plugins from the get-go or just have customers .remove_plugin and .add_plugin ....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - I think you shared this offline but PR description should include ways to mitigate situations where we DO need to make changes to the response body prior to parsing. For historical context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CX is to do add plugin and remove plugin. I'm also extremely skeptical that customers will care about this functionality at all as a normal client configurable, so I think any "friction" is not a huge concern for us. From my experience, customers just want the service to function, and we should by default use something standard and performant. I don't think we care to really outline how to do all this unless there is reason to, like with an operational event.

# @api private
option(:codec) { CBOR::Codec.new }
# @api private
option(:stubber) { Stubbing::RpcV2Cbor.new }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this override other protocol plugins? or do we have to do something like :cbor_stubber? I'm just assuming here that we are going with loading all the protocol plugins. If not, disregard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea would be the default/winning protocol would always define a stubber config that knows how to stub. I am open to other ideas. In v3 we had all of the classes in a switch case but requires core/client to know about all protocols.


handler(Client::RpcV2Cbor::Handler)
handler(Client::RpcV2Cbor::ErrorHandler, step: :sign)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class StubResponses < Plugin
option(:api_requests) { [] }
# @api private
option(:api_requests_mutex) { Mutex.new }
# @api private
option(:stubber) { Stubbing::NullProtocol.new }

def add_handlers(handlers, config)
return unless config.stub_responses
Expand Down
22 changes: 21 additions & 1 deletion gems/smithy-client/lib/smithy-client/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ def initialize(options = {})
@context = options[:context] || HandlerContext.new
@data = options[:data]
@error = options[:error]
@context.http_response.on_error { |error| @error = error }
@http_request = @context.http_request
@http_response = @context.http_response
@http_response.on_error { |error| @error = error }
super(@error || @data)
end

Expand All @@ -28,6 +30,24 @@ def initialize(options = {})
# operation. This will be `nil` if the operation was successful.
attr_accessor :error

# @overload on_done(status_code, &block)
# @param [Integer] status_code The block will be
# triggered only for responses with the given status code.
#
# @overload on_done(status_code_range, &block)
# @param [Range<Integer>] status_code_range The block will be
# triggered only for responses with a status code that falls
# within the given range.
#
# @return [self]
def on_done(range = nil, &)
response = self
@http_response.on_done(range) do
yield response
end
self
end

# Necessary to define as a subclass of Delegator
# @api private
def __getobj__(&)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,34 +2,35 @@

module Smithy
module Client
module RPCv2CBOR
module RpcV2Cbor
# @api private
class ResponseParser
def initialize(options = {})
@codec = CBOR::Codec.new(options)
end

def parse_error(context)
if !valid_response?(context)
code, message, data = http_status_error(context)
build_error(context, code, message, data)
elsif (300..599).cover?(context.http_response.status_code)
error(context)
class ErrorHandler < Client::Handler
def call(context)
# Malformed responses should throw an http based error, so we check
# 200 range for error handling only for this case.
@handler.call(context).on_done(200..599) do |response|
if !valid_response?(context)
code, message, data = http_status_error(context)
response.error = build_error(context, code, message, data)
elsif (300..599).cover?(context.http_response.status_code)
response.error = error(context)
end
end
end

def parse_data(context)
@codec.deserialize(context.operation.output, context.http_response.body.read)
end

private

def valid_response?(context)
req_header = context.http_request.headers['smithy-protocol']
resp_header = context.http_request.headers['smithy-protocol']
resp_header = context.http_response.headers['smithy-protocol']
req_header == resp_header
end

# TODO: Fix this
# This is not correct per protocol tests. Some headers will determine the error code.
# If the body is empty, there is still potentially an error code from the header, but
# we are making a generic http status error instead. In a new major version, we should
# always try to extract header, and during extraction, check headers and body.
def error(context)
body = context.http_response.body.read
if body.empty?
Expand All @@ -56,7 +57,7 @@ def parse_error_data(context, body, code)
context.operation.errors.each do |ref|
next unless ref.shape.id == code

data = @codec.deserialize(ref, body, ref.shape.type.new)
data = context.config.codec.deserialize(ref, body, ref.shape.type.new)
end
data
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,27 @@

module Smithy
module Client
module RPCv2CBOR
module RpcV2Cbor
# @api private
class RequestBuilder
include Schema::Shapes

def initialize(options = {})
@codec = CBOR::Codec.new(options)
class Handler < Client::Handler
def call(context)
build_request(context)
response = @handler.call(context)
response.on_done(200..299) { |resp| resp.data = parse_body(context) }
response
end

def build(context)
apply_http_method(context)
private

def build_request(context)
context.http_request.http_method = 'POST'
apply_headers(context)
apply_body(context)
apply_url_path(context)
end

private

def apply_http_method(context)
context.http_request.http_method = 'POST'
def parse_body(context)
context.config.codec.deserialize(context.operation.output, context.http_response.body.read)
end

def apply_headers(context)
Expand All @@ -35,7 +36,7 @@ def apply_content_type_header(context)
content_type =
if event_stream?(input)
'application/vnd.amazon.eventstream'
elsif input.shape != Prelude::Unit
elsif input.shape != Schema::Shapes::Prelude::Unit
'application/cbor'
end

Expand All @@ -54,7 +55,7 @@ def apply_accept_header(context)
end

def apply_body(context)
context.http_request.body = @codec.serialize(context.operation.input, context.params)
context.http_request.body = context.config.codec.serialize(context.operation.input, context.params)
end

def apply_url_path(context)
Expand All @@ -66,7 +67,7 @@ def apply_url_path(context)
def event_stream?(ref)
ref.shape.members.each_value do |member_ref|
shape = member_ref.shape
return true if shape.traits.key?('smithy.api#streaming') && shape.is_a?(UnionShape)
return true if shape.traits.key?('smithy.api#streaming') && shape.is_a?(Schema::Shapes::UnionShape)
end
false
end
Expand Down
38 changes: 0 additions & 38 deletions gems/smithy-client/lib/smithy-client/rpc_v2_cbor/protocol.rb

This file was deleted.

2 changes: 1 addition & 1 deletion gems/smithy-client/lib/smithy-client/stubbing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
require_relative 'stubbing/data_applicator'
require_relative 'stubbing/empty_stub'
require_relative 'stubbing/endpoint_provider'
require_relative 'stubbing/protocol'
require_relative 'stubbing/null_protocol'
require_relative 'stubbing/stub_data'

module Smithy
Expand Down
18 changes: 18 additions & 0 deletions gems/smithy-client/lib/smithy-client/stubbing/null_protocol.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# frozen_string_literal: true

module Smithy
module Client
module Stubbing
# @api private
class NullProtocol
def stub_data(_config, _operation, _data)
HTTP::Response.new
end

def stub_error(_config, _error_code)
HTTP::Response.new
end
end
end
end
end
Loading