[ECO-5156] Adds required changes to support message updates and deletes #2036
Conversation
WalkthroughThe pull request introduces the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 (
|
umair-ably
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/2036/features
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Source/ARTMessage.m (2)
28-38: Consider updating the description method to include new properties.For better debugging, consider including the new properties (
updateSerial,updatedAt,operation,refType,refSerial) in the description output.- (NSString *)description { NSMutableString *description = [[super description] mutableCopy]; [description deleteCharactersInRange:NSMakeRange(description.length - (description.length>2 ? 2:0), 2)]; [description appendFormat:@",\n"]; [description appendFormat:@" name: %@\n", self.name]; + if (self.updateSerial) { + [description appendFormat:@" updateSerial: %@\n", self.updateSerial]; + } + if (self.updatedAt) { + [description appendFormat:@" updatedAt: %@\n", self.updatedAt]; + } + if (self.operation) { + [description appendFormat:@" operation: %@\n", self.operation]; + } + if (self.refType) { + [description appendFormat:@" refType: %@\n", self.refType]; + } + if (self.refSerial) { + [description appendFormat:@" refSerial: %@\n", self.refSerial]; + } if (self.extras) { [description appendFormat:@" extras: %@\n", self.extras]; } [description appendFormat:@"}"]; return description; }
55-58: Consider updating messageSize calculation to include new properties.The
messageSizecalculation should account for the new properties to accurately reflect the message size.- (NSInteger)messageSize { // TO3l8* - return [super messageSize] + [self.name lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + NSInteger size = [super messageSize] + [self.name lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + if (self.updateSerial) { + size += [self.updateSerial lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } + if (self.refType) { + size += [self.refType lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } + if (self.refSerial) { + size += [self.refSerial lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } + return size; }Source/include/Ably/ARTMessageOperation.h (1)
8-8: Consider usingidinstead ofNSStringfor metadata dictionary values.The metadata values are currently restricted to strings, but allowing any object type would provide more flexibility for different use cases.
-@property (nonatomic, strong, nullable) NSDictionary<NSString *, NSString *> *metadata; +@property (nonatomic, strong, nullable) NSDictionary<NSString *, id> *metadata;Source/ARTJsonLikeEncoder.m (1)
305-307: Consider adding error handling for invalid operation data.While the type checking is good, consider adding error handling for cases where
readFromJSON:fails or returns nil.if (input[@"operation"] && [input[@"operation"] isKindOfClass:[NSDictionary class]]) { - message.operation = [ARTMessageOperation readFromJSON:input[@"operation"]]; + ARTMessageOperation *operation = [ARTMessageOperation readFromJSON:input[@"operation"]]; + if (!operation) { + ARTLogError(_logger, @"RS:%p ARTJsonLikeEncoder<%@>: Failed to parse operation data %@", _rest, [_delegate formatAsString], input[@"operation"]); + } + message.operation = operation; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Ably.xcodeproj/project.pbxproj(9 hunks)Source/ARTJsonLikeEncoder.m(2 hunks)Source/ARTMessage.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/include/Ably/ARTMessage.h(3 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Source/include/Ably/AblyPublic.h
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS17_2)
- GitHub Check: build
- GitHub Check: check (iOS, test_iOS17_2)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (7)
Source/ARTMessage.m (1)
45-51: LGTM! Properties are properly copied.The new properties for message updates and operations are correctly copied in the
copyWithZone:method. The properties are well-organized, with version-related properties (updateSerial,updatedAt) grouped together, followed by operation and reference properties.Ably.xcodeproj/project.pbxproj (1)
389-394:✅ Verification successful
LGTM! File organization looks correct.
The new
ARTMessageOperationfiles are properly integrated into the project:
- Header file is correctly marked as public
- Files are logically grouped with other message-related files
- Files are added to all necessary build phases and targets
Run this script to verify the file organization matches the project structure:
Also applies to: 1386-1387, 2211-2213
🏁 Script executed:
#!/bin/bash # Description: Verify the file organization matches the project structure # Check if the files exist in the expected locations echo "Checking file locations..." ls -l Source/ARTMessageOperation.m Source/include/Ably/ARTMessageOperation.h # Check if the files are grouped with other message-related files echo -e "\nChecking nearby files..." ls -l Source/include/Ably/ARTMessage.h Source/include/Ably/ARTBaseMessage.hLength of output: 621
File organization verified – LGTM!
The new
ARTMessageOperationfiles are correctly integrated into the project:
- The implementation file (
Source/ARTMessageOperation.m) and header file (Source/include/Ably/ARTMessageOperation.h) are both present in the expected locations.- The header is properly marked as public and logically grouped with related files, as confirmed by the nearby
ARTMessage.handARTBaseMessage.h.Great job!
Source/ARTMessageOperation.m (1)
26-38: LGTM! Robust implementation with proper type checking.The readFromJSON method includes appropriate null checks and type validation before assigning values.
Source/ARTJsonLikeEncoder.m (3)
305-307: LGTM! Proper type checking and error handling.The implementation correctly validates the operation dictionary before creating the ARTMessageOperation object.
28-28: LGTM!The import statement is correctly placed and necessary for the message operation functionality.
298-298: LGTM!The
updatedAtproperty assignment follows the established pattern for timestamp handling.Source/include/Ably/ARTMessage.h (1)
57-74: LGTM! Well-documented properties with clear purposes.The new properties are properly documented with clear explanations of their roles. The implementation follows consistent patterns with existing code.
github-actions
bot
temporarily deployed
to
staging/pull/2036/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2036/features
github-actions
bot
temporarily deployed
to
staging/pull/2036/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2036/features
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Source/ARTMessageOperation.m (1)
18-33:⚠️ Potential issueAdd type checking for clientId and description.
The method should validate the types of values from the dictionary before assignment, similar to the metadata check. This helps prevent runtime errors from invalid data types.
Apply this diff to add type checking:
+ (instancetype)createFromDictionary:(NSDictionary<NSString *, id> *)jsonObject { ARTMessageOperation *operation = [[ARTMessageOperation alloc] init]; - if (jsonObject[@"clientId"]) { + if (jsonObject[@"clientId"] && [jsonObject[@"clientId"] isKindOfClass:[NSString class]]) { operation.clientId = jsonObject[@"clientId"]; } - if (jsonObject[@"description"]) { + if (jsonObject[@"description"] && [jsonObject[@"description"] isKindOfClass:[NSString class]]) { operation.descriptionText = jsonObject[@"description"]; } id metadata = jsonObject[@"metadata"]; if (metadata && [metadata isKindOfClass:[NSDictionary class]]) { operation.metadata = metadata; } return operation; }
🧹 Nitpick comments (2)
Source/ARTMessageOperation.m (1)
18-18: Consider renaming the method for consistency.The method name
createFromDictionary:is inconsistent with its counterpartwriteToDictionary:. Consider renaming it toreadFromDictionary:for better naming consistency.Source/ARTJsonLikeEncoder.m (1)
305-308: Use a local variable for operation dictionary.Consider storing the operation dictionary in a local variable to avoid multiple dictionary lookups.
Apply this diff to improve the code:
- id operation = input[@"operation"]; - if (operation && [operation isKindOfClass:[NSDictionary class]]) { - message.operation = [ARTMessageOperation createFromDictionary:operation]; + id operationDict = input[@"operation"]; + if (operationDict && [operationDict isKindOfClass:[NSDictionary class]]) { + message.operation = [ARTMessageOperation createFromDictionary:operationDict]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Ably.xcodeproj/project.pbxproj(9 hunks)Source/ARTJsonLikeEncoder.m(2 hunks)Source/ARTMessage.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/include/Ably/ARTMessage.h(3 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Source/include/Ably/AblyPublic.h
- Source/ARTMessage.m
- Ably.xcodeproj/project.pbxproj
- Source/include/Ably/ARTMessageOperation.h
- Source/include/Ably/ARTMessage.h
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS17_2)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS17_2)
- GitHub Check: build
🔇 Additional comments (2)
Source/ARTMessageOperation.m (1)
6-16: LGTM!The method correctly handles nil values and follows a consistent pattern for writing properties to the dictionary.
Source/ARTJsonLikeEncoder.m (1)
28-28: LGTM!The import is correctly placed with other imports and is required for using the
ARTMessageOperationclass.
github-actions
bot
temporarily deployed
to
staging/pull/2036/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2036/features
github-actions
bot
temporarily deployed
to
staging/pull/2036/jazzydoc
ECO-5156: Adds required changes to support message updates and deletes....
mostly just extending the models to include the new Operation object
Summary by CodeRabbit