[ECO-4962] feat: add ChatApi implementation
#9
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatApi
participant RealtimeClient
participant Server
User->>ChatApi: getMessages(roomId, params)
ChatApi->>RealtimeClient: fetchMessages(roomId, params)
RealtimeClient->>Server: HTTP GET /messages
Server-->>RealtimeClient: return messages
RealtimeClient-->>ChatApi: messages
ChatApi-->>User: return messages
sequenceDiagram
participant User
participant ChatApi
participant RealtimeClient
participant Server
User->>ChatApi: sendMessage(roomId, params)
ChatApi->>RealtimeClient: sendMessage(roomId, params)
RealtimeClient->>Server: HTTP POST /messages
Server-->>RealtimeClient: return response
RealtimeClient-->>ChatApi: response
ChatApi-->>User: return response
Assessment against linked issues
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 (
|
| /** | ||
| * Represents the result of a paginated query. | ||
| */ | ||
| interface PaginatedResult<T> { |
There was a problem hiding this comment.
I use lightweight version of PaginatedResult, not sure we actually need first() and isLast() methods
Created basic ChatApi implementation to perform REST request to the Chat Backend
ttypic
force-pushed
the
fix/add-missing-interface
branch
from
3aab05c to
6b55e22
Compare
ttypic
force-pushed
the
ECO-4962/chat-api-implementation
branch
from
08ad21b to
727b8d2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chat-android/build.gradle.kts (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/ErrorCodes.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/Message.kt (2 hunks)
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1 hunks)
- gradle/libs.versions.toml (2 hunks)
Files skipped from review due to trivial changes (1)
- chat-android/src/main/java/com/ably/chat/Message.kt
Additional comments not posted (11)
chat-android/build.gradle.kts (2)
38-38: LGTM!The code change is approved. The addition of the
gsonlibrary for JSON handling aligns with the AI-generated summary.
37-37: Verify the impact of changing the dependency configuration fromimplementationtoapi.Changing the dependency configuration from
implementationtoapiforlibs.ably.androidwill expose the library's public APIs to other modules. This might have implications on the visibility and encapsulation of the library.Run the following script to verify the usage of the
ably.androidlibrary in the codebase:Verification successful
No direct usage of
ably.androidfound in the codebase.The change from
implementationtoapiforlibs.ably.androiddoes not currently impact the codebase, as there are no direct usages of the library in the Kotlin files. Future development might be affected if other modules start using it.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `ably.android` library in the codebase. # Test: Search for the usage of the `ably.android` library. Expect: Only intended usage. rg --type kotlin -A 5 $'import com\.ably\.android'Length of output: 51
Script:
#!/bin/bash # Description: Search for any usage of the `ably.android` library in the codebase, including indirect usage or different import patterns. # Test: Search for any reference to `ably.android` in the codebase. rg --type kotlin -A 5 'ably\.android'Length of output: 424
chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (5)
14-14: The existing comment has been addressed as thefirst()andisLast()methods are not included in thePaginatedResultinterface.
14-32: LGTM!The
PaginatedResultinterface is well-defined and provides the necessary functionality for paginated results.
34-35: LGTM!The
toPaginatedResultfunction is correctly implemented and handles the null case appropriately.
37-44: LGTM!The
EmptyPaginatedResultclass is correctly implemented and serves as a placeholder for empty paginated results.
46-66: LGTM!The
AsyncPaginatedResultWrapperclass is correctly implemented and provides a convenient way to wrap anAsyncHttpPaginatedResponseand expose it as aPaginatedResult<T>.gradle/libs.versions.toml (1)
17-17: LGTM!The changes to add the Gson library dependency are approved. The dependency is correctly defined in the
[versions]and[libraries]sections of thegradle/libs.versions.tomlfile.Gson is a widely used library for handling JSON serialization and deserialization in Java and Android projects. Including it as a dependency is a common practice and does not raise any concerns.
Also applies to: 40-40
chat-android/src/main/java/com/ably/chat/ChatApi.kt (3)
22-38: LGTM!The code changes are approved.
40-61: LGTM!The code changes are approved.
63-70: LGTM!The code changes are approved.
| params = params.toParams(), | ||
| ) { | ||
| Message( | ||
| timeserial = it.asJsonObject.get("timeserial").asString, |
There was a problem hiding this comment.
Do we need something here to validate that the response returned is valid in structure?
There was a problem hiding this comment.
Good question! @AndyTWF what do you think we should do if we got invalid JSON object? Should we throw AblyException with 500 code?
There was a problem hiding this comment.
Yeah I think that's wise - it should never happen, but I'd much prefer we shout loudly about something not being as we expect than let it slip by and have unintended side-effects.
Per Simon's spec suggestion for protocol v3, we should silently ignore stuff we aren't expecting!
There was a problem hiding this comment.
I added explicit validation and couple of unit tests that check we are silently ignore stuff we aren't expecting and if response is invalid or required fields are missing it will throw AblyException
ttypic
force-pushed
the
ECO-4962/chat-api-implementation
branch
from
f96899d to
9d3656c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chat-android/build.gradle.kts (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- detekt.yml (1 hunks)
- gradle/libs.versions.toml (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- chat-android/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- gradle/libs.versions.toml
Additional comments not posted (6)
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (5)
21-59: LGTM!The test function is well-written and tests the expected behavior of the
getMessagesfunction.
61-83: LGTM!The test function is well-written and tests the expected behavior of the
getMessagesfunction.
85-118: LGTM!The test function is well-written and tests the expected behavior of the
sendMessagefunction.
120-141: LGTM!The test function is well-written and tests the expected behavior of the
sendMessagefunction.
143-163: LGTM!The test function is well-written and tests the expected behavior of the
getOccupancyfunction.detekt.yml (1)
151-151: LGTM!The code changes are approved. Excluding test directories from the
StringLiteralDuplicationcheck is a good practice as test code often contains duplicated string literals for test data setup.
ttypic
force-pushed
the
ECO-4962/chat-api-implementation
branch
2 times, most recently
from
806da45 to
86da17b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chat-android/build.gradle.kts (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- detekt.yml (1 hunks)
- gradle/libs.versions.toml (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
Additional comments not posted (9)
chat-android/build.gradle.kts (4)
37-37: LGTM!The code changes are approved.
38-38: LGTM!The code changes are approved.
41-41: LGTM!The code changes are approved.
42-42: LGTM!The code changes are approved.
gradle/libs.versions.toml (4)
17-17: LGTM!The version declaration for the Gson library is approved.
18-18: LGTM!The version declaration for the MockK library is approved.
19-19: LGTM!The version declaration for the Coroutine library is approved.
42-42: LGTM!The library definitions for Gson, MockK, and Coroutine libraries are approved. They correctly reference the corresponding version declarations.
Also applies to: 44-44, 46-47
detekt.yml (1)
151-151: LGTM!The code changes are approved:
- Correctly adds the
excludesproperty under theStringLiteralDuplicationrule in thecomplexitysection.- Follows the YAML syntax.
- Excludes common test directories from the
StringLiteralDuplicationrule, which is a good practice as test code often contains duplicated string literals.- Covers test directories across different platforms (Android, common, JVM, JS, iOS).
ttypic
force-pushed
the
ECO-4962/chat-api-implementation
branch
from
86da17b to
5c3bcaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chat-android/build.gradle.kts (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- detekt.yml (1 hunks)
- gradle/libs.versions.toml (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- chat-android/build.gradle.kts
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
Additional comments not posted (5)
gradle/libs.versions.toml (4)
17-17: LGTM!The version declaration for the Gson library is approved.
18-18: LGTM!The version declaration for the MockK library is approved.
19-19: LGTM!The version declaration for the Coroutine library is approved.
42-42: LGTM!The library declarations for Gson, MockK, and Coroutine libraries are approved. They match the corresponding version references.
Also applies to: 44-44, 46-47
detekt.yml (1)
151-151: LGTM!The code changes are approved. Excluding test directories from the
StringLiteralDuplicationrule is a good practice.
| } | ||
|
|
||
| fun <T> AsyncHttpPaginatedResponse?.toPaginatedResult(transform: (JsonElement) -> T): PaginatedResult<T> = | ||
| this?.let { AsyncPaginatedResultWrapper(it, transform) } ?: EmptyPaginatedResult() |
There was a problem hiding this comment.
when would EmptyPaginatedResult() get returned?
There was a problem hiding this comment.
Right now when there is no body, core Java SDK return null. I don't like this because we already return container, that can be empty, so I return EmptyPaginatedResult instead. For end user there is no difference between empty list and empty body, the same behavior we have in js.
The other case is when we invoke next() and hasNext() is false. I personally would throw exception in this case, but decided not to do this, because we don't throw exception in core SDK, and silently return null. Once again don't want to return null, because you definitely don't want to do null-checks here.
There was a problem hiding this comment.
Interesting - I personally think returning null makes more sense when looking at the code but I am coming from an optional-centric (and crazy) language in Swift. This will likely be one of the points in which we deviate between Kotlin and Swift due to platform best practices
There was a problem hiding this comment.
Not sure it's Kotlin best practice, the best practice probably would be throwing exceptions, but I decide to silence them as Ably Java does. This will be an extremely small change.
And about optional-centricity: from the SDK user’s perspective, I personally don’t think PaginatedResult#next() should return a nullable result, both for Swift and Kotlin—I’ve never seen this before, except in Ably Java. :) Users will have to handle this nullability on their own, but there is no reason for that.
This commit adds validation for incoming JSONs from the Chat Backend. If any required fields are missing or have the wrong type, a bad request exception will be thrown on the SDK side.
ttypic
force-pushed
the
ECO-4962/chat-api-implementation
branch
from
5c3bcaf to
783cd4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- chat-android/build.gradle.kts (1 hunks)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt (1 hunks)
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1 hunks)
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1 hunks)
- detekt.yml (1 hunks)
- gradle/libs.versions.toml (2 hunks)
Files skipped from review due to trivial changes (1)
- chat-android/build.gradle.kts
Files skipped from review as they are similar to previous changes (4)
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- gradle/libs.versions.toml
Additional comments not posted (1)
detekt.yml (1)
151-151: Approve the exclusion of test directories from complexity checks.The addition of the
excludesproperty under thecomplexitysection is a good practice as it helps focus complexity analysis on production code, avoiding unnecessary complexity checks on test code. The directories specified (**/test/**,**/androidTest/**,**/commonTest/**,**/jvmTest/**,**/jsTest/**,**/iosTest/**) are typical for Kotlin projects and cover a comprehensive range of test directories across different platforms and test environments.
Resolves #8
Created basic ChatApi implementation to perform REST request to the Chat Backend
Summary by CodeRabbit
New Features
ChatApiclass for streamlined interaction with chat services, including message retrieval, sending, and occupancy information.PaginatedResultinterface.Improvements
Bug Fixes
Tests
ChatApiclass to ensure expected behavior.