string Attributes are not allowed to be empty

[BryanCrotaz]

API limitation

[brigand]

Have a minimal repro case? wrap({x: ''}) gives { x: { S: '' } } for me.

[brigand]

Oh, I see what you mean, it shouldn't allow empty strings.

[BryanCrotaz]

Yes - API doesn't allow empty strings.
http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_PutItem.html
We've made a fix in our fork, testing it on our system at the moment.

On 6 April 2016 at 14:30, Frankie Bagnardi notifications@github.com wrote:

Oh, I see what you mean, it shouldn't allow empty strings.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#7 (comment)

Bryan Crotaz
Managing Director
Silver Curve

[kayomarz]

The library avoids doing data validations. Invalid data in a DynamoDB request results in an error response thereby indicating the invalid data. Could you make use of the error response to capture invalid data?

Including data checks/validations in this library would help detect invalid data earlier - i.e. before making an HTTP API request. However in that case the library would need to validate data exactly as DynamoDB does - else the feature might not be worth having.

Does your use case need data validation to be done before making a DynamoDB API call?

ps: There was a line in the docs mentioning that this library doesn't perform such checks. This line currently seems to be missing in the docs.

[kayomarz]

@BryanCrotaz thanks for the pull request and test case.

As you mentioned, this library detects an empty string as { S: '' }. I would like to keep this behaviour intact.

The idea is to let this library do nothing more than help represent your data. If the data is invalid it would be better to let the application handle it.

Nonetheless, to attempt to do what you require, you can now make use of a hook. The hook reconcileFn gives an opportunity to the application to reconcile data so that it is valid for DynamoDb.

Suppose your application needs an empty string to be detected as {NULL: true} it can now be done as follows:

var dynamoDdDataTypes = require('dynamodb-data-types');
dynamoDdDataTypes.setGlobalOption({reconcileFn: dynamoDdDataTypes.reconcile})

See examples/05-reconcile-data.js for a full example.

The reconcile function gives the application an opportunity to reconcile data. A custom reconcile function may be used instead of lib.reconcile

This change is only in git, not yet moved to the NPM repo.

Let me know what you think. I hope this is useful for your use case.

@brigand would be great if you have any suggestions or alternate way to do this.

Thanks.

[BryanCrotaz]

The thing is that all applications need this - Dynamo will never accept {S: ''} - we're putting a burden on developers that doesn't need to be there

[BryanCrotaz]

I'd be happy with a compromise of making lib.reconcile the default behaviour - developers should not need to specify this every time.

[kayomarz]

If setting the global var seems an unnecessary step, another solution would
be to keep existing functions wrap() and wrap1() as they are and create two
new functions to address this discussion. Say vWrap() and vWrap1() to make
valid and then wrap; or any other appropriate names, maybe cWrap()
andcWrap1() for coerce and wrap.

This way, both approaches are supported.
On 10-Apr-2016 6:52 pm, "Bryan" notifications@github.com wrote:

I'd be happy with a compromise of making lib.reconcile the default
behaviour - developers should not need to specify this every time.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#7 (comment)

[BryanCrotaz]

I'm mostly just concerned that this should be a library that "just works". The default out of the box behaviour should enable the user to load and save data to and from Dynamo without having to think about it.

[BryanCrotaz]

When as a developer would you choose not to use vwrap / cwrap?

[kayomarz]

My point is to maintain separation of concerns. This library should only be
concerned with data representation.

To me, some reasons not to convert an Empty string into null are the
following:

• as a developer, if dynamoDB doesn't allow empty strings I would like to
  get an error and know about it.
• if data contains an Empty string, an application might expect to read
  back the same data (an empty string)/as far as possible. The fact that an
  empty string is illegal is something better handled by a prior step to
  writing data.
• it might help to think about why dynamoDB does not silently convert an
  Empty string into null.

Maybe its better to let the application decide what to do if an empty
string appears.

Ideally if you want to convert an Empty string into null before writing it
to DynamoDB, I would do it as:

Coerce(my data); // do stuff such as covert empty string to null
Wrap(coerced data); // what wrap() currently does

This is just my approach.

Maybe having vWrap() separate from wrap() is a way to let both approaches
co-exist.

Besides, changing the behaviour of wrap() might break applications using
this lib because its difficult presume how applications might have used it.
On 11-Apr-2016 10:39 pm, "Bryan" notifications@github.com wrote:

When as a developer would you choose not to use vwrap / cwrap?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#7 (comment)

[brigand]

The problem is that unwrap(wrap(x)) should return a clone of x. That said, it probably makes the most sense to not create invalid wrapped data.

Another option would be to create a custom structure for representing empty strings. For example, you could convert '' to {S: '6b8587a0-0ee5-4759-b4a5-600f30d785f8'} (a static uuid). Then unwrap can check for that exact uuid and convert it to an empty string. It seems pretty weird to do this, and you'd need to handle it specially if accessing dynamodb without this library, so I'm not sure if it's a good solution. It would preserve unwrap(wrap(x)) giving x. This would be a good candidate for an alternative method name.

[kayomarz]

Your uuid approach is interesting. Using it to overcome restrictions would
be a plus to have.
On 12-Apr-2016 12:26 am, "Frankie Bagnardi" notifications@github.com
wrote:

The problem is that unwrap(wrap(x)) should return a clone of x. That
said, it probably makes the most sense to not create invalid wrapped data.

Another option would be to create a custom structure for representing
empty strings. For example, you could convert '' to {S:
'6b8587a0-0ee5-4759-b4a5-600f30d785f8}' (a static uuid). Then unwrap can
check for that exact uuid and convert it to an empty string. It seems
pretty weird to do this, and you'd need to handle it specially if accessing
dynamodb without this library, so I'm not sure if it's a good solution. It
would preserve unwrap(wrap(x)) giving x. This would be a good candidate
for an alternative method name.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#7 (comment)

[bfsmith]

Just ran into this too and saw the reconcile change was reverted. I ended up writing some code to remove empty strings from my objects before putting them into .wrap(). Just thought I'd leave it for the next passer by.

const clean = (value) =>
  Object.getOwnPropertyNames(value)
    .reduce((o, k) => {
      const v = value[k];
      if (typeof (v) !== 'string' || v !== '') {
        if (typeof (v) === 'object') {
          o[k] = clean(v);
        } else {
          o[k] = v;
        }
      }
      return o;
    }, {});

[juanr2001]

@bfsmith this is late response, but you can use hapi joi to validate use inputs. https://hapi.dev/family/joi/?v=16.1.7