init#1
Conversation
kriszyp
left a comment
There was a problem hiding this comment.
Lots of awesome stuff in here, great work!
|
|
||
| if (Buffer.isBuffer(target.data)) { | ||
| bytes = target.data; | ||
| } else if (typeof target.data?.arrayBuffer === "function") { |
There was a problem hiding this comment.
This is looking for a Blob, I presume? Do we have any way that this can currently take place, through HTTP?
| const ImagesTable = databases.ImageOptimization.images; | ||
| const VariantsTable = databases.ImageOptimization.image_variants; | ||
|
|
||
| export class Images extends Resource { |
There was a problem hiding this comment.
Based on the signatures of the methods you are using, I think you want this class to have static loadAsInstance = false.
| } | ||
|
|
||
| // Upload and store original image | ||
| async post(target: any, data: any) { |
There was a problem hiding this comment.
It seems like it would be pretty intuitive to have a put() method here too, so the user can actually specify the id/path.
And I think these arguments are getting reversed due to the lack static loadAsInstance = false.
| await ImagesTable.put({ | ||
| id, | ||
| blob: bytes, | ||
| contentType: target.headers?.["content-type"] || "application/octet-stream", |
There was a problem hiding this comment.
Also, the data (the real one, which is confusing because the arguments are reversed), has a direct contentType property. But this is fine too.
| const variantKey = `${id}_${width || 'orig'}_${dpr}_${format}`; | ||
|
|
||
| // Try to get variant from database | ||
| let variant = await VariantsTable.get(variantKey); |
There was a problem hiding this comment.
This should be implemented as a caching table (see https://docs.harperdb.io/docs/developers/applications/caching for docs on this). Manually doing a get and put introduces a lot of race conditions.
There was a problem hiding this comment.
Sweet, thank you for the tip
| await sharp(bytes).metadata(); | ||
| } catch (err: any) { | ||
| logger.error("Invalid image format:", err); | ||
| return { status: 400, data: { error: "Invalid image format: " + err.message } }; |
There was a problem hiding this comment.
A Response object has to have a status and a headers otherwise, it is interpreted as a plain object to serialize. Errors should be thrown, not returned.
There was a problem hiding this comment.
If I throw errors in the response, I get 500 status codes for everything. Is there a workaround for this, or a recommended way to implement error handling properly with Harper so client errors (like 400) are returned with the correct status code?
There was a problem hiding this comment.
| @@ -0,0 +1 @@ | |||
| declare module 'harperdb'; No newline at end of file | |||
There was a problem hiding this comment.
Is this necessary for the types? It is not imported, right?
There was a problem hiding this comment.
yes I was getting errors until I added this bit in. Stolen from the early hints template
|
|
||
| export class Images extends Resource { | ||
| allowRead(user: User) { | ||
| return user?.role?.id === 'super_user'; |
There was a problem hiding this comment.
Wouldn't we typically want to allow broader access to images?
There was a problem hiding this comment.
Good point—this was originally pulled from the early hints template. I think it makes the most sense to restrict access for POST and PUT operations, while allowing broader read access for all image variants. It really depends on the usage of the api though, like if it used for something that allows public uploads we'd want to allow wider access to those endpoints as well. For the sake of this template, do you think we should stick with the secure defaults, or make it more permissive to cover broader use cases?
| return { status: 400, data: { error: "Invalid image format: " + err.message } }; | ||
| } | ||
|
|
||
| const id = Math.random().toString(36).slice(2, 10); |
There was a problem hiding this comment.
Harper itself can generate the ids with the create method, which uses UUIDs for strings (provides better guarantees of uniqueness), or auto-incrementation for numbers (which is faster).
There was a problem hiding this comment.
That's cool! Are there reference docs for this / other methods somewhere or can I just import it and call create(id). Im trying to find something about it but don't think I'm searching in the right place
There was a problem hiding this comment.
… for cache key parsing and content type formatting
… error status properties and adding checks for missing IDs
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Excellent work on this!
| - name: Create ImageOptimization database | ||
| run: | | ||
| curl -u admin:password \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"operation":"create_database","database":"ImageOptimization"}' \ | ||
| http://localhost:9925 | ||
|
|
||
| - name: Create images table | ||
| run: | | ||
| curl -u admin:password \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"operation":"create_table","database":"ImageOptimization","table":"images","primary_key":"id"}' \ | ||
| http://localhost:9925 | ||
|
|
||
| - name: Create image_variants table | ||
| run: | | ||
| curl -u admin:password \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"operation":"create_table","database":"ImageOptimization","table":"image_variants","primary_key":"id"}' \ | ||
| http://localhost:9925 |
There was a problem hiding this comment.
Are these not being created from the graphqlSchema ?
There was a problem hiding this comment.
No I don't think so, it was unable to find the db/tables without this step
There was a problem hiding this comment.
Maybe you need to restart after deploying the app.
There was a problem hiding this comment.
I tried adding a restart step after deploying but am still seeing connectivity issues without those steps to create the db and tables: failed gh action run
There was a problem hiding this comment.
Actually was able to remove them! I realized I was referencing the wrong tables in the resources file instead of the ones created by the schema so once I switched that was able to remove those steps
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
…ource methods for cleaner code
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
nothing to block on. this is awesome. great work
| if (res.status !== 200) { | ||
| console.log('Variant error response:', res.status, await res.text()); | ||
| } |
There was a problem hiding this comment.
Similar to above; I'd rather not mix debug code with test code. Just fail with the assertion
| const format = formatRaw?.toLowerCase() as VariantFormat; | ||
|
|
||
| if (!imageId || !/^[a-z0-9_-]+$/i.test(imageId)) return null; | ||
| if (!['webp', 'jpeg', 'avif', 'png'].includes(format)) return null; |
There was a problem hiding this comment.
You could still use a type assertion function here; just catch the error and return null
try {
assertFormat(format);
} catch (err) {
return null;
}
// Now `format` is typed as `VariantFormat` hereCo-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
kriszyp
left a comment
There was a problem hiding this comment.
I made a couple of minor notes, just FYI, but these are really minor. Great job with all of this! This is an excellent component!
| try { | ||
| cached = await VariantsTable.get(cacheKey); | ||
| } catch (err) { | ||
| logger.error('VariantsTable.get failed:', err); |
There was a problem hiding this comment.
FYI, Harper should log thrown errors for you, this will likely result in two log entries.
| throw err; | ||
| } | ||
| if (!image?.blob) { | ||
| logger.error('Image not found for imageId:', imageId); |
There was a problem hiding this comment.
This may be a little excessive log level since typically 404s are common and easily triggered by users. Also, you can return undefined to signal the entry is not found and Harper will return a 404 (quicker path since it doesn't involve throwing an error), or a return an object with status/header/body. But this will certainly work.

This api is responsible for uploading and retrieving optimized images using HarperDB as the backend. Key features include: