Update R2 Error Handling for Invalid Characters #5032

dbenCF · 2024-02-16T13:31:41Z

Fixes # [insert GH or internal issue number(s)].

What this PR solves / how to test:
Adds client side error handling for creating an R2 bucket with invalid characters
Author has addressed the following:

Tests
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- Included
- Not necessary because:
Associated docs
- Issue(s)/PR(s):R2: Update create-buckets.md cloudflare-docs#14355
- Not necessary because:

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

changeset-bot · 2024-02-16T13:31:44Z

🦋 Changeset detected

Latest commit: 3981253

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@cloudflare/vitest-pool-workers	Patch

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

github-actions · 2024-02-19T12:52:07Z

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/9889399659/npm-package-wrangler-5032

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5032/npm-package-wrangler-5032

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-wrangler-5032 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-create-cloudflare-5032 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-cloudflare-kv-asset-handler-5032

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-miniflare-5032

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-cloudflare-pages-shared-5032

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9889399659/npm-package-cloudflare-vitest-pool-workers-5032

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240701.0
`workerd`	1.20240701.0	1.20240701.0
`workerd --version`	1.20240701.0	2024-07-01

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

penalosa · 2024-02-20T10:59:08Z

.changeset/brown-ties-deliver.md

@@ -0,0 1,5 @@
+---
+"wrangler": major


This should be a patch/minor bump, not a major

Thanks for flagging, changed to patch.

packages/wrangler/src/r2/helpers.ts

codecov · 2024-02-29T17:12:48Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 70.36%. Comparing base (65da40a) to head (f8147db).
Report is 486 commits behind head on main.

❗ Current head f8147db differs from pull request most recent head 08c4258. Consider uploading reports for the commit 08c4258 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5032       /-   ##
==========================================
- Coverage   70.64%   70.36%   -0.29%     
==========================================
  Files         292      298        6     
  Lines       15178    15518      340     
  Branches     3860     3985      125     
==========================================
  Hits        10723    10919      196     
- Misses       4455     4599      144

Files	Coverage Δ
packages/wrangler/src/r2/index.ts	`86.62% <100.00%> (-0.28%)`	⬇️
packages/wrangler/src/r2/helpers.ts	`88.50% <80.00%> (-0.39%)`	⬇️

... and 64 files with indirect coverage changes

penalosa

This looks good! The only thing remaining is an update to the docs—it looks like you checked off the checkbox for this including docs, but didn't link the issue/PR. Would you be able to do that, and then we can get this merged and released?

penalosa · 2024-03-19T09:43:26Z

packages/wrangler/src/r2/helpers.ts

+ name: string | undefined
+): name is string {
+ return (
+ typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-]*$/.test(name)


Suggested change

typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-]*$/.test(name)

typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-] $/.test(name)

Can a bucket not have a single character for its name?

petebacondarwin · 2024-02-19T14:25:31Z

.changeset/brown-ties-deliver.md

@@ -0,0 1,5 @@
+---
+"wrangler": major


Suggested change

"wrangler": major

"wrangler": patch

Since this would fail anyway, with a worse message, I don't think this is a breaking change requiring a major bump.

petebacondarwin · 2024-07-11T09:47:46Z

packages/wrangler/src/r2/helpers.ts

+ name: string | undefined
+): name is string {
+ return (
+ typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-]*$/.test(name)


Can a bucket not have a single character for its name?

petebacondarwin · 2024-07-11T10:19:54Z

I ran the E2E tests locally.

* Adding skeleton for error handling in R2 * Add test and update error handling * fix function errors * Add changeset * Updating changeset * Edit changeset file * Remove unecessary blankspace * fix up tests --------- Co-authored-by: Peter Bacon Darwin <[email protected]>

dbenCF requested a review from a team as a code owner February 16, 2024 13:31

penalosa reviewed Feb 20, 2024

View reviewed changes

penalosa added the awaiting reporter response Needs clarification or followup from OP label Feb 20, 2024

dbenCF changed the title ~~Daniel/pcx 9673~~ Update R2 Error Handling for Invalid Characters Feb 22, 2024

admah removed the awaiting reporter response Needs clarification or followup from OP label Feb 29, 2024

dbenCF requested a review from penalosa March 19, 2024 09:41

penalosa reviewed Mar 19, 2024

View reviewed changes

petebacondarwin self-assigned this Jul 11, 2024

dbenCF added 6 commits July 11, 2024 10:46

Adding skeleton for error handling in R2

4b93730

Add test and update error handling

a9dafcf

fix function errors

8aba26e

Add changeset

3dead46

Updating changeset

58507bd

Edit changeset file

d08c030

petebacondarwin force-pushed the daniel/PCX-9673 branch from 08c4258 to 07d7569 Compare July 11, 2024 09:48

petebacondarwin approved these changes Jul 11, 2024

View reviewed changes

Remove unecessary blankspace

4c5b06d

petebacondarwin force-pushed the daniel/PCX-9673 branch from 07d7569 to 4c5b06d Compare July 11, 2024 09:51

fix up tests

3981253

petebacondarwin merged commit 75f7928 into cloudflare:main Jul 11, 2024
18 checks passed

workers-devprod mentioned this pull request Jul 11, 2024

Version Packages #6241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update R2 Error Handling for Invalid Characters #5032

Update R2 Error Handling for Invalid Characters #5032

dbenCF commented Feb 16, 2024 •

edited by petebacondarwin

Loading

changeset-bot bot commented Feb 16, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading

penalosa Feb 20, 2024

dbenCF Feb 22, 2024

codecov bot commented Feb 29, 2024 •

edited

Loading

penalosa left a comment

penalosa Mar 19, 2024

petebacondarwin Jul 11, 2024

petebacondarwin Feb 19, 2024

petebacondarwin Jul 11, 2024

petebacondarwin commented Jul 11, 2024

	typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-]*$/.test(name)
	typeof name === "string" && /^[a-zA-Z][a-zA-Z0-9-] $/.test(name)

Update R2 Error Handling for Invalid Characters #5032

Update R2 Error Handling for Invalid Characters #5032

Conversation

dbenCF commented Feb 16, 2024 • edited by petebacondarwin Loading

changeset-bot bot commented Feb 16, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Feb 19, 2024 • edited Loading

penalosa Feb 20, 2024

Choose a reason for hiding this comment

dbenCF Feb 22, 2024

Choose a reason for hiding this comment

codecov bot commented Feb 29, 2024 • edited Loading

Codecov Report

penalosa left a comment

Choose a reason for hiding this comment

penalosa Mar 19, 2024

Choose a reason for hiding this comment

petebacondarwin Jul 11, 2024

Choose a reason for hiding this comment

petebacondarwin Feb 19, 2024

Choose a reason for hiding this comment

petebacondarwin Jul 11, 2024

Choose a reason for hiding this comment

petebacondarwin commented Jul 11, 2024

dbenCF commented Feb 16, 2024 •

edited by petebacondarwin

Loading

changeset-bot bot commented Feb 16, 2024 •

edited

Loading

github-actions bot commented Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 29, 2024 •

edited

Loading