[CHAT-5063] feat: add createWrapperSdkProxy implementation for kotlin adapter of ably-java
#1066
Conversation
WalkthroughThis pull request introduces several additive changes across the codebase. New constructors and methods are added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as AblyRest/AblyRealtime Client
participant Method as createShallowCopy Method
participant Constructor as Protected Constructor
Client->>Method: Call createShallowCopy(httpCore, http)
Method->>Constructor: Invoke constructor(underlyingClient, httpCore, http)
Constructor-->>Method: Return shallow copy instance
Method-->>Client: Return new instance
sequenceDiagram
participant SDK as SDK/User
participant Adapter as RealtimeClientAdapter
participant Underlying as AblyRealtime javaClient
SDK->>Adapter: Call createWrapperSdkProxy(options)
Adapter->>Underlying: Inject SDK agents into HttpCore
Adapter->>Underlying: Create shallow copy with modified HTTP core
Adapter-->>SDK: Return WrapperRealtimeClient 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
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
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 🙂 |
| private val javaClient: AblyRealtime, | ||
| private val httpModule: Http, | ||
| private val agents: Map<String, String>, | ||
| ) : RealtimeClient by RealtimeClientAdapter(javaClient) { |
There was a problem hiding this comment.
Does the request method need to be overridden here to inject the wrapper SDK agents, or is there some other mechanism that handles this that I haven't understood?
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
New Features
Bug Fixes
Refactor