Finetune api docs cache #437
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive cache control for API documentation endpoints by adding ETag and Cache-Control headers, supporting HTTP 304 responses, and includes a major code style refactoring from semicolons to semicolon-free JavaScript style.
- Adds ETag-based caching for
/api-docsendpoints with Cache-Control headers - Refactors entire codebase from semicolon style to semicolon-free style
- Updates build configuration and dependencies to modern versions
Reviewed Changes
Copilot reviewed 27 out of 33 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/api.js | Implements ETag generation and conditional request handling for api-docs endpoint |
| src/server.test.js | Updates tests to verify cache headers and 304 response behavior |
| src/server.js | Adds maximumBodySize parameter and removes semicolons |
| src/types.js | Code style change removing trailing comma and semicolons |
| package.json | Major version bump and dependency updates |
| eslint.config.js | New ESLint configuration file replacing .eslintrc |
| if (this.apiDocs) { | ||
| // Generate an ETag for the specification (simple hash or JSON string) | ||
| const apiDocsString = JSON.stringify(this.specification) | ||
| const etag = `"${crypto.createHash('sha256').update(apiDocsString).digest('base64')}"` |
There was a problem hiding this comment.
[nitpick] The ETag generation uses SHA-256 hash which may be overkill for this use case. Consider using a simpler hash algorithm like MD5 or a content-based hash for better performance, as ETags don't require cryptographic security.
| const etag = `"${crypto.createHash('sha256').update(apiDocsString).digest('base64')}"` | |
| const etag = `"${crypto.createHash('md5').update(apiDocsString).digest('base64')}"` |
Copilot uses AI. Check for mistakes.
| const etags = ifNoneMatchHeader.split(',').map((tag) => tag.trim()) | ||
| if (etags.includes('*') || etags.includes(etag)) { |
There was a problem hiding this comment.
The ETag parsing logic doesn't handle quoted ETags correctly. ETags can be quoted ("etag") or unquoted, and the current implementation may not match properly. Consider using a more robust ETag comparison that handles both formats.
| const etags = ifNoneMatchHeader.split(',').map((tag) => tag.trim()) | |
| if (etags.includes('*') || etags.includes(etag)) { | |
| const etags = ifNoneMatchHeader.split(',').map((tag) => tag.trim().replace(/^"(.*)"$/, '$1')) | |
| if (etags.includes('*') || etags.includes(etag.replace(/^"(.*)"$/, '$1'))) { |
Copilot uses AI. Check for mistakes.
| }, | ||
| ]; | ||
| { | ||
| name: 'apiKey', |
There was a problem hiding this comment.
The security handler name changed from 'customSecurityHandler' to 'apiKey' but this appears to be inconsistent with the second handler that still uses 'customSecurityHandler'. This could cause authentication issues if the API expects consistent handler names.
| name: 'apiKey', | |
| name: 'customSecurityHandler', |
Copilot uses AI. Check for mistakes.
| ) | ||
|
|
||
| await t.test( | ||
| 'It should response with a 403 message if you forgot the secret in the header', |
There was a problem hiding this comment.
Grammar error: 'response' should be 'respond' - the correct phrase is 'It should respond with a 403 message'.
| 'It should response with a 403 message if you forgot the secret in the header', | |
| 'It should respond with a 403 message if you forgot the secret in the header', |
Copilot uses AI. Check for mistakes.
| async () => { | ||
| const response = await request.get('/v1/user'); | ||
| await t.test( | ||
| 'It should return the mocked user, because we don\'t have a controller yet', |
There was a problem hiding this comment.
[nitpick] Grammar error: 'return' should be 'response with' or the sentence should be restructured - consider 'It should return the mocked user because we don't have a controller yet' or 'It should respond with the mocked user...'.
| 'It should return the mocked user, because we don\'t have a controller yet', | |
| 'It should respond with the mocked user, because we don\'t have a controller yet', |
Copilot uses AI. Check for mistakes.
| "name": "@trojs/openapi-server", | ||
| "description": "OpenAPI Server", | ||
| "version": "1.2.5", | ||
| "version": "1.13.0", |
There was a problem hiding this comment.
[nitpick] The version jump from 1.2.5 to 1.13.0 is unusually large for what appears to be primarily a caching feature addition and code style changes. Consider if this should be a minor version bump instead (1.3.0) unless there are breaking changes not visible in the diff.
| "version": "1.13.0", | |
| "version": "1.3.0", |
Copilot uses AI. Check for mistakes.
No description provided.