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

OneTable removes empty strings in nested objects #540

Open
brewencoded opened this issue Oct 7, 2024 · 4 comments
Open

OneTable removes empty strings in nested objects #540

brewencoded opened this issue Oct 7, 2024 · 4 comments
Labels
bug Something isn't working discussion Question or issue being discussed fixed Issue has been fixed in the repo

Comments

@brewencoded
Copy link

I noticed an issue with create/update methods where Model fields with type: Object that have empty strings are being removed unless you use the nulls: true param. For example:

// model
Test: {
  pk: { type: String, value: 'TEST#${testId}' },
  testId: { type: String, required: true },
  value: { type: Object, required: true }
}
// code
const model = table.getModel('Test');
await model.create({
  testId: 'test123',
  value: { a: 'a', b: '' }
});

would lead to an object in the table that looks like:

{
  "pk": {
    "S": "TEST#test123"
  },
  "testId": {
    "S": "test123"
  },
  "value": {
    "M": {
      "a": {
        "S": "a"
      },
    }
  }
}

The property b, ends up missing.

After reviewing the code in Model.js, I noticed the lines:
https://github.com/sensedeep/dynamodb-onetable/blob/main/src/Model.js#L1617-L1622
and
https://github.com/sensedeep/dynamodb-onetable/blob/main/src/Model.js#L1999-L2001
Which reference an old issue where DynamoDB did not support storing empty strings.

This is no longer the case as of 2020. I was using this feature in DynamoDB-Toolbox previously:
https://aws.amazon.com/about-aws/whats-new/2020/05/amazon-dynamodb-now-supports-empty-values-for-non-key-string-and-binary-attributes-in-dynamodb-tables/

At the time that code was written it may have been accurate, but now I would consider it a bug. I'm working around this with nulls: true and transform for now. Any chance we could get this code updated?

@mobsense
Copy link
Contributor

If you comment those lines of code out, does it perform correctly?

@brewencoded
Copy link
Author

I got around to creating a test that inserts an entity with an empty string field. When I comment out https://github.com/sensedeep/dynamodb-onetable/blob/main/src/Model.js#L1617-L1620 it successfully inserts the nested empty string.

@brewencoded
Copy link
Author

I'm not sure what the exact solution is, but I think handleEmpties should respect the DynamoDBDocumentClient's marshallOptions.convertEmptyValues and then apply the nulls attribute before writing.

@mobsense
Copy link
Contributor

We don't use the DynamoDBDocumentClient but go directly to the layer below.

Just committing a fix that uses:

if (this.table.params.legacyEmpties === true) {
    properties[name] = this.handleEmpties(field, value)
}

Users who need the old behavior can set "handleEmpties: true" in the schema.params.

mobsense pushed a commit that referenced this issue Oct 29, 2024
@mobsense mobsense added bug Something isn't working discussion Question or issue being discussed fixed Issue has been fixed in the repo labels Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion Question or issue being discussed fixed Issue has been fixed in the repo
Projects
None yet
Development

No branches or pull requests

2 participants