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

Add support for bigint attribute type #435

Merged
merged 7 commits into from
Feb 17, 2023

Conversation

tixxit
Copy link
Contributor

@tixxit tixxit commented Feb 1, 2023

This PR adds support for a new type: 'bigint' attribute type. It requires the DocumentClient to have wrapNumbers: true configured in the options, but otherwise allows use of integers > 53 bits.

I'm hoping there is interest for merging this type of feature! Currently it is using native bigint support, which means I've update the target to ES2019. Alternatively, we could use JSBI if you'd prefer to stick with 2015.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think covers the bases for support. my only question is around the DocumentClient requirements; if the user doesn't specify wrapNumbers: true and attempts to use a bigint, what happens?

@jeremydaly thoughts?

@tixxit
Copy link
Contributor Author

tixxit commented Feb 16, 2023

Right now, if wrapNumbers: false and an attribute uses type: 'bigint', then Entity construction will fail with an error message telling the developers to use wrapNumbers: true in their table's DocumentClient. Not super ideal, but there is no way to know if DocumentClient lost precision without it, so it would otherwise have silently lost precision.

@shellscape
Copy link
Collaborator

shellscape commented Feb 16, 2023

And there's a test present for that error, so that looks good. I don't have merge capabilities, so we'll have to wait for other maintainers to have a look. @tixxit looks like you may need rebase from upstream/origin to bring your branch up to date.

Copy link
Collaborator

@naorpeled naorpeled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
left a small comment but good stuff,
thanks for creating this awesome PR!

once rebased from master, will merge this in and release :)

package.json Outdated Show resolved Hide resolved
@shellscape
Copy link
Collaborator

@naorpeled do you have merge permissions? just curious if there's a collaborator that has merge permissions or we need to bug jeremy.

@naorpeled
Copy link
Collaborator

@naorpeled do you have merge permissions? just curious if there's a collaborator that has merge permissions or we need to bug jeremy.

Yeah, the PR can't be merged before rebased

@tixxit
Copy link
Contributor Author

tixxit commented Feb 17, 2023

I merged the main branch and removed the unneeded test:bigint script. Thanks for the reviews!

@naorpeled naorpeled merged commit 8c89d80 into jeremydaly:main Feb 17, 2023
@shellscape
Copy link
Collaborator

@tixxit something that may have been overlooked: when returning attributes from DynamoDB, aws-sdk returns a NumberValue as a result of the wrapNumbers: true setting. Consumers of this will likely expect to get a bigint rather than NumberValue - I did. Since all numbers are wrapped, this changes the expected output of Attributes on Item for all numbers. So while there is bigint support, the documentation needs to be updated for the bigint type to alert users that the type of number and bigint attributes will be the wrapped NumberValue. Consumers that don't go to the AWS documentation to read about what side effects wrapNumbers will have will be caught off guard, and that's not a good DX.

@tixxit
Copy link
Contributor Author

tixxit commented Mar 8, 2023

@shellscape Are you seeing this when using parse/autoparse/etc? We should be converting NumberValue to the proper type, number or bigint regardless. The raw data from the DocumentClient will return the wrapped values though. We've been using this in prod for a bit and it's been good!

@shellscape
Copy link
Collaborator

shellscape commented Mar 8, 2023

@tixxit here's the entity we're using:

const Event = new Entity({
  attributes: {
    createdAt: {
      default: () => new Date().toISOString(),
      onUpdate: false,
      required: true,
      type: 'string'
    },
    event: { partitionKey: true, type: 'string' },
    data: { required: true, type: 'map' },
    id: { default: () => nanoid(), required: true, type: 'string' },
    sequence: { default: () => getSequence(), sortKey: true, type: 'bigint' },
    type: { type: 'string' },
    version: { type: 'string' }
  },
  name: 'Event',
  table,
  timestamps: false
});

Here's how we're calling out to the db:

export const listEventsByName = async (eventName: string, options: ListEventOptions = {}) => {
  log.debug('listEventsByName:', { eventName, options });
  const { limit = 100 } = options;
  const result = await Event.query(eventName, { limit });

  result.Items = result.Items.map((item) => unwrap(item as EventEntity)) as any;

  return result as EventEntityList;
};

// Note: Because the aws-sdk v2 returns a `NumberValue` wrapper for bigint, we must unwrap the value
// so we can use bigint as per usual elsewhere
const unwrap = (entity: EventEntity) => {
  return { ...entity, sequence: BigInt(entity.sequence.toString()) };
};

In this scenario, we're getting the wrapped value for sequence for each item in Items. Perhaps there's a hole in the tests?

@shellscape
Copy link
Collaborator

@tixxit as a followup, we're continuing to see this in new places. namely the map field seen above, where the result Items contain data in the data field that resembles:

  oneTimeFee: {
    value: NumberValue { wrapperName: 'NumberValue', value: '3000' },
    currency: 'CAD'
  },

We've not changed the parsing settings, using the default, so something is definitely awry there

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

Successfully merging this pull request may close these issues.

None yet

3 participants