[CHAT-5063] feat: add createWrapperSdkProxy implementation for kotlin adapter of ably-java
#1066
Conversation
WalkthroughThis PR introduces several improvements across dependency configuration, HTTP client handling, and SDK proxy functionality. The version catalog now includes updated coroutine and turbine versions along with new library declarations. Several HTTP and REST methods have been refactored to accept an explicit HTTP parameter. New constructors and exchange methods enhance client flexibility and dynamic agent support. Additionally, SDK wrapper interfaces and proxy clients have been introduced for both Realtime and REST clients, and the testing framework is extended with new dependencies, utility functions, and embedded server support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Http
participant AsyncHttpScheduler
Client->>Http: exchangeHttpCore(httpCore)
Http->>AsyncHttpScheduler: invoke new constructor with CloseableExecutor
AsyncHttpScheduler-->>Http: return new instance
sequenceDiagram
participant SDKClient
participant RealtimeClientAdapter
participant JavaClient
participant WrapperRealtimeClient
SDKClient->>RealtimeClientAdapter: createWrapperSdkProxy(options)
RealtimeClientAdapter->>JavaClient: inject dynamic agents & exchange HTTP core
RealtimeClientAdapter->>WrapperRealtimeClient: instantiate wrapped client
WrapperRealtimeClient-->>SDKClient: return proxy client instance
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
- we introduced several package-private methods that make it possible to use custom http module to invoke Rest API We want to be able to distinguish requests made inside the wrapper from those made by the core PubSub SDK without the wrapper, allowing us to track agents across wrapper SDKs such as the Chat SDK or Asset Tracking. To achieve this, we introduce special proxy Realtime and Rest clients that inject additional agents parameters into the underlying SDK.
ttypic
force-pushed
the
CHAT-5063/add-sdk-wrapper-kotlin
branch
from
9dd1d06 to
585e5d5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1066/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 6
🧹 Nitpick comments (21)
pubsub-adapter/src/main/kotlin/com/ably/http/HttpMethod.kt (2)
3-9: Add KDoc documentation to the enum class.Consider adding documentation to explain the purpose and usage of this enum class, especially since it's part of a public API.
+/** + * Represents HTTP methods supported by the Ably REST API. + * Used by the wrapper SDK proxy to make HTTP requests to Ably's servers. + */ enum class HttpMethod(private val method: String) {
3-12: Consider enhancing the enum implementation.The implementation looks good! Here are some suggestions to make it more robust:
- Consider implementing a common interface or marking the enum as sealed for better type safety
- Consider adding support for other HTTP methods like HEAD, OPTIONS that might be needed in the future
- Consider adding a companion object with a method to parse string representations safely
Here's an example implementation:
/** * Represents HTTP methods supported by the Ably REST API. * Used by the wrapper SDK proxy to make HTTP requests to Ably's servers. */ sealed class HttpMethod(private val method: String) { object Get : HttpMethod("GET") object Post : HttpMethod("POST") object Put : HttpMethod("PUT") object Delete : HttpMethod("DELETE") object Patch : HttpMethod("PATCH") // Add more methods as needed override fun toString() = method companion object { /** * Safely creates an HttpMethod from its string representation. * @param method The HTTP method string (case-insensitive) * @return The corresponding HttpMethod or null if not supported */ fun fromString(method: String): HttpMethod? = when (method.uppercase()) { "GET" -> Get "POST" -> Post "PUT" -> Put "DELETE" -> Delete "PATCH" -> Patch else -> null } } }pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
21-36: Consider robust error handling for history retrieval.Currently, calls to
javaChannel.historyandjavaChannel.historyAsyncare delegated directly. For improved resilience, consider how you might handle or propagate errors (e.g., network timeouts or permission issues) more gracefully using your wrapper.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeChannel.kt (2)
42-63: Consider clarifying concurrency and error handling forattach/detach.
When multiple threads invokeattachordetachconcurrently, certain race conditions might arise if the underlying implementation does not handle synchronization. Documenting or enforcing concurrency strategies (e.g., making the calls idempotent) within the contract would help consumers.
64-137: Unify event subscription overloads and enhance parameter clarity.
This interface provides multiplesubscribeoverloads, as well as multiplepublishoverloads. While they are flexible, consider introducing a single or vararg-based approach for event names to reduce repetition. Similarly, clarifying the concurrency model (e.g., which thread publishes messages) can help developers build robust integrations.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Client.kt (2)
17-44: Align doc references with Kotlin conventions.
These doc lines reference “{@link AutoCloseable}” in a JavaDoc style. For a Kotlin interface, consider using KDoc formatting (e.g.,[AutoCloseable]) to better align with Kotlin standards. This improves readability for Kotlin consumers.
73-174: Evaluate replacing sync/async methods with coroutines or reactive approaches.
Currently, the interface exposes synchronous methods (stats,request) and asynchronous counterparts (statsAsync,requestAsync). While this is fine, a unified coroutine- or reactive-based approach (e.g., using Flow, suspend functions, or RxJava Observables) could simplify usage. If you prefer the current approach, ensure thorough coverage in documentation and tests to help consumers handle both patterns effectively.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimePresence.kt (1)
37-74: Consolidatesubscribeoverloads for better maintainability.
Similar toRealtimeChannel, thesubscribeoverloads accept single or multiple actions. If you frequently change presence-subscription logic, consider standardizing the method signatures (e.g., a vararg of actions or a single method with optional parameters) to reduce overhead in code maintenance.pubsub-adapter/src/main/kotlin/com/ably/query/TimeUnit.kt (1)
1-16: Consider adding a default value constant.The documentation mentions that if omitted, the unit defaults to "minute". Consider adding a companion object with a default value for easier access:
enum class TimeUnit(private val unit: String) { Minute("minute"), Hour("hour"), Day("day"), Month("month"), ; override fun toString() = unit + + companion object { + val DEFAULT = Minute + } }pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt (1)
3-3: Consider validating agent map entries.The
WrapperSdkProxyOptionsdata class accepts any string key-value pairs. Consider adding validation or documentation for expected agent keys and values.-data class WrapperSdkProxyOptions(val agents: Map<String, String>) +data class WrapperSdkProxyOptions(val agents: Map<String, String>) { + init { + require(agents.all { (key, value) -> key.isNotBlank() && value.isNotBlank() }) { + "Agent keys and values must not be blank" + } + } +}pubsub-adapter/src/main/kotlin/io/ably/lib/Utils.kt (1)
7-16: Add input validation for parameters.The utility functions accept parameters without validation. Consider adding checks for:
- Negative timestamps
- Non-positive limits
- Blank client/connection IDs
Apply 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" } + start?.let { require(it >= 0) { "Start timestamp must not be negative" } } + end?.let { require(it >= 0) { "End timestamp must not be negative" } } + end?.let { start?.let { s -> require(it >= s) { "End must not be before start" } } } 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" } + start?.let { require(it >= 0) { "Start timestamp must not be negative" } } + end?.let { require(it >= 0) { "End timestamp must not be negative" } } + end?.let { start?.let { s -> require(it >= s) { "End must not be before start" } } } 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" } + clientId?.let { require(it.isNotBlank()) { "ClientId must not be blank" } } + connectionId?.let { require(it.isNotBlank()) { "ConnectionId must not be blank" } } 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/realtime/RealtimeChannelsAdapter.kt (1)
7-20: Consider abstracting common adapter code.The implementation is nearly identical to
RestChannelsAdapter. Consider creating a common base class or interface to reduce code duplication.Example abstraction:
abstract class BaseChannelsAdapter<T, C>(protected val javaChannels: C) : Channels<T> where C : AblyBase.Channels { override fun contains(name: String): Boolean = javaChannels.containsKey(name) override fun release(name: String) = javaChannels.release(name) protected abstract fun wrapChannel(channel: io.ably.lib.types.Channel): T override fun get(name: String): T = wrapChannel(javaChannels.get(name)) override fun get(name: String, options: ChannelOptions): T = wrapChannel(javaChannels.get(name, options)) override fun iterator(): Iterator<T> = iterator { javaChannels.entrySet().forEach { yield(wrapChannel(it.value)) } } } internal class RestChannelsAdapter(javaChannels: AblyBase.Channels) : BaseChannelsAdapter<RestChannel, AblyBase.Channels>(javaChannels) { override fun wrapChannel(channel: io.ably.lib.types.Channel) = RestChannelAdapter(channel) } internal class RealtimeChannelsAdapter(javaChannels: AblyRealtime.Channels) : BaseChannelsAdapter<RealtimeChannel, AblyRealtime.Channels>(javaChannels) { override fun wrapChannel(channel: io.ably.lib.types.Channel) = RealtimeChannelAdapter(channel) }pubsub-adapter/src/main/kotlin/com/ably/pubsub/RestChannel.kt (1)
17-17: Fix incorrect package reference in KDoc.The reference to
io.ably.types.Datashould beio.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/io/ably/lib/rest/RestPresenceAdapter.kt (1)
21-26: Consider using a data class for async method parameters.The async methods have long parameter lists which could be encapsulated in a data class for better maintainability.
Example:
data class HistoryParams( val start: Long?, val end: Long?, val limit: Int, val orderBy: OrderBy ) fun historyAsync( callback: Callback<AsyncPaginatedResult<PresenceMessage>>, params: HistoryParams )Also applies to: 31-38
java/src/main/java/io/ably/lib/rest/AblyRest.java (1)
38-44: Enhance constructor documentation.While the constructor implementation is correct, consider adding
@paramdocumentation for better clarity:/** * Constructor implementation to be able to have shallow copy of the client, * allowing us to modify certain fields while implementing a proxy for the Realtime/Rest SDK wrapper + * + * @param underlyingClient The original client to copy from + * @param httpCore The HTTP core implementation to use + * @param http The HTTP implementation to use */pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
57-63: Consider adding cancellation support for async operation.For better resource management, consider adding a way to cancel the async operation if the result is no longer needed.
fun historyAsync( callback: Callback<AsyncPaginatedResult<Message>>, start: Long? = null, end: Long? = null, limit: Int = 100, orderBy: OrderBy = OrderBy.NewestFirst, - ) + ): Cancellablelib/src/main/java/io/ably/lib/http/Http.java (1)
29-31: Add null check for httpCore parameter.The
exchangeHttpCoremethod should validate the input parameter to prevent NullPointerException.public Http exchangeHttpCore(HttpCore httpCore) { + if (httpCore == null) { + throw new IllegalArgumentException("httpCore cannot be null"); + } return new Http(asyncHttp.exchangeHttpCore(httpCore), new SyncHttpScheduler(httpCore)); }pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimePresenceAdapter.kt (1)
11-18: Optimize parameter list construction for single parameter case.The
buildListconstruction could be optimized when onlywaitForSyncparameter is present (the common case).override fun get(clientId: String?, connectionId: String?, waitForSync: Boolean): List<PresenceMessage> { + if (clientId == null && connectionId == null) { + return javaPresence.get(Param(Presence.GET_WAITFORSYNC, waitForSync)).toList() + } 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 javaPresence.get(*params.toTypedArray()).toList() }pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeChannelAdapter.kt (3)
40-45: Optimize array conversion for event names.The
eventNameslist is converted to array twice - once for subscribe and once for unsubscribe. Consider storing the array to avoid duplicate conversion.override fun subscribe(eventNames: List<String>, listener: ChannelBase.MessageListener): Subscription { - javaChannel.subscribe(eventNames.toTypedArray(), listener) + val events = eventNames.toTypedArray() + javaChannel.subscribe(events, listener) return Subscription { - javaChannel.unsubscribe(eventNames.toTypedArray(), listener) + javaChannel.unsubscribe(events, listener) } }
52-54: Add error handling for empty message list.The
publishmethod should validate that the message list is not empty before publishing.override fun publish(messages: List<Message>, listener: CompletionListener?) = + if (messages.isEmpty()) { + listener?.onError(ErrorInfo("Cannot publish empty message list", 40000)) + } else { javaChannel.publish(messages.toTypedArray(), listener) + }
55-57: Verify history parameters before querying.The
historymethod should validate that the start time is not after the end time when both are provided.override fun history(start: Long?, end: Long?, limit: Int, orderBy: OrderBy): PaginatedResult<Message> = + if (start != null && end != null && start > end) { + throw IllegalArgumentException("Start time cannot be after end time") + } javaChannel.history(buildHistoryParams(start, end, limit, orderBy).toTypedArray())
🛑 Comments failed to post (6)
lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java (1)
27-34: 💡 Verification agent
🧩 Analysis chain
Consider restricting access to internal clients.
While the method is documented as internal, its public access could lead to unintended usage. Consider using package-private access or moving it to an internal package.
Let's verify the usage pattern of this method:
🏁 Script executed:
#!/bin/bash # Search for usages of exchangeHttpCore to ensure it's only used by proxy/wrapper implementations rg "exchangeHttpCore" --type javaLength of output: 400
Restrict the access level of
exchangeHttpCoreto prevent unintended external use.The method is intended for internal use only, yet its public modifier could allow external access. Our search confirms that the only invocations occur within the library—specifically, in
lib/src/main/java/io/ably/lib/http/Http.java. Consider changing the modifier from public to package-private (or moving it to an internal package) to align with its documented internal purpose.pubsub-adapter/src/main/kotlin/com/ably/pubsub/RealtimeClient.kt (1)
5-11:
⚠️ Potential issueFix documentation about AutoCloseable.
The documentation mentions that the class implements
AutoCloseable, but the interface doesn't actually implement it. Either:
- Add
AutoCloseableto the interface, or- Remove the
AutoCloseablereference from the documentationIf option 1 is chosen, apply this diff:
-interface RealtimeClient : Client { +interface RealtimeClient : Client, AutoCloseable {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * A client that extends the functionality of the {@link Client} and provides additional realtime-specific features. * * This class implements {@link AutoCloseable} so you can use it in * try-with-resources constructs and have the JDK close it for you. */ interface RealtimeClient : Client, AutoCloseable {pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt (1)
15-19:
⚠️ Potential issueAdd runtime safety checks for type casting.
The extension functions use unsafe type casts which could fail at runtime if the client doesn't implement
SdkWrapperCompatible. Consider adding runtime checks or using safe casts.Apply this diff to add runtime safety:
fun RealtimeClient.createWrapperSdkProxy(options: WrapperSdkProxyOptions): RealtimeClient = - (this as SdkWrapperCompatible<*>).createWrapperSdkProxy(options) as RealtimeClient + when (this) { + is SdkWrapperCompatible<*> -> createWrapperSdkProxy(options) as? RealtimeClient + ?: throw IllegalStateException("Failed to create RealtimeClient proxy") + else -> throw IllegalStateException("Client does not implement SdkWrapperCompatible") + } fun RestClient.createWrapperSdkProxy(options: WrapperSdkProxyOptions): RestClient = - (this as SdkWrapperCompatible<*>).createWrapperSdkProxy(options) as RestClient + when (this) { + is SdkWrapperCompatible<*> -> createWrapperSdkProxy(options) as? RestClient + ?: throw IllegalStateException("Failed to create RestClient proxy") + else -> throw IllegalStateException("Client does not implement SdkWrapperCompatible") + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.fun RealtimeClient.createWrapperSdkProxy(options: WrapperSdkProxyOptions): RealtimeClient = when (this) { is SdkWrapperCompatible<*> -> createWrapperSdkProxy(options) as? RealtimeClient ?: throw IllegalStateException("Failed to create RealtimeClient proxy") else -> throw IllegalStateException("Client does not implement SdkWrapperCompatible") } fun RestClient.createWrapperSdkProxy(options: WrapperSdkProxyOptions): RestClient = when (this) { is SdkWrapperCompatible<*> -> createWrapperSdkProxy(options) as? RestClient ?: throw IllegalStateException("Failed to create RestClient proxy") else -> throw IllegalStateException("Client does not implement SdkWrapperCompatible") }pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channel.kt (1)
38-43: 💡 Verification agent
🧩 Analysis chain
Add parameter validation for limit.
The documentation states that a limit greater than 1,000 is invalid, but there's no validation mentioned. Consider adding validation in the implementing classes.
🏁 Script executed:
#!/bin/bash # Search for existing limit validations in channel implementations rg -A 5 "limit.*1000|1000.*limit" --type kotlinLength of output: 4917
Introduce limit parameter validation in Channel implementations.
The interface method for history currently accepts a limit without enforcing the documented maximum of 1000. Similar documentation in Presence and RestPresence indicates that values exceeding 1000 are invalid. Please add validation logic in the concrete implementations of this interface to ensure that the
limitparameter does not exceed 1000, either by clamping the value or by raising an appropriate error if an invalid limit is provided.pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt (1)
26-31: 💡 Verification agent
❓ Verification inconclusive
Add limit validation in implementing classes.
The documentation states that the maximum limit is 1000, but there's no validation enforcing this constraint. While interfaces typically don't contain implementation details, implementing classes should validate this limit.
🏁 Script executed:
#!/bin/bash # Search for existing limit validations in implementing classes rg -A 2 "limit: Int" | grep -B 2 "require|check|assert|throw.*limit"Length of output: 70
Add validation for the maximum history limit in all implementing classes
The interface in
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(lines 26-31) defines thehistorymethod with a default limit of 100, and the documentation specifies that the allowed maximum is 1000. However, our searches indicate that none of the implementing classes currently enforce this limit (e.g., via arequire(limit <= 1000)check).
- File:
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Presence.kt(interface definition forhistory)- Action: Ensure that each class implementing the
historymethod adds a validation check (for instance,require(limit <= 1000)) to enforce the maximum limit described in the documentation.lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
324-326:
⚠️ Potential issueGuard against potential NPE in wrapper SDK agents.
The
wrapperSDKAgentsfield is not initialized in the primary constructor, which could lead to aNullPointerExceptionincollectRequestHeaders. Consider initializing it as an empty map.- private Map<String, String> wrapperSDKAgents; + private Map<String, String> wrapperSDKAgents = new HashMap<>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public class HttpCore { // Other field declarations - private Map<String, String> wrapperSDKAgents; + private Map<String, String> wrapperSDKAgents = new HashMap<>(); // Constructor(s) and other methods private void collectRequestHeaders() { Map<String, String> additionalAgents = new HashMap<>(options.agents); additionalAgents.putAll(wrapperSDKAgents); requestHeaders.put(Defaults.ABLY_AGENT_HEADER, AgentHeaderCreator.create(additionalAgents, platformAgentProvider)); } // Other methods }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1194-1203: Consider adding documentation for the HTTP parameter.The implementation correctly uses the provided HTTP context. Consider adding documentation to explain the purpose of the
httpparameter and its relationship to wrapper SDK proxies.+ /** + * Internal implementation of history retrieval. + * @param http The HTTP context to use for the request, allowing for wrapper SDK proxy support + * @param params The history request parameters + * @return A result request object for retrieving the history + */ private BasePaginatedQuery.ResultRequest<Message> historyImpl(Http http, Param[] params) {lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
474-481: Align method visibility with its intended usage.The method is documented as [Internal Method] but is declared as public. Consider making it package-private to match its intended usage and prevent unintended external use.
- public HttpCore injectWrapperSdkAgents(Map<String, String> wrapperSDKAgents) { + HttpCore injectWrapperSdkAgents(Map<String, String> wrapperSDKAgents) { return new HttpCore(this, wrapperSDKAgents); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
android/src/main/java/io/ably/lib/rest/AblyRest.java(2 hunks)gradle/libs.versions.toml(2 hunks)java/src/main/java/io/ably/lib/rest/AblyRest.java(2 hunks)lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/http/Http.java(1 hunks)lib/src/main/java/io/ably/lib/http/HttpCore.java(4 hunks)lib/src/main/java/io/ably/lib/http/HttpScheduler.java(1 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(3 hunks)lib/src/main/java/io/ably/lib/realtime/ChannelBase.java(3 hunks)lib/src/main/java/io/ably/lib/realtime/Presence.java(4 hunks)lib/src/main/java/io/ably/lib/rest/AblyBase.java(1 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(10 hunks)lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- lib/src/main/java/io/ably/lib/util/AgentHeaderCreator.java
- lib/src/main/java/io/ably/lib/http/AsyncHttpScheduler.java
- lib/src/main/java/io/ably/lib/http/Http.java
- java/src/main/java/io/ably/lib/rest/AblyRest.java
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
- android/src/main/java/io/ably/lib/rest/AblyRest.java
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
- gradle/libs.versions.toml
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt
- lib/src/main/java/io/ably/lib/realtime/Presence.java
- pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt
- lib/src/main/java/io/ably/lib/http/HttpScheduler.java
- lib/src/main/java/io/ably/lib/rest/AblyBase.java
🧰 Additional context used
🪛 GitHub Actions: JavaDoc
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt
[error] 32-32: 'publish' overrides nothing.
[error] 36-36: 'publishAsync' overrides nothing.
🪛 GitHub Actions: Integration Test
lib/src/main/java/io/ably/lib/http/HttpCore.java
[error] 324-324: NullPointerException in collectRequestHeaders method: Cannot invoke "java.util.Map.size()" because "m" is null.
🔇 Additional comments (12)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (4)
50-140: LGTM! Well-structured implementation of HTTP context support for publish methods.The changes maintain existing functionality while cleanly introducing HTTP context support. The implementation preserves important features like message encoding, client ID validation, and idempotent publishing.
151-177: LGTM! Clean implementation of HTTP context support for history methods.The changes follow the same pattern as publish methods, maintaining existing functionality while adding HTTP context support. The implementation preserves important features like request ID generation and message body handling.
200-237: LGTM! Consistent implementation of HTTP context support for presence get methods.The changes maintain the same pattern as other methods, preserving existing functionality while adding HTTP context support. The implementation maintains important features like request ID generation and presence message handling.
260-303: LGTM! Well-implemented HTTP context support for presence history methods.The changes follow the consistent pattern seen throughout the file, maintaining existing functionality while adding HTTP context support. The implementation preserves important features like request ID generation and presence message handling.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
13-13: LGTM!The import for
Httpis correctly added and properly ordered.
1153-1159: LGTM! Well-structured implementation.The changes maintain backward compatibility while adding support for custom HTTP contexts. The delegation pattern is implemented correctly.
1186-1192: LGTM! Consistent implementation.The async implementation follows the same pattern as the synchronous version, maintaining backward compatibility while supporting custom HTTP contexts.
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (5)
15-18: LGTM! Well-structured proxy implementation.The
WrapperRestClientclass effectively implements the proxy pattern using Kotlin's delegation, while properly injecting the HTTP module for agent tracking. This aligns well with the PR's objective of distinguishing wrapper SDK requests.
20-25: LGTM! Clean channel wrapper implementation.The
WrapperRestChannelsclass correctly maintains the proxy chain by wrapping returned channels, ensuring consistent HTTP module propagation.
53-76: LGTM! Comprehensive presence wrapper implementation.The
WrapperRestPresenceclass properly handles all presence-related operations while maintaining consistent HTTP module usage for agent tracking.
1-14: LGTM! Well-organized imports and file structure.The file is well-structured with proper package declaration and organized imports. The implementation effectively supports the PR's goal of distinguishing wrapper SDK requests through consistent HTTP module injection.
27-51:⚠️ Potential issueFix method override declarations.
The JavaDoc pipeline is reporting that
publishandpublishAsyncmethods are not properly overriding their interface methods. Add theoverridemodifier to these methods:- fun publish(name: String, data: Any) = javaChannel.publish(httpModule, name, data) + override fun publish(name: String, data: Any) = javaChannel.publish(httpModule, name, data) - fun publish(messages: List<Message>) = javaChannel.publish(httpModule, messages.toTypedArray()) + override fun publish(messages: List<Message>) = javaChannel.publish(httpModule, messages.toTypedArray()) - fun publishAsync(name: String, data: Any, listener: CompletionListener) = javaChannel.publishAsync(httpModule, name, data, listener) + override fun publishAsync(name: String, data: Any, listener: CompletionListener) = javaChannel.publishAsync(httpModule, name, data, listener) - fun publishAsync(messages: List<Message>, listener: CompletionListener) = javaChannel.publishAsync(httpModule, messages.toTypedArray(), listener) + override fun publishAsync(messages: List<Message>, listener: CompletionListener) = javaChannel.publishAsync(httpModule, messages.toTypedArray(), listener)Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Actions: JavaDoc
[error] 32-32: 'publish' overrides nothing.
[error] 36-36: 'publishAsync' overrides nothing.
Special `SdkWrapperCompatible` introduced, allowing us to hide `createWrapperSdkProxy` from the public interface and use extension function instead
Add tests to verify agent propagation in the WrapperSdkProxy clients
ttypic
force-pushed
the
CHAT-5063/add-sdk-wrapper-kotlin
branch
from
585e5d5 to
122a729
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1066/features
github-actions
bot
temporarily deployed
to
staging/pull/1066/javadoc
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
lib/src/main/java/io/ably/lib/http/HttpCore.java (2)
116-126:⚠️ Potential issueAdd parameter validation and defensive copying in the constructor.
The constructor should validate input parameters and create a defensive copy of the mutable map to prevent potential issues:
- Null parameters could lead to NPE
- Direct reference to the mutable map could lead to unexpected modifications
Apply this diff to fix the issues:
private HttpCore(HttpCore underlyingHttpCore, Map<String, String> dynamicAgents) { + if (underlyingHttpCore == null) { + throw new IllegalArgumentException("underlyingHttpCore cannot be null"); + } + if (dynamicAgents == null) { + throw new IllegalArgumentException("dynamicAgents cannot be null"); + } this.options = underlyingHttpCore.options; this.auth = underlyingHttpCore.auth; this.platformAgentProvider = underlyingHttpCore.platformAgentProvider; this.scheme = underlyingHttpCore.scheme; this.port = underlyingHttpCore.port; this.hosts = underlyingHttpCore.hosts; this.proxyAuth = underlyingHttpCore.proxyAuth; this.engine = underlyingHttpCore.engine; - this.dynamicAgents = dynamicAgents; + this.dynamicAgents = new HashMap<>(dynamicAgents); }
332-335:⚠️ Potential issueAdd null checks to prevent NullPointerException in header collection.
The code assumes both
options.agentsanddynamicAgentsare non-null, which can lead to NPE.Apply this diff to fix the issues:
- Map<String, String> additionalAgents = new HashMap<>(); - if (options.agents != null) additionalAgents.putAll(options.agents); - if (dynamicAgents != null) additionalAgents.putAll(dynamicAgents); + Map<String, String> additionalAgents = new HashMap<>(); + if (options.agents != null) { + additionalAgents.putAll(options.agents); + } + if (dynamicAgents != null) { + additionalAgents.putAll(dynamicAgents); + } requestHeaders.put(Defaults.ABLY_AGENT_HEADER, AgentHeaderCreator.create(additionalAgents, platformAgentProvider));
🧹 Nitpick comments (5)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
77-87: Consider using copy() for more maintainable property copying.The current implementation manually copies each property, which could become error-prone if new properties are added to
ChannelOptionsin the future.Consider using Kotlin's data class copy mechanism if possible:
-private fun ChannelOptions.injectAgents(agents: Map<String, String>): ChannelOptions { - val options = ChannelOptions() - options.params = (this.params ?: mapOf()) + mapOf( - "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" "), - ) - options.modes = modes - options.cipherParams = cipherParams - options.attachOnSubscribe = attachOnSubscribe - options.encrypted = encrypted - return options -} +private fun ChannelOptions.injectAgents(agents: Map<String, String>): ChannelOptions = + copy( + params = (params ?: mapOf()) + mapOf( + "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" ") + ) + )If
ChannelOptionsis not a data class, consider making it one to leverage this pattern.pubsub-adapter/src/test/kotlin/com/ably/Utils.kt (1)
8-17: Add documentation and improve error handling.The
waitForfunction needs documentation to explain its behavior, and could benefit from improved error handling and flexibility.Consider these improvements:
+/** + * Polls a condition until it returns true or timeout is reached. + * @param timeoutInMs Maximum time to wait in milliseconds + * @param block Condition to check. Should return true when the wait condition is met. + * @throws TimeoutCancellationException if the condition is not met within the timeout + * @throws Exception if the block throws an exception + */ suspend fun waitFor(timeoutInMs: Long = 10_000, block: suspend () -> Boolean) { withContext(Dispatchers.Default) { withTimeout(timeoutInMs) { + var lastError: Throwable? = null do { - val success = block() + val success = try { + block() + } catch (e: Exception) { + lastError = e + false + } + if (!success && lastError != null) { + throw lastError + } delay(100) } while (!success) } } }Additionally, consider making the delay configurable to support different polling scenarios:
-suspend fun waitFor(timeoutInMs: Long = 10_000, block: suspend () -> Boolean) +suspend fun waitFor( + timeoutInMs: Long = 10_000, + delayMs: Long = 100, + block: suspend () -> Boolean +)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (1)
67-72:❓ Verification inconclusive
Add documentation for createWrapperSdkProxy method.
The method would benefit from documentation explaining its purpose and behavior.
Add KDoc:
+ /** + * Creates a proxy instance of the REST client that injects additional agent information + * into HTTP requests. This is useful for tracking requests made through wrapper SDKs. + * + * @param options Configuration options containing agent information to inject + * @return A new RestClient instance with the specified agent information + */ override fun createWrapperSdkProxy(options: WrapperSdkProxyOptions): RestClient { val httpCoreWithAgents = javaClient.httpCore.injectDynamicAgents(options.agents) val httpModule = javaClient.http.exchangeHttpCore(httpCoreWithAgents) val javaClientWithInjectedAgents = javaClient.createShallowCopy(httpCoreWithAgents, httpModule) return WrapperRestClient(javaClientWithInjectedAgents, httpModule) }Let's verify the existence of the required extension functions:
🏁 Script executed:
#!/bin/bash # Search for the required extension function declarations echo "Searching for extension functions..." rg -t kotlin "fun.*HttpCore\.injectDynamicAgents" -A 2 rg -t kotlin "fun.*Http\.exchangeHttpCore" -A 2 rg -t kotlin "fun.*AblyRest\.createShallowCopy" -A 2Length of output: 244
Action Required: Verify Extension Function Existence & Add Documentation
It appears that the earlier search did not produce any output. Please manually verify that the following extension functions:
HttpCore.injectDynamicAgentsHttp.exchangeHttpCoreAblyRest.createShallowCopyactually exist in the codebase. Once confirmed, update the
createWrapperSdkProxymethod inpubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.ktwith the following KDoc to improve clarity:+ /** + * Creates a proxy instance of the REST client that injects additional agent information + * into HTTP requests. This proxy is particularly useful for enhancing request tracking + * when operating within a wrapper SDK environment. + * + * @param options Configuration options containing agent information to inject. + * @return A new RestClient instance with the specified agent information. + */After verifying these extension functions and applying the documentation, please ensure the changes align with the intended design.
🏁 Script executed:
#!/bin/bash echo "Re-verifying the extension functions..." echo "Searching for 'injectDynamicAgents' extension function:" rg -t kotlin "fun .*injectDynamicAgents" -N echo "Searching for 'exchangeHttpCore' extension function:" rg -t kotlin "fun .*exchangeHttpCore" -N echo "Searching for 'createShallowCopy' extension function:" rg -t kotlin "fun .*createShallowCopy" -NLength of output: 592
Action Required: Verify Extension Functions & Update Documentation
The proposed KDoc update for the
createWrapperSdkProxymethod is a great improvement for clarity. Please add the following KDoc above the method inpubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt:/** * Creates a proxy instance of the REST client that injects additional agent information * into HTTP requests. This proxy is particularly useful for enhancing request tracking * when operating within a wrapper SDK environment. * * @param options Configuration options containing agent information to inject. * @return A new RestClient instance with the specified agent information. */However, the automated searches for the extension functions (
injectDynamicAgents,exchangeHttpCore, andcreateShallowCopy) did not return any results. This could mean one of two things:
- The extension functions might be declared using a different pattern or file structure.
- They might require manual verification.
Next Steps:
- Manually verify that the extension functions are defined in the codebase. These functions are critical since they are used in the proxy creation process.
- If any of these functions are missing or misnamed, please update the references accordingly.
pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentChannelParamTest.kt (1)
84-89: Consider using test configuration for credentials.The hardcoded key "xxxxx:yyyyyyy" should be moved to test configuration.
- val options = ClientOptions("xxxxx:yyyyyyy").apply { + val options = ClientOptions(TestConfig.VALID_API_KEY).apply {pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentHeaderTest.kt (1)
177-201: Consider improving test configuration.
- Move hardcoded credentials to test configuration
- Consider making the port configurable or using a random available port
- val options = ClientOptions("xxxxx:yyyyyyy").apply { + val options = ClientOptions(TestConfig.VALID_API_KEY).apply { - port = PORT + port = TestConfig.getAvailablePort()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
gradle/libs.versions.toml(2 hunks)lib/src/main/java/io/ably/lib/http/HttpCore.java(4 hunks)pubsub-adapter/build.gradle.kts(1 hunks)pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt(1 hunks)pubsub-adapter/src/test/kotlin/com/ably/EmbeddedServer.kt(1 hunks)pubsub-adapter/src/test/kotlin/com/ably/Utils.kt(1 hunks)pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentChannelParamTest.kt(1 hunks)pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentHeaderTest.kt(1 hunks)pubsub-adapter/src/test/kotlin/io/ably/lib/realtime/ChannelUtils.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- gradle/libs.versions.toml
- pubsub-adapter/src/main/kotlin/com/ably/pubsub/WrapperSdkProxy.kt
🧰 Additional context used
🪛 detekt (1.23.7)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
[warning] 32-32: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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
- GitHub Check: check-realtime
- GitHub Check: check (19)
🔇 Additional comments (18)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (3)
12-19: LGTM! Well-structured implementation using Kotlin delegation.The class effectively wraps the Java client while maintaining the ability to inject custom agent parameters, which aligns with the PR's objective.
42-59: LGTM! Clean implementation of channel wrapper.The class effectively wraps the channel operations while properly integrating the custom HTTP module for history queries.
61-75: LGTM! Clean implementation of presence wrapper.The class effectively wraps the presence operations while properly integrating the custom HTTP module for history queries.
pubsub-adapter/src/test/kotlin/io/ably/lib/realtime/ChannelUtils.kt (1)
5-6: LGTM! Well-designed extension property.The extension property provides a clean and type-safe way to access channel options from the
ChannelBaseclass.pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (2)
18-18: LGTM! Clean interface implementation.The addition of
SdkWrapperCompatible<RealtimeClient>interface aligns with the PR objective of distinguishing wrapper SDK requests.
70-75: LGTM! Well-structured proxy creation.The implementation follows a clean pattern:
- Injects agents into httpCore
- Exchanges httpCore in http module
- Creates a shallow copy with modified components
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (3)
15-18: LGTM! Elegant use of delegation pattern.The implementation cleanly delegates to RestClientAdapter while overriding channels to use the wrapper version.
27-51: LGTM! Consistent HTTP module injection.The implementation correctly injects the HTTP module into all channel operations, maintaining consistency across the API surface.
53-76: LGTM! Well-structured presence wrapper.The presence wrapper maintains consistency with the channel wrapper pattern and correctly handles HTTP module injection.
pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentChannelParamTest.kt (2)
18-39: LGTM! Comprehensive test coverage for basic channel creation.The test properly verifies the presence and absence of agent information in the appropriate scenarios.
41-81: LGTM! Thorough verification of custom options scenario.The test ensures that agent information coexists correctly with custom channel options.
pubsub-adapter/src/test/kotlin/com/ably/pubsub/SdkWrapperAgentHeaderTest.kt (2)
23-53: LGTM! Thorough verification of agent headers.The test properly verifies the presence of agent information in headers for different types of requests.
151-174: LGTM! Clean server setup and teardown.The embedded server setup provides good isolation for testing HTTP requests.
lib/src/main/java/io/ably/lib/http/HttpCore.java (2)
71-79: LGTM!The field is well-documented with clear purpose and follows Java field declaration conventions.
483-490: LGTM!The method is well-documented and has a clear, focused implementation.
pubsub-adapter/build.gradle.kts (3)
9-15: LGTM!The test dependencies are well-structured and align with the PR objectives for testing wrapper SDK functionality:
- Kotlin test for unit testing
- NanoHTTPD for embedded server testing
- Coroutines for async testing
- Turbine for Flow testing
17-19: LGTM!Standard JUnit platform configuration is correctly applied.
21-23: LGTM!The custom test task is well-defined with useful logging functionality.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
Outdated
Show resolved
Hide resolved
|
Had a quick review of the PR, seems I also need to have spec PR https://github.com/ably/specification/pull/274/files as a reference. I will do more deeper review once I understand the context properly. |
|
@sacOO7 the spec PR that you linked to is for the chat spec; the relevant one here is ably/specification#273 🙂 |
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
Outdated
Show resolved
Hide resolved
github-actions
bot
temporarily deployed
to
staging/pull/1066/features
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
84-89:⚠️ Potential issueImprove exception handling to preserve error context.
The current implementation swallows the exception when injecting agents fails, silently falling back to creating a channel without agents. This could mask potential issues and make debugging more difficult.
Consider one of these approaches:
- Log the exception before falling back:
try { WrapperRealtimeChannel(javaChannels.get(name, ChannelOptions().injectAgents(agents)), httpModule) } catch (e: AblyException) { + Log.w("WrapperRealtimeChannels", "Failed to inject agents, falling back to default channel", e) WrapperRealtimeChannel(javaChannels.get(name), httpModule) }
- Or, rethrow with additional context:
try { WrapperRealtimeChannel(javaChannels.get(name, ChannelOptions().injectAgents(agents)), httpModule) } catch (e: AblyException) { - WrapperRealtimeChannel(javaChannels.get(name), httpModule) + throw AblyException( + "Failed to create channel with agents: ${e.message}", + e.errorInfo, + e + ) }🧰 Tools
🪛 detekt (1.23.7)
[warning] 86-86: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/rest/AblyBase.java (1)
195-354: LGTM! Clean implementation of HTTP context propagation.The pattern of overloading methods to accept an Http parameter while maintaining backward compatibility through delegation is well-implemented. This change effectively supports the wrapper SDK functionality while ensuring existing code continues to work.
Consider adding documentation to clarify that these overloaded methods are intended for internal use by wrapper SDKs.
+ /** + * Internal method used by wrapper SDKs to execute time requests with a custom HTTP context. + * @param http The HTTP context to use for the request + * @throws AblyException + */ long time(Http http) throws AblyException {pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (1)
49-56: Consider handling null safety more robustly.The non-null assertion operator (!!) on the request method's return value could lead to runtime exceptions if the underlying call returns null.
Consider handling the null case explicitly:
- ) = javaClient.request(httpModule, method.toString(), path, params.toTypedArray(), body, headers.toTypedArray())!! + ) = javaClient.request(httpModule, method.toString(), path, params.toTypedArray(), body, headers.toTypedArray()) + ?: throw AblyException(ErrorInfo("Unexpected null response from request"), 50000)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
131-141: Add null safety check for agents map.The
injectAgentsfunction should handle the case where the agents map is empty to avoid adding an empty agent parameter.Apply this diff to add null safety:
private fun ChannelOptions.injectAgents(agents: Map<String, String>): ChannelOptions { val options = ChannelOptions() - options.params = (this.params ?: mapOf()) + mapOf( - "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" "), - ) + options.params = (this.params ?: mapOf()) + when { + agents.isNotEmpty() -> mapOf( + "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" ") + ) + else -> mapOf() + } options.modes = modes options.cipherParams = cipherParams options.attachOnSubscribe = attachOnSubscribe options.encrypted = encrypted return options }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/main/java/io/ably/lib/rest/AblyBase.java(7 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
🧰 Additional context used
🪛 detekt (1.23.7)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
[warning] 86-86: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (19)
🔇 Additional comments (13)
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt (1)
1-28: LGTM! Well-structured Kotlin extension functions.The extension functions provide a clean, idiomatic Kotlin interface to the underlying Java methods while maintaining the same functionality. This improves the developer experience for Kotlin users without introducing any behavioral changes.
lib/src/main/java/io/ably/lib/rest/AblyBase.java (1)
120-129: LGTM! Well-designed constructor for proxy implementation.The constructor properly copies all necessary fields from the underlying client while allowing injection of custom HTTP components. This design effectively supports the wrapper SDK functionality without breaking encapsulation.
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (5)
1-18: LGTM! Well-organized imports.The imports are comprehensive and properly organized, covering all necessary dependencies for the wrapper implementation.
19-74: LGTM! Well-structured wrapper implementation.The class effectively implements the wrapper pattern using delegation and properly forwards all operations to the underlying client with the custom HTTP module.
76-82: LGTM! Clean channels wrapper implementation.The class properly wraps the channels functionality while maintaining consistency with the wrapper pattern used throughout the codebase.
84-111: LGTM! Well-implemented channel wrapper.The class maintains consistency with the wrapper pattern and properly handles all channel operations including presence, publishing, and history.
113-137: LGTM! Consistent presence wrapper implementation.The class properly implements presence functionality while maintaining consistency with the wrapper pattern used throughout the codebase.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (3)
4-4: LGTM! Import consolidation is appropriate.The wildcard import from
com.ably.pubsubpackage efficiently handles multiple related classes needed for the wrapper SDK functionality.
18-18: LGTM! Interface implementation aligns with PR objectives.The addition of
SdkWrapperCompatible<RealtimeClient>interface properly enables wrapper SDK support while maintaining the internal visibility of the adapter.
70-74:❓ Verification inconclusive
Implementation looks good, verify state preservation.
The implementation correctly:
- Injects SDK agents into the HTTP core
- Creates a new HTTP module with the modified core
- Returns a wrapper client that maintains the original client state
Please verify that all state from the original
javaClientis properly preserved when creating the wrapper client. Run the following test to confirm:
🏁 Script executed:
#!/bin/bash # Search for all fields and methods accessed from javaClient in WrapperRealtimeClient ast-grep --pattern 'class WrapperRealtimeClient { $$$ $field = $client.$_ $$$ }'Length of output: 88
State Preservation Verification Needed for WrapperRealtimeClient Implementation
The implementation correctly injects SDK agents, creates a new HTTP module, and returns a wrapper client that ostensibly maintains the original client state. However, our automated check (using
ast-grep) did not yield any output regarding field or method access fromjavaClientwithinWrapperRealtimeClient. This inconclusive result requires a manual review to ensure that all necessary state fromjavaClientis properly preserved in the wrapper.
- Action: Please manually verify that all fields and methods from
javaClientneeded byWrapperRealtimeClientare correctly retained and referenced.- Note: Directly inspect
WrapperRealtimeClient.ktto confirm that any state required fromjavaClientis indeed copied or delegated as expected.pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (3)
1-13: LGTM! Well-organized imports and package structure.The imports are properly organized and the package structure aligns with the project's architecture.
96-113: LGTM! Clean implementation of WrapperRealtimeChannel.The class correctly implements the wrapper functionality, properly delegates to the adapter, and consistently injects the HTTP module.
115-129: LGTM! Clean implementation of WrapperRealtimePresence.The class correctly implements the wrapper functionality, properly delegates to the adapter, and consistently injects the HTTP module.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
Show resolved
Hide resolved
ttypic
force-pushed
the
CHAT-5063/add-sdk-wrapper-kotlin
branch
from
be5cc86 to
de92473
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1066/features
github-actions
bot
temporarily deployed
to
staging/pull/1066/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (2)
49-56:⚠️ Potential issueAdd agent injection to request method.
The request method should inject wrapper SDK agents into the headers to maintain consistent agent tracking across all requests.
Apply this diff to inject agents:
override fun request( path: String, method: HttpMethod, params: List<Param>, body: HttpCore.RequestBody?, headers: List<Param>, ) = javaClient.request( httpModule, method.toString(), path, params.toTypedArray(), body, - headers.toTypedArray() + (headers + agents.map { Param("X-Ably-Agent", "${it.key}/${it.value}") }).toTypedArray() )!!
84-89:⚠️ Potential issueImprove exception handling to preserve error context.
The current implementation swallows the exception when injecting agents fails.
Consider logging the exception before falling back:
try { WrapperRealtimeChannel(javaChannels.get(name, ChannelOptions().injectAgents(agents)), httpModule) } catch (e: AblyException) { + Log.w("WrapperRealtimeChannels", "Failed to inject agents, falling back to default channel", e) WrapperRealtimeChannel(javaChannels.get(name), httpModule) }
🧹 Nitpick comments (8)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
131-141: Use Kotlin's copy() for a more idiomatic implementation.The current implementation manually copies all properties. Consider using Kotlin's built-in copy() method for a more concise and maintainable solution.
Apply this diff:
private fun ChannelOptions.injectAgents(agents: Map<String, String>): ChannelOptions { - val options = ChannelOptions() - options.params = (this.params ?: mapOf()) + mapOf( - "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" "), - ) - options.modes = modes - options.cipherParams = cipherParams - options.attachOnSubscribe = attachOnSubscribe - options.encrypted = encrypted - return options + return copy( + params = (params ?: mapOf()) + mapOf( + "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" ") + ) + ) }pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (5)
19-23: Add KDoc documentation to describe the wrapper's purpose and behavior.Consider adding documentation to explain:
- The purpose of this wrapper class
- The relationship with the underlying Java client
- The significance of the httpModule parameter
+/** + * Internal wrapper for AblyRest client that injects custom HTTP module for tracking wrapper SDK requests. + * @property javaClient The underlying Ably REST client + * @property adapter The REST client adapter for delegation + * @property httpModule The HTTP module for making requests with custom agent information + */ internal class WrapperRestClient( private val javaClient: AblyRest, private val adapter: RestClientAdapter, val httpModule: Http, ) : RestClient by adapter {
76-82: Add KDoc documentation to describe the channels wrapper.Consider adding documentation to explain the purpose and behavior of this wrapper class.
+/** + * Internal wrapper for Ably Channels that ensures all channel instances are properly wrapped + * with the custom HTTP module. + * @property javaChannels The underlying Ably channels + * @property httpModule The HTTP module for making requests with custom agent information + */ internal class WrapperRestChannels(private val javaChannels: AblyBase.Channels, private val httpModule: Http) : Channels<RestChannel> by RestChannelsAdapter(javaChannels) {
84-89: Add KDoc documentation and message validation.Consider adding documentation and message validation for the channel wrapper.
+/** + * Internal wrapper for Ably Channel that ensures all operations use the custom HTTP module. + * @property javaChannel The underlying Ably channel + * @property httpModule The HTTP module for making requests with custom agent information + */ internal class WrapperRestChannel(private val javaChannel: Channel, private val httpModule: Http) : RestChannel by RestChannelAdapter(javaChannel) { + + private fun validateMessage(message: Message) { + require(message.name?.isNotEmpty() ?: true) { "Message name cannot be empty" } + require(message.data != null) { "Message data cannot be null" } + }
92-92: Add message validation for batch publish.Consider validating messages before publishing.
- override fun publish(messages: List<Message>) = javaChannel.publish(httpModule, messages.toTypedArray()) + override fun publish(messages: List<Message>) { + messages.forEach(::validateMessage) + javaChannel.publish(httpModule, messages.toTypedArray()) + }
113-117: Add KDoc documentation and parameter validation.Consider adding documentation and parameter validation for the presence wrapper.
+/** + * Internal wrapper for Ably Presence that ensures all operations use the custom HTTP module. + * @property javaPresence The underlying Ably presence + * @property httpModule The HTTP module for making requests with custom agent information + */ internal class WrapperRestPresence(private val javaPresence: Presence, private val httpModule: Http) : RestPresence by RestPresenceAdapter(javaPresence) { + + private fun validateLimit(limit: Int) { + require(limit > 0) { "Limit must be greater than 0" } + } + override fun get(limit: Int, clientId: String?, connectionId: String?): PaginatedResult<PresenceMessage> { + validateLimit(limit) javaPresence.get(buildRestPresenceParams(limit, clientId, connectionId).toTypedArray()) }pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (1)
67-71:❓ Verification inconclusive
Verify thread safety and resource cleanup.
The implementation looks correct but needs verification for:
- Thread safety of HTTP core exchange
- Proper cleanup of HTTP resources when proxy is no longer needed
Run this script to check for proper cleanup patterns in similar wrapper implementations:
🏁 Script executed:
#!/bin/bash # Search for cleanup patterns in wrapper implementations rg -A 10 "class.*Wrapper.*Client.*implements"Length of output: 47
ACTION: Verify Thread Safety and Resource Cleanup in the HTTP Proxy Implementation
The current implementation in
RestClientAdapter.ktcorrectly constructs the wrapper proxy. However, the recent shell script run did not reveal any explicit cleanup patterns (e.g., an overriddenclose()method or similar mechanism) in related wrapper client classes. Before merging, please verify:
- Thread Safety: Confirm that the HTTP core exchange (
javaClient.http.exchangeHttpCore) is implemented in a thread-safe manner or is otherwise protected against concurrent access issues.- Resource Cleanup: Check that HTTP resources are properly released when the proxy is no longer needed. In particular, inspect the implementation of
WrapperRestClient(or any similar classes) to ensure they include appropriate cleanup logic, such as an overriddenclose()/dispose()method or equivalent cleanup mechanisms.If these aspects are confirmed to be handled appropriately—either through explicit cleanup methods or via the HTTP core's internal resource management—feel free to update the documentation accordingly. Otherwise, consider adding the necessary cleanup logic.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (1)
70-74: Add error handling for HTTP core and module creation.While the implementation correctly sets up the wrapper SDK proxy, it could benefit from proper error handling to gracefully handle potential failures during HTTP core injection or module creation.
Consider adding try-catch blocks and validation:
override fun createWrapperSdkProxy(options: WrapperSdkProxyOptions): RealtimeClient { + require(options.agents.isNotEmpty()) { "Agents list cannot be empty" } + try { val httpCoreWithAgents = javaClient.httpCore.injectDynamicAgents(options.agents) val httpModule = javaClient.http.exchangeHttpCore(httpCoreWithAgents) return WrapperRealtimeClient(javaClient, this, httpModule, options.agents) + } catch (e: Exception) { + throw IllegalStateException("Failed to create wrapper SDK proxy", e) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(1 hunks)lib/src/main/java/io/ably/lib/rest/AblyBase.java(6 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: check (29)
- GitHub Check: check-realtime
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check-realtime-okhttp
🔇 Additional comments (8)
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (1)
1-138: Well-structured implementation with good separation of concerns!The wrapper implementation successfully achieves the goal of injecting custom HTTP module for tracking wrapper SDK requests. The code follows good design patterns and maintains consistency across all wrapper classes.
pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt (1)
17-17: LGTM! Interface implementation looks good.The class now implements
SdkWrapperCompatible<RestClient>interface to support wrapper SDK proxy functionality.lib/src/main/java/io/ably/lib/rest/AblyBase.java (4)
181-187: LGTM! Time retrieval refactoring looks good.The time() method has been properly refactored to support custom HTTP instances while maintaining backward compatibility.
202-208: LGTM! Async time retrieval implementation is correct.The timeAsync() method properly delegates to the new implementation while preserving the original behavior.
247-253: LGTM! Stats retrieval refactoring is well-implemented.The stats() method has been correctly refactored to support custom HTTP instances while maintaining the existing functionality.
302-309:✅ Verification successful
Verify error handling in request method.
The request() method implementation looks correct, but let's verify the error handling patterns are consistent across all HTTP methods.
🏁 Script executed:
#!/bin/bash # Search for error handling patterns in HTTP methods rg -A 5 "throw.*AblyException" "lib/src/main/java/io/ably/lib/rest/"Length of output: 43691
Consistent Error Handling in Request Method Verified
After reviewing the error handling patterns across the HTTP methods, the implementation in the
request()methods (lines 302–309 inlib/src/main/java/io/ably/lib/rest/AblyBase.java) is consistent with the rest of the codebase. This method propagates errors through theHttpPaginatedQuery.exec()call—just as other methods throwAblyExceptionwhen necessary. No changes are required.pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (2)
18-18: LGTM! Good interface implementation.The addition of
SdkWrapperCompatible<RealtimeClient>interface aligns well with the PR's objective to support wrapper SDK functionality while maintaining proper encapsulation.
70-74:❓ Verification inconclusive
Verify implementation against specification.
Please ensure this implementation aligns with the specification defined in ably/specification#273.
🌐 Web query:
What are the requirements for wrapper SDK proxy implementation in Ably's specification PR #273?💡 Result:
To address the requirements for wrapper SDK proxy implementation as outlined in Ably's specification PR #273, we analyze key aspects from multiple sources:
Protocol Version Requirements
- Wrapper SDKs must explicitly declare their supported protocol version when connecting through proxies[3].
- Protocol versions are integer values incremented when wire protocol or service behavior changes occur[3].
- Backward compatibility is enforced: Services must support older protocol versions announced by SDKs during connection[3].
Proxy Implementation Constraints
- Dependency management: Requires integration of network libraries like OkHttp (for Java/Android) to handle HTTP/WebSocket traffic through proxies[1][6].
- Secure configuration: Mandates use of port 443 for WebSocket connections where possible to avoid corporate firewall blocking[6].
- Authentication support: Must implement proxy credential handling through standardized options like:
ProxyOptions proxy = new ProxyOptions(); proxy.host = "proxy-host"; proxy.port = 8080; proxy.username = "user"; proxy.password = "pass"[1];Testing Requirements
- Requires integration with test proxies like Ably's SDK Test Proxy to validate:
- Connection fault tolerance
- Message continuity through proxy interruptions
- Protocol version negotiation scenarios[4]
Behavioral Guarantees
- Must maintain original message payload integrity when using delta compression through proxies[1]
- Must transparently handle WebSocket fallback mechanisms when proxies block standard connections[6]
- Must preserve client identity (clientId) during authentication token renewal through proxies[8]
This synthesis combines requirements from protocol specifications[3], implementation examples[1][6], testing frameworks[4], and authentication patterns[8] relevant to proxy-aware SDK implementations.
Citations:
- 1: https://github.com/ably/ably-java
- 2: https://fluttergems.dev/api/
- 3: https://github.com/ably/specification
- 4: https://github.com/ably/sdk-test-proxy
- 5: https://www.usenix.org/event/lisa10/tech/full_papers/lisa10_proceedings.pdf
- 6: https://ably.com/topic/websockets-nodejs
- 7: https://petsymposium.org/popets/2022/popets-2022-0129.pdf
- 8: https://pkg.go.dev/github.com/ably/ably-go/ably
- 9: https://download.objectiflune.com/ResourceCenter/doc/connect_ol/olconnect-user-guide-2023.2.pdf
- 10: https://www.apollographql.com/docs/apollo-server/data/subscriptions
Action Required: Confirm Protocol Version and Proxy Credential Handling
The current implementation in
RealtimeClientAdapter.ktleverages dynamic agent injection viajavaClient.httpCore.injectDynamicAgents(options.agents), and it wraps the client usingWrapperRealtimeClient. Per the specification in PR #273, please verify that:
- Explicit Protocol Version Declaration: The wrapper SDK must explicitly declare its supported protocol version when connecting through proxies. Confirm that this requirement is met in this implementation—either through the agents provided in
options.agentsor elsewhere in the proxy setup.- Proxy Credential and Secure Configuration: Ensure that proxy credential handling and secure configuration (e.g., using port 443 where applicable) are properly integrated, either in this method or within
WrapperRealtimeClientand related network component configurations.- End-to-End Testing: Validate that the implementation supports the testing scenarios described in the specification, including protocol version negotiation and connection resilience through proxies.
Please review these aspects and update the code or documentation if any required behavior is not clearly addressed.
ttypic
force-pushed
the
CHAT-5063/add-sdk-wrapper-kotlin
branch
from
de92473 to
86027f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (2)
49-56:⚠️ Potential issueAdd agent injection to request method.
The
requestmethod should inject wrapper SDK agents into the headers to maintain consistent agent tracking across all requests. This aligns with the PR's objective of distinguishing between wrapper SDK and core SDK requests.Apply this diff to inject agents:
override fun request( path: String, method: HttpMethod, params: List<Param>, body: HttpCore.RequestBody?, headers: List<Param>, ) = javaClient.request( httpModule, method.toString(), path, params.toTypedArray(), body, - headers.toTypedArray() + (headers + agents.map { Param("X-Ably-Agent", "${it.key}/${it.value}") }).toTypedArray() )!!
82-90:⚠️ Potential issueImprove exception handling to preserve error context.
The current implementation swallows the exception when injecting agents fails, silently falling back to creating a channel without agents. This could mask potential issues and make debugging more difficult.
Consider one of these approaches:
- Log the exception before falling back:
if (javaChannels.containsKey(name)) return WrapperRealtimeChannel(javaChannels.get(name), httpModule) return try { WrapperRealtimeChannel(javaChannels.get(name, ChannelOptions().injectAgents(agents)), httpModule) } catch (e: AblyException) { + Log.w("WrapperRealtimeChannels", "Failed to inject agents, falling back to default channel", e) WrapperRealtimeChannel(javaChannels.get(name), httpModule) }
- Or, rethrow with additional context:
if (javaChannels.containsKey(name)) return WrapperRealtimeChannel(javaChannels.get(name), httpModule) return try { WrapperRealtimeChannel(javaChannels.get(name, ChannelOptions().injectAgents(agents)), httpModule) } catch (e: AblyException) { - WrapperRealtimeChannel(javaChannels.get(name), httpModule) + throw AblyException( + "Failed to create channel with agents: ${e.message}", + e.errorInfo, + e + ) }
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (1)
260-303: LGTM! The Presence history methods are correctly refactored with a minor documentation issue.The refactoring follows the established pattern. However, there's a minor documentation issue to fix.
Remove the unnecessary
@throws AblyExceptiontag from thehistoryAsyncmethod's JavaDoc (line 290) as the method doesn't throw exceptions directly.- * @throws AblyException */ public void historyAsync(Param[] params, Callback<AsyncPaginatedResult<PresenceMessage>> callback) {pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (1)
70-74: Add validation, error handling, and documentation for the proxy creation method.While the implementation correctly injects SDK agents and creates the wrapper client, consider these improvements:
- Add KDoc documentation explaining the method's purpose and parameters
- Add null checks and validation for the options parameter
- Add error handling for HTTP core and module creation operations
Here's a suggested implementation:
+ /** + * Creates a proxy wrapper for this client that injects additional SDK agents into requests. + * + * @param options Configuration options containing SDK agents to inject + * @return A new RealtimeClient instance with injected SDK agents + * @throws IllegalArgumentException if options or agents are invalid + */ override fun createWrapperSdkProxy(options: WrapperSdkProxyOptions): RealtimeClient { + requireNotNull(options.agents) { "SDK agents must not be null" } + require(options.agents.isNotEmpty()) { "At least one SDK agent must be provided" } + val httpCoreWithAgents = javaClient.httpCore.injectDynamicAgents(options.agents) + ?: throw IllegalStateException("Failed to inject SDK agents into HTTP core") val httpModule = javaClient.http.exchangeHttpCore(httpCoreWithAgents) + ?: throw IllegalStateException("Failed to create HTTP module with injected agents") return WrapperRealtimeClient(javaClient, this, httpModule, options.agents) }pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
131-141: Consider using copy() for more maintainable property copying.Creating a new instance and manually copying properties is error-prone as future additions to
ChannelOptionsmight be missed. Consider using Kotlin'scopy()function if available, or create a proper copy constructor.If
ChannelOptionsis a data class or can be modified to be one, you could simplify this to:private fun ChannelOptions.injectAgents(agents: Map<String, String>): ChannelOptions { - val options = ChannelOptions() - options.params = (this.params ?: mapOf()) + mapOf( + return copy( + params = (params ?: mapOf()) + mapOf( "agent" to agents.map { "${it.key}/${it.value}" }.joinToString(" "), ) - options.modes = modes - options.cipherParams = cipherParams - options.attachOnSubscribe = attachOnSubscribe - options.encrypted = encrypted - return options }If
copy()is not available, consider adding a copy constructor toChannelOptionsto ensure all properties are properly copied.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/src/main/java/io/ably/lib/rest/AblyBase.java(6 hunks)lib/src/main/java/io/ably/lib/rest/ChannelBase.java(10 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt(3 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt(1 hunks)pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientUtils.kt
- pubsub-adapter/src/main/kotlin/io/ably/lib/rest/RestClientAdapter.kt
- lib/src/main/java/io/ably/lib/rest/AblyBase.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
- GitHub Check: check-rest
- GitHub Check: check (24)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (21)
- GitHub Check: check
- GitHub Check: check (19)
- GitHub Check: check-realtime
🔇 Additional comments (12)
lib/src/main/java/io/ably/lib/rest/ChannelBase.java (3)
50-140: LGTM! The publish methods have been correctly refactored to support custom HTTP handling.The changes follow a consistent pattern where public methods delegate to package-private overloads that accept an Http parameter, enabling wrapper SDKs to inject their own Http instance. The core publishing logic remains unchanged, maintaining backward compatibility.
151-177: LGTM! The history methods follow the same consistent pattern.The refactoring maintains the same pattern as the publish methods, allowing for custom HTTP handling while preserving the core functionality.
200-237: LGTM! The Presence get methods maintain consistency with the established pattern.The refactoring in the Presence inner class follows the same pattern, enabling custom HTTP handling while preserving the core functionality.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/RealtimeClientAdapter.kt (2)
4-4: LGTM! Import consolidation looks good.The wildcard import efficiently consolidates multiple classes from the
com.ably.pubsubpackage needed for the wrapper SDK functionality.
18-18: LGTM! Interface implementation aligns with PR objectives.The addition of
SdkWrapperCompatible<RealtimeClient>interface properly enables wrapper SDK support while maintaining the internal visibility of the adapter.pubsub-adapter/src/main/kotlin/io/ably/lib/rest/WrapperRestClient.kt (6)
1-15: LGTM! Well-organized imports and appropriate visibility modifiers.The imports are logically grouped and the internal visibility modifiers align with the PR objectives of encapsulating implementation details.
50-57: Add null check and error handling for request method.The
!!operator on line 55 could lead to runtime exceptions if the request fails. Consider handling the null case explicitly.override fun request( path: String, method: HttpMethod, params: List<Param>, body: HttpCore.RequestBody?, headers: List<Param>, - ) = javaClient.request(httpModule, method.toString(), path, params.toTypedArray(), body, headers.toTypedArray())!! + ) = javaClient.request(httpModule, method.toString(), path, params.toTypedArray(), body, headers.toTypedArray()) + ?: throw AblyException("Request failed to return a response", 500, 50000)
16-49: LGTM! Well-structured wrapper implementation.The implementation correctly wraps the AblyRest client while maintaining the ability to inject custom HTTP modules and agents. The
createWrapperSdkProxymethod properly merges existing and new agents, which aligns with the PR objectives of tracking agents across various wrapper SDKs.Also applies to: 58-75
77-84: LGTM! Clean and straightforward channels wrapper.The implementation correctly delegates to RestChannelsAdapter while providing the necessary channel access methods.
85-112: LGTM! Comprehensive channel wrapper with proper HTTP module injection.The implementation correctly wraps all channel functionality while ensuring the custom HTTP module is used consistently across all operations.
114-138: LGTM! Well-implemented presence wrapper with proper parameter handling.The implementation correctly wraps all presence functionality while ensuring proper parameter building for all operations.
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt (1)
96-113: LGTM! Well-structured wrapper implementations.Both
WrapperRealtimeChannelandWrapperRealtimePresenceclasses are well-implemented:
- Proper delegation to Java counterparts
- Consistent use of httpModule for history operations
- Clean and maintainable code structure
Also applies to: 115-129
pubsub-adapter/src/main/kotlin/io/ably/lib/realtime/WrapperRealtimeClient.kt
Show resolved
Hide resolved
We want to be able to distinguish requests made inside the wrapper from those made by the core PubSub SDK without the wrapper, allowing us to track agents across wrapper SDKs such as the Chat SDK or Asset Tracking. To achieve this, we introduce special proxy Realtime and Rest clients that inject additional agents parameters into the underlying SDK.
To do this we introduced several package-private methods that make it possible to use custom http module to invoke Rest API, so we can inject agent information.
Also special
SdkWrapperCompatibleintroduced, allowing us to hidecreateWrapperSdkProxyfrom the public interface and use extension function insteadSummary by CodeRabbit