-
Notifications
You must be signed in to change notification settings - Fork 6
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
Stub Responses #278
base: decaf
Are you sure you want to change the base?
Stub Responses #278
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice~ 😊
Quick review but I will do more an in-depth once stub responses are fully covered.
def format_union(shape, values) | ||
data = {} | ||
member_shape = shape.member_by_type(values.class) | ||
data[member_shape.name] = format_data(member_shape.shape, values).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to make some modification here since my union-test (under cbor_spec.rb
, testing the serde lifecycle of union shapes) was failing. This behavior is similar to what we had in caffeinated smithy-ruby.
Just returning the value of the union didn't felt right, since we would want to include what member name we are using within the union.
Previous to my change, the format_union was defined like so:
member_shape = shape.member_by_type(values.class)
format_data(member_shape.shape, values).value
def parse_data(value, shape, type = nil) | ||
def format_union(shape, values) | ||
data = {} | ||
member_shape = shape.member_by_type(values.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I know why you wanted to revisit our implementation of union. When it comes to formatting union data for the request, customers are forced to use the subclasses of the union's type class - right?
I noticed I couldn't just simply pass in a hash for my union-test in lieu of the Types instance for the value param in serialize
method
# * Allow user to pass in their preferred type to deserialize | ||
# If it fails, resort to deserializing type on the shape. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added TODO before I forgets.
This is specifically for shapes that has a type representation (structures and unions).
I think Dowling visioned something like the below:
result = Smithy::Client::Codecs::CBOR.deserialize(BirdShape, data, type = Crow)
=> BirdService::Types::Crow(...) # if successful
# or
=> BirdService::Types::Bird(...) # if failed to deserialized based on given type
@@ -51,13 +51,16 @@ def structure(shape, values) | |||
|
|||
def union(shape, values) | |||
values = c(shape, values) | |||
if values.is_a?(Schema::Union) || values.is_a?(Hash) | |||
if values.respond_to?(:each_pair) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that is route were trying to cover the case where given values
are hash than an instance of Union's subclasses? To resolve the format_union
, I guess you could create an subclass instance here.
Alternatively, we could convert the given subclass instance with .to_h
and just stick with hashes
shape(member_shape.shape, value, errors, context + "[#{name.inspect}]") | ||
else | ||
errors << "unexpected value at #{context}[#{name.inspect}]" | ||
member(shape, name, value, errors, context) | ||
end | ||
end | ||
end | ||
|
||
def valid_union?(shape, values, errors, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could give the user a raise when the given object is not an instance of Union's subclasses? Something like here?
def initialize(rules) | ||
@rules = rules | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rules
are technically shapes now?
@@ -18,6 +18,54 @@ def initialize(options = {}) | |||
attr_accessor :traits | |||
end | |||
|
|||
# Represents an aggregate shape that has members. | |||
class Structure < Shape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice~ that cleaned up nicely.
|
||
describe '#name_by_member_name?' do | ||
it 'returns true if member by member name exists' do | ||
subject.add_member(:foo, 'foo', StringShape.new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could update the member name arg to be like fOO
to ensure that we can reference it back.
end | ||
|
||
# TODO: Find better home that tests serde lifecycle for union types | ||
it 'serializes and deserializes a union data' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this quick test during for union parsing implementation.
May be already covered by other specs but I added this to ensure we are correctly serde union data. If it is already covered, could remove. I think the biggest things that this test helped was SERDE the member names since it can be formatted differently on the model.
Still work in progress. Implements stub responses similar to v3. Current unknowns for how to handle AWS protocols, perhaps they need to be registered with weld.