Breaking change: Blazor StateManager does not use the provided timeout parameter#3910
Merged
StefanOssendorf merged 15 commits intoMarimerLLC:mainfrom May 9, 2024
Merged
Breaking change: Blazor StateManager does not use the provided timeout parameter#3910StefanOssendorf merged 15 commits intoMarimerLLC:mainfrom
StefanOssendorf merged 15 commits intoMarimerLLC:mainfrom
Conversation
…ion support Updated the `ISessionManager` interface and `SessionManager` class to include a `TimeSpan` parameter in the `RetrieveSession` and `SendSession` methods for timeout specification. Overloads of these methods have been added to accept a `CancellationToken` parameter for operation cancellation. The `StateManager` class's `GetState` and `SaveState` methods have also been updated to accept a `TimeSpan` parameter for timeout. Added a new project `Csla.Blazor.WebAssembly.Tests` for unit tests of the `SessionManager` class, including tests for timeout and cancellation scenarios. The solution file and a new project settings file have been updated accordingly. The `SessionManager` class now handles `OperationCanceledException` by rethrowing the exception.
Refactored `SessionManagerTests.cs` and `SessionManager.cs` to improve session handling. The `SessionMessage` object has been changed from a local variable to a class-level variable, allowing it to be accessed across different methods. The `RetrieveSession` and `SendSession` methods have been updated to return the session and assert that the returned session is equal to `SessionValue.Session`. The exception type expected in these methods with zero timeout and cancelled cancellation token has been changed from `TaskCanceledException` to `TimeoutException`. The `GetCancellationToken` method has been simplified to create a `CancellationTokenSource` with a timeout directly. In `StateManager.cs`, the `GetState` method has been updated to not return a `Session` object, instead, it just retrieves the session without assigning it to a variable.
This commit includes a significant refactoring of the `SessionManagerTests.cs` file. The `GetHttpClient` method has been simplified by removing the creation of a new `MemoryStream` instance and the use of `CslaBinaryWriter`. The stream's position is no longer reset to 0, and the array is directly obtained from the stream. Several test methods have been removed, including `RetrieveSession_WithTimeoutValue_ShouldNotThrowException`, `RetrieveSession_WithValidCancellationToken_ShouldNotThrowException`, and `SendSession_WithZeroTimeout_ShouldThrowTimeoutException`. These tests were checking for specific exceptions or session values. The `SendSession_WithTimeoutValue_ShouldNotThrowException` test method has been modified by removing the assertion that was previously checking if the operation is successful. Lastly, the `RetrieveCachedSessionSession` method has been modified by removing the call to `GetCachedSession` method of `_sessionManager`.
Updated `SessionManager.cs` methods `RetrieveSession` and `SendSession` to handle `TaskCanceledException` internally and rethrow as `TimeoutException`. Simplified `SendSession` by removing exception handling and refactored `RetrieveSession` to move `stateResult` handling outside of try-catch block. Renamed test methods in `SessionManagerTests.cs` to reflect these changes and updated expected exception type.
` ` `The SaveState method in StateManager.cs has been converted from a synchronous method to an asynchronous one. This is indicated by the addition of the async keyword and the change in return type from void to Task. Additionally, the call to _sessionManager.SendSession(timeout) within the SaveState method has been updated to use the await keyword, making this method call awaited, in line with the change to make SaveState an asynchronous method.`
Updated `Grpc.Net.Client` and `Grpc.Tools` packages in `Csla.Channels.Grpc.csproj`. Refactored `InitializeUser` method in `ApplicationContextManagerBlazor.cs` and `ApplicationContextManagerInMemory.cs` to be asynchronous. Removed `System.Transactions` reference from `Csla.Tests.csproj` and deleted `DataPortalTestDatabaseDataSet.Designer.cs` and related files. Added new test classes `HttpProxyExtensionsTests.cs` and `HttpProxyOptionsExtensionsTests.cs`. Removed `packages.config.old` file. Refactored `HttpProxy.cs` and `HttpProxyExtensions.cs` and added new property and methods in `HttpProxyOptions.cs`. Removed "Csla.Generators.CSharp" project from the solution "csla.test.sln" and its dependencies.
Contributor
Author
|
@luizfbicalho agreed - see #3911 |
Contributor
Author
|
@rockfordlhotka can we finish this one first and then we proceed with the other changes? this way I can estimate all the interfaces and inplementations beforehand and create a new milestone? |
Member
|
I think that is a wise course of action, yes. |
StefanOssendorf
requested changes
May 4, 2024
Contributor
StefanOssendorf
left a comment
StefanOssendorf
left a comment
There was a problem hiding this comment.
Thanks for the re-publish of your PR. I found some minor stuff - docs missing - nothing with the code itself. And an important question for rocky.
Source/Csla.Blazor.WebAssembly.Tests/Csla.Blazor.WebAssembly.Tests.csproj
Outdated
Show resolved
Hide resolved
Contributor
|
@luizfbicalho Did you send a contributor agreement to rocky? I'll mark the PR as "missing agreement" until I hear from rocky or he removes the tag. Edit: You are a member of the csla developers so I think the agreement is there. 👍 |
StefanOssendorf
added
pr/NeedContributorAgreement
Contributor
Author
I sent to him, do you want me to send a copy? |
…error handling In this commit, several updates were made to the `SessionManager.cs` and `SessionManagerTests.cs` files. The variable `_sessionManager` was renamed to `_SessionManager` in `SessionManagerTests.cs`. The `Initialize` method was converted to an asynchronous method, and the `RetrieveSession` method call in it was updated to use `await`. XML comments were added to the `RetrieveSession`, `SendSession`, and `GetSession` methods in `SessionManager.cs` for better code documentation. The `RetrieveSession` and `SendSession` methods were updated to handle `TaskCanceledException` and throw a `TimeoutException` with a custom message. The `GetSession` method was updated to handle the case where `_session` is `null`, creating and returning a new `Session` object in this case. The `SendSession` method was updated to serialize the `_session` object and send it to the server if `SyncContextWithServer` is `true`. Finally, the `RetrieveSession` method was updated to retrieve the session from the server if `SyncContextWithServer` is `true`, deserializing and storing the retrieved session in `_session` or calling `GetSession` to get or create a new session if the retrieval is unsuccessful.
Refactored the `Initialize` method in `SessionManagerTests.cs` to be synchronous and removed the `RetrieveSession` call. The `RetrieveSession` call has been added to four test methods to ensure session retrieval before each test. Renamed and converted `RetrieveCAchedSessionSession` to an asynchronous method, adding a `RetrieveSession` call and an assertion for non-null cached sessions. Added a new test method `RetrieveNullCachedSessionSession` to assert null cached sessions.
Contributor
No, all good. I found the place where to look for if an agreement was submitted or not :) |
Updated variable names `_SessionManager` and `SessionValue` to `_sessionManager` and `_sessionValue` respectively, to adhere to the common C# naming convention for private fields. All instances of these variables in the code, including in the `Initialize()`, `Deserialize()`, `RetrieveSession()`, `SendSession()`, and `GetCachedSession()` methods, as well as in test assertions, have been updated accordingly.
This commit represents a significant shift in the mocking framework used for unit testing in the `Csla.Blazor.WebAssembly.Tests.csproj` project. The `Moq` package has been replaced with `NSubstitute` in the project file and throughout the `SessionManagerTests.cs` file. This includes changes in the way mocks are created, set up, and how return values are specified for mocked methods and properties. Additionally, a new `TestHttpMessageHandler` class has been added to `SessionManagerTests.cs` to mock the behavior of an `HttpClient`. The `GetHttpClient` method has been updated to use this new class, aligning with the switch from `Moq` to `NSubstitute`.
luizfbicalho
commented
May 8, 2024
Contributor
Author
luizfbicalho
left a comment
luizfbicalho
left a comment
There was a problem hiding this comment.
all changes made
Contributor
|
Sorry for the delay and thank you for your patience. LGTM :-) |
StefanOssendorf
approved these changes
May 9, 2024
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The previous PR had a merge problem
Closes #3800