Monark/images binding parity - #15305
Conversation
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
Just a few minor questions with regards to production parity.
Please get a review from your team too as we might not know the product as well as the images team. Thanks!
| creator: options?.creator, | ||
| }; | ||
|
|
||
| await this.env.IMAGES_STORE.put(id, new ArrayBuffer(0), { metadata }); |
There was a problem hiding this comment.
Does this mean we will serve a zero-byte image if a user tries to retrieve it while it's still a draft? Just wanna check if this matches production.
There was a problem hiding this comment.
Yes that is correct, a draft image has no content until the upload is completed, so retrieving it before that point returns empty content in prod too. not necessarily an image but a 0 bytes response with 200 OK
| requireSignedURLs: options?.requireSignedURLs ?? false, | ||
| meta: options?.metadata ?? {}, | ||
| variants: ["public"], | ||
| draft: true, |
There was a problem hiding this comment.
Do we need to exclude draft image from .list()?
There was a problem hiding this comment.
Nope, we keep it in. production includes them in list()/get() results and marks them via the draft field so callers can distinguish them
| const duration = expiresIn ?? DEFAULT_DIRECT_UPLOAD_EXPIRES_IN; | ||
| if ( | ||
| duration <= MIN_DIRECT_UPLOAD_EXPIRES_IN || | ||
| duration >= MAX_DIRECT_UPLOAD_EXPIRES_IN |
There was a problem hiding this comment.
Should it accept MIN_DIRECT_UPLOAD_EXPIRES_IN or MAX_DIRECT_UPLOAD_EXPIRES_IN as well?
There was a problem hiding this comment.
I know its a bit weird but we have it as an exclusive heck so we have to keep it like that here too 😅
| const metadata: ImageMetadata = { | ||
| id, | ||
| uploaded: new Date().toISOString(), | ||
| requireSignedURLs: options?.requireSignedURLs ?? false, |
There was a problem hiding this comment.
It looks like the docs suggest that images with a custom ID cannot be made private with signed URL tokens (requireSignedURLs=true), but local dev accepts the combination. Should we reject it for parity?
There was a problem hiding this comment.
Oh, nice catch 😁. Yes it should be as u said, its fixed now.
67b183e to
f36db9f
Compare
🦋 Changeset detectedLatest commit: f36db9f The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
3 participants
Fixes IMAGES-2371, IMAGES-2381, IMAGES-2409.
Adds local dev parity for three Images binding features that already shipped in workerd/
@cloudflare/workers-types: metadata filtering onlist(),signedUrl(), andcreateDirectUpload().list({ filter: { metadata } })now filters locally using the same operators as production (eq,in,gt,gte,lt,lte), including dot-notation nested fields.image(id).signedUrl()generates and verifies signed delivery URLs locally using a fixed dev-only signing secret. Images withrequireSignedURLs: truenow return 401 from the local delivery endpoint without a valid signature.createDirectUpload()creates a draft image and returns anuploadURLserved by a new local endpoint that accepts the completed upload asmultipart/form-data. Matches production'sexpiresInbounds and single-use/expiry behaviour.A picture of a cute animal (not mandatory, but encouraged)