dataconnect(change): Replace SecureRandom with process-unique ID generation#8154
Conversation
…sDataConnectGrpcStreamingServer.kt and TurbineUtils.kt This enables these classes to be used from both `androidTest` to `test` (thanks to the `@SharedWithAndroidTest` annotation and the associated gradle plugin) instead of just in `androidTest`. A future test will make use of this.
… streaming connection with the backend for realtime queries.
…le(): T` (moved from ConnectRPCIntegrationTest.kt)
…ic tests for DataConnectGrpcRPCs.connect()
…ared errors with itself instead of with the expected list [skip actions]
…sts if ConnectorStreamServiceCoroutineStub.connect() throws an exception
…cidentally public)
…tineScope() and CoroutineScope.createChildSupervisorScope() and modified the code base to use them This change improves the "dryness" of the code by factoring out repeated logic into helper methods. This will also benefit future code that also wants to create "child" CoroutineScope objects.
…aConnectSerialization' into RealtimeRewire2
…aconnect/CoroutineUtilsMoreHelpers
…aconnect/CoroutineUtilsMoreHelpers
…neration [skip actions] Reverts the use of `SecureRandom` (introduced in #7910) for generating request IDs. `SecureRandom` was adopted to ensure unique IDs, but it does not guarantee uniqueness. Furthermore, it introduces performance overhead and can block threads waiting for entropy, which required offloading ID generation to an I/O dispatcher. Security-grade randomness is not required for this feature. This change switches back to standard `Random` and uses an algorithm that guarantees process-wide uniqueness for request IDs. This simplifies the code by removing the need for background thread execution and lazy initialization.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces SecureRandom with Random across the firebase-dataconnect module to improve performance in non-security-sensitive contexts and introduces a new nextIdString utility for generating unique process-wide IDs. Feedback highlighted critical issues in the nextIdString implementation, specifically regarding potential ID collisions caused by incorrect hexadecimal padding for large sequence numbers and the inclusion of hexadecimal digits in the random padding alphabet. It is recommended to use Long.toString(16) for simplicity and correctness, and to restrict the padding alphabet to non-hexadecimal characters to guarantee uniqueness.
… the possibility of collisions with random values [no ci]
…eneratorUnitTest.kt [no ci]
…nts suggested by gemini-cli [no ci]
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces SecureRandom with Random across the firebase-dataconnect module to improve performance in non-security-sensitive contexts. It introduces a new utility, nextIdString, which ensures process-wide uniqueness by combining an atomic sequence number with random padding. Review feedback suggests a minor grammatical fix in the CHANGELOG and recommends using the class-level random instance instead of the global Random companion object in FirebaseDataConnectImpl.kt to enhance consistency and testability.
There was a problem hiding this comment.
Code Review
This pull request replaces SecureRandom with Random throughout the Data Connect SDK to optimize performance in non-security-sensitive contexts. It introduces a new nextIdString utility in IdStringGenerator.kt that ensures process-wide uniqueness by combining a prefix, random padding, and an atomic sequence number. The reviewer suggested using the injected random property instead of the global Random companion object in FirebaseDataConnectImpl to enhance testability and maintain consistency across the codebase.
There was a problem hiding this comment.
Code Review
This pull request replaces SecureRandom with Random throughout the firebase-dataconnect module, as high-security randomness is not required for the current use cases. A new utility function, nextIdString, has been introduced to generate unique process-wide identifiers using an atomic sequence and random padding. Feedback suggests utilizing the injected random instance within FirebaseDataConnectImpl rather than Random.Default to improve testability and maintain consistency.
…andom to generate request IDs`
|
/gemini review |
📝 PRs merging into main branchOur main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released. |
There was a problem hiding this comment.
Code Review
This pull request replaces the use of SecureRandom and various ad-hoc ID generation methods with a new IdStringGenerator utility to improve performance and consistency across the SDK. The new utility generates unique IDs by combining a prefix, random padding from a specific character set, and an atomic sequence number. The changes affect multiple components, including Auth, AppCheck, logging, and query management. Feedback suggests that if strict process-wide uniqueness is required, a shared generator or a static sequence number should be considered to further minimize collision risks across multiple instances.
…ator for every instance of FirebaseDataConnect.
2 participants
Summary
Replaces the use of
SecureRandomwith a more efficient, process-unique ID generation strategy using standardRandomfor Firebase Data Connect request and logging IDs.Highlights
SecureRandomwith standardRandomfor non-security-sensitive request IDs, eliminating the overhead and potential thread blocking associated with entropy collection inSecureRandom.IdStringGenerator, a utility class that generates process-unique ID strings by combining a prefix, random non-hexadecimal padding, and an atomically incremented hexadecimal sequence number.IdStringGeneratorensures process-wide unique IDs while maintaining a readable format and avoiding characters that are easily confused (e.g., 'i', 'l').SecureRandomwithout blocking the main thread.IdStringGeneratorUnitTestto verify the uniqueness, monotonicity, and concurrency safety of the new ID generation logic.Changelog
17 files changed
SecureRandomtoRandom.IdStringGeneratorand pass it to the base class.IdStringGeneratorand pass it to the base class.IdStringGeneratorfor invocation IDs.SecureRandommanagement and updatedFirebaseDataConnectImplinstantiation.SecureRandomtoIdStringGeneratorfor internal request and emulator IDs.IdStringGeneratorinstance for logger IDs.idStringGeneratorfrom thedataConnectinstance for request IDs.RealtimeQuerySubscriptionImplinstantiation.IdStringGeneratorfor request IDs and removed internal ID generation logic.IdStringGeneratorfor query request IDs.IdStringGeneratorin tests.IdStringGeneratorin tests.IdStringGeneratorand removed tests specific toSecureRandomseeding.SecureRandomarbitraries from Kotest property tests.