feat: kotlin wrapper for ably-java and ably-android
#1064
Conversation
WalkthroughThis pull request adds a new Kotlin-based module, pubsub-adapter, to the project. It updates build configurations by including the Kotlin JVM plugin and new dependency versions. Numerous new interfaces are defined for managing Ably PubSub functionalities such as channels, clients, and presence, along with corresponding adapter classes that bridge legacy implementations. Additionally, several utility functions, enums, and properties for Maven POM metadata are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RealtimeAdapter as RealtimeClientAdapter
participant Legacy as AblyRealtime
Client->>RealtimeAdapter: time()
RealtimeAdapter->>Legacy: time()
Legacy-->>RealtimeAdapter: current time
RealtimeAdapter-->>Client: current time
sequenceDiagram
participant Client as Client
participant RestAdapter as RestClientAdapter
participant Legacy as AblyRest
Client->>RestAdapter: request(path, method, params, body, headers)
RestAdapter->>Legacy: request(path, method, params, body, headers)
Legacy-->>RestAdapter: response
RestAdapter-->>Client: response
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
4f23918 to
464a8ff
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
464a8ff to
b43683d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
b43683d to
96aabf3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
![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
🧹 Nitpick comments (19)
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (1)
34-41: Avoid using the force-unwrapping!!operator.
ThelegacyClient.request(...)!!call can lead to a runtimeNullPointerExceptionif the underlying method unexpectedly returnsnull. Consider providing a safe fallback or throwing a more descriptive exception to avoid abrupt crashes.override fun request( method: String, path: String, params: List<Param>, body: HttpCore.RequestBody, headers: List<Param> ) = legacyClient.request(method, path, params.toTypedArray(), body, headers.toTypedArray()) - !! + ?: throw IllegalStateException("legacyClient.request(...) returned null")pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt (1)
14-175: Interface design is well-structured with comprehensive documentation.
The multiplesubscribe/unsubscribemethods provide fine-grained control, but you might consider using a single vararg-based overload or a sealed approach for event names to reduce overload complexity.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt (2)
48-49: Clarify concurrency handling for subscribed callbacks.While it's documented that these listeners are invoked on a background thread, consider clarifying which thread is used (e.g., a shared background thread pool vs. a dedicated queue) and whether additional synchronization is needed by callers. This helps maintain a consistent concurrency model for implementers.
136-137: Consider restrictingdatato typed payloads.Currently,
datais of typeAny?, which can reduce both clarity and type safety. Consider introducing a generic parameter or using a sealed class/interface to more explicitly define supported presence payload types and improve maintainability.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt (1)
1-24: Document the lifecycle of theclose()operation.Since
RealtimeClientextendsAutoCloseable, its implementations will be responsible for releasing resources (e.g., shutting down the connection). Consider adding KDoc about how and whenclose()should be invoked and the implications for ongoing channel or presence operations.pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt (1)
10-14: Consider adding null safety checks.While the implementation is correct, consider adding null safety checks when adapting legacy channels to ensure robustness.
- override fun get(name: String): RestChannel = RestChannelAdapter(legacyChannels.get(name)) + override fun get(name: String): RestChannel = legacyChannels.get(name)?.let(::RestChannelAdapter) + ?: throw IllegalStateException("Channel '$name' not found") - override fun get(name: String, options: ChannelOptions): RestChannel = - RestChannelAdapter(legacyChannels.get(name, options)) + override fun get(name: String, options: ChannelOptions): RestChannel = + legacyChannels.get(name, options)?.let(::RestChannelAdapter) + ?: throw IllegalStateException("Channel '$name' not found")pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt (1)
7-20: Consider extracting common adapter logic.This implementation is nearly identical to
RestChannelsAdapter. Consider extracting the common adapter logic to a base class to avoid code duplication.abstract class BaseChannelsAdapter<T : Channel>(protected val legacyChannels: AblyBase.Channels) : Channels<T> { override fun contains(name: String): Boolean = legacyChannels.containsKey(name) override fun release(name: String) = legacyChannels.release(name) protected abstract fun adaptChannel(channel: io.ably.lib.rest.Channel): T override fun get(name: String): T = legacyChannels.get(name)?.let(::adaptChannel) ?: throw IllegalStateException("Channel '$name' not found") override fun get(name: String, options: ChannelOptions): T = legacyChannels.get(name, options)?.let(::adaptChannel) ?: throw IllegalStateException("Channel '$name' not found") override fun iterator(): Iterator<T> = iterator { legacyChannels.entrySet().forEach { yield(adaptChannel(it.value)) } } } internal class RestChannelsAdapter(legacyChannels: AblyBase.Channels) : BaseChannelsAdapter<RestChannel>(legacyChannels) { override fun adaptChannel(channel: io.ably.lib.rest.Channel) = RestChannelAdapter(channel) } internal class RealtimeChannelsAdapter(legacyChannels: AblyRealtime.Channels) : BaseChannelsAdapter<RealtimeChannel>(legacyChannels) { override fun adaptChannel(channel: io.ably.lib.rest.Channel) = RealtimeChannelAdapter(channel) }pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt (1)
7-18: Optimize parameter conversion.The repeated conversion of
List<Param>toArray<Param>could be optimized by extracting it to an extension function. Additionally, consider caching the array for large parameter lists.+ private fun List<Param>.toParamArray() = toTypedArray() internal class RestPresenceAdapter(private val legacyPresence: Presence) : RestPresence { - override fun get(params: List<Param>): PaginatedResult<PresenceMessage> = - legacyPresence.get(params.toTypedArray()) + override fun get(params: List<Param>): PaginatedResult<PresenceMessage> = + legacyPresence.get(params.toParamArray()) override fun getAsync(params: List<Param>, callback: Callback<AsyncPaginatedResult<PresenceMessage>>) = - legacyPresence.getAsync(params.toTypedArray(), callback) + legacyPresence.getAsync(params.toParamArray(), callback) override fun history(params: List<Param>): PaginatedResult<PresenceMessage> = - legacyPresence.history(params.toTypedArray()) + legacyPresence.history(params.toParamArray()) override fun historyAsync(params: List<Param>, callback: Callback<AsyncPaginatedResult<PresenceMessage>>) = - legacyPresence.historyAsync(params.toTypedArray(), callback) + legacyPresence.historyAsync(params.toParamArray(), callback) }pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (2)
24-26: Fix typo in documentation.There's a typo in the documentation: "relqtes" should be "relates".
* Obtain recent history for this channel using the REST API. - * The history provided relqtes to all clients of this application, + * The history provided relates to all clients of this application, * not just this instance.
32-39: Enhance method documentation.Consider adding:
@throwsdocumentation for potential exceptions@sincetags for API versioning- Documentation for the
callbackparameter inhistoryAsync* @param params the request params. See the Ably REST API * documentation for more details. * @return Paginated result of Messages for this Channel. + * @throws IllegalArgumentException if params are invalid + * @since 1.0.0 */ fun history(params: List<Param>): PaginatedResult<Message> * Asynchronously obtain recent history for this channel using the REST API. * * @param params the request params. See the Ably REST API + * @param callback the callback to be called with the async result + * @throws IllegalArgumentException if params are invalid + * @since 1.0.0 */ fun historyAsync(params: List<Param>, callback: Callback<AsyncPaginatedResult<Message>>)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt (1)
16-18: Fix documentation reference.The documentation references
io.ably.types.Databut the correct package isio.ably.lib.types.Data.- * @param data the message payload; see [io.ably.types.Data] for details of supported data types. + * @param data the message payload; see [io.ably.lib.types.Data] for details of supported data types.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt (2)
20-20: Consider using default empty list for params.Since the parameters are optional with default values (as documented), consider providing a default empty list.
- fun get(params: List<Param>): PaginatedResult<PresenceMessage> + fun get(params: List<Param> = emptyList()): PaginatedResult<PresenceMessage>
37-37: Consider using default empty list for params.Since the parameters are optional with default values (as documented), consider providing a default empty list.
- fun getAsync(params: List<Param>, callback: Callback<AsyncPaginatedResult<PresenceMessage>>) + fun getAsync(params: List<Param> = emptyList(), callback: Callback<AsyncPaginatedResult<PresenceMessage>>)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt (1)
17-17: Consider using extension function for List to Array conversion.The conversion from
ListtoArrayis repeated in multiple methods. Consider extracting it to an extension function.Add the following extension function at the file level:
private fun <T> List<T>.toTypedArraySafe(): Array<T> = toTypedArray()Then update the methods:
- override fun publish(messages: List<Message>) = legacyChannel.publish(messages.toTypedArray()) + override fun publish(messages: List<Message>) = legacyChannel.publish(messages.toTypedArraySafe()) - override fun publishAsync(messages: List<Message>, listener: CompletionListener) = - legacyChannel.publishAsync(messages.toTypedArray(), listener) + override fun publishAsync(messages: List<Message>, listener: CompletionListener) = + legacyChannel.publishAsync(messages.toTypedArraySafe(), listener)Also applies to: 22-23
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (1)
14-14: Consider using a more idiomatic Kotlin factory function.The factory function could be more idiomatic by using an extension function.
-fun RealtimeClient(legacyClient: AblyRealtime): RealtimeClient = RealtimeClientAdapter(legacyClient) +fun AblyRealtime.asRealtimeClient(): RealtimeClient = RealtimeClientAdapter(this)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
26-26: Document exceptions and thread safety.Consider adding:
@Throwsannotation for potential exceptions- Thread safety documentation
+ /** + * @throws AblyException if the request fails + * @threadsafe This method is thread-safe + */ fun history(params: List<Param>): PaginatedResult<PresenceMessage>pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt (2)
8-15: Extract parameter building logic to a separate function.Consider extracting the parameter building logic to improve reusability and readability.
+ private fun buildGetParams(clientId: String?, connectionId: String?, waitForSync: Boolean): List<Param> = + buildList { + clientId?.let { add(Param(Presence.GET_CLIENTID, it)) } + connectionId?.let { add(Param(Presence.GET_CONNECTIONID, it)) } + add(Param(Presence.GET_WAITFORSYNC, waitForSync)) + } + override fun get(clientId: String?, connectionId: String?, waitForSync: Boolean): List<PresenceMessage> { - val params = buildList { - clientId?.let { add(Param(Presence.GET_CLIENTID, it)) } - connectionId?.let { add(Param(Presence.GET_CONNECTIONID, it)) } - add(Param(Presence.GET_WAITFORSYNC, waitForSync)) - } + val params = buildGetParams(clientId, connectionId, waitForSync) return legacyPresence.get(*params.toTypedArray()).toList() }
42-46: Add null safety checks for data parameter.Consider adding validation for the data parameter to prevent potential issues.
- override fun enter(data: Any?, listener: CompletionListener?) = legacyPresence.enter(data, listener) + override fun enter(data: Any?, listener: CompletionListener?) { + require(data == null || data is String || data is ByteArray || data is JsonElement) { + "Data must be null, String, ByteArray, or JsonElement" + } + legacyPresence.enter(data, listener) + }pubsub-adapter/build.gradle.kts (1)
1-10: LGTM! Consider adding version constraints.The build configuration is well-structured with appropriate plugins and dependencies. However, consider adding version constraints for the
:javaproject dependency to ensure build reproducibility.dependencies { - compileOnly(project(":java")) - testImplementation(project(":java")) + compileOnly(project(":java")) { + version { + strictly("[1.0,2.0)") // Example version range + } + } + testImplementation(project(":java")) { + version { + strictly("[1.0,2.0)") // Example version range + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/gradle.properties(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt(1 hunks)settings.gradle.kts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pubsub-adapter/gradle.properties
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check-realtime
- GitHub Check: check (19)
- GitHub Check: check-realtime-okhttp
🔇 Additional comments (13)
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (2)
1-13: Factory function approach looks good.
This top-level factory function cleanly wraps the adapter instantiation, providing a concise entry point for creatingRestClientAdapterinstances.
15-53: Adapter design is clear and consistent.
Your delegation tolegacyClienteffectively implements the adapter pattern for REST operations. Consider adding extra documentation around error handling or concurrency aspects if these methods are called from multiple threads concurrently.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt (1)
14-161: Interface is well-defined and aligns with AutoCloseable.
ExposingAutoCloseableensures that the implementing classes handle their resources properly. Ensure that any implementing class explicitly manages closure of underlying resources to prevent leaks.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt (1)
8-9: Looks good overall.The override of
channels: Channels<out RestChannel>is consistent with the existing design and properly documents the contract. No immediate issues found.pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt (2)
7-8: LGTM! Clean adapter implementation.The adapter pattern is well-implemented, providing a clean bridge between legacy and new interfaces. The
containsmethod correctly delegates to the legacycontainsKeymethod.
17-19: LGTM! Efficient iterator implementation.The iterator implementation efficiently uses Kotlin's sequence builder with
yieldfor lazy evaluation.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt (1)
6-44: Well-designed interface with comprehensive documentation!The interface provides a clean API with both synchronous and asynchronous methods, supporting single message and batch publishing operations. The documentation is thorough and includes spec references.
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt (1)
9-48: Excellent interface design with clear documentation!The generic interface provides a robust API for channel management:
- Extends
Iterablefor convenient iteration over channels- Clear documentation with spec references
- Well-defined preconditions for the
releasemethodpubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt (1)
8-29: Well-implemented adapter with clean delegation!The adapter pattern is implemented correctly with appropriate internal visibility and clean delegation to the legacy channel.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt (1)
7-53: LGTM!The adapter implementation is clean and correctly delegates all operations to the legacy channel. The conversion between Lists and Arrays is handled appropriately.
settings.gradle.kts (1)
17-17: LGTM!The
pubsub-adaptermodule is correctly included in the project configuration, following the existing naming convention.build.gradle.kts (1)
8-8: LGTM!The Kotlin JVM plugin is correctly added with
apply falseflag, allowing individual modules to opt-in to Kotlin support.gradle/libs.versions.toml (1)
22-22: Update to the latest stable Kotlin version.The specified Kotlin version (2.1.10) appears to be outdated. Consider updating to the latest stable version for security patches and new features.
Also applies to: 52-52
✅ Verification successful
🌐 Web query:
What is the latest stable version of Kotlin?💡 Result:
The latest stable version of Kotlin is 2.1.10, released on January 27, 2025[3][6]. This is a bug-fix update to the Kotlin 2.1 language release series, which introduced significant improvements like:
- Stable K2 compiler for all targets[4]
- Enhanced multiplatform support with stable Gradle DSL for compiler options[2][6]
- New language features in preview:
- Guard conditions in
whenstatements- Non-local
break/continue- Multi-dollar string interpolation[2]
Key details about current Kotlin versions:
Version Type Latest Release Status Key Features Language Release 2.1.0 Stable Foundation for K2 compiler improvements and multiplatform enhancements[4] Tooling Release 2.0.20 Stable Performance optimizations and Gradle compatibility updates[4] Bug Fix Release 2.1.10 Current Latest security patches and stability improvements[3][6] Experimental Build 2.1.20-Beta2 Development Testing new multiplatform DSL and kapt plugin improvements[5] The K2 compiler has been stable since Kotlin 2.0[4], and multiplatform development reached stability in Kotlin 1.9[9]. Developers are encouraged to update using Gradle with
kotlin("jvm") version "2.1.10"in build scripts[4].Citations:
- 1: https://blog.jetbrains.com/kotlin/category/releases/
- 2: https://blog.jetbrains.com/kotlin/2024/11/kotlin-2-1-0-released/
- 3: https://endoflife.date/kotlin
- 4: https://kotlinlang.org/docs/releases.html
- 5: https://kotlinlang.org/docs/whatsnew-eap.html
- 6: https://kotlinlang.org/docs/multiplatform-compatibility-guide.html
- 7: https://github.com/Kotlin/kotlinx.coroutines/releases
- 8: https://github.com/jetbrains/kotlin/releases
- 9: https://en.wikipedia.org/wiki/Kotlin_(programming_language)
Kotlin version is current and requires no update
The web query confirms that Kotlin 2.1.10 is indeed the latest stable version as of its January 27, 2025 release. Both line 22 and 52 in thegradle/libs.versions.tomlfile already reference the correct version.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
Outdated
Show resolved
Hide resolved
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt
Outdated
Show resolved
Hide resolved
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt
Outdated
Show resolved
Hide resolved
| import com.ably.pubsub.RealtimePresence | ||
| import io.ably.lib.types.* | ||
|
|
||
| internal class RealtimeChannelAdapter(private val legacyChannel: Channel) : RealtimeChannel { |
There was a problem hiding this comment.
Does legacy give the wrong vibe here? We're not really dealing with a "channel that isn't really a channel any more", just a different implementation?
There was a problem hiding this comment.
happy to rename :) what do you think is better: underlyingChannel, javaChannel, unwrappedChannel maybe something else?
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
96aabf3 to
227a060
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
40-45: 🛠️ Refactor suggestionUse a parameter object for history methods.
Having multiple parameters with default values can make the API less maintainable. Consider using a parameter object for better flexibility and future extensibility.
+/** + * Parameters for retrieving channel history. + * + * @property start The start of the query interval as a time in milliseconds since the epoch + * @property end The end of the query interval as a time in milliseconds since the epoch + * @property limit The maximum number of records to return (max: 1000) + * @property direction The direction of the query + */ +data class HistoryParams( + val start: Long? = null, + val end: Long? = null, + val limit: Int = 100, + val direction: Direction = Direction.Backwards +) { + init { + require(limit <= 1000) { "Limit must not exceed 1000" } + } +} - fun history( - start: Long? = null, - end: Long? = null, - limit: Int = 100, - direction: Direction = Direction.Backwards, - ): PaginatedResult<Message> + fun history(params: HistoryParams = HistoryParams()): PaginatedResult<Message> - fun historyAsync( - callback: Callback<AsyncPaginatedResult<Message>>, - start: Long? = null, - end: Long? = null, - limit: Int = 100, - direction: Direction = Direction.Backwards, - ) + fun historyAsync( + callback: Callback<AsyncPaginatedResult<Message>>, + params: HistoryParams = HistoryParams() + )Also applies to: 61-67
🧹 Nitpick comments (6)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt (1)
47-54: Consolidate publish overloads or provide rationale.Having multiple
publishoverloads is fine, but you might unify them into a single method that accepts a variation of parameters, or add a brief comment explaining the need for multiple signatures. This can reduce boilerplate and make the API more discoverable.pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt (1)
3-9: Consider additional HTTP methods if required.Currently, only GET, POST, PUT, DELETE, and PATCH are defined. If your use cases might require HEAD or OPTIONS in the future, consider adding them preemptively.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt (1)
12-17: Consider using a more concise parameter construction.The parameter list construction can be simplified using Kotlin's list builder functions.
- val params = buildList { - clientId?.let { add(Param(Presence.GET_CLIENTID, it)) } - connectionId?.let { add(Param(Presence.GET_CONNECTIONID, it)) } - add(Param(Presence.GET_WAITFORSYNC, waitForSync)) - } - return legacyPresence.get(*params.toTypedArray()).toList() + val params = listOfNotNull( + clientId?.let { Param(Presence.GET_CLIENTID, it) }, + connectionId?.let { Param(Presence.GET_CONNECTIONID, it) }, + Param(Presence.GET_WAITFORSYNC, waitForSync) + ) + return legacyPresence.get(*params.toTypedArray()).toList()pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (3)
4-4: Replace wildcard import with specific imports.Using wildcard imports can lead to naming conflicts and make dependencies harder to track. Consider explicitly importing only the required types from
io.ably.lib.types.-import io.ably.lib.types.* +import io.ably.lib.types.AsyncPaginatedResult +import io.ably.lib.types.Callback +import io.ably.lib.types.Message +import io.ably.lib.types.PaginatedResult
6-9: Enhance interface documentation.The current documentation could be more descriptive. Consider adding:
- Purpose and use cases of the channel
- Relationship with the Ably API
- Link to relevant Ably documentation
/** - * An interface representing a Channel in the Ably API. + * An interface representing a Channel in the Ably API, which enables real-time messaging + * and presence functionality. + * + * A Channel is the medium through which messages are distributed. Channels can represent + * different topics, rooms, or contexts in your application. + * + * @see <a href="https://ably.com/docs/realtime/channels">Ably Channels Documentation</a> */
11-22: Improve property documentation.The property documentation could be more descriptive and consistent:
- Add constraints and format requirements for the channel name
- Remove empty line in presence documentation
- Add more details about the presence functionality
/** - * The channel name. + * The channel name. Must be a non-empty string that uniquely identifies the channel. + * Channel names are case-sensitive and must match the pattern [a-zA-Z0-9_-]+ */ val name: String /** * A [Presence] object. - * - * + * Provides real-time presence functionality for this channel, allowing you to + * track the online status and receive updates about connected clients. * * Spec: RTL9 */ val presence: Presence
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/gradle.properties(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/Direction.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt(1 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- settings.gradle.kts
- build.gradle.kts
- pubsub-adapter/build.gradle.kts
- gradle/libs.versions.toml
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
- pubsub-adapter/gradle.properties
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check (29)
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (19)
- GitHub Check: check-realtime
- GitHub Check: check
🔇 Additional comments (7)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt (1)
10-10: Consider a more neutral property name.A previous reviewer raised concerns about the
legacyChannelnaming because it might imply obsolescence. Renaming it to something likedelegateChannelorwrappedChannelcan avoid negative connotations.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt (1)
154-154: Verify optionalclientIdusage.
leaveClientaccepts a nullableclientIdparameter. Is there a scenario where leaving a channel without specifying a client ID is valid? Confirm this aligns with your presence design and Ably specs.pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt (1)
1-12: LGTM! Well-designed interface.The
Subscriptioninterface is well-designed:
- Uses Kotlin's
fun interfaceappropriately for a single-method interface- Clear and descriptive naming
- Well-documented purpose
- Follows Single Responsibility Principle
pubsub-adapter/src/main/kotlin/com/ably/query/Direction.kt (1)
1-14: LGTM! Well-designed enum.The
Directionenum is well-designed:
- Clear and descriptive naming
- Well-documented purpose and behavior
- Appropriate encapsulation of string representation
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt (2)
20-20: Consider using named parameters for consistency.The method uses a list of parameters while
RealtimePresenceuses named parameters. We should maintain a consistent approach across the codebase.
37-37: Consider using named parameters for consistency.The async method also uses a list of parameters while
RealtimePresenceuses named parameters. We should maintain a consistent approach across the codebase.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
6-8: Update interface documentation to include all methods.The documentation comment should mention both retrieval of current and historic presence set, as the interface provides methods for both through its implementations.
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
227a060 to
6d70d6f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
27-32: Enhance parameter documentation with valid ranges.The
limitparameter documentation should explicitly state the valid range (1-1000). Additionally, consider documenting thatstartshould be less than or equal toendwhen both are provided.Apply this diff to improve the documentation:
/** * @param start (RSP4b1) - The time from which messages are retrieved, specified as milliseconds since the Unix epoch. * @param end (RSP4b1) - The time until messages are retrieved, specified as milliseconds since the Unix epoch. + * When both start and end are provided, start must be less than or equal to end. * @param direction (RSP4b2) - The order for which messages are returned in. Valid values are backwards which orders messages from most recent to oldest, * or forwards which orders messages from oldest to most recent. The default is backwards. - * @param limit (RSP4b3) - An upper limit on the number of messages returned. The default is 100, and the maximum is 1000. + * @param limit (RSP4b3) - An upper limit on the number of messages returned. Must be between 1 and 1000 (inclusive). Default is 100. */Also applies to: 50-56
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/gradle.properties(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/Direction.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt(1 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- settings.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt
- pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt
- pubsub-adapter/src/main/kotlin/com/ably/query/Direction.kt
- build.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt
- gradle/libs.versions.toml
- pubsub-adapter/build.gradle.kts
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt
- pubsub-adapter/gradle.properties
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (24)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-realtime
- GitHub Check: check-rest
- GitHub Check: build
🔇 Additional comments (7)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt (1)
5-35: LGTM! Well-documented interface with clear spec references.The interface is well-designed with:
- Clear documentation including spec references
- Consistent parameter naming and defaults
- Proper inheritance from base
Presenceinterfacepubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt (1)
13-39: LGTM! Clean adapter implementation.The adapter:
- Is correctly scoped as
internal- Properly delegates to legacy implementation
- Uses utility functions for parameter conversion
pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt (1)
7-16: Add parameter validation to utility functions.The functions should validate their input parameters to ensure data integrity:
limitshould be positive and not exceed 1000startandendtimestamps should be validated when both are providedApply this diff to add validation:
fun buildStatsParams( start: Long?, end: Long?, limit: Int, direction: Direction, unit: TimeUnit, ) = buildList { + require(limit in 1..1000) { "Limit must be between 1 and 1000" } + if (start != null && end != null) { + require(start <= end) { "Start time must be before or equal to end time" } + } addAll(buildHistoryParams(start, end, limit, direction)) add(Param("unit", unit.toString())) } fun buildHistoryParams( start: Long?, end: Long?, limit: Int, direction: Direction, ) = buildList { + require(limit in 1..1000) { "Limit must be between 1 and 1000" } + if (start != null && end != null) { + require(start <= end) { "Start time must be before or equal to end time" } + } start?.let { add(Param("start", it)) } end?.let { add(Param("end", it)) } add(Param("limit", limit)) add(Param("direction", direction.toString())) } fun buildRestPresenceParams( limit: Int, clientId: String?, connectionId: String?, ) = buildList { + require(limit in 1..1000) { "Limit must be between 1 and 1000" } add(Param("limit", limit)) clientId?.let { add(Param("clientId", it)) } connectionId?.let { add(Param("connectionId", it)) } }Also applies to: 18-28, 30-38
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt (4)
1-17: LGTM! Well-structured interface declaration with clear documentation.The interface is well-documented and follows Kotlin conventions. The AutoCloseable implementation is a good choice for resource management.
19-43: LGTM! Properties are well-defined with clear documentation.The properties are well-documented with spec references. The use of
outvariance for Channel inchannelsproperty ensures type safety while allowing covariance.
45-71: LGTM! Time-related methods are well-implemented.Both synchronous and asynchronous time methods are well-documented with clear spec references. The documentation properly guides users about when to use these methods versus
ClientOptions.queryTime.
119-175: LGTM! Request methods address previous review concerns.The implementation addresses previous review concerns:
- The
bodyparameter is correctly marked as nullable, addressing the concern about optional bodies in GET requests.- The use of
HttpMethodenum provides type safety for the method parameter.
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
6d70d6f to
e62d4aa
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
| start: Long? = null, | ||
| end: Long? = null, | ||
| limit: Int = 100, | ||
| direction: Direction = Direction.Backwards, |
There was a problem hiding this comment.
Possibly not here, but an opportunity to go with the new "orderBy" enum you proposed for Chat, even if we translate it under the hood?
| * @param listener a listener to be notified on completion of the operation. | ||
| * This listener is invoked on a background thread. | ||
| */ | ||
| fun updatePresence(msg: PresenceMessage, listener: CompletionListener? = null) |
There was a problem hiding this comment.
Whats the difference between this and update?
There was a problem hiding this comment.
This is basically simply send PresenceMessage you can perform any action, it's not part of the spec, so I deleted it. Thanks for pointing out
| import com.ably.pubsub.RealtimePresence | ||
| import io.ably.lib.types.* | ||
|
|
||
| internal class RealtimeChannelAdapter(private val legacyChannel: Channel) : RealtimeChannel { |
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
e62d4aa to
58fdc8a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (5)
6-9: Enhance interface documentation.Consider expanding the documentation to include:
- The interface's role in the PubSub system
- Its relationship with
RealtimeChannelandRestChannel- Common use cases
- Link to relevant Ably documentation
/** - * An interface representing a Channel in the Ably API. + * An interface representing a Channel in the Ably API. This serves as the base interface + * for both [RealtimeChannel] and [RestChannel], providing common channel functionality + * such as history retrieval and presence management. + * + * A channel is the medium through which messages are distributed. Channels can represent + * different topics, rooms, or contexts in your application. + * + * @see <a href="https://ably.com/docs/realtime/channels">Ably Channels Documentation</a> */
11-14: Add channel naming constraints to documentation.The documentation should specify channel naming rules and constraints.
/** * The channel name. + * + * Channel names: + * - Can contain any unicode characters except colon (':') + * - Are limited to 250 characters + * - Are case-sensitive + * + * @see <a href="https://ably.com/docs/realtime/channels#channel-naming">Channel Naming Rules</a> */
16-22: Expand presence documentation.The documentation should better explain the Presence functionality and its use cases.
/** * A [Presence] object. + * + * The Presence object enables clients to be notified when other clients enter or leave + * the channel (presence events) and get the set of current members on the channel + * (presence state). * + * Common use cases include: + * - Online status indicators + * - Typing indicators + * - User activity tracking * * Spec: RTL9 */
24-44: Add parameter validation for limit.The documentation states that "A limit greater than 1,000 is invalid" but there's no validation in place. Consider adding parameter validation or documenting the behavior when invalid values are provided.
/** * Obtain recent history for this channel using the REST API. * The history provided relates to all clients of this application, * not just this instance. * * @param start The start of the query interval as a time in milliseconds since the epoch. * A message qualifies as a member of the result set if it was received at or after this time. (default: beginning of time) * @param end The end of the query interval as a time in milliseconds since the epoch. * A message qualifies as a member of the result set if it was received at or before this time. (default: now) - * @param limit The maximum number of records to return. A limit greater than 1,000 is invalid. + * @param limit The maximum number of records to return. Must be between 1 and 1000 inclusive. + * @throws IllegalArgumentException if limit is less than 1 or greater than 1000 * @param orderBy The direction of this query. * * @return Paginated result of Messages for this Channel. */
45-63: Enhance documentation about threading behavior.The documentation should provide more details about the threading model and potential threading considerations.
/** * Asynchronously obtain recent history for this channel using the REST API. + * + * This method is non-blocking and will return immediately. The results will be + * delivered via the provided callback. * * @param start The start of the query interval as a time in milliseconds since the epoch. * A message qualifies as a member of the result set if it was received at or after this time. (default: beginning of time) * @param end The end of the query interval as a time in milliseconds since the epoch. * A message qualifies as a member of the result set if it was received at or before this time. (default: now) * @param limit The maximum number of records to return. A limit greater than 1,000 is invalid. * @param orderBy The direction of this query. * @param callback A Callback returning [AsyncPaginatedResult] object containing an array of [Message] objects. - * Note: This callback is invoked on a background thread. + * Note: This callback is invoked on a background thread. If you need to update UI elements + * in the callback, ensure you switch to the main/UI thread first. + * + * @throws IllegalArgumentException if limit is less than 1 or greater than 1000 */pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
26-31: Consider adding runtime validation for the limit parameter.The documentation specifies a maximum limit of 1000, but there's no runtime validation in the interface. Consider adding parameter validation in the implementing classes.
fun history( start: Long? = null, end: Long? = null, - limit: Int = 100, + limit: Int = 100, +) = apply { + require(limit in 1..1000) { "Limit must be between 1 and 1000" } orderBy: OrderBy = OrderBy.NewestFirst, ): PaginatedResult<PresenceMessage>pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt (1)
86-86: Consider clarifying the difference between enter and update methods.While both methods can be used to set presence data, their behaviors differ when called before entering the presence set. Consider adding a note in the documentation to clarify when to use each method.
/** * Enters the presence set for the channel, optionally passing a data payload. * A clientId is required to be present on a channel. * An optional callback may be provided to notify of the success or failure of the operation. + * Note: Use this method for initial presence registration. For subsequent updates, use update(). * * Spec: RTP8 */ fun enter(data: Any? = null, listener: CompletionListener? = null) /** * Updates the data payload for a presence member. * If called before entering the presence set, this is treated as an [PresenceMessage.Action.enter] event. * An optional callback may be provided to notify of the success or failure of the operation. + * Note: Use this method to update presence data after initial registration with enter(). * * Spec: RTP9 */ fun update(data: Any? = null, listener: CompletionListener? = null)Also applies to: 99-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/gradle.properties(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/OrderBy.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt(1 hunks)settings.gradle.kts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pubsub-adapter/src/main/kotlin/com/ably/query/OrderBy.kt
🚧 Files skipped from review as they are similar to previous changes (18)
- settings.gradle.kts
- build.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt
- pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt
- pubsub-adapter/gradle.properties
- gradle/libs.versions.toml
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt
- pubsub-adapter/build.gradle.kts
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-rest
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-realtime
🔇 Additional comments (12)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
1-5: LGTM!The package name and imports are well-organized and follow standard conventions.
pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt (1)
1-16: LGTM! The implementation is now complete and well-documented.The enum class correctly implements all required time units (minute, hour, day, month) with proper documentation and string representation.
pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt (1)
7-16: Add parameter validation to ensure data integrity.The parameter building functions should validate their inputs:
limitshould be positivestartandendtimestamps should be validated when both are providedApply this diff to add validation:
fun buildStatsParams( start: Long?, end: Long?, limit: Int, orderBy: OrderBy, unit: TimeUnit, ) = buildList { + require(limit > 0) { "Limit must be positive" } + if (start != null && end != null) { + require(start <= end) { "Start time must be before or equal to end time" } + } addAll(buildHistoryParams(start, end, limit, orderBy)) add(Param("unit", unit.toString())) } fun buildHistoryParams( start: Long?, end: Long?, limit: Int, orderBy: OrderBy, ) = buildList { + require(limit > 0) { "Limit must be positive" } + if (start != null && end != null) { + require(start <= end) { "Start time must be before or equal to end time" } + } start?.let { add(Param("start", it)) } end?.let { add(Param("end", it)) } add(Param("limit", limit)) add(Param("direction", orderBy.direction)) } fun buildRestPresenceParams( limit: Int, clientId: String?, connectionId: String?, ) = buildList { + require(limit > 0) { "Limit must be positive" } add(Param("limit", limit)) clientId?.let { add(Param("clientId", it)) } connectionId?.let { add(Param("connectionId", it)) } }Also applies to: 18-28, 30-38
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt (1)
13-39: LGTM! Well-structured adapter implementation.The adapter correctly:
- Implements the RestPresence interface
- Delegates to the legacy presence implementation
- Converts parameters using utility functions
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (2)
50-57: Replace unsafe null assertion with proper error handling.The use of
!!operator is unsafe and could lead to runtime exceptions.Apply this diff to handle the null case explicitly:
override fun request( path: String, method: HttpMethod, params: List<Param>, body: HttpCore.RequestBody?, headers: List<Param>, - ) = javaClient.request(method.toString(), path, params.toTypedArray(), body, headers.toTypedArray())!! + ) = javaClient.request(method.toString(), path, params.toTypedArray(), body, headers.toTypedArray()) + ?: throw IllegalStateException("Request failed: null response from legacy client")
14-18: LGTM! Well-structured adapter implementation with proper delegation.The adapter correctly:
- Provides a convenient factory function
- Implements all required properties and methods
- Properly delegates to the legacy client
Also applies to: 19-49, 58-68
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt (2)
10-10: LGTM! Well-structured adapter implementation.The adapter pattern is well-implemented, providing a clean Kotlin interface over the Java implementation.
26-31: LGTM! Excellent subscription management.The subscription implementation properly handles cleanup by returning a
Subscriptionobject that encapsulates the unsubscribe logic.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
6-8: Documentation has been previously addressed.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt (2)
17-17: LGTM! Good use of AutoCloseable.Implementing
AutoCloseableensures proper resource cleanup when used with try-with-resources.
136-142: Verify HTTP method handling in implementations.The interface uses an enum for HTTP methods, which is good for type safety. However, implementations should handle all possible HTTP methods correctly.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for HTTP method handling in implementations rg -A 5 "HttpMethod\." --type kotlinLength of output: 1079
I'll run an additional script to search if there are implementations of the
requestmethod (usingoverride fun request) within the codebase to verify that all HTTP methods provided by theHttpMethodenum are handled appropriately.
🏁 Script executed:
#!/bin/bash # Search for implementations of the request method rg -A 5 "override fun request" --type kotlinLength of output: 2467
I'll run a script to search for a
whenstatement that handles themethodargument in the implementations, which should confirm that all enum variants are appropriately managed.
🏁 Script executed:
#!/bin/bash # Search for handling of the 'method' parameter with a when statement in the repository rg -A 10 "when\s*\(method" --type kotlinLength of output: 42
HTTP Method Handling Verification – Manual Check Required
The implementations in both RestClientAdapter and RealtimeClientAdapter simply pass the HttpMethod parameter without explicit branching (e.g., using a
whenstatement). This suggests that handling might be delegated to an underlying HTTP client/library. However, because our searches did not reveal dedicated logic to explicitly handle all enum variants, please manually verify that the underlying implementation supports every HTTP method defined in HttpMethod.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt (1)
16-16: LGTM! Well-designed interface hierarchy.The interface properly extends
Presenceand provides a clear separation of concerns for realtime functionality.
This is the initial version of the Kotlin wrapper for `ably-java` and `ably-android`. The main goal is to provide an extendable public interface for the Ably PubSub SDK (the core SDK for Chat and other products). This wrapper SDK is the first step toward modernizing our libraries. It introduces a set of interfaces that can be modified, for example, to inject agent information or other data when used within new product SDKs. Note: In this PR we are not going to provide fully updated and idiomatic public API for `ably-java` and `ably-android`, this is the initial step of public API modernization, we will continue working on this.
Adapters for the Kotlin API for `ably-java` and `ably-android`
ttypic
force-pushed
the
create-kotlin-wrapper
branch
from
58fdc8a to
18a5020
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1064/features
github-actions
bot
temporarily deployed
to
staging/pull/1064/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt (1)
65-99:⚠️ Potential issueAdd unsubscribe methods to prevent memory leaks.
The interface is missing unsubscribe methods that correspond to the subscribe methods. This could lead to memory leaks if listeners aren't properly cleaned up.
Add the following methods to match the subscribe methods:
/** * Deregisters all matching message listeners. * * @param listener The listener to deregister. */ fun unsubscribe(listener: MessageListener) /** * Deregisters the listener for the specified event name. * * @param eventName The event name. * @param listener The listener to deregister. */ fun unsubscribe(eventName: String, listener: MessageListener) /** * Deregisters the listener for the specified event names. * * @param eventNames A list of event names. * @param listener The listener to deregister. */ fun unsubscribe(eventNames: List<String>, listener: MessageListener) /** * Deregisters all message listeners. */ fun unsubscribe()
🧹 Nitpick comments (7)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt (2)
13-41: Consider makingreasonproperty nullable.Since
reasonrepresents the last error which occurred on the channel (if any), it should be marked as nullable usingErrorInfo?.- val reason: ErrorInfo + val reason: ErrorInfo?
139-146: Consider returningthisfor method chaining.The
setOptionsmethod could returnRealtimeChannelto enable method chaining, which is a common pattern in Kotlin.- fun setOptions(options: ChannelOptions) + fun setOptions(options: ChannelOptions): RealtimeChannelpubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (5)
4-4: Consider using explicit imports instead of wildcard imports.Replace the wildcard import with explicit imports for the types you need from
io.ably.lib.types. This improves code readability and helps prevent potential naming conflicts.
6-9: Enhance interface documentation.As this is a public API, consider adding more comprehensive documentation:
- Thread safety guarantees
- Error handling behavior
- Common use cases or examples
- Link to Ably's documentation for this concept
11-14: Add channel name constraints to documentation.Consider documenting:
- Valid channel name patterns
- Length restrictions
- Reserved characters or prefixes
- Case sensitivity rules
16-22: Improve presence property documentation.The documentation has formatting issues and could be more descriptive:
- Remove empty lines in KDoc
- Add description of the Presence concept
- Explain common use cases
- Link to the full RTL9 spec
57-63: Consider modernizing async operations with coroutines.The current callback-based approach could be improved:
- Add a suspend function variant using coroutines
- Move the callback parameter to the end to follow Kotlin conventions
- Consider adding Flow API support for real-time updates
Example:
suspend fun history( start: Long? = null, end: Long? = null, limit: Int = 100, orderBy: OrderBy = OrderBy.NewestFirst, ): PaginatedResult<Message> fun historyAsync( start: Long? = null, end: Long? = null, limit: Int = 100, orderBy: OrderBy = OrderBy.NewestFirst, callback: Callback<AsyncPaginatedResult<Message>> )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
build.gradle.kts(1 hunks)gradle/libs.versions.toml(2 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/gradle.properties(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/OrderBy.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt(1 hunks)settings.gradle.kts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- build.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/query/OrderBy.kt
- settings.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/Subscription.kt
- pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt
- pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt
- pubsub-adapter/gradle.properties
- gradle/libs.versions.toml
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestPresence.kt
- pubsub-adapter/build.gradle.kts
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelsAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestPresenceAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestChannelAdapter.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check-rest
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check-realtime-okhttp
🔇 Additional comments (8)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt (3)
1-11: LGTM!Package name and imports are well-organized and follow Kotlin conventions.
43-64: LGTM!Channel lifecycle methods are well-documented and properly implemented with optional completion listeners.
113-113: Clarify the purpose of nullable parameters in publish method.The method signature allows both
nameanddatato be null, which could lead to confusion about what happens when both are null.Consider:
- Adding documentation to explain the behavior when both parameters are null.
- Or, if publishing with null values is not intended, remove the nullable types:
- fun publish(name: String? = null, data: Any? = null, listener: CompletionListener? = null) + fun publish(name: String, data: Any, listener: CompletionListener? = null)pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt (5)
1-17: LGTM! Well-structured interface declaration with clear documentation.The interface is well-organized with appropriate imports and clear documentation explaining its purpose and
AutoCloseableimplementation.
19-43: LGTM! Well-documented properties with appropriate type safety.Properties are well-defined with clear documentation and spec references. The use of
outvariance inChannels<out Channel>correctly enforces type safety for read-only channel access.
45-71: LGTM! Well-implemented time retrieval methods with clear documentation.Both synchronous and asynchronous time retrieval methods are well-documented with important notes about
ClientOptions.queryTime. The async callback's threading behavior is properly documented.
73-115: Add runtime validation for the limit parameter.The documentation specifies that the maximum limit is 1000, but there's no runtime validation in the interface. Consider adding parameter validation or clarifying that the enforcement is handled elsewhere.
Since this is an interface, you could add documentation about where the limit is enforced, or consider adding a companion object with validation:
interface Client : AutoCloseable { companion object { const val MAX_STATS_LIMIT = 1000 } // ... existing properties ... /** * @throws IllegalArgumentException if limit exceeds [MAX_STATS_LIMIT] */ fun stats( start: Long? = null, end: Long? = null, limit: Int = 100, orderBy: OrderBy = OrderBy.NewestFirst, unit: TimeUnit = TimeUnit.Minute, ): PaginatedResult<Stats> }
117-173: LGTM! Well-designed request methods with comprehensive documentation.The request methods are well-implemented with:
- Clear documentation including REST API reference links
- Appropriate default values
- Proper nullable parameters
- Consistent sync/async pattern
- make ErrorReason on the channel nullable - improve docstrings
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
60-65: 🛠️ Refactor suggestionAdd runtime validation for the limit parameter.
The documentation states that "A limit greater than 1,000 is invalid", but there's no runtime validation. Consider adding parameter validation to throw an appropriate exception when the limit exceeds 1,000.
🧹 Nitpick comments (1)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
18-28: Consider adding runtime validation for channel name constraints.The documentation specifies that channel names:
- Cannot contain colons (':')
- Are limited to 250 characters
- Are case-sensitive
Consider adding runtime validation to enforce these constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-rest
- GitHub Check: check (21)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (19)
- GitHub Check: check-realtime
🔇 Additional comments (2)
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (2)
6-16: Well-documented interface!The documentation is comprehensive, well-structured, and includes relevant links to official documentation.
30-44: Excellent documentation of the presence property!The documentation includes clear use cases and spec reference.
This is the initial version of the Kotlin wrapper for
ably-javaandably-android. The main goal is to provide an extendable public interface for the Ably PubSub SDK (the core SDK for Chat and other products). This wrapper SDK is the first step toward modernizing our libraries. It introduces a set of interfaces that can be modified, for example, to inject agent information or other data when used within new product SDKs.Note: In this PR we are not going to provide fully updated and idiomatic public API for
ably-javaandably-android, this is the initial step of public API modernization, we will continue working on this.Summary by CodeRabbit
New Features
Chores