Address code duplication in Access API tests and error handling #140
Conversation
WalkthroughThe changes introduced in this pull request focus on enhancing error handling and reducing code duplication across various API interaction tests and implementations. A new private utility function, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
SDK Examples Unit Test Results43 files 43 suites 8m 30s ⏱️ Results for commit 36d0930. ♻️ This comment has been updated with latest results. |
Common Integration Test Results1 files 1 suites 7s ⏱️ Results for commit 36d0930. ♻️ This comment has been updated with latest results. |
lealobanov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Line range hint
77-306: Maintain consistency in helper method usage across tests.While many test methods use the new helper methods, some still contain inline error handling. For example,
getNodeVersionInfoandgetAccountByAddresstests could use helper methods.Consider extracting these into helper methods for consistency:
private fun getNodeVersionInfo(): FlowNodeVersionInfo = safelyHandle( { handleResult(accessAPI.getNodeVersionInfo(), "Failed to get node version info") }, "Failed to retrieve node version info" ) private fun getAccountByAddress(address: FlowAddress): FlowAccount = safelyHandle( { handleResult(accessAPI.getAccountByAddress(address), "Failed to get account by address") }, "Failed to retrieve account by address" )
Line range hint
1-306: Overall architectural improvements align with PR objectives.The introduction of
safelyHandleand helper methods successfully addresses code duplication in the Access API tests. The changes make the code more maintainable and consistent with the PR objectives.However, consider the following architectural improvements:
- Create an error handling module that can be shared across different test classes
- Consider using a test fixture pattern to reduce setup code duplication
- Add documentation for the error handling pattern to help other contributors
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
87-112: LGTM! Consider adding KDoc comments.The helper functions are well-structured and provide comprehensive mock data. They effectively support test cases by creating reusable mock responses.
Consider adding KDoc comments to document the purpose and return values of these utility functions:
/** * Creates a mock node version info response for testing. * @return Access.GetNodeVersionInfoResponse with predefined test values */ fun createMockNodeVersionInfo(): Access.GetNodeVersionInfoResponse /** * Creates a mock transactions response containing the provided transactions. * @param transactions List of FlowTransaction to include in the response * @return Access.TransactionsResponse containing the provided transactions */ fun createTransactionsResponse(transactions: List<FlowTransaction>): Access.TransactionsResponsesdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
Line range hint
737-749: Consider extracting common error handling test logic.These error handling test blocks contain duplicated verification logic. Consider extracting this pattern into a helper method to reduce duplication.
private suspend fun verifyErrorChannel(errorChannel: ReceiveChannel<Throwable>, expectedMessage: String) { var receivedException: Throwable? = null val job = launch { receivedException = errorChannel.receiveCatching().getOrNull() } advanceUntilIdle() job.join() if (receivedException != null) { assertEquals(expectedMessage, receivedException!!.message) } else { fail("Expected error but got success") } errorChannel.cancel() }Then use it in the test cases:
verifyErrorChannel(errorChannel, testException.message!!)Also applies to: 760-772, 806-818, 855-867
405-405: Consider strengthening account verification.The account verification logic in these test cases could be strengthened by also verifying the code field of the FlowAccount.
assertResultSuccess(result) { assertEquals(flowAccount.address, it.address) assertEquals(flowAccount.keys, it.keys) assertEquals(flowAccount.contracts, it.contracts) assertEquals(flowAccount.balance.stripTrailingZeros(), it.balance.stripTrailingZeros()) + assertEquals(flowAccount.code, it.code) }Also applies to: 421-421, 437-437
Line range hint
876-882: Consider enhancing assertResultSuccess helper method.The helper method could be enhanced to provide more detailed error information by including the actual error message in the exception.
private fun <T> assertResultSuccess(result: FlowAccessApi.AccessApiCallResponse<T>, assertions: (T) -> Unit) { when (result) { is FlowAccessApi.AccessApiCallResponse.Success -> assertions(result.data) - is FlowAccessApi.AccessApiCallResponse.Error -> throw IllegalStateException("Request failed: ${result.message}", result.throwable) + is FlowAccessApi.AccessApiCallResponse.Error -> throw IllegalStateException( + "Request failed: ${result.message}. Error details: ${result.throwable?.message}", + result.throwable + ) } }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (2)
437-440: SimplifycompatibleRangeInitializationThe initialization of
compatibleRangecan be simplified using Kotlin's safe-call operator andletfunction, enhancing code readability.Apply this diff to streamline the code:
val compatibleRange = if (response.info.hasCompatibleRange()) { FlowCompatibleRange(response.info.compatibleRange.startHeight, response.info.compatibleRange.endHeight) -} else { null } +} else nullOr use safe-call with
let:+val compatibleRange = response.info.compatibleRange?.let { + FlowCompatibleRange(it.startHeight, it.endHeight) +}
26-41: Use Specific Exception Types in Catch BlocksCatching the general
Exceptionclass in thehandleApiCallmethod may obscure the root cause of errors. Using more specific exception types can aid in debugging and provide clearer error handling.Consider catching specific exceptions like
IOException,GrpcException, or any other relevant exceptions expected from the API calls.-} catch (e: Exception) { +} catch (e: IOException) {Adjust the exception types based on the exceptions that
apiCall()might throw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(7 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt(1 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt(6 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt(31 hunks)
🧰 Additional context used
📓 Learnings (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-07-03T12:30:04.496Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-10-08T17:04:37.869Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
🔇 Additional comments (7)
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (3)
15-15: LGTM! Good use of shared test utilities.
The import of createMockAccount from FlowAccessApiImplTest.Companion helps reduce code duplication between test classes, which aligns well with the PR objectives.
121-130: LGTM! Good refactoring of test utilities.
Making MockResponseFactory public and adding new account response factory methods improves code reuse across test classes, which aligns with the PR's goal of reducing duplication.
Line range hint 468-503: LGTM! Effective reduction of test code duplication.
The account-related test methods have been successfully refactored to use the shared createMockAccount utility, which:
- Reduces code duplication
- Maintains comprehensive assertions
- Improves consistency across test cases
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
14-22: LGTM! Good refactoring of imports.
The imports have been organized to leverage shared test utilities from AsyncFlowAccessApiImplTest, which helps reduce code duplication across test classes.
55-58: Well-structured utility methods in companion object.
Good addition of utility methods createMockTransaction and createMockAccount to centralize mock object creation. This promotes code reuse and maintains consistency in test data creation.
87-87: Consistent error handling pattern across test cases.
Good use of shared testException across error test cases. This standardizes error handling and makes the tests more maintainable.
Also applies to: 106-106, 142-142, 172-172, 204-204, 290-290, 322-322, 391-391
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
285-296: Validate Transactions Before Sending
In sendTransaction, ensuring that the transaction is valid before sending can prevent unnecessary API calls and potential errors.
Run the following script to verify the transaction's validity:
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt
Outdated
Show resolved
Hide resolved
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(7 hunks)
🧰 Additional context used
📓 Learnings (1)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-07-03T12:30:04.496Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
Learnt from: lealobanov
PR: onflow/flow-jvm-sdk#55
File: src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt:95-106
Timestamp: 2024-10-08T17:04:37.869Z
Learning: In the `TransactionIntegrationTest` class, exception handling is implemented using `IntegrationTestUtils.handleResult`.
🔇 Additional comments (2)
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt (2)
22-24: Well-implemented error handling utility!
The safelyHandle function effectively simplifies error handling by combining Result handling with exception handling. This implementation addresses the previous feedback about reducing error handling complexity.
27-52: Previous suggestion about error message duplication remains valid.
The helper methods consistently use both handleResult and safelyHandle, but there's still duplication in error messages as noted in the previous review.
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
Outdated
Show resolved
Hide resolved
Setup get collection endpoint
…ints Add GetProtocolStateSnapshotByBlockID and GetProtocolStateSnapshotByHeight to the SDK
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (23)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnector.kt (1)
15-19: Consider using domain-specific exceptions.Using a raw
Exceptionmakes it harder for clients to handle specific error cases. Consider creating domain-specific exceptions that better represent the failure modes.Example implementation:
sealed class CollectionApiException(message: String, cause: Throwable? = null) : Exception(message, cause) { class CollectionNotFound(id: FlowId, cause: Throwable? = null) : CollectionApiException("Collection not found: $id", cause) class ApiError(message: String, cause: Throwable? = null) : CollectionApiException(message, cause) }Then update the error handling:
- throw Exception(response.message, response.throwable) + throw CollectionApiException.ApiError(response.message, response.throwable)kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnector.kt (1)
7-9: Add KDoc documentation for the class.Consider adding KDoc documentation to describe the purpose and usage of this connector class.
+/** + * Internal connector class that provides a higher-level interface for retrieving protocol state + * snapshots through the Flow Access API. + * + * @property accessAPI The Flow Access API implementation used for making the actual API calls. + */ internal class GetProtocolStateAccessAPIConnector( private val accessAPI: FlowAccessApi )java-example/src/main/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnector.java (3)
28-36: Consider creating a custom exception type.Using
RuntimeExceptionis too generic and loses type safety. This makes it harder for clients to handle specific error cases.Consider creating a custom exception:
public class CollectionNotFoundException extends RuntimeException { private final FlowId collectionId; public CollectionNotFoundException(FlowId collectionId, String message, Throwable cause) { super(message, cause); this.collectionId = collectionId; } public FlowId getCollectionId() { return collectionId; } }
28-28: Add method documentation.The new method lacks JavaDoc documentation explaining its purpose, parameters, return value, and potential exceptions.
Consider adding documentation:
+ /** + * Retrieves a full collection of transactions by ID. + * + * @param collectionId The ID of the collection to retrieve + * @return List of transactions in the collection + * @throws RuntimeException if the collection cannot be retrieved + */ public List<FlowTransaction> getFullCollectionById(FlowId collectionId) {
28-29: Add parameter validation.The method should validate the
collectionIdparameter before making the API call.Add parameter validation:
public List<FlowTransaction> getFullCollectionById(FlowId collectionId) { + if (collectionId == null) { + throw new IllegalArgumentException("collectionId cannot be null"); + } AccessApiCallResponse<List<FlowTransaction>> response = accessAPI.getFullCollectionById(collectionId);kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt (1)
25-51: Enhance test coverage with additional assertions and error scenarios.The current tests only verify that the snapshots are non-null. Consider:
- Adding assertions to validate snapshot properties
- Adding tests for error scenarios (e.g., invalid blockId/height)
- Testing edge cases (e.g., height = 0)
Example enhancement for the blockId test:
@Test fun `Can get protocol state snapshot by blockId`() { block = getLatestBlockOrThrow() val snapshot = protocolStateConnector.getProtocolStateSnapshotByBlockId(block.id) assertNotNull(snapshot) assertEquals(block.height, snapshot.height) // Add more relevant assertions } @Test fun `Should handle invalid blockId`() { val invalidBlockId = "invalid-id" assertThrows<Exception> { protocolStateConnector.getProtocolStateSnapshotByBlockId(invalidBlockId) } }java-example/src/test/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnectorTest.java (2)
17-17: Consider initializing FlowBlock in setupThe
blockfield is declared but not initialized in the setup method. Since it's used in multiple test methods, consider initializing it in the setup to avoid redundant calls togetLatestBlock().@BeforeEach public void setup() { protocolStateConnector = new GetProtocolStateAccessAPIConnector(accessAPI); + block = getLatestBlock(); }Also applies to: 22-24
13-14: Add class and method documentationConsider adding Javadoc documentation to describe:
- The purpose of this test class
- The test scenarios covered
- The expected behavior for each test method
Example:
+/** + * Integration tests for GetProtocolStateAccessAPIConnector. + * Verifies the functionality of retrieving protocol state snapshots + * through different methods: latest, by block ID, and by height. + */ @FlowEmulatorProjectTest(flowJsonLocation = "../flow/flow.json") public class GetProtocolStateAccessAPIConnectorTest {java-example/src/main/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnector.java (1)
7-7: Add class-level documentation.Since this is a public API class, please add Javadoc documentation describing:
- The purpose of this connector
- The relationship with FlowAccessApi
- Any important usage notes or considerations
Example:
/** * Connector class that provides methods to retrieve protocol state snapshots from the Flow blockchain * using the Flow Access API. * * This class wraps the {@link FlowAccessApi} and provides convenient methods to retrieve protocol * state snapshots by different criteria (latest, block ID, or height). */kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnectorTest.kt (2)
11-11: Consider using explicit imports instead of wildcard imports.While wildcard imports reduce verbosity, explicit imports make it clearer which classes are being used and help prevent potential naming conflicts.
-import org.onflow.flow.sdk.* +import org.onflow.flow.sdk.FlowAccessApi +import org.onflow.flow.sdk.FlowCollection +import org.onflow.flow.sdk.FlowId +import org.onflow.flow.sdk.FlowTransaction +import org.onflow.flow.sdk.SignatureAlgorithm
54-64: Enhance test coverage with additional assertions and error scenarios.While the basic validations are good, consider enhancing the test coverage:
- Add assertions for:
- Expected number of transactions
- Additional transaction properties (e.g., status, proposalKey)
- Add error scenario tests (e.g., invalid collection ID)
Here's a suggested enhancement:
@Test fun `Can fetch full collection by ID`() { val collectionTransactions: List<FlowTransaction> = connector.getFullCollectionById(collectionId) assertNotNull(collectionTransactions, "Collection transactions should not be null") assertTrue(collectionTransactions.isNotEmpty(), "Collection transactions should not be empty") + assertEquals(1, collectionTransactions.size, "Expected exactly one transaction") val firstTransaction = collectionTransactions.first() assertNotNull(firstTransaction.id, "Transaction ID should not be null") assertNotNull(firstTransaction.script, "Transaction script should not be null") + assertNotNull(firstTransaction.proposalKey, "Transaction proposal key should not be null") + assertNotNull(firstTransaction.status, "Transaction status should not be null") } + +@Test +fun `Should handle invalid collection ID`() { + val invalidId = FlowId("0000000000000000") + assertThrows<IllegalArgumentException>("Should throw exception for invalid collection ID") { + connector.getFullCollectionById(invalidId) + } +}java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java (1)
57-67: Consider enhancing test coverage with additional assertions.While the basic happy path is well tested, consider adding:
- Error case testing (e.g., invalid collection ID)
- Verification of all transactions in the collection
- Additional transaction properties validation beyond ID and script
Example assertions to add:
// Verify all transactions in collection fullCollectionResponse.forEach(tx -> { assertNotNull(tx.getId(), "All transaction IDs should be non-null"); assertNotNull(tx.getScript(), "All transaction scripts should be non-null"); }); // Verify collection size matches expected assertEquals(expectedSize, fullCollectionResponse.size(), "Collection should contain expected number of transactions");sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
47-48: LGTM! Consider adding KDoc documentation.The new
getFullCollectionByIdmethod is a logical addition that complements the existinggetCollectionByIdmethod by providing direct access to the collection's transactions.Consider adding KDoc documentation to clarify:
- The relationship between this method and
getCollectionById- When to use one over the other
- Any performance implications of retrieving full transaction details
+ /** + * Retrieves all transactions in a collection by its ID. + * Unlike [getCollectionById], this method returns the complete transaction details. + * + * @param id The ID of the collection + * @return A response containing the list of transactions in the collection + */ fun getFullCollectionById(id: FlowId): AccessApiCallResponse<List<FlowTransaction>>sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
35-36: Add documentation to clarify the difference between getCollectionById and getFullCollectionById.The interface now has two methods for retrieving collections:
getCollectionByIdreturningFlowCollection?getFullCollectionByIdreturningList<FlowTransaction>Please add KDoc comments to explain:
- The difference between these methods
- When to use each method
- The relationship between
FlowCollectionand the list of transactionskotlin-example/README.md (1)
74-76: Enhance documentation clarity for collection retrieval methods.While the additions accurately reflect the new functionality, consider expanding the documentation to better distinguish between the two methods and their use cases:
- Get collection by id - Get full collection by id (returns all transactions in collection response) + Get collection by id - Retrieves basic collection information + Get full collection by id - Retrieves complete collection data including all associated transactionsAlso consider adding:
- Example usage scenarios for each method
- Notes about error handling behavior
🧰 Tools
🪛 LanguageTool
[style] ~74-~74: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../GetCollectionAccessAPIConnector.kt) - Get collection by id - Get full collection ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt (2)
233-243: Add test cases for edge cases and error scenarios.While the happy path is covered, consider adding test cases for:
- Empty collection
- Error response from the API
- Collection with multiple transactions
This would ensure comprehensive test coverage for the
getFullCollectionByIdfunctionality.@Test fun `Test getFullCollectionById with empty collection`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowId = FlowId("01") `when`(flowAccessApi.getFullCollectionById(flowId)) .thenReturn(FlowAccessApi.AccessApiCallResponse.Success(emptyList())) val result = flowAccessApi.getFullCollectionById(flowId) assertEquals(FlowAccessApi.AccessApiCallResponse.Success(emptyList()), result) } @Test fun `Test getFullCollectionById with error`() { val flowAccessApi = mock(FlowAccessApi::class.java) val flowId = FlowId("01") val error = RuntimeException("API Error") `when`(flowAccessApi.getFullCollectionById(flowId)) .thenReturn(FlowAccessApi.AccessApiCallResponse.Error("Failed to get collection", error)) val result = flowAccessApi.getFullCollectionById(flowId) assertTrue(result is FlowAccessApi.AccessApiCallResponse.Error) }
409-429: Enhance test data and validation for protocol state snapshots.Consider the following improvements:
- Use more realistic test data instead of the simple "snapshot" string
- Add validation for the snapshot content
- Consider adding test cases for error scenarios
private companion object { val TEST_SNAPSHOT_DATA = ByteArray(32) { it.toByte() } // More realistic test data } @Test fun `Test getProtocolStateSnapshotByBlockId`() { val flowAccessApi = mock(FlowAccessApi::class.java) val snapshot = FlowSnapshot(TEST_SNAPSHOT_DATA) `when`(flowAccessApi.getProtocolStateSnapshotByBlockId(blockId)) .thenReturn(FlowAccessApi.AccessApiCallResponse.Success(snapshot)) val result = flowAccessApi.getProtocolStateSnapshotByBlockId(blockId) assertTrue(result is FlowAccessApi.AccessApiCallResponse.Success) result.data.let { data -> assertArrayEquals(TEST_SNAPSHOT_DATA, data.bytes) } }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (1)
457-481: LGTM! Consider extracting common snapshot handling logic.The implementation is correct and follows established patterns. However, there's an opportunity to reduce code duplication across the three snapshot-related methods (getLatestProtocolStateSnapshot, getProtocolStateSnapshotByBlockId, getProtocolStateSnapshotByHeight).
Consider extracting the common snapshot handling logic into a private method:
+ private fun handleSnapshotResponse(response: Access.ProtocolStateSnapshotResponse): FlowAccessApi.AccessApiCallResponse<FlowSnapshot> = + FlowAccessApi.AccessApiCallResponse.Success(FlowSnapshot(response.serializedSnapshot.toByteArray())) override fun getLatestProtocolStateSnapshot(): FlowAccessApi.AccessApiCallResponse<FlowSnapshot> = try { val ret = api.getLatestProtocolStateSnapshot( Access.GetLatestProtocolStateSnapshotRequest .newBuilder() .build() ) - FlowAccessApi.AccessApiCallResponse.Success(FlowSnapshot(ret.serializedSnapshot.toByteArray())) + handleSnapshotResponse(ret) } catch (e: Exception) { FlowAccessApi.AccessApiCallResponse.Error("Failed to get latest protocol state snapshot", e) } override fun getProtocolStateSnapshotByBlockId(blockId: FlowId): FlowAccessApi.AccessApiCallResponse<FlowSnapshot> = try { val ret = api.getProtocolStateSnapshotByBlockID( Access.GetProtocolStateSnapshotByBlockIDRequest .newBuilder() .setBlockId(blockId.byteStringValue) .build() ) - FlowAccessApi.AccessApiCallResponse.Success(FlowSnapshot(ret.serializedSnapshot.toByteArray())) + handleSnapshotResponse(ret) } catch (e: Exception) { FlowAccessApi.AccessApiCallResponse.Error("Failed to get protocol state snapshot by block ID", e) } override fun getProtocolStateSnapshotByHeight(height: Long): FlowAccessApi.AccessApiCallResponse<FlowSnapshot> = try { val ret = api.getProtocolStateSnapshotByHeight( Access.GetProtocolStateSnapshotByHeightRequest .newBuilder() .setBlockHeight(height) .build() ) - FlowAccessApi.AccessApiCallResponse.Success(FlowSnapshot(ret.serializedSnapshot.toByteArray())) + handleSnapshotResponse(ret) } catch (e: Exception) { FlowAccessApi.AccessApiCallResponse.Error("Failed to get protocol state snapshot by height", e) }sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (1)
121-130: Consider adding validation for null parameters.While the mock factory methods are well-structured, they should validate input parameters to fail fast during testing.
object MockResponseFactory { fun accountResponse(mockAccount: FlowAccount) = Access.AccountResponse + .also { require(mockAccount != null) { "mockAccount cannot be null" } } .newBuilder() .setAccount(mockAccount.builder().build()) .build() fun getAccountResponse(mockAccount: FlowAccount) = Access.GetAccountResponse + .also { require(mockAccount != null) { "mockAccount cannot be null" } } .newBuilder() .setAccount(mockAccount.builder().build()) .build()sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (3)
55-58: Consider enhancing mock object creation utilities.While the utility methods provide consistent mock objects, they could be more robust with input validation and documentation.
Consider these improvements:
companion object { - fun createMockTransaction(flowId: FlowId = FlowId("01")) = FlowTransaction( + /** + * Creates a mock transaction for testing purposes. + * @param flowId The ID for the transaction, defaults to "01" + * @throws IllegalArgumentException if flowId is empty + */ + fun createMockTransaction(flowId: FlowId = FlowId("01")) = require(flowId.bytes.isNotEmpty()) { + "Flow ID cannot be empty" + }.let { + FlowTransaction( FlowScript("script"), emptyList(), flowId, 123L, FlowTransactionProposalKey(FlowAddress("02"), 1, 123L), FlowAddress("02"), emptyList() ) + }
402-408: Enhance error message specificity.The error message could be more specific about what failed during the transaction result retrieval.
- assertEquals("Failed to get transaction result by index", (result as FlowAccessApi.AccessApiCallResponse.Error).message) + assertEquals("Failed to get transaction result by index: ${flowId.base16Value}[${index}]", (result as FlowAccessApi.AccessApiCallResponse.Error).message)
592-623: Consider adding edge cases for protocol state snapshot tests.While the happy path is well tested, consider adding test cases for:
- Empty snapshots
- Invalid block IDs
- Error scenarios with specific error messages
Example additional test:
@Test fun `Test getProtocolStateSnapshotByBlockId with empty snapshot`() { val emptySnapshot = FlowSnapshot(ByteArray(0)) `when`( mockApi.getProtocolStateSnapshotByBlockID(any()) ).thenReturn(AsyncFlowAccessApiImplTest.MockResponseFactory.protocolStateSnapshotResponse(emptySnapshot)) val result = flowAccessApiImpl.getProtocolStateSnapshotByBlockId(blockId) assertResultSuccess(result) { assertEquals(emptySnapshot, it) } }sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt (1)
500-505: Extract complex transform logic into a private helper method for readabilityThe
transformlambda ingetNodeVersionInfocontains complex logic that spans multiple lines, which can reduce readability. Extracting this logic into a separate private method will enhance code clarity and maintainability.Proposed refactor:
Create a private helper method:
private fun toFlowNodeVersionInfo(response: Access.GetNodeVersionInfoResponse): FlowNodeVersionInfo { val compatibleRange = if (response.info.hasCompatibleRange()) { FlowCompatibleRange( startHeight = response.info.compatibleRange.startHeight, endHeight = response.info.compatibleRange.endHeight ) } else { null } return FlowNodeVersionInfo( semver = response.info.semver, commit = response.info.commit, sporkId = response.info.sporkId.toByteArray(), protocolVersion = response.info.protocolVersion, sporkRootBlockHeight = response.info.sporkRootBlockHeight, nodeRootBlockHeight = response.info.nodeRootBlockHeight, compatibleRange = compatibleRange ) }Update the
transformlambda ingetNodeVersionInfo:transform = { response -> - val compatibleRange = if (response.info.hasCompatibleRange()) { - FlowCompatibleRange(response.info.compatibleRange.startHeight, response.info.compatibleRange.endHeight) - } else { null } - FlowNodeVersionInfo(response.info.semver, response.info.commit, response.info.sporkId.toByteArray(), response.info.protocolVersion, response.info.sporkRootBlockHeight, response.info.nodeRootBlockHeight, compatibleRange) + toFlowNodeVersionInfo(response) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
java-example/README.md(1 hunks)java-example/src/main/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnector.java(2 hunks)java-example/src/main/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnector.java(1 hunks)java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java(2 hunks)java-example/src/test/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnectorTest.java(1 hunks)kotlin-example/README.md(1 hunks)kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnector.kt(1 hunks)kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnector.kt(1 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnectorTest.kt(2 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getNetworkParams/GetNetworkParamsAccessAPIConnectorTest.kt(1 hunks)kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt(1 hunks)sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt(7 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt(2 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt(2 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImpl.kt(1 hunks)sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt(2 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/FlowAccessApiTest.kt(3 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt(9 hunks)sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt(30 hunks)
✅ Files skipped from review due to trivial changes (1)
- kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getNetworkParams/GetNetworkParamsAccessAPIConnectorTest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/intTest/org/onflow/flow/sdk/transaction/TransactionIntegrationTest.kt
🧰 Additional context used
🪛 LanguageTool
java-example/README.md
[style] ~73-~73: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...etCollectionAccessAPIConnector.java) - Get collection by id - Get full collection ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
kotlin-example/README.md
[style] ~74-~74: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../GetCollectionAccessAPIConnector.kt) - Get collection by id - Get full collection ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (21)
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnector.kt (1)
15-19: Verify consistent error handling across API implementations.
Since this PR aims to address error handling duplication, let's verify if similar patterns exist in other API implementations.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Error handling pattern is consistently implemented across the codebase
The error handling pattern for AccessApiCallResponse is consistently implemented across all API connectors and examples:
- All implementations use the same when-expression pattern to handle
SuccessandErrorcases - Error cases consistently throw exceptions with the response message and throwable
- The pattern in the reviewed file matches the established pattern in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar error handling patterns across API implementations
# Expected: Find other instances of AccessApiCallResponse handling to ensure consistent refactoring
# Look for similar when-expression patterns with AccessApiCallResponse
rg -l "AccessApiCallResponse" | xargs rg -A 3 "when.*AccessApiCallResponse"
# Look for similar exception throwing patterns
rg "throw.*Exception.*message.*throwable"
Length of output: 30669
kotlin-example/src/main/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnector.kt (1)
1-6: LGTM!
The package structure and imports are well-organized and follow Kotlin conventions.
java-example/src/main/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnector.java (1)
7-8: LGTM!
The added import is necessary for the new method's return type.
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt (2)
1-19: LGTM! Class setup follows best practices.
The test class is properly structured with appropriate annotations and dependencies.
20-24: LGTM! Clean and focused setup.
The setup method is concise and properly initializes the test dependencies.
java-example/src/main/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnector.java (1)
1-12: LGTM! Clean class structure with proper dependency injection.
The class is well-organized with appropriate imports and follows good practices by using constructor injection for the FlowAccessApi dependency.
kotlin-example/src/test/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnectorTest.kt (1)
54-64: Verify potential test duplication across Access API test files.
Given that one of the PR objectives is to address code duplication in Access API tests, we should verify if similar test methods exist in other test files.
java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java (2)
14-14: LGTM!
The List import is correctly added and necessary for handling the collection of transactions in the new test method.
57-67: Verify potential test duplication across API test files.
While this test adds valuable coverage, we should ensure it doesn't introduce duplication, given the PR's objective to reduce test duplication in Access API.
✅ Verification successful
Let me gather more context about potential test duplication by examining the full test file content.
Let me check other test files in the codebase to see if there are similar test patterns or duplicated test logic.
Let me check the content of GetTransactionAccessAPIConnectorTest.java since it seems most relevant for potential test duplication.
No test duplication found - test methods serve distinct purposes
After analyzing the codebase, the test method canFetchFullCollectionById in GetCollectionAccessAPIConnectorTest.java is not duplicated. While there are similar assertion patterns across test files, they serve different purposes:
- GetCollectionAccessAPIConnectorTest tests collection-specific functionality
- GetTransactionAccessAPIConnectorTest tests individual transaction retrieval
- Other test files use similar assertions but test different API endpoints
The test aligns with the PR's objective as it provides unique coverage for collection-related functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar test patterns across test files
rg -l "getFullCollectionById" --type java
rg -l "canFetchFullCollection" --type java
# Look for similar assertion patterns
rg "Collection.*should not be (null|empty)" --type java
Length of output: 1043
Script:
#!/bin/bash
# Get the full content of the test file to analyze all test methods
cat "java-example/src/test/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnectorTest.java"
Length of output: 3095
Script:
#!/bin/bash
# Search for other test files that might contain similar test patterns
fd "Test.java$" --type f --exec rg -l "FlowCollection|FlowTransaction|getCollection" {}
# Look for similar assertion patterns in other test files
rg "assert.*Transaction.*should not be (null|empty)" --type java
Length of output: 2456
Script:
#!/bin/bash
# Get the content of GetTransactionAccessAPIConnectorTest.java to compare test patterns
cat "java-example/src/test/java/org/onflow/examples/java/getTransaction/GetTransactionAccessAPIConnectorTest.java"
Length of output: 3142
sdk/src/main/kotlin/org/onflow/flow/sdk/FlowAccessApi.kt (1)
81-84: LGTM! Methods follow consistent API patterns.
The new protocol state snapshot methods maintain consistency with existing API patterns and provide flexible ways to retrieve snapshots at specific points in the blockchain.
Let's verify the consistency of similar method pairs across the API:
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information to verify the API patterns.
Verified: API methods maintain consistent patterns across the codebase
The codebase shows consistent patterns for methods that fetch data by BlockId and Height:
- Both
getProtocolStateSnapshotByBlockIdandgetProtocolStateSnapshotByHeightfollow the same return type pattern (AccessApiCallResponse<FlowSnapshot>) - Similar consistent patterns are seen in other method pairs like
getBlockByHeight/getBlockById, maintaining the same return types and parameter structures - The implementation in both
FlowAccessApiImplandAsyncFlowAccessApiImplmaintains this consistency - Test coverage confirms the symmetrical behavior of these method pairs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all "ByBlockId" and "ByHeight" method pairs follow consistent patterns
# Expected: Each "ByBlockId" method should have a corresponding "ByHeight" method with similar structure
# Search for method pairs
echo "Analyzing method pairs..."
rg "ByBlockId|ByHeight" --type kotlin
Length of output: 24177
sdk/src/main/kotlin/org/onflow/flow/sdk/AsyncFlowAccessApi.kt (1)
35-36: LGTM! The new methods maintain interface consistency.
The additions:
- Follow the established async pattern using CompletableFuture
- Use consistent return type wrappers (AccessApiCallResponse)
- Maintain logical grouping of related operations
- Support the PR's objective of improving error handling
Also applies to: 69-72
java-example/README.md (1)
73-75: LGTM! Documentation accurately reflects the new collection retrieval capabilities.
The additions clearly document both the basic and full collection retrieval functionality, maintaining consistency with the rest of the README.
The repetitive "Get" beginnings are intentional and consistent with the documentation style throughout the README.
🧰 Tools
🪛 LanguageTool
[style] ~73-~73: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...etCollectionAccessAPIConnector.java) - Get collection by id - Get full collection ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
kotlin-example/README.md (1)
74-75: LGTM! Consistent bullet point format.
The repetitive use of "Get" at the beginning of each bullet point is acceptable and common in API documentation as it maintains consistency and clarity.
🧰 Tools
🪛 LanguageTool
[style] ~74-~74: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .../GetCollectionAccessAPIConnector.kt) - Get collection by id - Get full collection ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
sdk/src/main/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImpl.kt (1)
236-247: LGTM! Implementation follows established patterns.
The implementation is well-structured, follows the class's error handling patterns, and properly handles the conversion of protobuf responses to domain objects.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt (5)
15-15: LGTM! Import added for shared mock creation method.
The import of createMockAccount from the companion class aligns with the PR objective of reducing code duplication.
87-106: LGTM! Well-structured mock creation helper function.
The createMockNodeVersionInfo function provides a comprehensive mock with all necessary fields and proper builder pattern usage.
108-112: LGTM! Concise helper function for transaction response creation.
The createTransactionsResponse function follows the builder pattern and handles list conversion properly.
161-164: LGTM! Consistent mock factory method for full collection response.
The method follows the established pattern and properly handles the transaction list conversion.
Line range hint 483-518: LGTM! Improved test maintainability through shared mock creation.
The refactoring to use createMockAccount across account-related tests reduces code duplication and improves maintainability, aligning with the PR objectives.
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/FlowAccessApiImplTest.kt (2)
14-22: LGTM! Good use of shared test utilities.
Importing shared mock factories and constants from AsyncFlowAccessApiImplTest reduces code duplication between sync and async implementations.
Line range hint 1-924: Well-organized test suite with good code reuse.
The test suite effectively achieves the PR objective of reducing code duplication by:
- Utilizing shared mock responses and utilities
- Maintaining consistent error handling patterns
- Following a clear test organization structure
This improves maintainability and reduces the likelihood of inconsistencies between sync and async implementations.
.../src/main/kotlin/org/onflow/examples/kotlin/getCollection/GetCollectionAccessAPIConnector.kt
Show resolved
Hide resolved
...ain/kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnector.kt
Show resolved
Hide resolved
...le/src/main/java/org/onflow/examples/java/getCollection/GetCollectionAccessAPIConnector.java
Show resolved
Hide resolved
...kotlin/org/onflow/examples/kotlin/getProtocolState/GetProtocolStateAccessAPIConnectorTest.kt
Show resolved
Hide resolved
...t/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnectorTest.java
Show resolved
Hide resolved
.../main/java/org/onflow/examples/java/getProtocolState/GetProtocolStateAccessAPIConnector.java
Show resolved
Hide resolved
| fun getProtocolStateSnapshotByBlockId(blockId: FlowId): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowSnapshot>> | ||
|
|
||
| fun getProtocolStateSnapshotByHeight(height: Long): CompletableFuture<FlowAccessApi.AccessApiCallResponse<FlowSnapshot>> | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the code review and search results, I can now generate the final response since we've verified:
- The implementation consistency across sync/async APIs
- The proper test coverage for all methods
- The data structure (FlowSnapshot) and its usage
Documentation needed for protocol state snapshot methods
The new methods follow a consistent implementation pattern but lack essential documentation. Please add KDoc comments explaining:
- The purpose and contents of
FlowSnapshot - The difference between getting latest vs specific snapshots
- When to use block ID vs height-based retrieval
- Common use cases for historical snapshots
🔗 Analysis chain
Add documentation and verify consistency with other snapshot-related code.
The new protocol state snapshot methods follow the interface's pattern, but documentation would help users understand:
- What a protocol state snapshot contains
- When to use these methods vs getLatestProtocolStateSnapshot
- Common use cases for accessing historical snapshots
Let's verify the consistency of snapshot-related code:
Let me search for any documentation or comments about protocol state snapshots to better understand what they contain.
Let me try a different search pattern to find documentation about protocol state snapshots.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in snapshot-related code
# Test: Look for other snapshot-related code to ensure consistent implementation
rg -A 5 "ProtocolStateSnapshot|FlowSnapshot"
Length of output: 44324
Script:
#!/bin/bash
# Look for documentation or comments about protocol state snapshots
rg -i "protocol.*state.*snapshot" --type md
rg -B2 -A2 "\/\*\*.*protocol.*state.*snapshot"
rg -B2 -A2 "\/\/.*protocol.*state.*snapshot"
Length of output: 339
Script:
#!/bin/bash
# Look for documentation about protocol state snapshots with fixed regex
rg -i "protocol.+state.+snapshot" --type md
rg -B2 -A2 "\*.+protocol.+state.+snapshot"
rg -B2 -A2 "//.+protocol.+state.+snapshot"
# Also check for FlowSnapshot class definition and documentation
rg -B5 -A2 "class FlowSnapshot"
Length of output: 711
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@lealobanov - the first doc suggestions seems reasonable, could apply that across new entities where there's not much doc yet. Covering that in a new PR is fine too. The rest i don't think are needed
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Show resolved
Hide resolved
sdk/src/test/kotlin/org/onflow/flow/sdk/impl/AsyncFlowAccessApiImplTest.kt
Show resolved
Hide resolved
Closes: #138
Description
Addresses code duplication in error handling and tests related to the
FlowAccessApiandAsyncFlowAccessApiclasses (see issue #138)For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores