dynamo_request_parser: adding support for transactions by hallie · Pull Request #7244 · envoyproxy/envoy

hallie · 2019-06-12T03:19:04Z

Description:

Adds check for new types of dynamodb operations
- TransactWriteItems
- TransactGetItems
Adds awareness for three new types of dynamodb errors
- IdempotentParameterMismatchException
- TransactionCanceledException
- TransactionInProgressException

Risk Level: Low
Testing: mostly just planning to copy existing tests for BATCH_OPERATIONS
Docs Changes: None
Release Notes:
Fixes #7240

Signed-off-by: Hallie Lomax <hallie@lyft.com> missing colon Signed-off-by: Hallie Lomax <hallie@lyft.com> missing colon.. Signed-off-by: Hallie Lomax <hallie@lyft.com> adding test Signed-off-by: Hallie Lomax <hallie@lyft.com> another missing colon Signed-off-by: Hallie Lomax <hallie@lyft.com> attempt format fix Signed-off-by: Hallie Lomax <hallie@lyft.com> one more Signed-off-by: Hallie Lomax <hallie@lyft.com> fixing json Signed-off-by: Hallie Lomax <hallie@lyft.com>

source/extensions/filters/http/dynamo/dynamo_request_parser.cc

Signed-off-by: Hallie Lomax <hallie@lyft.com>

junr03 · 2019-06-18T19:47:36Z

@hallie thanks for working on this! Do you mind merging in master? I will do a pass now.

hallie · 2019-06-18T20:34:25Z

@junr03 will do!

Signed-off-by: Hallie Lomax <hallie@lyft.com>

junr03

Thanks for contributing @hallie! A few comments

source/extensions/filters/http/dynamo/dynamo_request_parser.h

junr03 · 2019-06-18T20:46:08Z

test/extensions/filters/http/dynamo/dynamo_request_parser_test.cc

 }

