feat: x-ar-io-root-transaction-id header #225
Conversation
| FROM new_transactions | ||
| WHERE id = @id | ||
| UNION | ||
| SELECT null, data_size, content_type, content_encoding, true AS stable | ||
| SELECT null AS data_root, data_size, content_type, content_encoding, root_transaction_id, true AS stable |
There was a problem hiding this comment.
changed to null AS data_root for readability
| coreRow?.root_transaction_id !== null && | ||
| coreRow?.root_transaction_id !== undefined |
There was a problem hiding this comment.
checking for null AND undefined because it can return null for transactions where root_transaction_id doesn't exist and return undefined for bundles/data items.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #225 +/- ##
===========================================
- Coverage 68.72% 68.67% -0.06%
===========================================
Files 32 32
Lines 7978 7986 +8
Branches 434 434
===========================================
+ Hits 5483 5484 +1
- Misses 2494 2501 +7
Partials 1 1 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the addition of a new property 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: 1
🧹 Outside diff range and nitpick comments (4)
src/routes/data/handlers.test.ts (1)
700-732: LGTM: Test case for data items with root transaction ID.The test case correctly verifies that the 'x-ar-io-root-transaction-id' header is set for data items when a rootTransactionId is present. This aligns with the expected behavior.
Consider adding a test case for when
rootTransactionIdis not present in the data attributes to ensure the header is not set in that scenario. This would provide more comprehensive coverage.test/end-to-end/data.test.ts (2)
282-334: LGTM: Comprehensive test suite for X-AR-IO-Root-Transaction-Id header.The new test suite thoroughly checks the behavior of the
X-AR-IO-Root-Transaction-Idheader for both bundle transactions and data items. The setup and teardown procedures are well-defined, and the test cases cover the expected scenarios.Consider adding a comment explaining the purpose of the 10-second wait after queueing the bundle (line 316). This will help other developers understand why this delay is necessary.
323-333: LGTM: Well-structured test cases for header verification.Both test cases effectively verify the presence or absence of the
x-ar-io-root-transaction-idheader for bundle transactions and data items.For consistency, consider using a constant for the header name 'x-ar-io-root-transaction-id' instead of repeating the string. This would make future updates easier if the header name changes.
src/database/standalone-sqlite.ts (1)
984-996: LGTM with a minor suggestion.The addition of
rootTransactionIdis well-implemented and aligns with the PR objective. The code organization has been improved by moving thecontentTypevariable.Consider using the nullish coalescing operator for a more concise null check:
- const rootTransactionId = - coreRow?.root_transaction_id !== null && - coreRow?.root_transaction_id !== undefined - ? toB64Url(coreRow.root_transaction_id) - : undefined; + const rootTransactionId = coreRow?.root_transaction_id != null + ? toB64Url(coreRow.root_transaction_id) + : undefined;This change maintains the same logic while making the code more readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/constants.ts (1 hunks)
- src/database/sql/core/data-attributes.sql (1 hunks)
- src/database/standalone-sqlite.ts (1 hunks)
- src/routes/data/handlers.test.ts (1 hunks)
- src/routes/data/handlers.ts (1 hunks)
- src/types.d.ts (1 hunks)
- test/end-to-end/data.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (13)
src/database/sql/core/data-attributes.sql (5)
4-6: LGTM: Consistent addition of root_transaction_idThe addition of
root_transaction_idas NULL in the stable_transactions SELECT statement is appropriate and maintains consistency with the rest of the query structure.
8-10: LGTM: Consistent treatment of root_transaction_idThe addition of
root_transaction_idas NULL in the new_transactions SELECT statement mirrors the changes in the stable_transactions statement, maintaining consistency across the query.
16-18: LGTM: Consistent changes, verify root_transaction_idThe changes in this SELECT statement are consistent with those in the bundles.stable_data_items statement, which is good for maintaining uniformity. However, please verify that the
root_transaction_idcolumn exists in thebundles.new_data_itemstable.You can use the following script to verify the table structure:
#!/bin/bash # Description: Verify the structure of bundles.new_data_items table # Test: Check if root_transaction_id exists in bundles.new_data_items echo "Checking for root_transaction_id in bundles.new_data_items:" rg -i "create\s+table.*bundles\.new_data_items" -A 20
12-14: Verify implications of NULL data_root and confirm root_transaction_id
- Setting
data_rootto NULL is a change that might affect downstream logic. Please verify that this change doesn't break any existing functionality.- The inclusion of
root_transaction_idwithout modification suggests this column exists in thebundles.stable_data_itemstable. Please confirm this is the case.To verify these changes, you can run the following script:
Line range hint
1-21: Overall structure looks good, consider downstream impactsThe changes to the UNION query are consistent across all four SELECT statements, which is excellent. The addition of
root_transaction_idaligns well with the broader changes mentioned in the PR summary.However, there are two points to consider:
The setting of
data_rootto NULL for bundle-related tables (lines 12 and 16) might have implications on how data is processed downstream. Please ensure that any code relying ondata_rootfrom these tables can handle NULL values appropriately.The
LIMIT 1at the end of the query (line 21) means only one row will be returned, even if multiple matches are found across the different tables. This behavior hasn't changed, but it's worth confirming that this is the intended behavior, especially with the addition of the newroot_transaction_idcolumn.To verify the impact of these changes, you can run the following script:
src/constants.ts (1)
27-27: LGTM: New header constant added correctly.The new
rootTransactionIdconstant is well-placed and follows the existing naming conventions. It aligns with the PR objective of adding an 'x-ar-io-root-transaction-id' header.src/types.d.ts (2)
510-510: LGTM: Addition ofrootTransactionIdaligns with PR objective.The addition of the optional
rootTransactionIdproperty to theContiguousDataAttributesinterface aligns well with the PR objective of adding an 'X-AR-IO-Root-Transaction-Id' header. This change enhances the data model to support tracking of root transaction IDs, which can be valuable for tracing or auditing purposes.
509-509: Verify the impact of makingcontentTypeoptional.The change from
contentType: string | undefinedtocontentType?: stringprovides more flexibility but could potentially affect parts of the codebase that assumecontentTypeis always present. Please ensure that this change doesn't introduce any unintended side effects in the existing codebase.To verify the impact, you can run the following script:
✅ Verification successful
Impact of making
contentTypeoptional: VerifiedNo direct accesses to
contentTypewithout null checks were found, ensuring that making it optional does not introduce unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of contentType that might assume it's always defined rg -i 'contentType\s*[!?]' --type tsLength of output: 853
Script:
#!/bin/bash # Search for direct accesses to contentType without optional chaining or null checks rg 'contentType\.' --type tsLength of output: 30
src/routes/data/handlers.test.ts (1)
677-698: LGTM: Test case for transactions without root transaction ID.The test case correctly verifies that the 'x-ar-io-root-transaction-id' header is not set for transactions. This aligns with the expected behavior.
test/end-to-end/data.test.ts (1)
49-50: LGTM: New constant for bundle transaction ID.The addition of
bundle1constant follows the existing naming convention and will be useful for the new test suite.src/database/standalone-sqlite.ts (2)
995-996: LGTM! Changes improve code consistency and readability.The addition of
rootTransactionIdto the returned object and the direct inclusion ofcontentTypein the return statement are well-implemented. These changes align with the PR objective and improve code readability.
Line range hint
1-2500: Verify integration ofrootTransactionIdacross the system.While the changes to
getDataAttributesare well-implemented, it appears thatrootTransactionIdis not used or propagated in other parts of this file. Consider reviewing if this new property should be integrated into other methods or classes for consistency and completeness.To ensure proper integration, please run the following script to check for potential usage points of
rootTransactionId:This script will help identify areas where
rootTransactionIdmight need to be added or used for complete integration.src/routes/data/handlers.ts (1)
123-125:⚠️ Potential issueEnsure
rootTransactionIdis safely handled before adding to response headersPlease verify that
dataAttributes.rootTransactionIdis properly validated or sanitized before including it in the response headers to prevent potential security risks such as header injection attacks or unintended exposure of sensitive information.
| describe('X-AR-IO-Root-Transaction-Id', () => { | ||
| it("shouldn't return root transaction id for transactions", async () => { | ||
| app.get( | ||
| '/:id', | ||
| createDataHandler({ | ||
| log, | ||
| dataIndex, | ||
| dataSource, | ||
| blockListValidator, | ||
| manifestPathResolver, | ||
| }), | ||
| ); | ||
|
|
||
| return request(app) | ||
| .get('/not-a-real-id') | ||
| .expect(200) | ||
| .then((res: any) => { | ||
| assert.equal( | ||
| res.headers['x-ar-io-root-transaction-id'], | ||
| undefined, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| it('should return root transaction id for data items', async () => { | ||
| dataIndex.getDataAttributes = () => | ||
| Promise.resolve({ | ||
| size: 10, | ||
| contentType: 'application/octet-stream', | ||
| rootTransactionId: 'root-tx', | ||
| isManifest: false, | ||
| stable: true, | ||
| verified: true, | ||
| signature: null, | ||
| }); | ||
|
|
||
| app.get( | ||
| '/:id', | ||
| createDataHandler({ | ||
| log, | ||
| dataIndex, | ||
| dataSource, | ||
| blockListValidator, | ||
| manifestPathResolver, | ||
| }), | ||
| ); | ||
|
|
||
| return request(app) | ||
| .get('/not-a-real-id') | ||
| .expect(200) | ||
| .then((res: any) => { | ||
| assert.equal( | ||
| res.headers['x-ar-io-root-transaction-id'], | ||
| 'root-tx', | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for comprehensive coverage.
The new test cases for the X-AR-IO-Root-Transaction-Id header are well-implemented and cover the main scenarios. To further improve the test suite, consider adding the following test cases:
- Test when
rootTransactionIdis undefined or null in the data attributes. - Test with different HTTP methods (e.g., HEAD request) to ensure consistent behavior.
- Test error scenarios, such as when
getDataAttributesfails.
These additional tests would provide more comprehensive coverage and help catch potential edge cases.
No description provided.