feat: Add livequery support #411
Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThis change introduces comprehensive support for Parse Live Query functionality. It adds interfaces, classes, and infrastructure for WebSocket-based real-time queries, including live query controllers, subscriptions, event argument types, and connection data models. Service hubs and the main Parse client are extended to support configuration and orchestration of live query services, with new extension methods for connecting and disconnecting from the live query server. Changes
Sequence Diagram(s)Live Query Subscription FlowsequenceDiagram
participant ClientApp
participant ParseQuery
participant ParseLiveQuery
participant ParseLiveQueryController
participant IWebSocketClient
participant Parse Server
ClientApp->>ParseQuery: GetLive()
ParseQuery->>ParseLiveQuery: new ParseLiveQuery(...)
ClientApp->>ParseLiveQuery: SubscribeAsync()
ParseLiveQuery->>ParseLiveQueryController: SubscribeAsync(liveQuery)
ParseLiveQueryController->>IWebSocketClient: OpenAsync(serverUri)
IWebSocketClient->>Parse Server: Establish WebSocket Connection
ParseLiveQueryController->>IWebSocketClient: SendAsync(subscribe message)
IWebSocketClient->>Parse Server: Send subscribe message
Parse Server-->>IWebSocketClient: Send live query events (create, update, etc.)
IWebSocketClient-->>ParseLiveQueryController: MessageReceived event
ParseLiveQueryController-->>ParseLiveQuerySubscription: OnCreate/OnUpdate/...
ParseLiveQuerySubscription-->>ClientApp: Raise Create/Update/... event
Live Query Connection ManagementsequenceDiagram
participant ClientApp
participant IServiceHub
participant IParseLiveQueryController
ClientApp->>IServiceHub: ConnectLiveQueryServerAsync(onError)
IServiceHub->>IParseLiveQueryController: ConnectAsync()
IParseLiveQueryController->>IWebSocketClient: OpenAsync(serverUri)
IWebSocketClient->>Parse Server: Establish WebSocket Connection
Parse Server-->>IWebSocketClient: Connection confirmation
IWebSocketClient-->>IParseLiveQueryController: MessageReceived event
ClientApp->>IServiceHub: DisconnectLiveQueryServerAsync()
IServiceHub->>IParseLiveQueryController: CloseAsync()
IParseLiveQueryController->>IWebSocketClient: CloseAsync()
IWebSocketClient->>Parse Server: Close WebSocket Connection
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (5)
Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs (2)
19-19: Consider adding connection state visibility.The interface lacks a way to query the current connection state, which could be useful for clients to determine if they need to reconnect or if operations are valid.
Consider adding a property like:
+ /// <summary> + /// Gets a value indicating whether the WebSocket connection is currently open. + /// </summary> + bool IsConnected { get; }
48-48: Missing cancellation token default parameter.The
SendAsyncmethod lacks a default value for the cancellation token parameter, which is inconsistent with the other methods.Apply this diff for consistency:
- public Task SendAsync(string message, CancellationToken cancellationToken); + public Task SendAsync(string message, CancellationToken cancellationToken = default);Parse/Platform/Queries/ParseQuery.cs (1)
915-919: Add XML documentation for the new method.The
GetLive()method lacks XML documentation, which is inconsistent with the rest of the codebase that has comprehensive documentation.Add XML documentation:
+/// <summary> +/// Creates a live query from this query that can be used to receive real-time updates +/// when objects matching the query are created, updated, or deleted. +/// </summary> +/// <returns>A new ParseLiveQuery instance configured with this query's parameters.</returns> public ParseLiveQuery<T> GetLive()Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs (1)
19-22: Documentation comment is redundant.The constructor's XML documentation largely repeats information already available in the class-level documentation and property documentation.
Consider simplifying or removing the redundant constructor documentation:
- /// <summary> - /// Represents the event arguments provided to Live Query event handlers in the Parse platform. - /// This class provides information about the current and original state of the Parse object - /// involved in the Live Query operation. - /// </summary> + /// <summary> + /// Initializes a new instance with the current and original object states. + /// </summary>Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs (1)
20-25: Fix documentation inconsistency in constructor comment.The constructor documentation mentions "current and original state of the Parse object" but the class only contains a single
Objectproperty representing the current state. This inconsistency should be resolved./// <summary> - /// Represents the event arguments provided to Live Query event handlers in the Parse platform. - /// This class provides information about the current and original state of the Parse object - /// involved in the Live Query operation. + /// Initializes a new instance of the ParseLiveQueryEventArgs class with the specified Parse object. /// </summary> + /// <param name="current">The current state of the Parse object associated with the event.</param> internal ParseLiveQueryEventArgs(ParseObject current) => Object = current;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
Parse.sln(2 hunks)Parse/Abstractions/Infrastructure/CustomServiceHub.cs(4 hunks)Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs(1 hunks)Parse/Abstractions/Infrastructure/IMutableServiceHub.cs(3 hunks)Parse/Abstractions/Infrastructure/IServiceHub.cs(3 hunks)Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs(1 hunks)Parse/Abstractions/Platform/LiveQueries/IParseLiveQuerySubscription.cs(1 hunks)Parse/Infrastructure/Execution/TextWebSocketClient.cs(1 hunks)Parse/Infrastructure/LateInitializedMutableServiceHub.cs(5 hunks)Parse/Infrastructure/MutableServiceHub.cs(6 hunks)Parse/Infrastructure/OrchestrationServiceHub.cs(4 hunks)Parse/Infrastructure/ServiceHub.cs(4 hunks)Parse/Platform/LiveQueries/ParseLiveQuery.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs(1 hunks)Parse/Platform/ParseClient.cs(1 hunks)Parse/Platform/Queries/ParseQuery.cs(1 hunks)Parse/Utilities/ObjectServiceExtensions.cs(5 hunks)
🔇 Additional comments (20)
Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs (1)
29-38: Ensure proper error handling in implementations.While the interface design is solid, implementations should handle common WebSocket scenarios like connection failures, network timeouts, and invalid URIs. Consider documenting expected exceptions in XML comments.
The interface methods should specify what exceptions can be thrown. Consider updating the documentation to include common exceptions like
InvalidOperationExceptionfor operations on closed connections orArgumentExceptionfor invalid URIs.Parse.sln (1)
20-21: LGTM! Solution file properly configured.The addition of the ParseApp project follows standard Visual Studio solution conventions with proper GUID assignment and build configurations for both Debug and Release modes.
Also applies to: 36-39
Parse/Abstractions/Infrastructure/CustomServiceHub.cs (1)
8-8: LGTM! Consistent implementation following established patterns.The additions properly extend the service hub abstraction to include live query capabilities. The new properties follow the same delegation pattern as existing properties and maintain consistency with the class design.
Also applies to: 35-35, 47-47, 67-67
Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs (1)
8-51: Well-designed event args class with comprehensive documentation.The class follows established EventArgs patterns with immutable properties and an internal constructor for controlled instantiation. The documentation is thorough and clearly explains each property's purpose.
Parse/Infrastructure/OrchestrationServiceHub.cs (2)
9-9: Good addition of required using statement.The using statement for live query abstractions is correctly added to support the new properties.
37-37: Well-integrated live query service properties.The new properties (
WebSocketClient,LiveQueryController,LiveQueryServerConnectionData) follow the established orchestration pattern perfectly, using the sameCustom ?? Defaultfallback approach as existing services.Also applies to: 49-50, 69-70
Parse/Abstractions/Infrastructure/IServiceHub.cs (2)
10-10: Correct addition of live query abstractions using statement.The using statement is properly added to support the new live query interface types.
30-30: Well-positioned live query service properties in interface.The new properties are logically positioned within the interface structure and follow consistent naming conventions with existing services.
Also applies to: 43-43, 50-50
Parse/Infrastructure/ServiceHub.cs (2)
10-10: Correct addition of required using statements.Both abstraction and platform using statements for live queries are properly added to support the new implementations.
Also applies to: 24-24
42-42: Excellent implementation of live query services with proper dependency injection.The live query services are well-integrated following the established ServiceHub pattern:
LiveQueryServerConnectionDataas a simple propertyWebSocketClientwith lazy initialization ofTextWebSocketClientLiveQueryControllerwith proper dependency injection ofWebSocketClientandDecoderThe dependency hierarchy is logical and the lazy initialization pattern is maintained consistently.
Also applies to: 55-55, 62-62
Parse/Infrastructure/LateInitializedMutableServiceHub.cs (1)
75-79: LGTM! Proper lazy initialization pattern implemented.The WebSocketClient and LiveQueryController properties follow the established lazy initialization pattern correctly, with appropriate dependency ordering (WebSocketClient is initialized before LiveQueryController which depends on it).
Also applies to: 111-115, 178-178
Parse/Abstractions/Infrastructure/IMutableServiceHub.cs (1)
22-22: Clean interface extension for live query support.The additions follow the established pattern of the mutable service hub interface, properly extending functionality without breaking existing contracts.
Also applies to: 35-35, 42-42
Parse/Utilities/ObjectServiceExtensions.cs (1)
291-317: Well-designed extension methods for live query management.The ConnectLiveQueryServerAsync and DisconnectLiveQueryServerAsync methods provide a clean API for managing live query connections with appropriate optional parameters and proper async patterns.
Parse/Platform/ParseClient.cs (1)
100-159: Constructor implementation follows established patterns.The new constructor overload properly handles separate configurations for the main server and live query server, maintaining consistency with the existing constructor design.
Parse/Infrastructure/MutableServiceHub.cs (1)
39-39: Proper integration of live query services.The MutableServiceHub correctly extends the service initialization pattern to include live query functionality, with appropriate dependency ordering and consistent null-coalescing initialization.
Also applies to: 52-52, 59-59, 73-76, 93-93, 100-100
Parse/Abstractions/Platform/LiveQueries/IParseLiveQuerySubscription.cs (1)
9-77: Well-designed interface with comprehensive event coverage.The interface provides a clean abstraction for live query subscriptions with proper event handling for all object lifecycle events and appropriate async method signatures with cancellation token support.
Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs (1)
8-127: Excellent interface design with comprehensive functionality.The interface provides a complete contract for live query management with proper async patterns, exception documentation, and comprehensive method coverage for all live query operations.
Parse/Platform/LiveQueries/ParseLiveQuery.cs (1)
18-96: Well-structured live query implementation with modern C# patterns.The class design is clean with proper encapsulation, immutable composition pattern, and modern C# features like collection expressions in
MergeWatchers.Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs (1)
14-51: Well-implemented subscription class with proper event handling.The class correctly implements the interface with proper event definitions and event handler methods that safely invoke events with null checks.
Also applies to: 98-142
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
26-26: Make ClientId setter private to ensure immutability.The
ClientIdis only set once during connection establishment and should not be modified externally.-private string ClientId { get; set; } +private string ClientId { get; private set; }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (1)
108-109: Fix cancellation token usage in SubscribeAsync.The method ignores the implicit cancellation token and uses
CancellationToken.None, which prevents callers from cancelling the subscription operation.The method should accept a cancellation token parameter:
-public async Task<IParseLiveQuerySubscription> SubscribeAsync() => - await Services.LiveQueryController.SubscribeAsync(this, CancellationToken.None); +public async Task<IParseLiveQuerySubscription> SubscribeAsync(CancellationToken cancellationToken = default) => + await Services.LiveQueryController.SubscribeAsync(this, cancellationToken);
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (1)
63-77: Simplify KeyWatchers assignment logic.The assignment at line 71 is unnecessary since it's conditionally overwritten at line 75. This makes the code harder to follow.
private ParseLiveQuery(ParseLiveQuery<T> source, IEnumerable<string> watchedKeys = null) { ArgumentNullException.ThrowIfNull(source); Services = source.Services; ClassName = source.ClassName; Filters = source.Filters; KeySelections = source.KeySelections; - KeyWatchers = source.KeyWatchers; - if (watchedKeys is not null) - { - KeyWatchers = new ReadOnlyCollection<string>(MergeWatchers(watchedKeys).ToList()); - } + KeyWatchers = watchedKeys is not null + ? new ReadOnlyCollection<string>(MergeWatchers(watchedKeys).ToList()) + : source.KeyWatchers; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Parse/Platform/LiveQueries/ParseLiveQuery.cs(1 hunks)Parse/Platform/Queries/ParseQuery.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Parse/Platform/Queries/ParseQuery.cs
🔇 Additional comments (2)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (2)
79-79: LGTM!The method correctly uses modern C# collection expression syntax and handles null values appropriately.
90-98: LGTM!The method correctly builds query parameters with appropriate null checks to avoid adding empty collections.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
Parse/Infrastructure/Execution/TextWebSocketClient.cs (2)
82-82: Make buffer size configurable.The buffer size is hardcoded to 4KB, which limits flexibility for different use cases.
Add a configurable property:
+private int BufferSize { get; set; } = 1024 * 4; private async Task ListenForMessages(CancellationToken cancellationToken) { - byte[] buffer = new byte[1024 * 4]; + byte[] buffer = new byte[BufferSize];
167-171: Add WebSocket state validation.The method should verify the WebSocket is open before sending to avoid exceptions.
public async Task SendAsync(string message, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(webSocket); + if (webSocket.State != WebSocketState.Open) + { + throw new InvalidOperationException("WebSocket is not connected or not in an open state."); + } await webSocket.SendAsync(Encoding.UTF8.GetBytes(message), WebSocketMessageType.Text, true, cancellationToken); }Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs (2)
78-79: Fix cancellation token usage.The method ignores the
cancellationTokenparameter.public async Task UpdateAsync<T1>(ParseLiveQuery<T1> liveQuery, CancellationToken cancellationToken = default) where T1 : ParseObject => - await Services.LiveQueryController.UpdateSubscriptionAsync(liveQuery, RequestId, cancellationToken); + await Services.LiveQueryController.UpdateSubscriptionAsync(liveQuery, RequestId, cancellationToken);Also consider simplifying the generic parameter since the class is already generic:
-public async Task UpdateAsync<T1>(ParseLiveQuery<T1> liveQuery, CancellationToken cancellationToken = default) where T1 : ParseObject => +public async Task UpdateAsync(ParseLiveQuery<T> liveQuery, CancellationToken cancellationToken = default) =>
88-89: Fix cancellation token usage in CancelAsync.The method ignores the
cancellationTokenparameter.public async Task CancelAsync(CancellationToken cancellationToken = default) => - await Services.LiveQueryController.UnsubscribeAsync(RequestId, cancellationToken); + await Services.LiveQueryController.UnsubscribeAsync(RequestId, cancellationToken);Parse/Platform/LiveQueries/ParseLiveQuery.cs (1)
39-42: Add null validation for critical constructor parameters.The constructor should validate
serviceHubandclassNameparameters.internal ParseLiveQuery(IServiceHub serviceHub, string className, IDictionary<string, object> filters, IEnumerable<string> selectedKeys = null, IEnumerable<string> watchedKeys = null) { + ArgumentNullException.ThrowIfNull(serviceHub); + ArgumentException.ThrowIfNullOrWhiteSpace(className); ArgumentNullException.ThrowIfNull(filters);Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
262-262: Add safe casting to prevent InvalidCastException.Direct casting to
boolcould throw if the value is null or not a boolean.- (bool) message["reconnect"]); + message.TryGetValue("reconnect", out object reconnectValue) && reconnectValue is bool reconnect ? reconnect : false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Parse/Infrastructure/Execution/TextWebSocketClient.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQuery.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs(1 hunks)Parse/Platform/ParseClient.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs
- Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs
- Parse/Platform/ParseClient.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Parse/Infrastructure/Execution/TextWebSocketClient.cs (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (9)
Task(297-305)Task(307-308)Task(310-323)Task(350-389)Task(391-416)Task(440-458)Task(479-488)Task(505-514)Task(525-533)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Parse/Infrastructure/Execution/TextWebSocketClient.cs (2)
183-187: Add WebSocket state validation to match documented exceptions.The method correctly handles null checks but doesn't validate the WebSocket state before sending, which could result in exceptions that don't match the documented behavior.
public async Task SendAsync(string message, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(webSocket); + + if (webSocket.State != WebSocketState.Open) + { + throw new InvalidOperationException($"WebSocket is not in Open state. Current state: {webSocket.State}"); + } + await webSocket.SendAsync(Encoding.UTF8.GetBytes(message), WebSocketMessageType.Text, true, cancellationToken); }
55-67: Address remaining race condition in connection logic.While the WebSocket initialization is properly synchronized, the state check and connection attempt are still not atomic. Multiple threads could pass the state check simultaneously and attempt to connect concurrently.
Consider this approach to ensure atomic connection logic:
public async Task OpenAsync(string serverUri, CancellationToken cancellationToken = default) { + ClientWebSocket webSocketToConnect = null; + lock (connectionLock) { webSocket ??= new ClientWebSocket(); + + if (webSocket.State != WebSocketState.Open && webSocket.State != WebSocketState.Connecting) + { + webSocketToConnect = webSocket; + } } - if (webSocket.State != WebSocketState.Open && webSocket.State != WebSocketState.Connecting) + if (webSocketToConnect != null) { - await webSocket.ConnectAsync(new Uri(serverUri), cancellationToken); + await webSocketToConnect.ConnectAsync(new Uri(serverUri), cancellationToken); StartListening(cancellationToken); } }
🧹 Nitpick comments (2)
Parse/Infrastructure/Execution/TextWebSocketClient.cs (2)
77-78: Remove unnecessary null-forgiving operator.The null-forgiving operator (!) is unnecessary and potentially misleading since the null-conditional operator (?.) can return null.
-public async Task CloseAsync(CancellationToken cancellationToken = default) => - await webSocket?.CloseAsync(WebSocketCloseStatus.NormalClosure, String.Empty, cancellationToken)!; +public async Task CloseAsync(CancellationToken cancellationToken = default) +{ + if (webSocket != null) + { + await webSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, String.Empty, cancellationToken); + } +}
80-137: Excellent exception handling and partial message support!The method properly handles various exception types and correctly manages partial WebSocket messages. However, consider making the buffer size configurable for different use cases.
Consider adding a configurable buffer size:
+private const int DefaultBufferSize = 1024 * 4; +private readonly int bufferSize; +// Add to constructor or make it a property +public int BufferSize { get; init; } = DefaultBufferSize; private async Task ListenForMessages(CancellationToken cancellationToken) { - byte[] buffer = new byte[1024 * 4]; + byte[] buffer = new byte[BufferSize]; // ... rest of the method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Parse/Infrastructure/Execution/TextWebSocketClient.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQuery.cs(1 hunks)
🔇 Additional comments (11)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (9)
1-9: LGTM - Clean imports and namespace declaration.The using statements are appropriate and well-organized, importing necessary System libraries and Parse-specific abstractions for live query functionality.
12-18: LGTM - Well-documented generic class declaration.The XML documentation clearly explains the purpose and constraints of the ParseLiveQuery class. The generic constraint
where T : ParseObjectis appropriate for the Parse SDK context.
20-37: LGTM - Well-designed immutable properties.The properties follow good immutability practices with get-only accessors and appropriate access levels. Using
ReadOnlyCollection<string>for collections ensures the class remains immutable after construction.
39-58: LGTM - Robust constructor with proper validation.The constructor properly validates all critical parameters and handles optional collections correctly. The use of
ToList()prevents potential deferred execution issues with the input enumerables.
65-79: LGTM - Well-designed composition constructor.The private constructor for query composition properly copies all properties from the source and allows extending watched keys. Good validation and immutability design.
83-90: LGTM - Clean fluent API method.The
Watchmethod provides a clean fluent interface for adding watched keys, maintaining immutability by returning a new instance.
92-100: LGTM - Correct parameter building logic.The
BuildParametersmethod correctly constructs the dictionary needed for live query operations, properly handling optional collections and converting them to arrays as expected by the Parse Server API.
102-111: LGTM - Proper async subscription with cancellation support.The
SubscribeAsyncmethod correctly accepts a cancellation token parameter and passes it through to the controller. This addresses the previous review concern about cancellation token usage.
81-81: Verify C# 12 collection expression compatibility.The code uses C# 12 collection expression syntax
[..]which may not be compatible with older target frameworks. Ensure your project targets .NET 8+ or C# 12+.#!/bin/bash # Description: Check the target framework and C# language version in project files # Expected: Confirm .NET 8+ or C# 12+ support for collection expressions echo "Checking .csproj files for target framework and language version..." fd -e csproj --exec cat {} \; | grep -E "(TargetFramework|LangVersion)" echo -e "\nChecking Directory.Build.props for global settings..." find . -name "Directory.Build.props" -exec cat {} \;Parse/Infrastructure/Execution/TextWebSocketClient.cs (2)
11-44: LGTM! Good class structure with proper thread safety.The class is well-structured with appropriate encapsulation, proper field declarations, and includes a connection lock for thread safety. The use of events for message notification follows good event-driven design patterns.
143-169: Well-implemented listener management with proper fault monitoring.The method correctly prevents multiple concurrent listeners and properly monitors the listening task for faults without causing application crashes. The fault handling approach is much improved.
…many improvements
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
564-564: Consider potential deadlock in Dispose method.Using
GetAwaiter().GetResult()in a disposal context could potentially cause deadlocks in certain synchronization contexts, though this is a common pattern for dispose methods.Consider using
ConfigureAwait(false)to reduce deadlock risk:- CloseAsync().GetAwaiter().GetResult(); + CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Parse/Abstractions/Infrastructure/CustomServiceHub.cs(4 hunks)Parse/Abstractions/Infrastructure/ILiveQueryServerConnectionData.cs(1 hunks)Parse/Abstractions/Infrastructure/IMutableServiceHub.cs(3 hunks)Parse/Abstractions/Infrastructure/IServiceHub.cs(3 hunks)Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs(1 hunks)Parse/Abstractions/Platform/LiveQueries/IParseLiveQuerySubscription.cs(1 hunks)Parse/Infrastructure/Execution/TextWebSocketClient.cs(1 hunks)Parse/Infrastructure/LateInitializedMutableServiceHub.cs(3 hunks)Parse/Infrastructure/LiveQueryServerConnectionData.cs(1 hunks)Parse/Infrastructure/MutableServiceHub.cs(6 hunks)Parse/Infrastructure/OrchestrationServiceHub.cs(4 hunks)Parse/Infrastructure/ServiceHub.cs(4 hunks)Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)Parse/Platform/ParseClient.cs(1 hunks)Parse/Utilities/ObjectServiceExtensions.cs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- Parse/Infrastructure/LiveQueryServerConnectionData.cs
🚧 Files skipped from review as they are similar to previous changes (12)
- Parse/Infrastructure/OrchestrationServiceHub.cs
- Parse/Abstractions/Infrastructure/CustomServiceHub.cs
- Parse/Utilities/ObjectServiceExtensions.cs
- Parse/Abstractions/Infrastructure/IMutableServiceHub.cs
- Parse/Abstractions/Infrastructure/IServiceHub.cs
- Parse/Infrastructure/ServiceHub.cs
- Parse/Platform/ParseClient.cs
- Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs
- Parse/Infrastructure/Execution/TextWebSocketClient.cs
- Parse/Abstractions/Platform/LiveQueries/IParseLiveQuerySubscription.cs
- Parse/Infrastructure/LateInitializedMutableServiceHub.cs
- Parse/Infrastructure/MutableServiceHub.cs
🔇 Additional comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
20-124: Excellent implementation of previous feedback.The class now properly implements thread safety with
volatilestate field,ConcurrentDictionarycollections,Interlocked.Incrementfor request IDs, and a properIDisposablepattern. The async patterns have been correctly implemented withTaskCompletionSourceinstead of blocking calls.
Parse/Abstractions/Infrastructure/ILiveQueryServerConnectionData.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
174-174: Use safe type conversion to prevent runtime exceptions.The direct use of
Convert.ToInt32()andConvert.ToBoolean()without null checking can throw exceptions if the message values are null or invalid.Apply safe conversion patterns as previously suggested:
- int requestId = Convert.ToInt32(message["requestId"]); + if (!message.TryGetValue("requestId", out object requestIdObj) || !int.TryParse(requestIdObj?.ToString(), out int requestId)) + return;Also applies to lines 189, 209, 229, 249, 262, 264, 273, 285.
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
169-257: Extract repeated client ID validation into a helper method.Multiple event processing methods duplicate the same client ID validation pattern, violating DRY principles.
Consider extracting this into a helper method:
private bool ValidateClientMessage(IDictionary<string, object> message, out int requestId) { requestId = 0; if (!(message.TryGetValue("clientId", out object clientIdObj) && clientIdObj is string clientId && clientId == ClientId)) return false; return message.TryGetValue("requestId", out object requestIdObj) && int.TryParse(requestIdObj?.ToString(), out requestId); }Then update each event processing method:
- string clientId = message["clientId"] as string; - if (clientId != ClientId) - return; - int requestId = Convert.ToInt32(message["requestId"]); + if (!ValidateClientMessage(message, out int requestId)) + return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Parse/Abstractions/Infrastructure/ILiveQueryServerConnectionData.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Parse/Abstractions/Infrastructure/ILiveQueryServerConnectionData.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (6)
Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs (2)
ParseLiveQueryErrorEventArgs(8-51)ParseLiveQueryErrorEventArgs(45-50)Parse/Platform/LiveQueries/ParseLiveQuery.cs (6)
IDictionary(92-100)Task(110-111)ParseLiveQuery(18-112)ParseLiveQuery(39-58)ParseLiveQuery(65-79)ParseLiveQuery(90-90)Parse/Platform/ParseClient.cs (7)
IDictionary(185-196)IDictionary(198-201)ParseClient(19-220)ParseClient(58-58)ParseClient(66-98)ParseClient(107-159)ParseClient(164-164)Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs (9)
OnDelete(141-142)OnLeave(130-133)OnUpdate(118-121)OnEnter(107-110)OnCreate(98-99)Task(78-79)Task(88-89)ParseLiveQuerySubscription(14-143)ParseLiveQuerySubscription(58-63)Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs (5)
Task(41-41)Task(64-64)Task(84-84)Task(101-101)Task(114-114)Parse/Infrastructure/Execution/TextWebSocketClient.cs (4)
Task(57-74)Task(84-90)Task(92-149)Task(195-203)
🔇 Additional comments (3)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (3)
20-124: Class structure and constructor implementation look excellent.The implementation properly addresses thread safety concerns with
ConcurrentDictionarycollections, volatile state field, and usesInterlockedoperations for request ID generation. The constructor is well-designed with clear parameter requirements.
352-534: Excellent async implementation with proper resource management.The public API methods demonstrate best practices with TaskCompletionSource usage, proper cancellation token handling, and comprehensive resource cleanup. The refactored
SendAndWaitForSignalAsynchelper eliminates code duplication effectively.
544-567: Proper IDisposable pattern implementation.The dispose pattern correctly implements standard .NET practices with appropriate resource cleanup and proper handling of the async CloseAsync call within the synchronous dispose context.
|
Is this work in progress? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
575-584: Consider potential deadlock risk in Dispose method.The current implementation calls
CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult()which could potentially cause deadlocks in certain synchronization contexts, particularly in ASP.NET applications.Consider one of these alternatives:
Option 1: Use async disposal pattern (if targeting .NET Standard 2.1+):
-public class ParseLiveQueryController : IParseLiveQueryController, IDisposable +public class ParseLiveQueryController : IParseLiveQueryController, IDisposable, IAsyncDisposable +public async ValueTask DisposeAsync() +{ + await CloseAsync(); + GC.SuppressFinalize(this); +} private void Dispose(bool disposing) { if (disposed) return; if (disposing) { - CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + // For sync disposal, best effort cleanup without waiting + _ = Task.Run(async () => await CloseAsync()); } disposed = true; }Option 2: Keep current approach but document the risk:
Add XML documentation warning about potential deadlock when calling Dispose from UI thread or ASP.NET request context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)
🔇 Additional comments (5)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (5)
20-104: LGTM! Thread safety improvements are well implemented.The class properly implements IDisposable and uses thread-safe collections (ConcurrentDictionary) and volatile state field. The previous thread safety concerns have been well addressed.
358-408: LGTM! Async pattern implementation is excellent.The
ConnectAsyncmethod properly usesTaskCompletionSourceand async/await patterns, eliminating the previous blocking issues. The timeout handling and state management are well implemented.
410-435: LGTM! Excellent refactoring that eliminates code duplication.The
SendAndWaitForSignalAsynchelper method is well-designed and eliminates the code duplication that existed in the subscription methods. The async pattern usage and resource cleanup are properly implemented.
437-533: LGTM! Subscription methods are well implemented.The subscription methods properly use thread-safe operations (
Interlocked.Increment) and the new helper method to eliminate code duplication. Error handling for closed connections is appropriate.
535-552: LGTM! Resource cleanup is properly implemented.The
CloseAsyncmethod correctly unsubscribes from event handlers and clears all collections. The resource cleanup addresses the previous concerns about memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
318-327: Add null check for ConnectionSignal to prevent potential race condition.The
ProcessConnectionMessagemethod callsConnectionSignal.TrySetResult()without checking ifConnectionSignalis null. There's a potential race condition where this could be called afterConnectAsynchas setConnectionSignalto null in its finally block.void ProcessConnectionMessage(IDictionary<string, object> message) { if (!(message.TryGetValue("clientId", out object clientIdObj) && clientIdObj is string clientId)) return; ClientId = clientId; _state = ParseLiveQueryState.Connected; - ConnectionSignal.TrySetResult(); + ConnectionSignal?.TrySetResult(); }
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
587-596: Consider using fire-and-forget pattern to avoid potential deadlocks.Using
ConfigureAwait(false).GetAwaiter().GetResult()in theDisposemethod could potentially cause deadlocks in certain synchronization contexts.private void Dispose(bool disposing) { if (disposed) return; if (disposing) { - CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + try + { + CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + } + catch + { + // Suppress exceptions during disposal + } } disposed = true; }Alternatively, consider making the cleanup fire-and-forget:
- CloseAsync().ConfigureAwait(false).GetAwaiter().GetResult(); + _ = Task.Run(async () => await CloseAsync().ConfigureAwait(false));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)
🔇 Additional comments (8)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (8)
118-124: LGTM! Constructor properly initializes all dependencies.The constructor correctly initializes all required fields and sets the initial state appropriately.
422-447: LGTM! Excellent async helper method implementation.The
SendAndWaitForSignalAsyncmethod provides a clean, reusable pattern for async operations with proper timeout and cancellation handling.
471-489: LGTM! Well-implemented subscription method with proper validation.The
SubscribeAsyncmethod correctly validates the connection state, uses thread-safe ID generation, and properly manages the subscription lifecycle.
279-294: LGTM! Robust error message processing with safe parsing.The
ProcessErrorMessagemethod properly validates all required fields and uses safe parsing patterns to prevent exceptions.
169-192: LGTM! Excellent helper methods for safe message processing.The
ValidateClientMessageandGetDictEntrymethods provide consistent, safe patterns for validating and extracting data from WebSocket messages.
357-368: LGTM! Safe WebSocket message handling with proper validation.The
WebSocketClientOnMessageReceivedmethod properly validates the parsed JSON before processing, preventing null reference exceptions.
98-104: LGTM! Thread-safe state and collection management.The implementation properly uses a volatile backing field for state and ConcurrentDictionary for all shared collections, ensuring thread safety.
16-21: Excellent overall implementation of the live query controller.This implementation demonstrates good software engineering practices with:
- Proper thread safety using concurrent collections and volatile fields
- Clean async/await patterns without blocking calls
- Comprehensive error handling and validation
- Proper resource management with IDisposable implementation
- Well-structured code with good separation of concerns
The refactoring based on previous review feedback has resulted in a robust, maintainable implementation.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)
🔇 Additional comments (3)
Parse/Platform/LiveQueries/ParseLiveQueryController.cs (3)
126-167: The ProcessMessage method structure looks good.The switch statement is clean and delegates to specific handler methods. The individual process methods provide good separation of concerns and safe message handling.
422-447: Excellent refactoring of async pattern.The
SendAndWaitForSignalAsynchelper method effectively reduces code duplication and properly implements the async pattern with timeout handling and resource cleanup.
98-104: Good implementation of thread safety.The use of
volatilefor the state field andConcurrentDictionaryfor collections addresses the concurrent access concerns effectively.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs (5)
10-18: Fix property documentation inconsistency.The documentation states "Gets or sets" but the property is read-only. Update the documentation to accurately reflect the property's behavior.
/// <summary> - /// Gets or sets the error message associated with a live query operation. + /// Gets the error message associated with a live query operation. /// </summary>
20-28: Fix property documentation inconsistency.The documentation states "Gets or sets" but the property is read-only. Update the documentation to accurately reflect the property's behavior.
/// <summary> - /// Gets or sets the error code associated with a live query operation. + /// Gets the error code associated with a live query operation. /// </summary>
30-40: Fix property documentation inconsistency.The documentation states "Gets or sets" but the property is read-only. Update the documentation to accurately reflect the property's behavior.
/// <summary> - /// Gets or sets a value indicating whether the client should attempt to reconnect + /// Gets a value indicating whether the client should attempt to reconnect /// after an error occurs during a live query operation. /// </summary>
42-42: Add missing XML documentation for LocalException property.The
LocalExceptionproperty lacks XML documentation, which is inconsistent with the comprehensive documentation provided for other properties in the class.+ /// <summary> + /// Gets the local exception that occurred during the live query operation, if any. + /// </summary> + /// <remarks> + /// The <see cref="LocalException"/> property contains the actual exception instance + /// that was thrown locally during the live query operation. This can be used for + /// detailed error analysis and debugging purposes. + /// </remarks> public Exception LocalException { get; }
44-47: Improve constructor documentation.The constructor documentation duplicates the class summary, which is not helpful. Constructor documentation should describe the constructor's purpose and parameters.
/// <summary> - /// Represents the arguments for an error event that occurs during a live query in the Parse platform. + /// Initializes a new instance of the <see cref="ParseLiveQueryErrorEventArgs"/> class. /// </summary> + /// <param name="code">The error code associated with the live query operation.</param> + /// <param name="error">The error message describing the issue.</param> + /// <param name="reconnect">A value indicating whether the client should attempt to reconnect.</param> + /// <param name="localException">The local exception that occurred, if any.</param>Parse/Infrastructure/Execution/TextWebSocketClient.cs (4)
88-94: Remove redundant null conditional operator.Line 92 uses the null conditional operator
?.after already checking for null, which is redundant and could cause confusion.public async Task CloseAsync(CancellationToken cancellationToken = default) { if (webSocket is not null) { - await webSocket?.CloseAsync(WebSocketCloseStatus.NormalClosure, String.Empty, cancellationToken)!; + await webSocket.CloseAsync(WebSocketCloseStatus.NormalClosure, String.Empty, cancellationToken); } }
133-133: Fix double semicolon syntax error.Line 133 has an extra semicolon that should be removed.
- MessageReceived?.Invoke(this, new MessageReceivedEventArgs(fullMessage));; + MessageReceived?.Invoke(this, new MessageReceivedEventArgs(fullMessage));
120-134: Consider optimizing partial message handling for better memory efficiency.The current implementation creates a new StringBuilder for each partial message. For large messages, this could be more memory-efficient by using a MemoryStream or reusing buffers.
Consider this more efficient approach:
else { - // Handle partial messages by accumulating data until EndOfMessage is true - StringBuilder messageBuilder = new StringBuilder(); - messageBuilder.Append(Encoding.UTF8.GetString(buffer, 0, result.Count)); - while (!result.EndOfMessage) - { - result = await webSocket.ReceiveAsync( - new ArraySegment<byte>(buffer), - cancellationToken); - messageBuilder.Append(Encoding.UTF8.GetString(buffer, 0, result.Count)); - } - string fullMessage = messageBuilder.ToString(); - MessageReceived?.Invoke(this, new MessageReceivedEventArgs(fullMessage)); + // Handle partial messages by accumulating bytes until EndOfMessage is true + using var messageStream = new MemoryStream(); + messageStream.Write(buffer, 0, result.Count); + while (!result.EndOfMessage) + { + result = await webSocket.ReceiveAsync( + new ArraySegment<byte>(buffer), + cancellationToken); + messageStream.Write(buffer, 0, result.Count); + } + string fullMessage = Encoding.UTF8.GetString(messageStream.ToArray()); + MessageReceived?.Invoke(this, new MessageReceivedEventArgs(fullMessage)); }
17-17: Consider making the class public if external extensibility is needed.The class is currently marked as
internal, which limits its usage to the Parse SDK assembly. If there's a need for external consumers to provide custom WebSocket implementations or for testing purposes, consider making itpublic.-class TextWebSocketClient(int bufferSize) : IWebSocketClient +public class TextWebSocketClient(int bufferSize) : IWebSocketClient
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs(1 hunks)Parse/Infrastructure/Execution/MessageReceivedEventArgs.cs(1 hunks)Parse/Infrastructure/Execution/TextWebSocketClient.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryController.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs(1 hunks)Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Parse/Infrastructure/Execution/MessageReceivedEventArgs.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs
- Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs
- Parse/Platform/LiveQueries/ParseLiveQueryController.cs
🔇 Additional comments (4)
Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs (1)
8-53: Well-designed EventArgs implementation.The class follows good practices with immutable properties and proper inheritance from
EventArgs. Theinternalconstructor is appropriate for SDK components, ensuring controlled instantiation while allowing internal components to create error event instances.Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs (1)
1-71: Well-designed WebSocket client interface.The interface is clean, well-documented, and follows .NET conventions. The event-driven design with typed EventArgs, comprehensive XML documentation, and proper cancellation token support throughout all async methods make this a solid foundation for the live query system.
Parse/Infrastructure/Execution/TextWebSocketClient.cs (2)
61-78: Thread-safe connection logic looks good.The implementation properly addresses the race condition concerns from previous reviews by using a lock to ensure atomic state checking and connection setup. The approach of capturing the WebSocket reference inside the lock and performing the async operation outside prevents deadlocks while maintaining thread safety.
96-155: Comprehensive exception handling and partial message support.The implementation properly handles all the concerns raised in previous reviews:
- Catches and handles multiple exception types (OperationCanceledException, WebSocketException, and general exceptions)
- Properly handles partial WebSocket messages by checking EndOfMessage flag
- Raises appropriate events for different error scenarios
- Uses configurable buffer size
Added:
ParseLiveQueryclassParseLiveQueryControllerclassParseLiveQuerySubscriptionclassParseQuery.GetLive()for creating aParseLiveQueryLiveQueryServerConnectionDataas aServerConnectionDatafield toParseClientto save live query server info.ConnectLiveQueryServerAsync(...)andDisconnectLiveQueryServerAsync(...)toObjectServiceExtensionsfor connecting to and disconnecting respectively from the given parse live query server.Summary by CodeRabbit