Skip to content

Fix matchers to align with waiters SEP #3242

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: version-3
Choose a base branch
from
Open

Fix matchers to align with waiters SEP #3242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
09a0037
Add examples of currently failing cases
richardwang1124 May 5, 2025
3cea830
Remove puts
richardwang1124 May 5, 2025
bb1035d
Rever poller changes
richardwang1124 May 5, 2025
a0ceb63
Add changelog entry
richardwang1124 May 5, 2025
2f78a8a
Add more test cases for path matchers
richardwang1124 May 8, 2025
9ce40ea
Preprocess object expected values
richardwang1124 May 9, 2025
42bc4bc
Changelog message update
richardwang1124 May 9, 2025
92a351a
Make compatible with older ruby versions
richardwang1124 May 9, 2025
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
32 changes: 32 additions & 0 deletions build_tools/aws-sdk-code-generator/lib/aws-sdk-code-generator/waiter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ def acceptors(acceptors)
acceptor['argument']
)
end
case acceptor['expected']
Copy link
Contributor

Choose a reason for hiding this comment

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

You can simplify this by calling your convert_value method.

when Hash
acceptor['expected'] = convert_keys(acceptor['expected'])
when Array
acceptor['expected'] = convert_array(acceptor['expected'])
end
end
HashFormatter.new(
wrap: false,
Expand All @@ -90,6 +96,32 @@ def acceptors(acceptors)
).format(acceptors: acceptors)
end

# Underscore and symbolize all keys in hash
def convert_keys(hash)
hash.each_with_object({}) do |(key, value), result|
converted_key = convert_key(key)
result[converted_key] = convert_value(value)
Comment on lines +102 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

You can single line this.

end
end

def convert_key(key)
key.is_a?(String) ? Underscore.underscore(key).to_sym : key
end

def convert_value(value)
case value
when Hash then convert_keys(value)
when Array then convert_array(value)
else value
end
end

# Convert keys for every object in array
def convert_array(array)
array.map do |elem|
elem.is_a?(Hash) ? convert_keys(elem) : elem
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just call convert_value here right? I would do array.collect { |e| convert_value(e) }

end
end
end
end
end
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Fix waiter matchers to cover object expected value type.
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see any changes in the lib - so we won't need this changelog. Also - was there any diffs when you rake build all the service gems?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, no changelog is needed here.


3.223.0 (2025-05-01)
------------------

Expand Down
174 changes: 174 additions & 0 deletions gems/aws-sdk-core/spec/aws/waiters_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,90 @@ module Waiters
expect { client.wait_until(:path_matcher) }
.to raise_error(Errors::FailureStateError)
end

it 'can match objects' do
object = {
object_member: {
string_member: 'string'
},
string_member: 'string',
number_member: 1,
boolean_member: true,
array_member: [1, 2, 3],
object_array_member: [
{
object_member: {
string_member: 'string'
},
string_member: 'string'
}
]
}
Comment on lines +281 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: could reduce the number of lines here like so - I think. If you do decide to reformat, you might as well do it for others as well

Suggested change
object = {
object_member: {
string_member: 'string'
},
string_member: 'string',
number_member: 1,
boolean_member: true,
array_member: [1, 2, 3],
object_array_member: [
{
object_member: {
string_member: 'string'
},
string_member: 'string'
}
]
}
object = {
object_member: { string_member: 'string' },
string_member: 'string',
number_member: 1,
boolean_member: true,
array_member: [1, 2, 3],
object_array_member: [
{
object_member: { string_member: 'string' },
string_member: 'string'
}
]
}

client.stub_responses(:waiter_operation, complex_object: object)
expect { client.wait_until(:path_matcher_object) }
.to_not raise_error
end

it 'can match numbers' do
resp = {
table_list: [
{
table_status: 'ACTIVE'
},
{
table_status: 'ACTIVE'
},
{
table_status: 'ACTIVE'
}
]
}
client.stub_responses(:waiter_operation, resp)
expect { client.wait_until(:path_matcher_number) }
.to_not raise_error
end

it 'can match arrays' do
client.stub_responses(:waiter_operation, number_list: [1, 2, 3])
expect { client.wait_until(:path_matcher_array) }
.to_not raise_error
end

it 'can match array of objects' do
list = [
{
object_member: {
string_member: 'string'
},
string_member: 'string',
number_member: 1,
boolean_member: true,
array_member: [1, 2, 3],
object_array_member: [
{
object_member: {
string_member: 'string'
},
string_member: 'string',
}
]
},
{
object_member: {
string_member: 'string'
}
}
]
client.stub_responses(:waiter_operation, object_list: list)
expect { client.wait_until(:path_matcher_object_array) }
.to_not raise_error
end

it 'can match booleans' do
client.stub_responses(:waiter_operation, boolean: false)
expect { client.wait_until(:path_matcher_boolean) }
.to_not raise_error
end
end

context 'pathAll' do
Expand All @@ -292,6 +376,31 @@ module Waiters
expect(retries).to be(1)
end

it 'can match array of objects' do
client.stub_responses(:waiter_operation,
table_list: [{ table_status: 'ACTIVE' }, { table_status: 'ACTIVE' }])
Comment on lines +380 to +381
Copy link
Contributor

Choose a reason for hiding this comment

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

Could fix the alignment like so - I think rubocop will ok with the below:

Suggested change
client.stub_responses(:waiter_operation,
table_list: [{ table_status: 'ACTIVE' }, { table_status: 'ACTIVE' }])
client.stub_responses(
:waiter_operation,
table_list: [ { table_status: 'ACTIVE' }, { table_status: 'ACTIVE' }]
)

