Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ECO-4996] chore: added spec adjustments #28

Merged
merged 1 commit into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions chat-android/src/main/java/com/ably/chat/ChatApi.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import kotlin.coroutines.suspendCoroutine

private const val API_PROTOCOL_VERSION = 3
private const val PROTOCOL_VERSION_PARAM_NAME = "v"
private const val RESERVED_ABLY_CHAT_KEY = "ably-chat"
private val apiProtocolParam = Param(PROTOCOL_VERSION_PARAM_NAME, API_PROTOCOL_VERSION.toString())

internal class ChatApi(private val realtimeClient: RealtimeClient, private val clientId: String) {
Expand Down Expand Up @@ -47,11 +48,15 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
* @return sent message instance
*/
suspend fun sendMessage(roomId: String, params: SendMessageParams): Message {
validateSendMessageParams(params)

val body = JsonObject().apply {
addProperty("text", params.text)
// (CHA-M3b)
params.headers?.let {
add("headers", it.toJson())
}
// (CHA-M3b)
params.metadata?.let {
add("metadata", it.toJson())
}
Expand All @@ -62,6 +67,7 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
"POST",
body,
)?.let {
// (CHA-M3a)
Message(
timeserial = it.requireString("timeserial"),
clientId = clientId,
Expand All @@ -74,6 +80,30 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
} ?: throw AblyException.fromErrorInfo(ErrorInfo("Send message endpoint returned empty value", HttpStatusCodes.InternalServerError))
}

private fun validateSendMessageParams(params: SendMessageParams) {
// (CHA-M3c)
if (params.metadata?.containsKey(RESERVED_ABLY_CHAT_KEY) == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Metadata contains reserved 'ably-chat' key",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}

// (CHA-M3d)
if (params.headers?.keys?.any { it.startsWith(RESERVED_ABLY_CHAT_KEY) } == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Headers contains reserved key with reserved 'ably-chat' prefix",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}
}
Comment on lines +83 to +105
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error message clarity and grammar

In the validateSendMessageParams function, the error messages can be made clearer:

  • Line 88: Change "Metadata contains reserved 'ably-chat' key" to "Metadata contains the reserved key 'ably-chat'".
  • Line 99: Change "Headers contains reserved key with reserved 'ably-chat' prefix" to "Headers contain keys with the reserved prefix 'ably-chat'".

Apply this diff to improve the error messages:

-                        "Metadata contains reserved 'ably-chat' key",
+                        "Metadata contains the reserved key 'ably-chat'",
-                        "Headers contains reserved key with reserved 'ably-chat' prefix",
+                        "Headers contain keys with the reserved prefix 'ably-chat'",
📝 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.

Suggested change
private fun validateSendMessageParams(params: SendMessageParams) {
// (CHA-M3c)
if (params.metadata?.containsKey(RESERVED_ABLY_CHAT_KEY) == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Metadata contains reserved 'ably-chat' key",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}
// (CHA-M3d)
if (params.headers?.keys?.any { it.startsWith(RESERVED_ABLY_CHAT_KEY) } == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Headers contains reserved key with reserved 'ably-chat' prefix",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}
}
private fun validateSendMessageParams(params: SendMessageParams) {
// (CHA-M3c)
if (params.metadata?.containsKey(RESERVED_ABLY_CHAT_KEY) == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Metadata contains the reserved key 'ably-chat'",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}
// (CHA-M3d)
if (params.headers?.keys?.any { it.startsWith(RESERVED_ABLY_CHAT_KEY) } == true) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"Headers contain keys with the reserved prefix 'ably-chat'",
HttpStatusCodes.BadRequest,
ErrorCodes.InvalidRequestBody,
),
)
}
}


/**
* return occupancy for specified room
*/
Expand Down Expand Up @@ -104,6 +134,7 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c
}

override fun onError(reason: ErrorInfo?) {
// (CHA-M3e)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential null value for reason in error handling

Since reason is nullable (ErrorInfo?), passing it directly to AblyException.fromErrorInfo(reason) may result in a NullPointerException if reason is null. Consider providing a default ErrorInfo or handling the null case to prevent unexpected errors.

Apply this diff to handle a null reason:

-                        continuation.resumeWithException(AblyException.fromErrorInfo(reason))
+                        val errorInfo = reason ?: ErrorInfo("Unknown error", HttpStatusCodes.InternalServerError)
+                        continuation.resumeWithException(AblyException.fromErrorInfo(errorInfo))

Committable suggestion was skipped due to low confidence.

continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
},
Expand Down
10 changes: 10 additions & 0 deletions chat-android/src/main/java/com/ably/chat/ErrorCodes.kt
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ object ErrorCodes {
* The request cannot be understood
*/
const val BadRequest = 40_000

/**
* Invalid request body
*/
const val InvalidRequestBody = 40_001

/**
* Internal error
*/
const val InternalError = 50_000
}

/**
Expand Down
18 changes: 18 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Message.kt
Original file line number Diff line number Diff line change
Expand Up @@ -68,3 +68,21 @@ data class Message(
*/
val headers: MessageHeaders,
)

/**
* (CHA-M2a)
* @return true if the timeserial of the corresponding realtime channel message comes first.
*/
fun Message.isBefore(other: Message): Boolean = Timeserial.parse(timeserial) < Timeserial.parse(other.timeserial)

/**
* (CHA-M2b)
* @return true if the timeserial of the corresponding realtime channel message comes second.
*/
fun Message.isAfter(other: Message): Boolean = Timeserial.parse(timeserial) > Timeserial.parse(other.timeserial)

/**
* (CHA-M2c)
* @return true if they have the same timeserial.
*/
fun Message.isAtTheSameTime(other: Message): Boolean = Timeserial.parse(timeserial) == Timeserial.parse(other.timeserial)
Comment on lines +84 to +88
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

LGTM: Correct implementation of isAtTheSameTime comparison.

The isAtTheSameTime function correctly compares the timeserial values of two Message objects for equality using Timeserial.parse. This implementation aligns with the specification (CHA-M2c) and provides a clear way to determine if two messages occurred at the same time.

Suggestion: Consider refactoring to reduce code duplication.

While the current implementation is correct, there's an opportunity to reduce code duplication across the three comparison functions. Consider introducing a private comparison function that all three public functions can use:

private fun compareTimeserial(other: Message): Int =
    Timeserial.parse(timeserial).compareTo(Timeserial.parse(other.timeserial))

fun Message.isBefore(other: Message): Boolean = compareTimeserial(other) < 0
fun Message.isAfter(other: Message): Boolean = compareTimeserial(other) > 0
fun Message.isAtTheSameTime(other: Message): Boolean = compareTimeserial(other) == 0

This refactoring would centralize the parsing logic, making the code more maintainable and less prone to inconsistencies if changes are needed in the future.

36 changes: 33 additions & 3 deletions chat-android/src/main/java/com/ably/chat/Messages.kt
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ data class SendMessageParams(
)

interface MessagesSubscription : Subscription {
/**
* (CHA-M5j)
* Get the previous messages that were sent to the room before the listener was subscribed.
* @return paginated result of messages, in newest-to-oldest order.
*/
suspend fun getPreviousMessages(start: Long? = null, end: Long? = null, limit: Int = 100): PaginatedResult<Message>
}

Expand All @@ -195,6 +200,18 @@ internal class DefaultMessagesSubscription(

override suspend fun getPreviousMessages(start: Long?, end: Long?, limit: Int): PaginatedResult<Message> {
val fromSerial = fromSerialProvider().await()

// (CHA-M5j)
if (end != null && end > Timeserial.parse(fromSerial).timestamp) {
throw AblyException.fromErrorInfo(
ErrorInfo(
"The `end` parameter is specified and is more recent than the subscription point timeserial",
HttpStatusCodes.BadRequest,
ErrorCodes.BadRequest,
),
)
}

Comment on lines +203 to +214
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential parsing errors in getPreviousMessages

In the getPreviousMessages method, consider handling potential exceptions that may arise from Timeserial.parse(fromSerial). If fromSerial is not in the expected format, parse might throw an exception, which could cause the application to crash.

You can wrap the parsing in a try-catch block to handle possible exceptions:

val fromSerialTimestamp = try {
    Timeserial.parse(fromSerial).timestamp
} catch (e: Exception) {
    throw AblyException.fromErrorInfo(
        ErrorInfo(
            "Invalid fromSerial format",
            HttpStatusCodes.BadRequest,
            ErrorCodes.BadRequest,
        ),
        e
    )
}

val queryOptions = QueryOptions(start = start, end = end, limit = limit, orderBy = NewestFirst)
return chatApi.getMessages(
roomId = roomId,
Expand All @@ -217,6 +234,7 @@ internal class DefaultMessages(
private var lock = Any()

/**
* (CHA-M1)
* the channel name for the chat messages channel.
*/
private val messagesChannelName = "$roomId::\$chat::\$chatMessages"
Expand Down Expand Up @@ -249,8 +267,9 @@ internal class DefaultMessages(
)
listener.onEvent(MessageEvent(type = MessageEventType.Created, message = chatMessage))
}

// (CHA-M4d)
channel.subscribe(MessageEventType.Created.eventName, messageListener)
// (CHA-M5) setting subscription point
associateWithCurrentChannelSerial(deferredChannelSerial)

return DefaultMessagesSubscription(
Expand Down Expand Up @@ -293,10 +312,11 @@ internal class DefaultMessages(
private fun associateWithCurrentChannelSerial(channelSerialProvider: DeferredValue<String>) {
if (channel.state === ChannelState.attached) {
channelSerialProvider.completeWith(requireChannelSerial())
return
}

channel.once(ChannelState.attached) {
channelSerialProvider.completeWith(requireChannelSerial())
channelSerialProvider.completeWith(requireAttachSerial())
}
}

Expand All @@ -307,6 +327,13 @@ internal class DefaultMessages(
)
}

private fun requireAttachSerial(): String {
return channel.properties.attachSerial
?: throw AblyException.fromErrorInfo(
ErrorInfo("Channel has been attached, but attachSerial is not defined", HttpStatusCodes.BadRequest, ErrorCodes.BadRequest),
)
}

Comment on lines +330 to +336
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle potential null attachSerial in requireAttachSerial

The requireAttachSerial method assumes that channel.properties.attachSerial is not null after the channel is attached. However, there might be scenarios where attachSerial is still null due to network delays or other issues.

Consider adding a retry mechanism or a timeout to handle cases where attachSerial is not immediately available.

private fun addListener(listener: Messages.Listener, deferredChannelSerial: DeferredValue<String>) {
synchronized(lock) {
listeners += listener to deferredChannelSerial
Expand All @@ -319,9 +346,12 @@ internal class DefaultMessages(
}
}

/**
* (CHA-M5c), (CHA-M5d)
*/
private fun updateChannelSerialsAfterDiscontinuity() {
val deferredChannelSerial = DeferredValue<String>()
associateWithCurrentChannelSerial(deferredChannelSerial)
deferredChannelSerial.completeWith(requireAttachSerial())
Comment on lines +349 to +354
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure thread safety in updateChannelSerialsAfterDiscontinuity

In updateChannelSerialsAfterDiscontinuity, you're updating the listeners map within a synchronized block. Ensure that all access to listeners is properly synchronized to prevent concurrent modification exceptions.

Also, handle possible exceptions from requireAttachSerial() to prevent the method from failing silently.


synchronized(lock) {
listeners = listeners.mapValues {
Expand Down
1 change: 1 addition & 0 deletions chat-android/src/main/java/com/ably/chat/Room.kt
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ interface Room {
val occupancy: Occupancy

/**
* (CHA-RS2)
* Returns an object that can be used to observe the status of the room.
*
* @returns The status observable.
Expand Down
8 changes: 4 additions & 4 deletions chat-android/src/main/java/com/ably/chat/RoomOptions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,25 @@ data class RoomOptions(
* use {@link RoomOptionsDefaults.presence} to enable presence with default options.
* @defaultValue undefined
*/
val presence: PresenceOptions = PresenceOptions(),
val presence: PresenceOptions? = null,

/**
* The typing options for the room. To enable typing in the room, set this property. You may use
* {@link RoomOptionsDefaults.typing} to enable typing with default options.
*/
val typing: TypingOptions = TypingOptions(),
val typing: TypingOptions? = null,

/**
* The reactions options for the room. To enable reactions in the room, set this property. You may use
* {@link RoomOptionsDefaults.reactions} to enable reactions with default options.
*/
val reactions: RoomReactionsOptions = RoomReactionsOptions,
val reactions: RoomReactionsOptions? = null,

/**
* The occupancy options for the room. To enable occupancy in the room, set this property. You may use
* {@link RoomOptionsDefaults.occupancy} to enable occupancy with default options.
*/
val occupancy: OccupancyOptions = OccupancyOptions,
val occupancy: OccupancyOptions? = null,
)

/**
Expand Down
13 changes: 13 additions & 0 deletions chat-android/src/main/java/com/ably/chat/RoomStatus.kt
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import io.ably.lib.types.ErrorInfo
*/
interface RoomStatus {
/**
* (CHA-RS2a)
* The current status of the room.
*/
val current: RoomLifecycle

/**
* (CHA-RS2b)
* The current error, if any, that caused the room to enter the current status.
*/
val error: ErrorInfo?
Expand All @@ -36,57 +38,68 @@ interface RoomStatus {
}

/**
* (CHA-RS1)
* The different states that a room can be in throughout its lifecycle.
*/
enum class RoomLifecycle(val stateName: String) {
/**
* (CHA-RS1a)
* A temporary state for when the library is first initialized.
*/
Initialized("initialized"),

/**
* (CHA-RS1b)
* The library is currently attempting to attach the room.
*/
Attaching("attaching"),

/**
* (CHA-RS1c)
* The room is currently attached and receiving events.
*/
Attached("attached"),

/**
* (CHA-RS1d)
* The room is currently detaching and will not receive events.
*/
Detaching("detaching"),

/**
* (CHA-RS1e)
* The room is currently detached and will not receive events.
*/
Detached("detached"),

/**
* (CHA-RS1f)
* The room is in an extended state of detachment, but will attempt to re-attach when able.
*/
Suspended("suspended"),

/**
* (CHA-RS1g)
* The room is currently detached and will not attempt to re-attach. User intervention is required.
*/
Failed("failed"),

/**
* (CHA-RS1h)
* The room is in the process of releasing. Attempting to use a room in this state may result in undefined behavior.
*/
Releasing("releasing"),

/**
* (CHA-RS1i)
* The room has been released and is no longer usable.
*/
Released("released"),
}

/**
* Represents a change in the status of the room.
* (CHA-RS4)
*/
data class RoomStatusChange(
/**
Expand Down
65 changes: 65 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Timeserial.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package com.ably.chat

import io.ably.lib.types.AblyException
import io.ably.lib.types.ErrorInfo

/**
* Represents a parsed timeserial.
*/
data class Timeserial(
/**
* The series ID of the timeserial.
*/
val seriesId: String,

/**
* The timestamp of the timeserial.
*/
val timestamp: Long,

/**
* The counter of the timeserial.
*/
val counter: Int,

/**
* The index of the timeserial.
*/
val index: Int?,
) : Comparable<Timeserial> {
@Suppress("ReturnCount")
override fun compareTo(other: Timeserial): Int {
val timestampDiff = timestamp.compareTo(other.timestamp)
if (timestampDiff != 0) return timestampDiff

// Compare the counter
val counterDiff = counter.compareTo(other.counter)
if (counterDiff != 0) return counterDiff

// Compare the seriesId lexicographically
val seriesIdDiff = seriesId.compareTo(other.seriesId)
if (seriesIdDiff != 0) return seriesIdDiff

// Compare the index, if present
return if (index != null && other.index != null) index.compareTo(other.index) else 0
}
Comment on lines +31 to +45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify compareTo method using compareValuesBy

The current implementation of the compareTo method performs multiple sequential comparisons, which can be simplified for readability. Kotlin provides the compareValuesBy function to streamline this process.

Apply this refactored implementation:

override fun compareTo(other: Timeserial): Int = compareValuesBy(
    this, other,
    Timeserial::timestamp,
    Timeserial::counter,
    Timeserial::seriesId,
    { it.index }  // Handles nulls appropriately
)

Note: compareValuesBy considers null less than any non-null value. Ensure this behavior aligns with the intended ordering for Timeserial instances in your application.


⚠️ Potential issue

Potential issue with index comparison in compareTo method

In the compareTo method, when comparing the index property (line 44), if one index is null and the other is not, the method returns 0, treating them as equal. This could lead to incorrect ordering of Timeserial instances where index values differ in nullability. Consider defining a clear ordering between null and non-null index values, such as treating null as less than any non-null value.

Apply this diff to adjust the comparison logic:

return when {
    index != null && other.index != null ->
        index.compareTo(other.index)
    index == null && other.index != null ->
        -1
    index != null && other.index == null ->
        1
    else ->
        0
}
📝 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.

Suggested change
override fun compareTo(other: Timeserial): Int {
val timestampDiff = timestamp.compareTo(other.timestamp)
if (timestampDiff != 0) return timestampDiff
// Compare the counter
val counterDiff = counter.compareTo(other.counter)
if (counterDiff != 0) return counterDiff
// Compare the seriesId lexicographically
val seriesIdDiff = seriesId.compareTo(other.seriesId)
if (seriesIdDiff != 0) return seriesIdDiff
// Compare the index, if present
return if (index != null && other.index != null) index.compareTo(other.index) else 0
}
override fun compareTo(other: Timeserial): Int {
val timestampDiff = timestamp.compareTo(other.timestamp)
if (timestampDiff != 0) return timestampDiff
// Compare the counter
val counterDiff = counter.compareTo(other.counter)
if (counterDiff != 0) return counterDiff
// Compare the seriesId lexicographically
val seriesIdDiff = seriesId.compareTo(other.seriesId)
if (seriesIdDiff != 0) return seriesIdDiff
// Compare the index, if present
return when {
index != null && other.index != null ->
index.compareTo(other.index)
index == null && other.index != null ->
-1
index != null && other.index == null ->
1
else ->
0
}
}


companion object {
@Suppress("DestructuringDeclarationWithTooManyEntries")
fun parse(timeserial: String): Timeserial {
val matched = """(\w+)@(\d+)-(\d+)(?::(\d+))?""".toRegex().matchEntire(timeserial)
?: throw AblyException.fromErrorInfo(
ErrorInfo("invalid timeserial", HttpStatusCodes.InternalServerError, ErrorCodes.InternalError),
)
Comment on lines +52 to +53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing Import Statements for HttpStatusCodes and ErrorCodes

The file Timeserial.kt does not include import statements for HttpStatusCodes and ErrorCodes. Please add the necessary imports to prevent potential compilation issues.

  • import com.ably.chat.ErrorCodes.HttpStatusCodes
  • import com.ably.chat.ErrorCodes
🔗 Analysis chain

Verify definitions of HttpStatusCodes and ErrorCodes

In lines 52-53, HttpStatusCodes.InternalServerError and ErrorCodes.InternalError are referenced without visible imports or definitions. Please verify that these classes or objects are properly imported or defined in the project to prevent compilation issues.

Run the following script to search for definitions:


Please verify that Timeserial.kt includes the necessary import statements for HttpStatusCodes and ErrorCodes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for definitions of HttpStatusCodes and ErrorCodes in the project.

# Expected result: Files where HttpStatusCodes and ErrorCodes are defined.

rg --type kotlin 'object HttpStatusCodes|object ErrorCodes'

Length of output: 216


Script:

#!/bin/bash
# Description: Check if Timeserial.kt imports HttpStatusCodes and ErrorCodes

# Expected result: Lines importing HttpStatusCodes and ErrorCodes

rg --type kotlin '^import .*HttpStatusCodes$|^import .*ErrorCodes$' chat-android/src/main/java/com/ably/chat/Timeserial.kt

Length of output: 124

Comment on lines +51 to +53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using a more appropriate HTTP status code

When throwing an AblyException due to an invalid timeserial (lines 51-53), the code uses HttpStatusCodes.InternalServerError. Since the error results from invalid client input, it might be more appropriate to use HttpStatusCodes.BadRequest to indicate a client-side error.

Apply this diff to update the status code:

throw AblyException.fromErrorInfo(
    ErrorInfo(
        "invalid timeserial",
-       HttpStatusCodes.InternalServerError, ErrorCodes.InternalError
+       HttpStatusCodes.BadRequest, ErrorCodes.InvalidRequest
    ),
)

Ensure that HttpStatusCodes.BadRequest and ErrorCodes.InvalidRequest are properly defined or imported.

📝 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.

Suggested change
?: throw AblyException.fromErrorInfo(
ErrorInfo("invalid timeserial", HttpStatusCodes.InternalServerError, ErrorCodes.InternalError),
)
?: throw AblyException.fromErrorInfo(
ErrorInfo(
"invalid timeserial",
HttpStatusCodes.BadRequest, ErrorCodes.InvalidRequest
),
)


val (seriesId, timestamp, counter, index) = matched.destructured

return Timeserial(
seriesId = seriesId,
timestamp = timestamp.toLong(),
counter = counter.toInt(),
index = if (index.isNotBlank()) index.toInt() else null,
)
}
}
}
2 changes: 2 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Utils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ fun ChatChannelOptions(init: (ChannelOptions.() -> Unit)? = null): ChannelOption
options.params = (options.params ?: mapOf()) + mapOf(
AGENT_PARAMETER_NAME to "chat-kotlin/${BuildConfig.APP_VERSION}",
)
// (CHA-M4a)
options.attachOnSubscribe = false
return options
Comment on lines +45 to 47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Impact of setting attachOnSubscribe to false needs addressing in Messages.kt

The Messages.kt file uses channel.subscribe without explicit channel attachment. Please ensure that channels are explicitly attached before subscribing to prevent potential issues.

🔗 Analysis chain

Verify the impact of setting attachOnSubscribe to false

The addition of options.attachOnSubscribe = false is a significant change in channel subscription behavior. This means the channel won't be automatically attached when a subscription is created.

  1. Ensure this change aligns with the requirements specified in ticket CHA-M4a.
  2. Verify that all relevant parts of the codebase are updated to handle this new behavior, particularly where channel subscriptions are created.
  3. Consider adding a comment explaining why this change was made for future maintainers.

To ensure this change doesn't introduce unexpected behavior, please run the following script to check for any existing usages that might be affected:

Review the results to ensure that explicit channel.attach() calls are added where necessary.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for channel subscription creation and attachment in the codebase

echo "Searching for channel subscription creation:"
rg --type kotlin "channel\.subscribe" -A 5

echo "\nSearching for explicit channel attachments:"
rg --type kotlin "channel\.attach"

Length of output: 1141

}

Expand Down
Loading