Add TransactWriteItems #174

ryanblock · 2023-09-03T21:10:40Z

Thank you for helping out! ✨

We really appreciate your commitment to improving Dynalite

To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:

Forked the repo and created your branch from main
Made sure tests pass (run npm it from the repo root)
Expanded test coverage related to your changes:
- Added and/or updated unit tests (if appropriate)
- Added and/or updated integration tests (if appropriate)
Updated relevant documentation
Summarized your changes in changelog.md
Linked to any related issues, PRs, etc. below that may relate to, consume, or necessitate these changes

Please also be sure to completed the CLA (if you haven't already).

Learn more about contributing to Architect here.

Thanks again!

…ransactWriteItems

…etes

…ransactWriteItems � Conflicts: � package-lock.json

Update readme, links

Testing locally, all these took <1s to run

Add some verbose logging

… interface Update readme

…moDB

CLAassistant · 2023-09-03T21:10:46Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 4 committers have signed the CLA.

✅ ryanblock
❌ n0286293
❌ tume
❌ jpeterschmidt

n0286293 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dimaqq

Minor comments mostly

dimaqq · 2023-09-04T01:01:21Z

test/helpers.js

+  request(opts(target, data), function (err, res) {
+    if (err) return done(err)
+    if (typeof res.body !== 'object') {
+      return done(new Error('Not JSON: ' + res.body))


super-minor: I'd use a template literal instead

dimaqq · 2023-09-04T01:02:59Z

validations/transactWriteItems.js

+              children: 'AttrStruct<ValueStruct>',
+            },
+            ExpressionAttributeNames: {
+              type: 'Map<java.lang.String>',


Is a Java reference an artefact of the validation library?
It's odd to see it here.

DynamoDB's java leaks through in its validation, I think this is expected.

dimaqq · 2023-09-04T01:03:41Z

validations/transactWriteItems.js

+              children: 'AttrStruct<ValueStruct>',
+            },
+            ExpressionAttributeNames: {
+              type: 'Map<java.lang.String>',


dimaqq · 2023-09-04T01:06:33Z

validations/transactWriteItems.js

+        msg = validations.validateAttributeValue(request.Put.Item[key])
+        if (msg) return msg


subject to style debate:

Suggested change

msg = validations.validateAttributeValue(request.Put.Item[key])

if (msg) return msg

if (msg = validations.validateAttributeValue(request.Put.Item[key]))

return msg

Those inline assignments within conditionals are clever but are also imo a bit unusual, and certainly more difficult to debug later. They also blow up eslint:recommended on no-cond-assign.

dimaqq · 2023-09-04T01:10:07Z

validations/transactWriteItems.js

+  var i, request, msg, key
+  for (i = 0; i < data.TransactItems.length; i++) {
+    request = data.TransactItems[i]


subject to style debate, or maybe performance?

I'd rather move the variable declaration into the loop:

Suggested change

var i, request, msg, key

for (i = 0; i < data.TransactItems.length; i++) {

request = data.TransactItems[i]

for (const request of data.TransactItems) {

let msg;

dimaqq · 2023-09-04T01:15:13Z

test/transactWriteItem.js

+      var requests = [], i
+      for (i = 0; i < 26; i++) {
+        requests.push(i % 2 ? { Delete: { Key: { a: { S: String(i) } } } } : { Put: { Item: { a: { S: String(i) } } } })
+      }


Maybe something like this:

Suggested change

var requests = [], i

for (i = 0; i < 26; i++) {

requests.push(i % 2 ? { Delete: { Key: { a: { S: String(i) } } } } : { Put: { Item: { a: { S: String(i) } } } })

}

const requests = new Array(26).fill(null).map(() => ({...}))

dimaqq · 2023-09-04T01:16:37Z

test/transactWriteItem.js

+  assertTransactionCanceled = helpers.assertTransactionCanceled.bind(null, target),
+  assertNotFound = helpers.assertNotFound.bind(null, target)
+
+describe('transactWriteItem', function () {


This file really needs a review, specifically that correct exceptions are raised in corner cases, in a way that dynalite behaviour matches AWS dynamodb.

I'm flagging it here because GitHub hides this diff by default calling it "too large".

ryanblock · 2023-09-04T17:46:27Z

actions/transactWriteItems.js

+    async.each(operationsbyTable, function ([ tableName, ops ], callback) {
+      var itemDb = store.getItemDb(tableName)
+
+      store.getTable(tableName, function (err, table) {


I think we need to dig into this @jpeterschmidt – do you recall what was intended here? It looks like the callback can be called in a variety of different ways and may be called multiple times.

For example, the callback on L29 may be called many times across the indexUpdates array, and will not cancel operations. And then itemDb.batch on L32 may also be calling the same callback at the same time. Moreover, if there's an error it will call said callback twice, with the results in the error position of the errback.

ryanblock · 2023-09-04T17:48:52Z

validations/transactWriteItems.js

+              children: 'AttrStruct<ValueStruct>',
+            },
+            ExpressionAttributeNames: {
+              type: 'Map<java.lang.String>',


DynamoDB's java leaks through in its validation, I think this is expected.

ryanblock · 2023-09-04T18:01:02Z

validations/transactWriteItems.js

+  var i, request, msg, key
+  for (i = 0; i < data.TransactItems.length; i++) {
+    request = data.TransactItems[i]


ryanblock · 2023-09-04T18:04:53Z

validations/transactWriteItems.js

+        msg = validations.validateAttributeValue(request.Put.Item[key])
+        if (msg) return msg


Those inline assignments within conditionals are clever but are also imo a bit unusual, and certainly more difficult to debug later. They also blow up eslint:recommended on no-cond-assign.

jpshack-at-palomar · 2024-02-24T10:18:06Z

@ryanblock What is the must fix here to get this merged? If we are just a few hours of work away I'd be happy pitch in.

ryanblock · 2024-02-24T16:36:30Z

@jpshack-at-palomar please read my comments above; this PR has fundamental issues with how continuation passing works, and it cannot be merged in its current state. As a programmer, I do not feel confident making claims that anything is a few hours of work away, let alone something as complex as this.

I was hoping the author would help us get this PR into shape, but since we haven't heard back my plan has been to hopefully snag a few bits here and there, but probably rewrite. As ever, I'd be happy for someone in the community to start work on a fresh PR.

tume and others added 30 commits April 14, 2020 14:21

Add initial transactWriteItem implementation

c2b1520

Merge branch 'master' of https://github.com/mhart/dynalite into add_t…

1b2b329

…ransactWriteItems

start working on TransactWriteItem tests

6caa634

implement atomic transactions

ff3faa5

implement ConsumedCapacity

c2d001f

fix updateItem exports

1514097

tests++

e00f8f4

implement ConditionCheck

c1d7814

actually release item locks after transaction completes

dd2e829

update indexes and test that that actually happens

e8149c2

remove console log debug statements

d427708

add new line to end of db/index.js file

cfc381c

fix transactWriteItems ignoring condition expressions in Puts and Del…

53e4733

…etes

Merge branch 'master' of https://github.com/mhart/dynalite into add_t…

eaf9535

…ransactWriteItems � Conflicts: � package-lock.json

Update CI, add CoC, issue templates, etc.

223dab0

Update package.json metadata

b89c8fb

Remove package-lock.json

623cf28

Update license to Apache 2.0 for future versions

ac5c635

Update Mocha, resolve inconsequential cve warnings

a5c1de5

Bump minimist, aws4

4b0a140

Bump big.js

817b2f3

Bump async

6d150eb

Bump Level deps (levelup, leveldown, memdown, subleveldown)

d6f0d34

Update package.json keywords, add engines field, add changelog

a4220fb

Update readme, links

Add eslint

f1099df

Less shouty readme

e745ccc

Uncomment tests that appear to perform quickly

173b15c

Testing locally, all these took <1s to run

Add --verbose, -v, --debug, -d flags, and -h help alias

3c8b9fc

Add some verbose logging

Tidy up -v|d CLI flags

8a2fa9a

Add --host / options.host option for specifying a particular host…

10ed925

… interface Update readme

ryanblock added 3 commits September 3, 2023 13:39

Add a bit more logging

bff0ae2

Wrap slow tests in a SLOW_TESTS env var when running on remote Dyna…

cecc3e1

…moDB

Update lint rules

246f021

Merge branch 'main' into libertymutual-add_transactWriteItems

a53e219

ryanblock mentioned this pull request Sep 3, 2023

Add TransactWriteItems #143

Open

dimaqq reviewed Sep 4, 2023

View reviewed changes

ryanblock commented Sep 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TransactWriteItems #174

Add TransactWriteItems #174

ryanblock commented Sep 3, 2023

CLAassistant commented Sep 3, 2023 •

edited

Loading

dimaqq left a comment

dimaqq Sep 4, 2023

dimaqq Sep 4, 2023

ryanblock Sep 4, 2023

dimaqq Sep 4, 2023

dimaqq Sep 4, 2023

ryanblock Sep 4, 2023

dimaqq Sep 4, 2023

ryanblock Sep 4, 2023

dimaqq Sep 4, 2023

dimaqq Sep 4, 2023

ryanblock Sep 4, 2023

ryanblock Sep 4, 2023

ryanblock Sep 4, 2023

ryanblock Sep 4, 2023

jpshack-at-palomar commented Feb 24, 2024

ryanblock commented Feb 24, 2024

		msg = validations.validateAttributeValue(request.Put.Item[key])
		if (msg) return msg

Add TransactWriteItems #174

Are you sure you want to change the base?

Add TransactWriteItems #174

Conversation

ryanblock commented Sep 3, 2023

Thank you for helping out! ✨

We really appreciate your commitment to improving Dynalite

CLAassistant commented Sep 3, 2023 • edited Loading

dimaqq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpshack-at-palomar commented Feb 24, 2024

ryanblock commented Feb 24, 2024

CLAassistant commented Sep 3, 2023 •

edited

Loading