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

Throw an error for KV expirations above the maximum 32-bit signed integer #487

Merged
merged 5 commits into from
Feb 7, 2023

Conversation

huw
Copy link
Contributor

@huw huw commented Feb 2, 2023

Mostly self-explanatory, but a couple of thoughts:

  1. I didn’t check whether cacheTtl on get is also subject to this limit.
  2. I didn’t match the error to Cloudflare’s one, which is TypeError: Value out of range. Must be between -2147483648 and 2147483647 (inclusive).. My assumption is that Cloudflare are directly throwing a database error and this could be subject to change in the future. But if you want the errors to match let me know ^_^
  3. I didn’t split out the message from the existing range ones, but can also do this if you’d like

You have more important things to do than edit this PR, so just let me know what changes you’d like and I will implement them ❤️

Resolves #485

@changeset-bot
Copy link

changeset-bot bot commented Feb 2, 2023

⚠️ No Changeset found

Latest commit: a153bf4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@huw
Copy link
Contributor Author

huw commented Feb 2, 2023

Sorry mate, I’ll throw on a changeset in a touch.

@mrbbot
Copy link
Contributor

mrbbot commented Feb 2, 2023

Hey! Thanks for the PR! Don't worry about the changeset. It's currently broken for this repository. If you could update this PR with the exact error messages though, that would be amazing! 🤩

@huw
Copy link
Contributor Author

huw commented Feb 3, 2023

I triple-checked how Cloudflare was throwing and it was definitely just throwing raw TypeErrors, including for highly negative numbers. So I adjusted the order of operations a bit, too. Probably worth reporting internally because they don’t throw any sugar with it.

Worth noting that wrangler kv:key put --binding=KV "test" "test" --expiration 99999999999999999999 returns:

✘ [ERROR] A request to the Cloudflare API (/accounts/ab7f41b44c7d2dae5b0b6de1a92da9b3/storage/kv/namespaces/eb30ab7762214404abe12084e87a0d13/values/test) failed.

  could not parse expiration argument as integer: 100000000000000000000 [code: 10033]

Which is also missing the usual syntactic sugar when running through Wrangler—as well as rounding incorrectly!

Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you! 😃

@mrbbot mrbbot merged commit 25a3ef9 into cloudflare:master Feb 7, 2023
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.

KV expiration & TTLs can only be 32-bit signed integers in practice
2 participants