-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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()) | ||
} | ||
|
@@ -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, | ||
|
@@ -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, | ||
), | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* return occupancy for specified room | ||
*/ | ||
|
@@ -104,6 +134,7 @@ internal class ChatApi(private val realtimeClient: RealtimeClient, private val c | |
} | ||
|
||
override fun onError(reason: ErrorInfo?) { | ||
// (CHA-M3e) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential null value for Since Apply this diff to handle a - continuation.resumeWithException(AblyException.fromErrorInfo(reason))
+ val errorInfo = reason ?: ErrorInfo("Unknown error", HttpStatusCodes.InternalServerError)
+ continuation.resumeWithException(AblyException.fromErrorInfo(errorInfo))
|
||
continuation.resumeWithException(AblyException.fromErrorInfo(reason)) | ||
} | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion LGTM: Correct implementation of The 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
} | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential parsing errors in In the 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, | ||
|
@@ -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" | ||
|
@@ -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( | ||
|
@@ -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()) | ||
} | ||
} | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle potential null The Consider adding a retry mechanism or a timeout to handle cases where |
||
private fun addListener(listener: Messages.Listener, deferredChannelSerial: DeferredValue<String>) { | ||
synchronized(lock) { | ||
listeners += listener to deferredChannelSerial | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure thread safety in In Also, handle possible exceptions from |
||
|
||
synchronized(lock) { | ||
listeners = listeners.mapValues { | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Simplify The current implementation of the 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: Potential issue with In the 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Missing Import Statements for The file
🔗 Analysis chainVerify definitions of In lines 52-53, Run the following script to search for definitions: Please verify that 🏁 Scripts executedThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Apply this diff to update the status code: throw AblyException.fromErrorInfo(
ErrorInfo(
"invalid timeserial",
- HttpStatusCodes.InternalServerError, ErrorCodes.InternalError
+ HttpStatusCodes.BadRequest, ErrorCodes.InvalidRequest
),
) Ensure that 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Impact of setting The 🔗 Analysis chainVerify the impact of setting The addition of
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 🏁 Scripts executedThe 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 |
||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error message clarity and grammar
In the
validateSendMessageParams
function, the error messages can be made clearer:"Metadata contains reserved 'ably-chat' key"
to"Metadata contains the reserved key 'ably-chat'"
."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:
📝 Committable suggestion