feat: remove DynamodbWrapper client dependency #32

lfantone · 2021-01-21T15:41:10Z

No description provided.

crash7 · 2021-01-21T17:50:22Z

src/with-paginator-helper.js

+    }
+  }
+
+  result.Items = items;


You may want to update the other fields that refer to the amount of items read (like count or consumed capacity)

Good catch, I forgot about the Count, ScannedCount and ConsumedCapacity

nfantone

Thanks for this! Left some comments here and there. The only bits and blobs I see missing are the batchWrite operations which would now behave differently after the removal on dynamodb-wrapper. Not really sold on keeping current functionality, though - so opening the discussion. We might as well cut a new major release and make flynamo be on par with the official AWS.DynamoDB client.

nfantone · 2021-01-21T17:47:22Z

src/get-all.js


+const DEFAULT_OPTIONS = {


I'd freeze this object and add docs to the props.

const DEFAULT_OPTIONS = Object.freeze({ ... });

nfantone · 2021-01-21T17:50:52Z

src/get-all.js

+  autopagination: true,
+  raw: false
+};
+const mergeWithDefaults = mergeRight(DEFAULT_OPTIONS);


This kinda works, but won't default to values in DEFAULT_OPTIONS for null, undefined or empty properties. Maybe remove those before merging?

const mergeWithDefaults = compose(mergeRight(DEFAULT_OPTIONS), rejectNilOrEmpty);

If the user put a nil value, why this should change it?

I can add the _ rejectNilOrEmpty_ but we don't have ramda-land here, do see that is necessary to add another dependency just for this?

nfantone · 2021-01-21T19:40:45Z

src/get-all.test.js

    const { getAll } = createAllGetter({ scan: mockScan });
    await getAll();
    expect(mockScan).toHaveBeenCalled();
  });

