feat: Add livequery support#411
Conversation
|
🚀 Thanks for opening this pull request! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Parse Live Query support: new connection-data types, WebSocket client interface and text implementation, message parser/builder, controller and subscription types, ParseQuery→LiveQuery integration, service-hub wiring and ParseClient overloads, extension helpers, and many unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant ParseQuery
participant ParseLiveQuery
participant LiveQueryController
participant WebSocketClient
participant Server
App->>ParseQuery: GetLive()
ParseQuery->>ParseLiveQuery: create live query
App->>ParseLiveQuery: SubscribeAsync()
ParseLiveQuery->>LiveQueryController: SubscribeAsync(liveQuery)
LiveQueryController->>WebSocketClient: OpenAsync(serverUri) / SendAsync(connect/subscribe)
WebSocketClient->>Server: WebSocket frames (connect/subscribe)
Server-->>WebSocketClient: event messages
WebSocketClient-->>LiveQueryController: MessageReceived
LiveQueryController-->>ParseLiveQuerySubscription: OnCreate/OnUpdate/OnDelete...
ParseLiveQuerySubscription-->>App: Raise event handlers
sequenceDiagram
autonumber
participant App
participant IServiceHub
participant LiveQueryController
participant WebSocketClient
App->>IServiceHub: ConnectLiveQueryServerAsync(onError)
IServiceHub->>LiveQueryController: ConnectAsync()
LiveQueryController->>WebSocketClient: OpenAsync(serverUri)
LiveQueryController->>WebSocketClient: SendAsync(connect message)
App->>IServiceHub: DisconnectLiveQueryServerAsync()
IServiceHub->>LiveQueryController: CloseAsync()
LiveQueryController->>WebSocketClient: CloseAsync()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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.
🧹 Nitpick comments (1)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (1)
96-96: Defensively copywherepayload to avoid shared-state mutation.
BuildParameters()currently passesFiltersby reference. A shallow copy here reduces accidental mutation risk when downstream code manipulates payload dictionaries.♻️ Proposed refactor
- Dictionary<string, object> result = new Dictionary<string, object> { ["className"] = ClassName, ["where"] = Filters }; + Dictionary<string, object> result = new Dictionary<string, object> + { + ["className"] = ClassName, + ["where"] = new Dictionary<string, object>(Filters) + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse/Platform/LiveQueries/ParseLiveQuery.cs` at line 96, BuildParameters is assigning Filters directly into the result dict, creating shared-state risk; instead create a shallow defensive copy for the "where" payload (handle null Filters) so downstream mutations don't affect the original Filters. Update the line that constructs result (the Dictionary<string, object> with ["className"] = ClassName, ["where"] = Filters) to set ["where"] to a new shallow copy of Filters (e.g., a new Dictionary constructed from Filters) rather than the original Filters reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Parse/Platform/LiveQueries/ParseLiveQuery.cs`:
- Line 96: BuildParameters is assigning Filters directly into the result dict,
creating shared-state risk; instead create a shallow defensive copy for the
"where" payload (handle null Filters) so downstream mutations don't affect the
original Filters. Update the line that constructs result (the Dictionary<string,
object> with ["className"] = ClassName, ["where"] = Filters) to set ["where"] to
a new shallow copy of Filters (e.g., a new Dictionary constructed from Filters)
rather than the original Filters reference.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Parse.Tests/ClientTests.csParse/Platform/LiveQueries/ParseLiveQuery.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Parse.Tests/ClientTests.cs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Parse/Platform/LiveQueries/ParseLiveQuery.cs (2)
117-118: Add.ConfigureAwait(false)in libraryasyncmethods.In library code,
awaitwithout.ConfigureAwait(false)can cause deadlocks when callers run in a synchronization-context-bound environment (e.g., classic ASP.NET, WPF, WinForms). The Parse .NET SDK is a library and should propagate this pattern consistently.♻️ Proposed refactor
- public async Task<IParseLiveQuerySubscription> SubscribeAsync(CancellationToken cancellationToken = default) => - await Services.LiveQueryController.SubscribeAsync(this, cancellationToken); + public async Task<IParseLiveQuerySubscription> SubscribeAsync(CancellationToken cancellationToken = default) => + await Services.LiveQueryController.SubscribeAsync(this, cancellationToken).ConfigureAwait(false);Alternatively, since there is no
asyncbody logic, the async/await can be dropped entirely:- public async Task<IParseLiveQuerySubscription> SubscribeAsync(CancellationToken cancellationToken = default) => - await Services.LiveQueryController.SubscribeAsync(this, cancellationToken); + public Task<IParseLiveQuerySubscription> SubscribeAsync(CancellationToken cancellationToken = default) => + Services.LiveQueryController.SubscribeAsync(this, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse/Platform/LiveQueries/ParseLiveQuery.cs` around lines 117 - 118, The SubscribeAsync method in ParseLiveQuery.cs should avoid awaiting without ConfigureAwait in library code; change the implementation of ParseLiveQuery.SubscribeAsync to either append .ConfigureAwait(false) to the await (i.e., await Services.LiveQueryController.SubscribeAsync(this, cancellationToken).ConfigureAwait(false)) or remove the async/await entirely and return the Task directly (return Services.LiveQueryController.SubscribeAsync(this, cancellationToken)); update the IParseLiveQuerySubscription return accordingly so the library no longer captures a synchronization context.
79-79:MergeWatcherscan be madestatic.The method only uses
KeyWatchers(passed implicitly viathis) and the parameterkeys. SinceKeyWatchersis already read fromthisafter it is assigned (line 73), the logic is correct. However, making the helperstaticmakes the intent explicit and prevents future accidental reads of uninitialized instance state from any refactor that reorders constructor lines.♻️ Proposed refactor
- private HashSet<string> MergeWatchers(IEnumerable<string> keys) => [..(KeyWatchers ?? Enumerable.Empty<string>()).Concat(keys)]; + private static HashSet<string> MergeWatchers(ReadOnlyCollection<string> existing, IEnumerable<string> keys) => + [..(existing ?? Enumerable.Empty<string>()).Concat(keys)];Update the call site:
- if (watchedKeys is not null) - KeyWatchers = new ReadOnlyCollection<string>([.. MergeWatchers(watchedKeys)]); + if (watchedKeys is not null) + KeyWatchers = new ReadOnlyCollection<string>([.. MergeWatchers(source.KeyWatchers, watchedKeys)]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse/Platform/LiveQueries/ParseLiveQuery.cs` at line 79, Make MergeWatchers a static helper to avoid accidental instance reads: change its signature to accept the instance KeyWatchers explicitly (e.g. private static HashSet<string> MergeWatchers(IEnumerable<string> keyWatchers, IEnumerable<string> keys) => new HashSet<string>((keyWatchers ?? Enumerable.Empty<string>()).Concat(keys));) and update all call sites that currently call MergeWatchers(keys) to pass KeyWatchers (or KeyWatchers ?? Enumerable.Empty<string()) as the first argument so callers become MergeWatchers(KeyWatchers, keys). Ensure you reference the existing MergeWatchers and KeyWatchers symbols when making these edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Parse/Platform/LiveQueries/ParseLiveQuery.cs`:
- Around line 117-118: The SubscribeAsync method in ParseLiveQuery.cs should
avoid awaiting without ConfigureAwait in library code; change the implementation
of ParseLiveQuery.SubscribeAsync to either append .ConfigureAwait(false) to the
await (i.e., await Services.LiveQueryController.SubscribeAsync(this,
cancellationToken).ConfigureAwait(false)) or remove the async/await entirely and
return the Task directly (return
Services.LiveQueryController.SubscribeAsync(this, cancellationToken)); update
the IParseLiveQuerySubscription return accordingly so the library no longer
captures a synchronization context.
- Line 79: Make MergeWatchers a static helper to avoid accidental instance
reads: change its signature to accept the instance KeyWatchers explicitly (e.g.
private static HashSet<string> MergeWatchers(IEnumerable<string> keyWatchers,
IEnumerable<string> keys) => new HashSet<string>((keyWatchers ??
Enumerable.Empty<string>()).Concat(keys));) and update all call sites that
currently call MergeWatchers(keys) to pass KeyWatchers (or KeyWatchers ??
Enumerable.Empty<string()) as the first argument so callers become
MergeWatchers(KeyWatchers, keys). Ensure you reference the existing
MergeWatchers and KeyWatchers symbols when making these edits.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Parse.Tests/RoleTests.csParse/Platform/LiveQueries/ParseLiveQuery.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Parse.Tests/RoleTests.cs
|
I notice that you resolved review feedback without comment. Could you please review these. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Parse.Tests/ClientTests.cs (1)
58-70: Consider asserting theTestproperty is preserved.The test verifies the
ServerURIis correctly assigned whenTest = true, but doesn't verify theTestproperty itself is stored and accessible. Adding this assertion would make the test more complete.🔧 Suggested enhancement
ParseClient client = new(data); Assert.AreEqual("https://parse.example.com/", client.ServerConnectionData.ServerURI); + Assert.IsTrue(client.ServerConnectionData.Test); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/ClientTests.cs` around lines 58 - 70, The test TestConstructorWithTestTrue should also assert that the Test flag is preserved: after constructing ParseClient with the ServerConnectionData (class ServerConnectionData) where Test = true, add an assertion that client.ServerConnectionData.Test is true (e.g., use Assert.IsTrue or Assert.AreEqual(true, ...)) so the Test property is validated in the ParseClient constructor behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Parse.Tests/ClientTests.cs`:
- Around line 58-70: The test TestConstructorWithTestTrue should also assert
that the Test flag is preserved: after constructing ParseClient with the
ServerConnectionData (class ServerConnectionData) where Test = true, add an
assertion that client.ServerConnectionData.Test is true (e.g., use Assert.IsTrue
or Assert.AreEqual(true, ...)) so the Test property is validated in the
ParseClient constructor behavior.
|
@mtrezza I think the latest changes cover a lot of new code untested until now. I hope it will now pass the coverage check. |
|
Let's see... |
|
Oh my! It seems that I have to write tests from some old classes just because I added a few fields and methods. 😭 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
Parse.Tests/TextWebSocketClientTests.cs (1)
56-86:⚠️ Potential issue | 🟠 MajorAvoid hardcoded public echo servers in tests; make URL configurable and skip when unset.
This is still brittle for CI and uses a non-reserved domain in
Parse.Tests.Based on learnings: In Parse.Tests, only use RFC 2606/6761 special-use domains (e.g.,
*.example.org,localhost,invalid) and avoid non-standard/public domains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/TextWebSocketClientTests.cs` around lines 56 - 86, Replace the hardcoded echo URL in the TestSendAndReceive test by reading a configurable test endpoint and skip the test when it's not provided: update TestSendAndReceive to obtain the WebSocket URL from a test config or environment variable (e.g., TEST_WEBSOCKET_ECHO_URL), defaulting to null, and if the value is null or empty call Assert.Inconclusive or use TestContext to skip; when provided use that value in TextWebSocketClient.OpenAsync instead of "wss://echo.websocket.org"; ensure the fallback for local CI uses a special-use domain (e.g., wss://echo.example.org or localhost) and keep all existing logic around TextWebSocketClient, OpenAsync, SendAsync, MessageReceived, and CloseAsync unchanged.Parse/Platform/LiveQueries/ParseLiveQueryController.cs (1)
565-575:⚠️ Potential issue | 🟠 MajorCancel
ConnectionSignalinCloseAsyncto unblock pending connect waiters.Only subscription/unsubscription waiters are canceled right now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse/Platform/LiveQueries/ParseLiveQueryController.cs` around lines 565 - 575, In CloseAsync add cancellation of any pending connection waiters by calling TrySetCanceled on the ConnectionSignal(s) before clearing them so connect waiters are unblocked; specifically, alongside the existing loops that call TrySetCanceled on SubscriptionSignals and UnsubscriptionSignals (and their subsequent Clear calls), call TrySetCanceled on the ConnectionSignal (or iterate ConnectionSignals.Values if it is a collection) and then clear the connection signal(s) to mirror the existing teardown for subscriptions and unsubscriptions in ParseLiveQueryController.CloseAsync.Parse.Tests/LiveQuerySubscriptionTests.cs (2)
92-106:⚠️ Potential issue | 🟡 MinorAssert event handlers actually fired in Update/Enter/Leave/Delete tests.
Without an invocation flag assertion, these tests can pass even if event dispatch is broken.
Also applies to: 128-142, 163-177, 196-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/LiveQuerySubscriptionTests.cs` around lines 92 - 106, The tests attach handlers (e.g., subscription.Update) then call the direct invoker (subscription.OnUpdate) but never assert the handler actually ran; add a local boolean flag (e.g., bool invoked = false) before wiring the handler, set invoked = true inside the handler body, and after calling subscription.OnUpdate(state, originalState) assert Assert.IsTrue(invoked, "Update handler was not invoked"); apply the same pattern to the other tests that use subscription.Enter/OnEnter, subscription.Leave/OnLeave and subscription.Delete/OnDelete so each test verifies the event handler executed.
123-125:⚠️ Potential issue | 🟡 Minor
stateandoriginalStateare aliased in Enter/Leave tests, weakening before/after checks.Create a separate copied
MutableObjectStateforstatebefore mutating fields.💡 Proposed fix pattern
- MutableObjectState state = originalState; - state["key"] = "after"; + MutableObjectState state = new() + { + ObjectId = originalState.ObjectId, + ClassName = originalState.ClassName, + CreatedAt = originalState.CreatedAt, + ServerData = new Dictionary<string, object>(originalState.ServerData) + }; + state["key"] = "after";Also applies to: 158-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/LiveQuerySubscriptionTests.cs` around lines 123 - 125, In the Enter/Leave tests the code aliases MutableObjectState by assigning "MutableObjectState state = originalState;" and then mutating state, which makes before/after assertions invalid; change this to create an independent copy (e.g., create a new MutableObjectState populated from originalState via a copy constructor, Clone method, or by copying entries) before mutating so state and originalState are distinct; apply the same fix to the other occurrence (the block around lines 158-160) and ensure subsequent assertions compare the unchanged originalState to the modified state.
🧹 Nitpick comments (3)
Parse.Tests/OrchestrationServiceHubTests.cs (2)
587-629: Missing test coverage: accessing properties when Custom or Default is null.This test verifies behavior when
Custom.PropertyandDefault.Propertyreturn null, but doesn't cover the scenario wherehub.Customorhub.Defaultthemselves are null (verified as the initial state inConstructor_ShouldCreateInstanceWithNullDefaults).Looking at
OrchestrationServiceHub, accessinghub.ClonerwhenCustomis null would throwNullReferenceExceptionbecause of the expressionCustom.Cloner ?? Default.Cloner.Consider adding a test to verify the expected behavior:
[TestMethod] [ExpectedException(typeof(NullReferenceException))] public void Property_ShouldThrowWhenCustomIsNull() { OrchestrationServiceHub hub = new OrchestrationServiceHub(); hub.Default = new Mock<IServiceHub>().Object; // Custom is null - accessing Cloner should throw _ = hub.Cloner; }Or if this should be supported, the
OrchestrationServiceHubimplementation needs null-conditional operators (Custom?.Cloner).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/OrchestrationServiceHubTests.cs` around lines 587 - 629, OrchestrationServiceHub currently dereferences Custom and Default directly (e.g., in the Cloner getter using Custom.Cloner ?? Default.Cloner) which can throw when Custom or Default is null; update the property getters in OrchestrationServiceHub to use null-conditional operators and fallbacks (e.g., Custom?.Cloner ?? Default?.Cloner) for each exposed property (Cloner, MetadataController, WebClient, CacheController, ClassController, InstallationController, CommandRunner, WebSocketClient, CloudCodeController, ConfigurationController, FileController, ObjectController, QueryController, LiveQueryController, SessionController, UserController, CurrentUserController, AnalyticsController, InstallationCoder, PushChannelsController, PushController, CurrentInstallationController, ServerConnectionData, LiveQueryMessageParser, LiveQueryMessageBuilder, LiveQueryServerConnectionData, Decoder, InstallationDataFinalizer) so accessing hub.Property when Custom or Default is null does not throw; run tests and remove any tests that expect NullReferenceException or add new tests verifying null-tolerant behavior as needed.
24-30: Placeholder test provides minimal value.This test only verifies the constructor doesn't throw, which is already covered by
Constructor_ShouldCreateInstanceWithNullDefaults. The name suggests it should test composition behavior, but the implementation doesn't match. Consider either removing this test or adding meaningful composition assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/OrchestrationServiceHubTests.cs` around lines 24 - 30, The test Orchestration_ShouldComposeServicesCorrectly currently only asserts the constructor and is redundant; update it to actually verify composition by invoking the orchestration/composition behavior on OrchestrationServiceHub and asserting that expected services are registered or non-null (e.g., call a Compose/Initialize method if present, then assert hub.GetService<ExpectedService>() != null or hub.RegisteredServices contains ExpectedServiceType), or if you prefer to remove redundancy simply delete this test and keep Constructor_ShouldCreateInstanceWithNullDefaults; reference OrchestrationServiceHub, Orchestration_ShouldComposeServicesCorrectly, and any Compose/Initialize/GetService/Register methods or RegisteredServices property to locate and implement the fix.Parse.Tests/ObjectServiceExtensionsTests.cs (1)
182-209: Add failure-path tests for live-query connect/disconnect.These tests cover only success paths. Please add assertions for behavior when
LiveQueryControlleris null and whenConnectAsync/CloseAsyncthrows, so error-path contracts are pinned down.Suggested additional test cases
+ [TestMethod] + public async Task ConnectLiveQueryServerAsync_NullController_Throws() + { + Mock<IServiceHub> mockHub = new Mock<IServiceHub>(); + mockHub.Setup(h => h.LiveQueryController).Returns((IParseLiveQueryController)null); + await Assert.ThrowsExceptionAsync<InvalidOperationException>( + () => mockHub.Object.ConnectLiveQueryServerAsync((s, e) => { })); + } + + [TestMethod] + public async Task DisconnectLiveQueryServerAsync_CloseThrows_Propagates() + { + Mock<IServiceHub> mockHub = new Mock<IServiceHub>(); + Mock<IParseLiveQueryController> mockLq = new Mock<IParseLiveQueryController>(); + mockLq.Setup(l => l.CloseAsync(It.IsAny<CancellationToken>())) + .ThrowsAsync(new Exception("close failed")); + mockHub.Setup(h => h.LiveQueryController).Returns(mockLq.Object); + + await Assert.ThrowsExceptionAsync<Exception>(() => mockHub.Object.DisconnectLiveQueryServerAsync()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Parse.Tests/ObjectServiceExtensionsTests.cs` around lines 182 - 209, Add tests that cover two failure paths: 1) when IServiceHub.LiveQueryController is null: create a Mock<IServiceHub> with LiveQueryController returning null and assert calling ConnectLiveQueryServerAsync and DisconnectLiveQueryServerAsync does not call ConnectAsync/CloseAsync (verify zero invocations) and does not crash; 2) when ConnectAsync/CloseAsync throw: setup mock IParseLiveQueryController to have ConnectAsync/CloseAsync throw (use ThrowsAsync) and add tests asserting the caller's contract (use Assert.ThrowsExceptionAsync to verify the exception is propagated, or Assert.DoesNotThrow equivalent if the intended behavior is to swallow—match existing project behavior) referencing ConnectLiveQueryServerAsync, DisconnectLiveQueryServerAsync, LiveQueryController, ConnectAsync and CloseAsync to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Parse.Tests/ObjectServiceExtensionsTests.cs`:
- Around line 35-49: The test class calls ParseClient.Publicize() twice (once in
SetUp and once in the ObjectServiceExtensionsTests constructor), causing global
client state mutation and potential cross-test bleed; remove the
constructor-side call by deleting the ParseClient.Publicize() invocation in the
ObjectServiceExtensionsTests() constructor (leave the SetUp() Publicize() call
intact so each test run initializes global state only once per test setup).
In `@Parse.Tests/TextWebSocketClientTests.cs`:
- Around line 24-29: The test SendAsync_ThrowsIfNotOpen currently assigns the
Task returned by Assert.ThrowsExceptionAsync<ArgumentNullException> to a
variable without awaiting it, so the exception assertion never runs; fix by
making the test method async (change signature from void to Task) and await
Assert.ThrowsExceptionAsync<ArgumentNullException>(async () => await
client.SendAsync("msg")) (or simply await the returned task) so that the
exception from TextWebSocketClient.SendAsync is actually observed and asserted.
Ensure the method name SendAsync_ThrowsIfNotOpen remains and that the
TextWebSocketClient instance is used as before.
In `@Parse/Platform/LiveQueries/ParseLiveQueryController.cs`:
- Around line 586-634: Dispose() currently sets the disposed flag such that the
async cleanup in DisposeAsync/Dispose(bool) triggers ThrowIfDisposed() inside
CloseAsync() and skips real cleanup; change the disposal flow so the cleanup
runs regardless of the thrown guard: either set disposed = true only after
cleanup or, better, extract the actual cleanup logic from CloseAsync() into a
new private method (e.g., CloseResourcesAsync or CleanupResourcesAsync) that
does WebSocket closing, event unsubscription and signal clearing without calling
ThrowIfDisposed(), then have both CloseAsync() and DisposeAsync() call that new
method; update Dispose(bool) to await or synchronously invoke the cleanup helper
instead of fire-and-forget so resources are reliably released.
- Around line 373-381: The catch blocks catch OperationCanceledException and
always throw TimeoutException; instead detect whether the original caller token
was canceled and only translate to TimeoutException when the timeout source
canceled. In the catch(OperationCanceledException) in ParseLiveQueryController
methods, check the incoming CancellationToken (the method parameter named
cancellationToken) — if cancellationToken.IsCancellationRequested then rethrow
or throw OperationCanceledException to preserve caller cancellation; otherwise
treat it as a timeout and run the cleanup (await CloseAsync(...)) and then throw
the TimeoutException. Ensure any linked CancellationTokenSource used for
CancelAfter remains scoped so you can distinguish the original caller token
versus the timeout.
- Around line 401-407: ConnectAsync currently does a racy check of _state ==
ParseLiveQueryState.Closed and can start multiple concurrent
EstablishConnectionAsync calls; make ConnectAsync single-flight by atomically
transitioning state or serializing entry so only one caller proceeds to call
EstablishConnectionAsync: for example, use a lock or Interlocked.CompareExchange
on _state (checking for ParseLiveQueryState.Closed and setting to a
Connecting/Opening sentinel) so subsequent callers see the new state and await
the in-progress connection; ensure you do not re-register event handlers
(MessageReceived, WebsocketError, UnknownError) or overwrite ConnectionSignal
from concurrent callers—only the winner of the atomic transition registers
handlers and sets ConnectionSignal, and losers should await the existing
connection task or signal.
---
Duplicate comments:
In `@Parse.Tests/LiveQuerySubscriptionTests.cs`:
- Around line 92-106: The tests attach handlers (e.g., subscription.Update) then
call the direct invoker (subscription.OnUpdate) but never assert the handler
actually ran; add a local boolean flag (e.g., bool invoked = false) before
wiring the handler, set invoked = true inside the handler body, and after
calling subscription.OnUpdate(state, originalState) assert
Assert.IsTrue(invoked, "Update handler was not invoked"); apply the same pattern
to the other tests that use subscription.Enter/OnEnter,
subscription.Leave/OnLeave and subscription.Delete/OnDelete so each test
verifies the event handler executed.
- Around line 123-125: In the Enter/Leave tests the code aliases
MutableObjectState by assigning "MutableObjectState state = originalState;" and
then mutating state, which makes before/after assertions invalid; change this to
create an independent copy (e.g., create a new MutableObjectState populated from
originalState via a copy constructor, Clone method, or by copying entries)
before mutating so state and originalState are distinct; apply the same fix to
the other occurrence (the block around lines 158-160) and ensure subsequent
assertions compare the unchanged originalState to the modified state.
In `@Parse.Tests/TextWebSocketClientTests.cs`:
- Around line 56-86: Replace the hardcoded echo URL in the TestSendAndReceive
test by reading a configurable test endpoint and skip the test when it's not
provided: update TestSendAndReceive to obtain the WebSocket URL from a test
config or environment variable (e.g., TEST_WEBSOCKET_ECHO_URL), defaulting to
null, and if the value is null or empty call Assert.Inconclusive or use
TestContext to skip; when provided use that value in
TextWebSocketClient.OpenAsync instead of "wss://echo.websocket.org"; ensure the
fallback for local CI uses a special-use domain (e.g., wss://echo.example.org or
localhost) and keep all existing logic around TextWebSocketClient, OpenAsync,
SendAsync, MessageReceived, and CloseAsync unchanged.
In `@Parse/Platform/LiveQueries/ParseLiveQueryController.cs`:
- Around line 565-575: In CloseAsync add cancellation of any pending connection
waiters by calling TrySetCanceled on the ConnectionSignal(s) before clearing
them so connect waiters are unblocked; specifically, alongside the existing
loops that call TrySetCanceled on SubscriptionSignals and UnsubscriptionSignals
(and their subsequent Clear calls), call TrySetCanceled on the ConnectionSignal
(or iterate ConnectionSignals.Values if it is a collection) and then clear the
connection signal(s) to mirror the existing teardown for subscriptions and
unsubscriptions in ParseLiveQueryController.CloseAsync.
---
Nitpick comments:
In `@Parse.Tests/ObjectServiceExtensionsTests.cs`:
- Around line 182-209: Add tests that cover two failure paths: 1) when
IServiceHub.LiveQueryController is null: create a Mock<IServiceHub> with
LiveQueryController returning null and assert calling
ConnectLiveQueryServerAsync and DisconnectLiveQueryServerAsync does not call
ConnectAsync/CloseAsync (verify zero invocations) and does not crash; 2) when
ConnectAsync/CloseAsync throw: setup mock IParseLiveQueryController to have
ConnectAsync/CloseAsync throw (use ThrowsAsync) and add tests asserting the
caller's contract (use Assert.ThrowsExceptionAsync to verify the exception is
propagated, or Assert.DoesNotThrow equivalent if the intended behavior is to
swallow—match existing project behavior) referencing
ConnectLiveQueryServerAsync, DisconnectLiveQueryServerAsync,
LiveQueryController, ConnectAsync and CloseAsync to locate the code under test.
In `@Parse.Tests/OrchestrationServiceHubTests.cs`:
- Around line 587-629: OrchestrationServiceHub currently dereferences Custom and
Default directly (e.g., in the Cloner getter using Custom.Cloner ??
Default.Cloner) which can throw when Custom or Default is null; update the
property getters in OrchestrationServiceHub to use null-conditional operators
and fallbacks (e.g., Custom?.Cloner ?? Default?.Cloner) for each exposed
property (Cloner, MetadataController, WebClient, CacheController,
ClassController, InstallationController, CommandRunner, WebSocketClient,
CloudCodeController, ConfigurationController, FileController, ObjectController,
QueryController, LiveQueryController, SessionController, UserController,
CurrentUserController, AnalyticsController, InstallationCoder,
PushChannelsController, PushController, CurrentInstallationController,
ServerConnectionData, LiveQueryMessageParser, LiveQueryMessageBuilder,
LiveQueryServerConnectionData, Decoder, InstallationDataFinalizer) so accessing
hub.Property when Custom or Default is null does not throw; run tests and remove
any tests that expect NullReferenceException or add new tests verifying
null-tolerant behavior as needed.
- Around line 24-30: The test Orchestration_ShouldComposeServicesCorrectly
currently only asserts the constructor and is redundant; update it to actually
verify composition by invoking the orchestration/composition behavior on
OrchestrationServiceHub and asserting that expected services are registered or
non-null (e.g., call a Compose/Initialize method if present, then assert
hub.GetService<ExpectedService>() != null or hub.RegisteredServices contains
ExpectedServiceType), or if you prefer to remove redundancy simply delete this
test and keep Constructor_ShouldCreateInstanceWithNullDefaults; reference
OrchestrationServiceHub, Orchestration_ShouldComposeServicesCorrectly, and any
Compose/Initialize/GetService/Register methods or RegisteredServices property to
locate and implement the fix.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Parse.Tests/LiveQueryControllerTests.csParse.Tests/LiveQuerySubscriptionTests.csParse.Tests/LiveQueryTests.csParse.Tests/MutableServiceHubTests.csParse.Tests/ObjectServiceExtensionsTests.csParse.Tests/OrchestrationServiceHubTests.csParse.Tests/TextWebSocketClientTests.csParse/Platform/LiveQueries/ParseLiveQueryController.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Parse.Tests/LiveQueryTests.cs
|
@mtrezza When you're available, please… |
|
Done |
|
Re-running .NET 9.0 -- timed out after 30 mins. |
|
@mtrezza Please, is there something required from me? I can't see it. |
|
@theSlyest how's it going? |
The features and quality are satisfactory; I just need to bring the test coverage rate up to a sufficient level. |
|
@mtrezza I fixed the issue |
|
restarted ci |
|
@mtrezza Now, what am I missing? |
There was a problem hiding this comment.
Pull request overview
Adds first-class Parse LiveQuery (WebSocket) support to the SDK by introducing live query types, wiring them into IServiceHub/ParseClient, and providing helper APIs and tests to exercise the new behavior.
Changes:
- Introduces LiveQuery core types (controller, query wrapper, subscriptions, message builder/parser, event args).
- Extends
ParseClientandIServiceHubto carry LiveQuery connection/configuration and expose LiveQuery services. - Adds a WebSocket client implementation plus broad unit/integration test coverage for LiveQuery and service hub composition.
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Parse/Utilities/ObjectServiceExtensions.cs | Adds connect/disconnect helpers for LiveQuery |
| Parse/Platform/Queries/ParseQuery.cs | Adds GetLive() to create LiveQuery from query |
| Parse/Platform/ParseClient.cs | Adds LiveQuery-enabled constructor and config plumbing |
| Parse/Platform/LiveQueries/ParseLiveQuerySubscription.cs | New subscription type + event dispatch |
| Parse/Platform/LiveQueries/ParseLiveQueryMessageParser.cs | New incoming message parsing helpers |
| Parse/Platform/LiveQueries/ParseLiveQueryMessageBuilder.cs | New outgoing message builder helpers |
| Parse/Platform/LiveQueries/ParseLiveQueryEventArgs.cs | Event args for single-object events |
| Parse/Platform/LiveQueries/ParseLiveQueryErrorEventArgs.cs | Event args for controller errors |
| Parse/Platform/LiveQueries/ParseLiveQueryDualEventArgs.cs | Event args carrying original/current |
| Parse/Platform/LiveQueries/ParseLiveQueryController.cs | Core controller: connect/subscribe/dispatch |
| Parse/Platform/LiveQueries/ParseLiveQuery.cs | LiveQuery wrapper + watch/subscribe |
| Parse/Infrastructure/ServiceHub.cs | Wires LiveQuery services into ServiceHub |
| Parse/Infrastructure/OrchestrationServiceHub.cs | Routes LiveQuery services through orchestration hub |
| Parse/Infrastructure/MutableServiceHub.cs | Adds LiveQuery fields + default initialization |
| Parse/Infrastructure/LiveQueryServerConnectionData.cs | New LiveQuery server config struct |
| Parse/Infrastructure/LateInitializedMutableServiceHub.cs | Late-init support for LiveQuery services |
| Parse/Infrastructure/Execution/TextWebSocketClient.cs | New WebSocket client implementation |
| Parse/Infrastructure/Execution/MessageReceivedEventArgs.cs | New WebSocket message event args |
| Parse/Abstractions/Platform/LiveQueries/IParseLiveQuerySubscription.cs | New LiveQuery subscription interface |
| Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryMessageParser.cs | New parser abstraction |
| Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryMessageBuilder.cs | New builder abstraction |
| Parse/Abstractions/Platform/LiveQueries/IParseLiveQueryController.cs | New controller abstraction |
| Parse/Abstractions/Infrastructure/IServiceHub.cs | Adds LiveQuery-related service properties |
| Parse/Abstractions/Infrastructure/IMutableServiceHub.cs | Adds mutable LiveQuery-related properties |
| Parse/Abstractions/Infrastructure/ILiveQueryServerConnectionData.cs | New LiveQuery connection data abstraction |
| Parse/Abstractions/Infrastructure/Execution/IWebSocketClient.cs | New WebSocket client abstraction |
| Parse/Abstractions/Infrastructure/CustomServiceHub.cs | Exposes LiveQuery services from CustomServiceHub |
| Parse.Tests/UserTests.cs | Adjusts logout test setup behavior |
| Parse.Tests/TextWebSocketClientTests.cs | Adds unit/integration tests for WebSocket client |
| Parse.Tests/SessionTests.cs | Fixes namespace declaration |
| Parse.Tests/ServiceHubTests.cs | Adds ServiceHub tests including LiveQuery services |
| Parse.Tests/RoleTests.cs | Adds role validation tests |
| Parse.Tests/RelationTests.cs | Removes SignUp calls from relation tests |
| Parse.Tests/QueryTests.cs | Adds query parameter builder tests |
| Parse.Tests/Parse.Tests.csproj | Updates test SDK + MSTest package versions |
| Parse.Tests/OrchestrationServiceHubTests.cs | Adds orchestration hub tests incl. LiveQuery |
| Parse.Tests/ObjectServiceExtensionsTests.cs | Adds tests for new LiveQuery extension methods |
| Parse.Tests/ObjectCoderTests.cs | Fixes namespace declaration |
| Parse.Tests/MutableServiceHubTests.cs | Adds MutableServiceHub LiveQuery defaults tests |
| Parse.Tests/MessageReceivedEventArgsTests.cs | Adds tests for new event args type |
| Parse.Tests/LiveQueryTests.cs | Adds tests for ParseLiveQuery + GetLive |
| Parse.Tests/LiveQuerySubscriptionTests.cs | Adds tests for subscription events + controller calls |
| Parse.Tests/LiveQueryMessageParserTests.cs | Adds tests for parser behavior and validation |
| Parse.Tests/LiveQueryMessageBuilderTests.cs | Adds tests for connect/subscribe/update/unsubscribe messages |
| Parse.Tests/LiveQueryEventArgsTests.cs | Adds tests for event args |
| Parse.Tests/LiveQueryErrorEventArgsTests.cs | Adds tests for error event args |
| Parse.Tests/LiveQueryDualEventArgsTests.cs | Adds tests for dual event args |
| Parse.Tests/LiveQueryControllerTests.cs | Adds tests for controller internal message handling |
| Parse.Tests/InfrastructureTests.cs | Adds tests for infrastructure utility wrappers |
| Parse.Tests/EncoderTests.cs | Ensures client is publicized for tests |
| Parse.Tests/DecoderTests.cs | Ensures client is publicized for tests |
| Parse.Tests/ClientTests.cs | Adds tests for new client constructors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static async Task ConnectLiveQueryServerAsync(this IServiceHub serviceHub, EventHandler<ParseLiveQueryErrorEventArgs> onError = null) | ||
| { | ||
| if (onError is not null) | ||
| { | ||
| serviceHub.LiveQueryController.Error += onError; | ||
| } | ||
| await serviceHub.LiveQueryController.ConnectAsync(); | ||
| } |
There was a problem hiding this comment.
These public extension methods call serviceHub.LiveQueryController without validating that (a) serviceHub is non-null and (b) live query support is configured (the ServiceHub implementation can return null controllers when LiveQueryServerConnectionData is unset). As-is, callers can get a NullReferenceException that’s hard to diagnose; consider throwing ArgumentNullException/InvalidOperationException with a clear message when LiveQuery isn’t configured.
| CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| TaskCompletionSource tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); | ||
| signalDictionary.TryAdd(requestId, tcs); | ||
|
|
||
| try | ||
| { | ||
| await SendMessage(message, cancellationToken); | ||
| cts.CancelAfter(Timeout); | ||
| await tcs.Task.WaitAsync(cts.Token); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| if (cts.IsCancellationRequested) | ||
| throw; | ||
|
|
||
| throw new TimeoutException($"Operation timeout for request {requestId}"); | ||
| } |
There was a problem hiding this comment.
Same issue as connection: cts.CancelAfter(Timeout) guarantees cts.IsCancellationRequested is true when WaitAsync(cts.Token) is canceled, so this code will never throw the intended TimeoutException and will instead propagate OperationCanceledException on timeouts. Use the original cancellationToken to detect caller cancellation and treat other cancellations as timeouts.
| public IObjectState GetObjectState(IDictionary<string, object> message) | ||
| { | ||
| IDictionary<string, object> current = GetDictionary(message, "object") | ||
| ?? throw new ArgumentException("Message does not contain a valid object state.", nameof(message)); | ||
|
|
||
| return ParseObjectCoder.Instance.Decode(current, Decoder, ParseClient.Instance.Services); | ||
| } | ||
|
|
||
| public IObjectState GetOriginalState(IDictionary<string, object> message) | ||
| { | ||
| IDictionary<string, object> original = GetDictionary(message, "original") | ||
| ?? throw new ArgumentException("Message does not contain a valid original object state.", nameof(message)); | ||
|
|
||
| return ParseObjectCoder.Instance.Decode(original, Decoder, ParseClient.Instance.Services); | ||
| } |
There was a problem hiding this comment.
GetObjectState/GetOriginalState decode using ParseClient.Instance.Services, which couples parsing to the global client and can decode with the wrong IServiceHub (e.g., multiple ParseClient instances or a non-publicized client). Prefer injecting the IServiceHub (or an IParseObjectCoder configured with the right hub) into the parser and using that instead of the static ParseClient.Instance reference.
| private async Task<IDictionary<string, object>> AppendSessionToken(IDictionary<string, object> message) | ||
| { | ||
| if (message is null) | ||
| throw new ArgumentNullException(nameof(message)); | ||
|
|
||
| string sessionToken = await ParseClient.Instance.Services.GetCurrentSessionToken(); | ||
| if (sessionToken is not null) | ||
| { | ||
| Dictionary<string, object> copy = new Dictionary<string, object>(message) | ||
| { | ||
| { "sessionToken", sessionToken } | ||
| }; | ||
| return copy; | ||
| } | ||
|
|
||
| return message; | ||
| } | ||
|
|
||
| public async Task<IDictionary<string, object>> BuildConnectMessage() | ||
| { | ||
| ILiveQueryServerConnectionData lqData = ParseClient.Instance.Services.LiveQueryServerConnectionData | ||
| ?? throw new InvalidOperationException("LiveQueryServerConnectionData is not configured"); | ||
|
|
||
| return await AppendSessionToken(new Dictionary<string, object> | ||
| { | ||
| { "op", "connect" }, | ||
| { "applicationId", lqData.ApplicationID ?? throw new InvalidOperationException("LiveQueryServerConnectionData is not configured")}, | ||
| { "windowsKey", lqData.Key ?? throw new InvalidOperationException("LiveQueryServerConnectionData is not configured") } | ||
| }); |
There was a problem hiding this comment.
This builder reads session token and LiveQuery connection settings via ParseClient.Instance.Services, which makes message construction dependent on a global client. That can break if the SDK is used with multiple ParseClient instances or without calling Publicize(). Consider passing the relevant IServiceHub/ILiveQueryServerConnectionData into the builder (constructor injection) and using that instead of ParseClient.Instance.
| /// <returns>A new ParseLiveQuery instace configured with this query's parameters.</returns> | ||
| public ParseLiveQuery<T> GetLive() |
There was a problem hiding this comment.
Typo in XML doc: "instace" → "instance".
| internal void OnCreate(IObjectState objectState); | ||
| internal void OnEnter(IObjectState objectState, IObjectState originalState); | ||
| internal void OnUpdate(IObjectState objectState, IObjectState originalState); | ||
| internal void OnLeave(IObjectState objectState, IObjectState originalState); | ||
| internal void OnDelete(IObjectState objectState); |
There was a problem hiding this comment.
internal interface members without a body are not valid in C# (interface members are public by default unless they have a default implementation). These internal void On* declarations will fail to compile. Consider moving these callbacks to a separate internal interface (implemented by the concrete subscription) that the controller holds internally, or make them explicit methods on the concrete type instead of part of the public interface.
| using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
|
|
||
| try | ||
| { | ||
| await SendMessage(message, cancellationToken); | ||
| cts.CancelAfter(Timeout); | ||
| await ConnectionSignal.Task.WaitAsync(cts.Token); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| try | ||
| { | ||
| await CloseAsync(CancellationToken.None); | ||
| } | ||
| catch { } // Ignore cleanup errors | ||
| if (cts.IsCancellationRequested) | ||
| throw; | ||
|
|
||
| throw new TimeoutException("Live query server connection request has reached timeout"); | ||
| } |
There was a problem hiding this comment.
The timeout-handling logic can never throw the documented TimeoutException. cts.CancelAfter(Timeout) will always make cts.IsCancellationRequested true when WaitAsync(cts.Token) throws, so this catch block always rethrows OperationCanceledException. To differentiate user cancellation vs timeout, check cancellationToken.IsCancellationRequested (or use a separate timeout CTS) and only throw TimeoutException when the original token wasn’t canceled.
| ClientWebSocket webSocketToConnect = null; | ||
| lock (connectionLock) | ||
| { | ||
| webSocket ??= new ClientWebSocket(); | ||
| if (webSocket.State != WebSocketState.Open && webSocket.State != WebSocketState.Connecting) | ||
| { | ||
| webSocketToConnect = webSocket; | ||
| } | ||
| } | ||
| listeningCts?.Cancel(); | ||
| listeningCts?.Dispose(); | ||
| listeningCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); | ||
| if (webSocketToConnect is not null) | ||
| { | ||
| await webSocketToConnect.ConnectAsync(new Uri(serverUri), cancellationToken); | ||
| StartListening(listeningCts.Token); | ||
| } |
There was a problem hiding this comment.
ClientWebSocket instances cannot be re-used after a successful ConnectAsync/close cycle. Here webSocket is created once and then re-used whenever State != Open && State != Connecting, which will make reconnect attempts fail (typically with InvalidOperationException). Consider creating a new ClientWebSocket instance whenever connecting (e.g., replace the field when the state is not Open/Connecting, and dispose the old instance on CloseAsync).
| private async Task OpenAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| if (ParseClient.Instance.Services is null) | ||
| { | ||
| throw new InvalidOperationException("ParseClient.Services must be initialized before connecting to the LiveQuery server."); | ||
| } | ||
|
|
||
| if (ParseClient.Instance.Services.LiveQueryServerConnectionData is null) | ||
| { | ||
| throw new InvalidOperationException("ParseClient.Services.LiveQueryServerConnectionData must be initialized before connecting to the LiveQuery server."); | ||
| } | ||
|
|
||
| await WebSocketClient.OpenAsync(ParseClient.Instance.Services.LiveQueryServerConnectionData.ServerURI, cancellationToken); | ||
| } |
There was a problem hiding this comment.
OpenAsync pulls configuration from ParseClient.Instance.Services, which couples the controller to the global client and can open the wrong server URI (or throw) when multiple clients exist or the current client isn’t publicized. Since the controller is created off an IServiceHub, consider injecting the hub (or at least ILiveQueryServerConnectionData) into the controller and using that instead of ParseClient.Instance.
| ILiveQueryServerConnectionData GenerateLiveQueryServerConnectionData() => liveQueryConfiguration switch | ||
| { | ||
| null => throw new ArgumentNullException(nameof(liveQueryConfiguration)), | ||
| LiveQueryServerConnectionData { Test: true, ServerURI: { } } data => data, | ||
| LiveQueryServerConnectionData { Test: true } data => new LiveQueryServerConnectionData | ||
| { | ||
| ApplicationID = data.ApplicationID, | ||
| Headers = data.Headers, | ||
| MasterKey = data.MasterKey, | ||
| Test = data.Test, | ||
| Key = data.Key, | ||
| ServerURI = "wss://api.parse.com/1/" | ||
| }, | ||
| { ServerURI: "wss://api.parse.com/1/" } => throw new InvalidOperationException("Since the official parse server has shut down, you must specify a URI that points to a hosted instance."), | ||
| { ApplicationID: { }, ServerURI: { }, Key: { } } data => data, | ||
| _ => throw new InvalidOperationException("The IServerConnectionData implementation instance provided to the ParseClient constructor must be populated with the information needed to connect to a Parse server instance.") | ||
| }; |
There was a problem hiding this comment.
The fallback error message in GenerateLiveQueryServerConnectionData() refers to IServerConnectionData, but this constructor parameter is ILiveQueryServerConnectionData. Updating the message would make failures easier to diagnose (and avoids implying the wrong type/parameter).
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
New Features
Tests