+TEST(DynamoRequestParser, parseTableNameTransactOperation) {
+  std::vector<std::string> supported_transact_operations{"TransactGetItems", "TransactWriteItems"};


You could use the statically defined constant vector RequestParser::TRANSACT_OPERATIONS to prevent drift, no?

so this list is private, meaning i can't call it outside of the class. and all the other operations vectors are private as well, meaning that i'd either have to convert them all to public or not follow the set convention, so i'm just reverting back to having a separate list in the test (which is what the other tests for this file do)

junr03 · 2019-06-18T20:48:04Z

source/extensions/filters/http/dynamo/dynamo_request_parser.cc

    // 4xx
    "AccessDeniedException",
    "ConditionalCheckFailedException",
+    "IdempotentParameterMismatchException",


Can we add tests for the failure scenarios that lead to these type of errors?

Judging from the implementation of the function that uses this list, its just checking if this error name exists in the error string returned from dynamo. And the only tests for that function are passing a string with one of the error names in it, rather than actually mimicking the error itself. So is this necessary to add?

junr03 · 2019-06-18T23:07:57Z

/wait

Signed-off-by: Hallie Lomax <hallie@lyft.com>

stale · 2019-07-01T01:50:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

hallie · 2019-07-01T03:51:32Z

@junr03 could I get another review? :0

junr03

Thanks @hallie for the changes. @zuercher can you take a last pass?

junr03 · 2019-07-01T17:56:44Z

source/extensions/filters/http/dynamo/dynamo_request_parser.cc

    for (const Json::ObjectSharedPtr& transact_item : transact_items) {
-      next_table_name = getTableNameFromTransactItem(*transact_item);
-      if (next_table_name.empty()) {
+      auto next_table_name = getTableNameFromTransactItem(*transact_item);


convention for the envoy codebase is to explicitly add the type here. Also I would add const here as you don't expect to change the table name.

I think it's ok to make this const auto.

zuercher

Looks good. Let's switch the one type to const, and then I'll approve.

zuercher · 2019-07-02T17:06:21Z

source/extensions/filters/http/dynamo/dynamo_request_parser.cc

    for (const Json::ObjectSharedPtr& transact_item : transact_items) {
-      next_table_name = getTableNameFromTransactItem(*transact_item);
-      if (next_table_name.empty()) {
+      auto next_table_name = getTableNameFromTransactItem(*transact_item);


I think it's ok to make this const auto.

@hallie

Agree with @hallie that the existing tests are sufficient.

Signed-off-by: Hallie Lomax <hallie@lyft.com>

hallie · 2019-07-02T17:20:29Z

@zuercher updated! :D

Signed-off-by: Hallie Lomax <hallie@lyft.com>

hallie force-pushed the adding-support-for-transaction branch from 60dc23c to a956054 Compare June 12, 2019 03:20

hallie changed the title ~~adding support for transactions to dynamo_request_parser~~ dynamo_request_parser: adding support for transactions Jun 12, 2019

mattklein123 assigned junr03 Jun 12, 2019

hallie force-pushed the adding-support-for-transaction branch from 89108a5 to 7746282 Compare June 12, 2019 16:35

hallie commented Jun 12, 2019

View reviewed changes

source/extensions/filters/http/dynamo/dynamo_request_parser.cc Outdated Show resolved Hide resolved

hallie marked this pull request as ready for review June 12, 2019 20:16

hallie added 9 commits June 12, 2019 13:26

fix format

88bff56

Signed-off-by: Hallie Lomax <hallie@lyft.com>

confirming single table transaction by parsing request

9708283

Signed-off-by: Hallie Lomax <hallie@lyft.com>

fixing tidy

f09f1bf

Signed-off-by: Hallie Lomax <hallie@lyft.com>

konmari

4723bf9

Signed-off-by: Hallie Lomax <hallie@lyft.com>

get object instead of pointer

74fd22d

Signed-off-by: Hallie Lomax <hallie@lyft.com>

passed in wrong variable

da015df

Signed-off-by: Hallie Lomax <hallie@lyft.com>

change function name and deref pointer

193e32c

Signed-off-by: Hallie Lomax <hallie@lyft.com>

from json to pointer

3e6fec0

Signed-off-by: Hallie Lomax <hallie@lyft.com>

tidy

e1dc082

Signed-off-by: Hallie Lomax <hallie@lyft.com>

Merge branch 'master' into adding-support-for-transaction

23adac4

Signed-off-by: Hallie Lomax <hallie@lyft.com>

junr03 previously requested changes Jun 18, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jun 18, 2019

reset back to working build and reattempt optional table name

5988ebc

Signed-off-by: Hallie Lomax <hallie@lyft.com>

hallie force-pushed the adding-support-for-transaction branch from b60e605 to 5988ebc Compare June 23, 2019 18:29

hallie added 5 commits June 23, 2019 11:39

forgot to make the table_name optional

363ab0d

Signed-off-by: Hallie Lomax <hallie@lyft.com>

std::move

43a7619

Signed-off-by: Hallie Lomax <hallie@lyft.com>

remove move

bb13023

Signed-off-by: Hallie Lomax <hallie@lyft.com>

remove make_optional?

f8d3181

Signed-off-by: Hallie Lomax <hallie@lyft.com>

add default value

d5f4712

Signed-off-by: Hallie Lomax <hallie@lyft.com>

std::string

75f08c5

Signed-off-by: Hallie Lomax <hallie@lyft.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 1, 2019

junr03 reviewed Jul 1, 2019

View reviewed changes

junr03 assigned zuercher Jul 1, 2019

zuercher reviewed Jul 2, 2019

View reviewed changes

const auto

c53d259

Signed-off-by: Hallie Lomax <hallie@lyft.com>

Merge branch 'master' into adding-support-for-transaction

0fa0230

Signed-off-by: Hallie Lomax <hallie@lyft.com>

zuercher approved these changes Jul 2, 2019

View reviewed changes

mattklein123 merged commit 80194c9 into envoyproxy:master Jul 3, 2019

junr03 mentioned this pull request Jul 10, 2019

docs: cleanup for 1.11 release #7522

Merged

Conversation

hallie commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

junr03 commented Jun 18, 2019

Uh oh!

hallie commented Jun 18, 2019

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 18, 2019

Uh oh!

stale bot commented Jul 1, 2019

Uh oh!

hallie commented Jul 1, 2019

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hallie commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hallie commented Jun 12, 2019 •

edited

Loading