-  test('should return `undefined` if no items are returned', async () => {
-    const mockScan = jest.fn().mockResolvedValue({});
+  test('should return `[]` if no items are returned', async () => {


While I agree [] makes more sense, this would be a breaking change.

I revert this change to return undefined instead

nfantone · 2021-01-21T19:56:26Z

src/count.js


 /**
 * @private
 */
-const createCount = scan => pipeP(scan, prop('Count'));
+const createCount = scan => params => withPaginatorHelper(scan, params, true).then(prop('Count'));


I like the simplicity of what we had before. Maybe we could make the withPaginatorHelper return an async function that expects params? (see comment further below)

Suggested change

const createCount = scan => params => withPaginatorHelper(scan, params, true).then(prop('Count'));

const createCount = scan => compose(andThen(prop('Count')), withPaginator(scan));

nfantone · 2021-01-21T20:40:42Z

src/get-all.js

 /**
 * @private
 */
-const createGetAll = scan => pipeP(scan, unwrapAll('Items'));
+const createGetAll = scan => (params, options = {}) => {


Following the idea of having the paginator helper return a function accepting params, this could be turned into something like:

const createGetAll = scan => (params, options = {}) => { const { raw, autopagination } = mergeWithDefaults(options); const scanFn = autopagination ? withPaginator(scan) : scan; return scanFn(params).then( raw ? unwrapOverAll('Items') : unwrapAll('Items') ); };

nfantone · 2021-01-21T20:57:52Z

src/get.js

@@ -13,7 +13,8 @@ const addTableName = require('./table-name');
 /**
 * @private
 */
-const getUnwrappedItem = getItem => pipeP(apply(getItem), unwrapProp('Item'));
+const getUnwrappedItem = getItem =>
+  compose(andThen(unwrapProp('Item')), request => request.promise(), apply(getItem));


Since this is now used everywhere, what about adding some helpers including a swap in replacement for andThen that automatically invokes request.promise() first?

'use strict'; const R = require('ramda'); /** * Invokes `promise` on the given object passing * in no arguments. * * @param {object} obj The obj to invoke `promise` on. * @returns {Promise.<*>} A `Promise` as returned by a call to `promise`. */ const toPromise = R.invoker(0, 'promise') /** * Returns the result of applying an `fn` function to the value inside a fulfilled promise, * after having invoked `promise()` on the target object. * * @private * @see https://ramdajs.com/docs/#andThen * @param {Function} fn The function to apply. Can return a value or a promise of a value. */ const andThen = fn => compose( R.andThen(fn), toPromise ); module.exports = { toPromise, andThen };

nfantone · 2021-01-21T21:03:11Z

src/query.js

 const { unwrapAll, unwrapOverAll } = require('./wrapper');
 const addTableName = require('./table-name');
+const withPaginatorHelper = require('./with-paginator-helper');
+
+const DEFAULT_OPTIONS = {


Same as the scan comments above.

nfantone · 2021-01-21T21:09:45Z

src/with-paginator-helper.js

+  if (autopagination) {
+    while (result.LastEvaluatedKey) {
+      result = await methodFn({ ...params, ExclusiveStartKey: result.LastEvaluatedKey }).promise();
+      result.Items && result.Items.length >= 0 && items.push(...result.Items);


Suggested change

result.Items && result.Items.length >= 0 && items.push(...result.Items);

isNotNilOrEmpty(result.Items) && items.push(...result.Items);

nfantone · 2021-01-21T21:10:30Z

src/with-paginator-helper.js

+ * @param {boolean} autopagination Whether to return all the records from DynamoDB or just the first batch of records.
+ * @returns {Promise} A promise that resolves to the response from DynamoDB.
+ */
+async function withPaginatorHelper(methodFn, params, autopagination) {


I'd turn this into a factory function expecting an operationFn and returning another function that takes in only input as argument ("operation" and "input" as that's what the AWS.DynamoDB typings typically calls them). The autopagination bit seems redundant to me.

function withPaginator(operationFn) { async function autopaginate(input) { /* ... */ } return autopaginate; }

lfantone · 2021-01-22T14:36:28Z

What the wrapper is doing for those cases is to be able to do an unlimited amount of request to the database taking care of the 25 request limit that the aws-sdk client has.
So maybe it is a good idea to keep the wrapper client for those batch methods.

Thoughts?

github-actions · 2021-01-26T09:00:24Z

➿ Code coverage

Filename	Statements	Branches	Functions	Lines
src/with-paginator.js	100%	95.83%	100%	100%
src/update.js	100%	100%	100%	100%
src/wrapper.js	95%	100%	83.33%	100%
src/table-name.js	100%	100%	100%	100%
src/return-values.js	100%	100%	100%	100%
src/map-merge-args.js	100%	100%	100%	100%
src/cast-array.js	100%	100%	100%	100%
src/generate-key.js	100%	100%	100%	100%
src/and-then.js	100%	100%	100%	100%
src/builders/append.js	100%	100%	100%	100%
src/builders/catcon.js	100%	100%	100%	100%
src/builders/put.js	100%	100%	100%	100%
src/builders/update-expression.js	100%	100%	100%	100%
src/batch-get-item.js	100%	100%	100%	100%
src/get.js	100%	100%	100%	100%
src/query.js	100%	75%	100%	100%
src/remove.js	100%	100%	100%	100%
src/batch-write-item.js	100%	100%	100%	100%
src/generate-item.js	100%	100%	100%	100%
src/insert.js	100%	100%	100%	100%
src/builders/remove.js	100%	100%	100%	100%
src/get-all.js	100%	60%	100%	100%
src/count.js	100%	100%	100%	100%
src/describe-table.js	100%	100%	100%	100%

crash7 · 2024-04-14T20:27:21Z

What the wrapper is doing for those cases is to be able to do an unlimited amount of request to the database taking care of the 25 request limit that the aws-sdk client has. So maybe it is a good idea to keep the wrapper client for those batch methods.

Thoughts?

@lfantone we may need to drop the dependency because it's blocking the upgrade to aws-sdk v3. Maybe we can bring the implementation for the batch methods to flynamo itself?

lfantone · 2024-04-14T21:08:44Z

Yes, feel free to remove it

feat: remove DynamodbWrapper client dependency

4233c2c

lfantone self-assigned this Jan 21, 2021

lfantone added 5 commits January 21, 2021 18:04

fix: pass method fn instead of string

73f3fb0

chore: remove dynamodbwrapper dependency

af31af3

feat: remove deprecated pipeP

94abfc6

test: modify to use native dynamodb client

3216cac

chore: update type declaration

e66922a

lfantone marked this pull request as ready for review January 21, 2021 17:14

lfantone requested a review from nfantone as a code owner January 21, 2021 17:14

lfantone requested review from a team, crash7, ftschopp and ivelazco and removed request for a team January 21, 2021 17:14

lfantone added 2 commits January 21, 2021 18:20

chore: rename dynamoWrapper

b59162d

docs: remove old reference for wrapper

8f37a8f

crash7 reviewed Jan 21, 2021

View reviewed changes

nfantone requested changes Jan 21, 2021

View reviewed changes

lfantone added 2 commits January 22, 2021 15:24

feat: add aggregate function for counts and capacities

c5ac2c9

feat: update use of with-paginator

d21058d

lfantone added 3 commits January 22, 2021 16:11

test: remove only from test

fd41694

fix: compose scan/query methods with toPromise

70a2596

fix: add missing toPromise

f357ab6

crash7 marked this pull request as draft February 22, 2022 18:21

crash7 mentioned this pull request Apr 20, 2024

Remove dynamodb-wrapper dependency #59

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove DynamodbWrapper client dependency #32

feat: remove DynamodbWrapper client dependency #32

lfantone commented Jan 21, 2021

crash7 Jan 21, 2021

lfantone Jan 22, 2021

nfantone left a comment

nfantone Jan 21, 2021

nfantone Jan 21, 2021 •

edited

Loading

lfantone Jan 22, 2021

nfantone Jan 21, 2021 •

edited

Loading

lfantone Jan 22, 2021 •

edited

Loading

nfantone Jan 21, 2021 •

edited

Loading

nfantone Jan 21, 2021

nfantone Jan 21, 2021

nfantone Jan 21, 2021

nfantone Jan 21, 2021

nfantone Jan 21, 2021

lfantone commented Jan 22, 2021

github-actions bot commented Jan 26, 2021

crash7 commented Apr 14, 2024

lfantone commented Apr 14, 2024

	const createCount = scan => params => withPaginatorHelper(scan, params, true).then(prop('Count'));
	const createCount = scan => compose(andThen(prop('Count')), withPaginator(scan));

	result.Items && result.Items.length >= 0 && items.push(...result.Items);
	isNotNilOrEmpty(result.Items) && items.push(...result.Items);

feat: remove DynamodbWrapper client dependency #32

Are you sure you want to change the base?

feat: remove DynamodbWrapper client dependency #32

Conversation

lfantone commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfantone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfantone Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfantone Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

lfantone Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

nfantone Jan 21, 2021 • edited Loading

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

lfantone commented Jan 22, 2021

github-actions bot commented Jan 26, 2021

crash7 commented Apr 14, 2024

lfantone commented Apr 14, 2024

nfantone Jan 21, 2021 •

edited

Loading

nfantone Jan 21, 2021 •

edited

Loading

lfantone Jan 22, 2021 •

edited

Loading

nfantone Jan 21, 2021 •

edited

Loading