Added the features for storing drafts #1642
Conversation
…raft's life. 2) Added the feature to set the expiration interval for drafts in appsettings files.
WalkthroughThe pull request introduces enhancements to draft storage functionality across multiple files in the OutOfSchool project. The changes primarily focus on adding time-to-live (TTL) capabilities for draft storage using Redis. A new configuration class Changes
Sequence DiagramsequenceDiagram
participant Client
participant DraftStorageController
participant DraftStorageService
participant CacheService
participant RedisCache
Client->>DraftStorageController: Get Draft TTL
DraftStorageController->>DraftStorageService: GetTimeToLiveAsync(key)
DraftStorageService->>CacheService: GetTimeToLiveAsync(key)
CacheService->>RedisCache: Retrieve TTL
RedisCache-->>CacheService: Return TimeSpan
CacheService-->>DraftStorageService: Return TimeSpan
DraftStorageService-->>DraftStorageController: Return TimeSpan
DraftStorageController-->>Client: Return Draft TTL
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
…ller classes; 2) Added attributes to actions of WorkshopDraftStorageController class; 3) Reordered Using-directives in CacheService class.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
…controller to the inherited WorkshopDraftStorageController class.
- used TimeSpan.Zero as value of slidingExpirationInterval parameter in WriteAsync() method of CacheService class. 2) Added GetRedisConnectionString() method to RedisConfig class. 3) Changed CacheService class: - added private method GetExpirationIntervalOptions(); - used GetExpirationIntervalOptions() method in methods - GetOrAddAsync<T>() and WriteAsync(); - used GetRedisConnectionString() method of RedisConfig class for getting connection string to Redis. 4) Removed SlidingExpirationInterval property from RedisForDraftConfig class. 5) Added [Authorize] attribute to all Actions of DraftStorageController<T> class for proper authorization in inheritance classes. 6) Fixed Startup class: - used GetRedisConnectionString() method of RedisConfig class for getting connection string to Redis. 7) Fixed tests in DraftStorageServiceTests class.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
OutOfSchool/OutOfSchool.Redis/RedisConfig.cs (1)
31-34: Consider masking or encrypting sensitive data.Building and returning a Redis connection string that includes the password plainly may inadvertently lead to logging or exposure of credentials. While it is common to pass this string to the Redis client, please ensure that logs or error messages never leak this string. If possible, consider using more secure mechanisms to handle the password (e.g., environment variables, secret managers).
OutOfSchool/OutOfSchool.BusinessLogic/Services/DraftStorage/DraftStorageService.cs (1)
61-67: Support for absolute expiration only.Here, you set
redisConfig.AbsoluteExpirationRelativeToNowIntervalbut passTimeSpan.Zerofor sliding expiration. Confirm that the business logic requires no sliding expiration. If a sliding expiration is needed, pass a nonzeroTimeSpan. Otherwise, this usage is valid.OutOfSchool/OutOfSchool.WebApi/Controllers/V1/DraftStorageController.cs (1)
8-14: Documentation clarifies security responsibilities.Stating that inherited controllers must handle permissions is crucial to avoid accidental exposure. Make sure all inheriting classes apply
[Authorize]or a custom permission attribute.OutOfSchool/OutOfSchool.WebApi.Tests/Services/DraftStorage/DraftStorageServiceTests.cs (3)
37-40: Consider making the expiration interval configurable.The hardcoded 1-minute interval might make tests brittle. Consider extracting it to a constant or test parameter for better flexibility.
+ private const int DEFAULT_EXPIRATION_MINUTES = 1; redisConfigMock.Setup(c => c.Value).Returns(new RedisForDraftConfig { - AbsoluteExpirationRelativeToNowInterval = TimeSpan.FromMinutes(1) + AbsoluteExpirationRelativeToNowInterval = TimeSpan.FromMinutes(DEFAULT_EXPIRATION_MINUTES) });
133-149: Consider a more descriptive test name.The current test name could be more specific about what it's verifying. Consider renaming to better reflect that it's testing the retrieval of TTL value.
- public async Task GetTimeToLiveAsync_WhenDraftExistsInCache_ShouldRestoreAppropriatedEntity() + public async Task GetTimeToLiveAsync_WhenDraftExistsInCache_ShouldReturnRemainingTimeToLive()
151-166: Consider a more descriptive test name.The current test name could be more specific about the expected null result.
- public async Task GetTimeToLiveAsync_WhenDraftIsAbsentInCache_ShouldRestoreDefaultEntity() + public async Task GetTimeToLiveAsync_WhenDraftIsAbsentInCache_ShouldReturnNull()OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/WorkshopDraftStorageControllerTests.cs (1)
Line range hint
216-238: Extract the magic number.Consider extracting the hardcoded 1-minute value to a constant for better maintainability.
+ private const int DEFAULT_TTL_MINUTES = 1; + [Test] public async Task GetTimeToLiveOfDraft_WhenDraftExistsInCache_ReturnsDraftAtActionResult() { // Arrange - TimeSpan? timeToLive = TimeSpan.FromMinutes(1); + TimeSpan? timeToLive = TimeSpan.FromMinutes(DEFAULT_TTL_MINUTES);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
OutOfSchool/OutOfSchool.BusinessLogic/Services/DraftStorage/DraftStorageService.cs(4 hunks)OutOfSchool/OutOfSchool.BusinessLogic/Services/DraftStorage/IDraftStorageService.cs(1 hunks)OutOfSchool/OutOfSchool.Redis/CacheService.cs(7 hunks)OutOfSchool/OutOfSchool.Redis/IReadWriteCacheService.cs(1 hunks)OutOfSchool/OutOfSchool.Redis/RedisConfig.cs(2 hunks)OutOfSchool/OutOfSchool.Redis/RedisForDraftConfig.cs(1 hunks)OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/WorkshopDraftStorageControllerTests.cs(4 hunks)OutOfSchool/OutOfSchool.WebApi.Tests/Services/DraftStorage/DraftStorageServiceTests.cs(5 hunks)OutOfSchool/OutOfSchool.WebApi/Controllers/V1/DraftStorageController.cs(3 hunks)OutOfSchool/OutOfSchool.WebApi/Controllers/V1/WorkshopDraftStorageController.cs(1 hunks)OutOfSchool/OutOfSchool.WebApi/Startup.cs(2 hunks)OutOfSchool/OutOfSchool.WebApi/appsettings.Development.json(1 hunks)OutOfSchool/OutOfSchool.WebApi/appsettings.Release.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- OutOfSchool/OutOfSchool.BusinessLogic/Services/DraftStorage/IDraftStorageService.cs
- OutOfSchool/OutOfSchool.Redis/RedisForDraftConfig.cs
- OutOfSchool/OutOfSchool.Redis/IReadWriteCacheService.cs
- OutOfSchool/OutOfSchool.WebApi/Startup.cs
- OutOfSchool/OutOfSchool.WebApi/appsettings.Development.json
- OutOfSchool/OutOfSchool.WebApi/appsettings.Release.json
🔇 Additional comments (16)
OutOfSchool/OutOfSchool.WebApi/Controllers/V1/WorkshopDraftStorageController.cs (1)
12-12: Enforce permission checks consistently.Adding
[HasPermission(Permissions.WorkshopAddNew)]ensures that only users with the correct permission can access this controller. Verify that downstream endpoints follow similar permission checks to maintain consistent authorization for all draft-related operations.OutOfSchool/OutOfSchool.Redis/RedisConfig.cs (1)
14-14: No functional impact.The added blank line before the
Passwordproperty does not affect functionality. It is fine to leave it for clarity or remove it to maintain consistency in style.OutOfSchool/OutOfSchool.BusinessLogic/Services/DraftStorage/DraftStorageService.cs (5)
1-2: New imports look correct.The addition of
using Microsoft.Extensions.Options;properly manages injection of configuration objects. There are no apparent issues.
16-16: Clear naming for config usage.Declaring
RedisForDraftConfig redisConfigclarifies that it contains draft-specific Redis settings, aiding maintainability.
23-25: Constructor injection for draft config.Injecting
IOptions<RedisForDraftConfig>is a standard way to manage strongly typed configs in .NET. This aligns well with best practices.
29-29: Assign only once in a safe context.Storing
redisConfig.Valueis correct. Just ensure that no re-initialization is necessary if configuration changes at runtime. If dynamic config reloading is needed, you might rely onIOptionsSnapshot.
90-99: Method to retrieve remaining TTL is well-structured.
GetTimeToLiveAsyncprovides a clean approach to query a draft’s remaining lifetime. Ensure that calling code handles anullresult correctly. If further modifications are needed, consider returning a defaultTimeSpan.Zeroinstead ofnull.OutOfSchool/OutOfSchool.WebApi/Controllers/V1/DraftStorageController.cs (5)
4-4: Added import is valid.
using System.Net.Mime;is relevant for specifying content types, especially helpful when dealing with JSON or other MIME types in responses.
27-46: Conditional response pattern is appropriate.The
RestoreDraftmethod returnsNoContentif there is no draft, orOkwhen a draft is found. This is clear and follows RESTful conventions.
47-66: Time-to-live retrieval integrated cleanly.The
GetTimeToLiveOfDraftmethod’s conditionalNoContentorOkresponse matches the approach used inRestoreDraft. This keeps the controller consistent.
Line range hint
67-94: Explicit authorization for storing drafts.The
[Authorize]attribute ensures that only authenticated users can store drafts. This is consistent with the broader security stance described in the class’s XML commentary.
95-106: Draft removal logic is secure and consistent.The
[Authorize]attribute again protects the endpoint. ReturningNoContentafter removal is standard practice. Everything looks good here.OutOfSchool/OutOfSchool.WebApi.Tests/Services/DraftStorage/DraftStorageServiceTests.cs (1)
88-89: LGTM!Good update to use the configured expiration interval from Redis config.
OutOfSchool/OutOfSchool.Redis/CacheService.cs (1)
247-262: LGTM!Well-structured and clearly documented handling of sliding expiration cases.
OutOfSchool/OutOfSchool.WebApi.Tests/Controllers/WorkshopDraftStorageControllerTests.cs (2)
Line range hint
189-214: LGTM!Good update to return 204 No Content when draft is not found, following REST best practices.
240-259: LGTM!Well-structured test that properly verifies the behavior when no TTL is found.
- code throws exception if redisConfig.AbsoluteExpirationRelativeToNowInterval property is null.
|
DmyMi
deleted the
HlibBondarev/Add_GetTimeToLive_to_DraftStorageService
branch
Summary by CodeRabbit
New Features
Improvements
Configuration