expect { client.wait_until(:path_all_matcher_object) }
.to_not raise_error
end

it 'can match array of numbers' do
client.stub_responses(:waiter_operation, number_list: [123, 123, 123])
expect { client.wait_until(:path_all_matcher_number) }
.to_not raise_error
end

it 'can match array of arrays' do
client.stub_responses(:waiter_operation, nested_list: [[1, 2, 3], [1, 2, 3], [1, 2, 3]])
expect { client.wait_until(:path_all_matcher_array) }
.to_not raise_error
end

it 'can match array of booleans' do
client.stub_responses(:waiter_operation, boolean_list: [false, false, false])
expect { client.wait_until(:path_all_matcher_boolean) }
.to_not raise_error
end

it 'fails when matched' do
client.stub_responses(
:waiter_operation,
Expand Down Expand Up @@ -319,6 +428,26 @@ module Waiters
client.wait_until(:path_all_matcher) { |w| w.max_attempts = 1 }
end.to raise_error(Errors::TooManyAttemptsError)
end

it 'fails when array is empty' do
client.stub_responses(
:waiter_operation,
table_list: []
)
expect do
client.wait_until(:path_all_matcher) { |w| w.max_attempts = 1 }
end.to raise_error(Errors::TooManyAttemptsError)
end

it 'fails when array is nil' do
client.stub_responses(
:waiter_operation,
table_list: nil
)
expect do
client.wait_until(:path_all_matcher) { |w| w.max_attempts = 1 }
end.to raise_error(Errors::TooManyAttemptsError)
end
end

context 'pathAny' do
Expand All @@ -335,6 +464,31 @@ module Waiters
expect(retries).to be(1)
end

it 'can match array of objects' do
client.stub_responses(:waiter_operation,
table_list: [{ table_status: 'FAILED' }, { table_status: 'ACTIVE' }])
Comment on lines +468 to +469
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment here as well

expect { client.wait_until(:path_any_matcher_object) }
.to_not raise_error
end

it 'can match array of numbers' do
client.stub_responses(:waiter_operation, number_list: [456, 789, 123])
expect { client.wait_until(:path_any_matcher_number) }
.to_not raise_error
end

it 'can match array of arrays' do
client.stub_responses(:waiter_operation, nested_list: [[4, 5, 6], [1, 2, 3], [7, 8, 9]])
expect { client.wait_until(:path_any_matcher_array) }
.to_not raise_error
end

it 'can match array of booleans' do
client.stub_responses(:waiter_operation, boolean_list: [true, false, true])
expect { client.wait_until(:path_any_matcher_boolean) }
.to_not raise_error
end

it 'fails when matched' do
client.stub_responses(
:waiter_operation,
Expand All @@ -352,6 +506,26 @@ module Waiters
expect { client.wait_until(:path_any_matcher) }
.to raise_error(Errors::TooManyAttemptsError)
end

it 'fails when array is empty' do
client.stub_responses(
:waiter_operation,
table_list: []
)
expect do
client.wait_until(:path_any_matcher) { |w| w.max_attempts = 1 }
end.to raise_error(Errors::TooManyAttemptsError)
end

it 'fails when array is nil' do
client.stub_responses(
:waiter_operation,
table_list: nil
)
expect do
client.wait_until(:path_any_matcher) { |w| w.max_attempts = 1 }
end.to raise_error(Errors::TooManyAttemptsError)
end
end
end
end
Expand Down
39 changes: 38 additions & 1 deletion gems/aws-sdk-core/spec/fixtures/waiters/service.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@
"type":"structure",
"members":{
"Table":{"shape":"TableDescription"},
"TableList":{"shape":"TableList"}
Copy link
Contributor

Choose a reason for hiding this comment

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

This example was pretty much retrofitted by dynamodb. If it's not too much trouble, we should probably slightly rewrite this - maybe we can do away with the "Table" concepts, make the operation just called operation, etc. Isn't TableList just the same as ObjectList pretty much?

"TableList":{"shape":"TableList"},
"ComplexObject":{"shape":"ComplexObject"},
"NumberList":{"shape":"NumberList"},
"BooleanList":{"shape":"BooleanList"},
"ObjectList":{"shape":"ObjectList"},
"NestedList":{"shape":"NestedList"},
"Boolean":{"shape":"BooleanMember"}
}
},
"TableDescription":{
Expand All @@ -49,6 +55,37 @@
"type":"list",
"member":{"shape":"TableDescription"}
},
"ComplexObject":{
"type":"structure",
"members":{
"ObjectMember":{"shape":"ComplexObject"},
"StringMember":{"shape":"StringMember"},
"NumberMember":{"shape":"NumberMember"},
"BooleanMember":{"shape":"BooleanMember"},
"ArrayMember":{"shape":"NumberList"},
"ObjectArrayMember":{"shape":"ObjectList"}
}
},
"StringMember": {"type":"string"},
"NumberMember":{"type":"integer"},
"BooleanMember":{"type":"boolean"},
"NumberList": {
"type":"list",
"member":{"shape":"NumberListMember"}
},
"NumberListMember":{"type":"integer"},
"ObjectList": {
"type":"list",
"member":{"shape":"ComplexObject"}
},
"BooleanList": {
"type":"list",
"member":{"shape":"BooleanMember"}
},
"NestedList": {
"type":"list",
"member":{"shape":"NumberList"}
},
"ResourceNotFoundException":{
"type":"structure",
"members":{
Expand Down
Loading