Skip to content
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.

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

BatchWriteRequest should not require modelClazz parameter in put method #342

Open
LouDou opened this issue Mar 20, 2021 · 1 comment
Open

Comments

@LouDou
Copy link

LouDou commented Mar 20, 2021

Is your feature request related to a problem? Please describe.

I don't really understand why, in particular, the BatchWriteRequest.put() method requires passing in the modelClazz argument before the array of models.

I would like to be able to pass in an array of mixed Model types, and have the class correctly determine where to write each one.

Describe the solution you'd like

It is quite straightforward to find the constructor from a JS object, using obj.constructor. Therefore, internally, the BatchWriteRequest class can determine the constructor object upon which to read metadata about the Model.

I've tested this alternative interface, and it works as such:

import * as DE from '@shiftcoders/dynamo-easy';

// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
class AutoBatchWrite extends DE.BatchWriteRequest {
  protected itemCount = 0;

  putMixed(models: unknown[]) {
    if (this.itemCount + models.length > DE.BATCH_WRITE_MAX_REQUEST_ITEM_COUNT) {
      throw new Error(`batch write takes at max ${DE.BATCH_WRITE_MAX_REQUEST_ITEM_COUNT} items`);
    }

    for (const model of models) {
      const meta = Reflect.getMetadata('sc-reflect:model', model.constructor);
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      const item = { PutRequest: { Item: DE.toDb(model, model.constructor) } };
      const tableName = meta.tableName;
      this.params.RequestItems[tableName] = this.params.RequestItems[tableName] || [];
      this.params.RequestItems[tableName].push(item);
    }
    this.itemCount += models.length;

    return this;
  }
}

Note that the ts/eslint comments are required here for this proof because:
a) the properties required to make this work are private inside BatchWriteRequest - this would be moot if this implementation was in BatchWriteRequest to start with
b) model.constructor for T = unknown type doesn't match ModelConstructor<T> - I'm not completely sure about the solution for this one. Particularly because there's no such thing as a base model type, and the type of object in the models array can be mixed.

Additional context

That's about it... I love this library, by the way. Even if this improvement is not considered, I can still make good use of it :)

Regards,
Doug.

@LouDou
Copy link
Author

LouDou commented Mar 20, 2021

Here's the full working PoC: https://github.com/LouDou/de-mm-poc

de-mm-poc$ yarn
de-mm-poc$ yarn start
$ curl -XPOST -H'Content-Type: application/json' -d'{}' http://localhost:3000/local/hello

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant