Introducing DynamoDB interception via SDK client class replacement#1495
Introducing DynamoDB interception via SDK client class replacement#1495aschenzle wants to merge 12 commits intoWebFuzzing:masterfrom
Conversation
…ing transactional operations and SQL like queries.
…ing transactional operations and SQL like queries.
…ing transactional operations and SQL like queries.
| Collections.sort(tableNames); | ||
| return tableNames; | ||
| } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Why is is defaulted to empty?
There was a problem hiding this comment.
Seemed like the best option, as we don't want the interception to throw exceptions and retuning null requires handling it. We are just getting less metadata about the interception for this particular request if for some weird reason it fails (Only case I can think this failing would be a dependency problem with another version of the SDK breaking API compatibility, not expected from AWS).
There was a problem hiding this comment.
Why don't you encapsulate this and throw a RuntimeException?
There was a problem hiding this comment.
Just committed a refactored version.
… updating a comment.
… handler cannot invoke DDB methods needed to obtain tablename. Extracted constants. Added Javadoc and improved comments.
| } catch (NoSuchMethodException ignored) { | ||
| // Ignored in the case we are calling for a batch request, it will be handled below in the next section | ||
| // Ignore as BatchGetItem requests do not have tableName and are handled by extractBatchTableNames. | ||
| return null; |
There was a problem hiding this comment.
shouldn't this throw an exception?
There was a problem hiding this comment.
it seems to be differently treated from extractBatchTableNames()
There was a problem hiding this comment.
Check the comment on extractTableNames method which is the entrypoint for both GetItem (single table request) and BatchGetItem (possibly multiple Tables). For all requests we first try to extract a singleTableName (this method) if it fails (as BatchGetItem doesn't have a tablesName method) and returns null we know it's a Batch request and we call extractBatchTableNames (line 207) to get a List of tables. This is tested on testBatchGetItem on the test suite.
There was a problem hiding this comment.
In fact this applies too all methods not only "gets"
2 participants
Intercepting main operations (Query, Scan, GetItem, BatchGetItems, PutItem, UpdateItem and Delete Item) for both sync and async DynamoDB SDK clients. The interception happens at the SDK level and I've verified it works via an integration test using Docker. There was a dependency conflict while bringing the AWS SDK through Maven, Netty versions would get mixed up for Redis end to end tests so I've made the decision to exclude Netty from Lettuce package and use the Netty version packaged on AWS SDK. All e2e tests are passing.
As this is my first PR I'm looking for exhaustive feedback to quickly learn any conventions I've might missed.