feat(data-handlers): add digest, etag, stable and verified headers #216
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #216 +/- ##
===========================================
+ Coverage 68.70% 68.72% +0.01%
===========================================
Files 32 32
Lines 7916 7919 +3
Branches 431 431
===========================================
+ Hits 5439 5442 +3
Misses 2476 2476
Partials 1 1 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThe changes in this pull request introduce new properties to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/routes/data/handlers.test.ts (2)
43-47: Approve new methods and suggest improvementsThe addition of
getDataItemAttributesandgetTransactionAttributesmethods to thedataIndexobject is approved. These new methods align with the changes in thecreateDataHandlerfunction.However, I have a few suggestions to improve the test coverage:
Verify if returning
undefinedis the intended behavior for these mock methods. If specific attributes are expected in the actual implementation, consider mocking realistic return values.Add tests that explicitly check the behavior of
createDataHandlerwhen these methods are called. This will ensure that the handler correctly utilizes the new attributes.Would you like assistance in drafting additional test cases for these new methods?
Line range hint
1-478: Overall assessment and suggestion for improvementThe changes to this test file are minimal and focused. The addition of
getDataItemAttributesandgetTransactionAttributesmethods to the mockdataIndexobject suggests an expansion of functionality in thecreateDataHandlerfunction.While the existing tests remain intact and should continue to pass, there's an opportunity to enhance the test coverage:
Consider adding new test cases that specifically exercise the newly added methods. This will ensure that the
createDataHandlerfunction correctly utilizes these new attributes in various scenarios.Review the
createDataHandlerimplementation to identify any new behaviors or edge cases introduced by these attributes, and add corresponding tests.If these new attributes are critical to the function's behavior, consider adding negative test cases (e.g., when these methods return null or throw errors) to ensure robust error handling.
Would you like assistance in outlining some additional test cases to cover these new methods?
src/routes/data/handlers.ts (1)
44-63: LGTM! Consider minor readability improvement.The implementation of
setDigestStableVerifiedHeaderslooks good. It correctly handles the case whendataAttributesis undefined and sets appropriate headers based on the data attributes.Consider using object destructuring for
dataAttributesto improve readability:const setDigestStableVerifiedHeaders = ({ res, dataAttributes, }: { res: Response; dataAttributes: ContiguousDataAttributes | undefined; }) => { - if (dataAttributes !== undefined) { + if (dataAttributes) { + const { stable, verified, hash } = dataAttributes; - res.setHeader(headerNames.stable, dataAttributes.stable ? 'true' : 'false'); + res.setHeader(headerNames.stable, stable ? 'true' : 'false'); res.setHeader( headerNames.verified, - dataAttributes.verified ? 'true' : 'false', + verified ? 'true' : 'false', ); - if (dataAttributes.hash !== undefined) { + if (hash) { - res.setHeader(headerNames.digest, dataAttributes.hash); - res.setHeader('ETag', dataAttributes.hash); + res.setHeader(headerNames.digest, hash); + res.setHeader('ETag', hash); } } };test/end-to-end/data.test.ts (2)
332-339: Consider extracting repeated constants into variablesThe value
'gkOH8JBTdKr_wD9SriwYwCM6p7saQAJFU60AREREQLA'is used multiple times in these assertions. To improve maintainability and avoid duplication, consider defining it as a constant at the beginning of the test case.
467-474: Reuse constants for repeated header valuesSimilar to earlier, the constant
'gkOH8JBTdKr_wD9SriwYwCM6p7saQAJFU60AREREQLA'is repeated in multiple assertions. Extracting this value into a constant can enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/constants.ts (1 hunks)
- src/routes/data/handlers.test.ts (1 hunks)
- src/routes/data/handlers.ts (3 hunks)
- test/end-to-end/data.test.ts (2 hunks)
🔇 Additional comments (7)
src/constants.ts (2)
23-25: LGTM! New headers added correctly.The new header names (digest, stable, and verified) have been added correctly, following the existing naming convention with the 'X-AR-IO-' prefix. This maintains consistency with the current codebase structure.
23-25:⚠️ Potential issueConsider adding the etag header mentioned in the PR objectives.
The PR objectives mention adding an etag header, but it's not present in the current changes. Consider adding it to align with the stated objectives:
export const headerNames = { hops: 'X-AR-IO-Hops', origin: 'X-AR-IO-Origin', originNodeRelease: 'X-AR-IO-Origin-Node-Release', digest: 'X-AR-IO-Digest', stable: 'X-AR-IO-Stable', verified: 'X-AR-IO-Verified', + etag: 'ETag', cache: 'X-Cache', arnsTtlSeconds: 'X-ArNS-TTL-Seconds', arnsResolvedId: 'X-ArNS-Resolved-Id', arnsProcessId: 'X-ArNS-Process-Id', };Note that 'ETag' is the standard HTTP header name for entity tags, so it doesn't follow the 'X-AR-IO-' prefix convention.
To ensure we're not duplicating an existing etag header definition, let's check the codebase:
✅ Verification successful
Adding the
ETagheader aligns with the PR objectives and does not duplicate existing headers.
- Include the
etag: 'ETag'line insrc/constants.tsas suggested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing etag header definitions rg --type typescript "etag.*:.*['\"](ETag|etag)['\"]" --ignore-caseLength of output: 108
Script:
#!/bin/bash # Search for any existing etag header definitions in TypeScript files by specifying the file extensions rg "etag\s*:\s*['\"](ETag|etag)['\"]" --ignore-case --glob "*.ts" --glob "*.tsx"Length of output: 86
src/routes/data/handlers.ts (3)
124-124: LGTM! Good integration of new header-setting function.The addition of
setDigestStableVerifiedHeaderscall in thesetDataHeadersfunction ensures consistent setting of digest, stable, and verified headers for all data responses. This change aligns well with the PR objective.
173-174: LGTM! Consistent header setting for range requests.The addition of
setDigestStableVerifiedHeaderscall in thehandleRangeRequestfunction ensures that digest, stable, and verified headers are consistently set for range requests as well. This change aligns with the PR objective and maintains consistency with the non-range request handling.
Line range hint
1-724: Overall assessment: Well-implemented feature with consistent header handling.The changes in this PR successfully implement the addition of digest, etag, stable, and verified headers across different request handling scenarios. The new
setDigestStableVerifiedHeadersfunction is well-designed and consistently used in bothsetDataHeadersandhandleRangeRequestfunctions. This ensures that these headers are set for all types of responses, including range requests.The implementation is clean, consistent, and aligns well with the existing code structure. It effectively achieves the PR objective of adding these new headers to enhance the information provided in the HTTP responses.
test/end-to-end/data.test.ts (2)
340-341: Verify header value types forx-ar-io-stableandx-ar-io-verifiedYou are asserting the headers
x-ar-io-stableandx-ar-io-verifiedagainst the string'false'. If these headers are meant to represent boolean values, ensure that they are consistently returned as strings. Otherwise, consider parsing them to boolean before assertion.
475-476: Confirm consistency of header value typesThe assertions for
x-ar-io-stableandx-ar-io-verifiedcompare against the string'false'. Verify that the API consistently returns these header values as strings. If they represent boolean values, consider converting them accordingly in your tests.
No description provided.