-
Notifications
You must be signed in to change notification settings - Fork 593
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
fix: do not report D1 user errors to Sentry #6192
Conversation
🦋 Changeset detectedLatest commit: 6ae256d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-wrangler-6192 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6192/npm-package-wrangler-6192 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-wrangler-6192 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-create-cloudflare-6192 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-cloudflare-kv-asset-handler-6192 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-miniflare-6192 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-cloudflare-pages-shared-6192 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9777722331/npm-package-cloudflare-vitest-pool-workers-6192 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
} | ||
); | ||
} catch (x) { | ||
if (x instanceof APIError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw the API endpoints now correctly throw a 400 Bad Request
when a user error occurs - might be worth specifically looking for that type of response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the way the cfetch code works it drops the HTTP status code by the time it generates an APIError
. All we get back is:
"result": [],
"success": false,
"errors": [
{
"code": 7500,
"message": "no such table: Customers"
}
],
"messages": []
}
When creating the APIError, we do grab the error code from the first error in the response. In the example above 7500.
We could consider fixing the cfetch to throw an APIError whenever we don't get a 2xx response but I worry that this might break other APIs.
What this PR solves / how to test
When we run
wrangler d1 execute
we can get fatal errors due to invalid SQL being sent to the remote D1 instances.These were being treated as bugs in Wrangler and were reported to Sentry.
This PR changes these errors so that they are not reported to Sentry.
Author has addressed the following