fix: remove all compiler warnings #43
Conversation
… all related namespaces
WalkthroughThis update migrates the project from "DocumentService" to "OsmoDoc," renaming namespaces, files, and documentation accordingly. It introduces Redis-based JWT token storage and revocation, adds login and token management endpoints, and refactors PDF and Word generation logic for improved async operations and configuration handling. Docker and environment configurations are updated to support Redis integration. Changes
Sequence Diagram(s)User Login and JWT Token Storage with RedissequenceDiagram
participant User
participant LoginController
participant AuthenticationHelper
participant RedisTokenStoreService
participant Redis
User->>LoginController: POST /api/login (email)
LoginController->>AuthenticationHelper: Generate JWT token
AuthenticationHelper-->>LoginController: JWT token
LoginController->>RedisTokenStoreService: StoreTokenAsync(token, email)
RedisTokenStoreService->>Redis: SET valid_token:{token}
Redis-->>RedisTokenStoreService: OK
RedisTokenStoreService-->>LoginController: (done)
LoginController-->>User: BaseResponse(token, status=Success)
JWT Validation on Protected EndpointsequenceDiagram
participant User
participant API
participant RedisTokenStoreService
participant Redis
User->>API: Request with JWT token
API->>RedisTokenStoreService: IsTokenValidAsync(token)
RedisTokenStoreService->>Redis: EXISTS valid_token:{token}
Redis-->>RedisTokenStoreService: true/false
RedisTokenStoreService-->>API: true/false
alt token valid
API-->>User: Proceed (authorized)
else token revoked
API-->>User: 401 Unauthorized
end
PDF Generation (HTML/EJS Template)sequenceDiagram
participant Client
participant PdfController
participant PdfDocumentGenerator
participant OS/wkhtmltopdf
Client->>PdfController: POST /api/pdf (Base64, DocumentData, etc.)
PdfController->>PdfDocumentGenerator: GeneratePdf(templatePath, metaData, ...)
alt EJS template
PdfDocumentGenerator->>OS/wkhtmltopdf: Convert EJS to HTML
OS/wkhtmltopdf-->>PdfDocumentGenerator: HTML file
end
PdfDocumentGenerator->>OS/wkhtmltopdf: Convert HTML to PDF
OS/wkhtmltopdf-->>PdfDocumentGenerator: PDF file
PdfDocumentGenerator-->>PdfController: PDF path
PdfController-->>Client: PDF (as Base64)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…ding unique id in the filename
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
.env.example (1)
3-4: Reorder keys to satisfy dotenv-linter
The linter flags an [UnorderedKey] warning:REDIS_PORTshould alphabetically precedeREDIS_URL.Apply this diff:
-REDIS_URL=redis:6379 -REDIS_PORT=6379 +REDIS_PORT=6379 +REDIS_URL=redis:6379OsmoDoc/Pdf/PdfDocumentGenerator.cs (1)
23-50: Fix method signature order and improve parameter validation.The method signature uses
async staticwhich should bestatic asyncfor consistency with C# conventions. Also, the parameter validation could be more specific.- public async static Task GeneratePdf(string templatePath, List<ContentMetaData> metaDataList, string outputFilePath, bool isEjsTemplate, string? serializedEjsDataJson) + public static async Task GeneratePdf(string templatePath, List<ContentMetaData> metaDataList, string outputFilePath, bool isEjsTemplate, string? serializedEjsDataJson)Also consider using
ArgumentExceptioninstead ofArgumentNullExceptionfor empty/whitespace strings:- if (string.IsNullOrWhiteSpace(templatePath)) - { - throw new ArgumentNullException(nameof(templatePath)); - } + if (string.IsNullOrWhiteSpace(templatePath)) + { + throw new ArgumentException("Template path cannot be null or empty.", nameof(templatePath)); + }OsmoDoc/Word/WordDocumentGenerator.cs (2)
24-24: Consider HttpClient disposal for better resource management.While static HttpClient instances are generally acceptable for static classes, consider implementing IDisposable pattern or using a factory pattern for better resource management in production applications.
This is acceptable for the current implementation but could be improved for better resource management:
- private static readonly HttpClient _httpClient = new HttpClient(); + private static readonly HttpClient _httpClient = new HttpClient() + { + Timeout = TimeSpan.FromSeconds(30) // Add timeout for better reliability + };
301-302: Address the FIXME comment for performance optimization.The TODO comment indicates a known performance issue with the nested loop operation that should be addressed.
The current implementation has O(n²) complexity for image replacement. Consider creating a lookup dictionary for drawings by name to improve performance:
+ // Create a lookup dictionary for better performance + var drawingLookup = drawings + .Where(d => d.Descendants<DocProperties>().FirstOrDefault()?.Name != null) + .ToDictionary( + d => d.Descendants<DocProperties>().First().Name!, + d => d + ); + + foreach (var imagePlaceholder in imagePlaceholders) + { + if (drawingLookup.TryGetValue(imagePlaceholder.Key, out Drawing? drawing)) + { + // Process the specific drawing + } + }Would you like me to generate a complete refactored version of the image replacement logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.env.example(1 hunks)OsmoDoc.API/Controllers/LoginController.cs(1 hunks)OsmoDoc.API/Controllers/PdfController.cs(1 hunks)OsmoDoc.API/Controllers/WordController.cs(1 hunks)OsmoDoc.API/Helpers/CommonMethodsHelper.cs(1 hunks)OsmoDoc.API/Models/BaseResponse.cs(1 hunks)OsmoDoc.API/Models/LoginRequestDTO.cs(1 hunks)OsmoDoc.API/Models/PdfGenerationRequestDTO.cs(1 hunks)OsmoDoc.API/Models/RevokeTokenRequestDTO.cs(1 hunks)OsmoDoc.API/Models/WordGenerationRequestDTO.cs(1 hunks)OsmoDoc.API/OsmoDoc.API.csproj(1 hunks)OsmoDoc.API/Program.cs(1 hunks)OsmoDoc/OsmoDoc.csproj(1 hunks)OsmoDoc/Pdf/Models/ContentMetaData.cs(1 hunks)OsmoDoc/Pdf/PdfDocumentGenerator.cs(1 hunks)OsmoDoc/Services/Interfaces/IRedisTokenStoreService.cs(1 hunks)OsmoDoc/Services/RedisTokenStoreService.cs(1 hunks)OsmoDoc/Word/Models/ContentData.cs(1 hunks)OsmoDoc/Word/Models/DocumentData.cs(1 hunks)OsmoDoc/Word/Models/TableData.cs(1 hunks)OsmoDoc/Word/WordDocumentGenerator.cs(1 hunks)docker-compose.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- OsmoDoc/Pdf/Models/ContentMetaData.cs
- OsmoDoc/Word/Models/DocumentData.cs
- OsmoDoc/OsmoDoc.csproj
- OsmoDoc.API/OsmoDoc.API.csproj
- OsmoDoc.API/Helpers/CommonMethodsHelper.cs
- OsmoDoc/Word/Models/TableData.cs
- OsmoDoc/Services/RedisTokenStoreService.cs
- OsmoDoc.API/Models/LoginRequestDTO.cs
- OsmoDoc.API/Program.cs
- OsmoDoc.API/Models/BaseResponse.cs
- OsmoDoc.API/Models/PdfGenerationRequestDTO.cs
- OsmoDoc.API/Controllers/LoginController.cs
- OsmoDoc.API/Models/WordGenerationRequestDTO.cs
- OsmoDoc/Word/Models/ContentData.cs
- docker-compose.yaml
- OsmoDoc.API/Controllers/PdfController.cs
- OsmoDoc.API/Models/RevokeTokenRequestDTO.cs
- OsmoDoc/Services/Interfaces/IRedisTokenStoreService.cs
- OsmoDoc.API/Controllers/WordController.cs
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 4-4: [UnorderedKey] The REDIS_PORT key should go before the REDIS_URL key
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
.env.example (1)
1-1: Descriptive JWT_KEY placeholder
TheJWT_KEYplaceholder clearly instructs users to replace it with a strong key of at least 32 characters.OsmoDoc/Pdf/PdfDocumentGenerator.cs (5)
84-109: Well-implemented template replacement logic.The method correctly handles file reading, placeholder replacement, and temporary file creation with proper null checking and unique naming using GUIDs.
111-171: Excellent cross-platform implementation with proper async process handling.The method correctly handles platform-specific differences and includes proper validation, async process execution, and cleanup. The use of
ProcessStartInfowith proper redirection and error handling is well-implemented.
173-241: Well-structured EJS to HTML conversion with proper resource management.The method correctly handles temporary file creation, JSON validation, cross-platform command execution, and cleanup. The use of null-coalescing operator for default JSON content is a good practice.
243-270: Clean and focused helper methods.Both
IsValidJSONandEjsToHtmlArgumentsBasedOnOSare well-implemented with proper exception handling and clear platform-specific logic. The JSON validation approach usingJToken.Parseis appropriate.
1-271: Comprehensive and well-structured PDF generation implementation.This implementation successfully addresses the PR objectives by providing proper validation, null checking, and robust error handling. The code demonstrates excellent practices in:
- Asynchronous programming with proper async/await usage
- Cross-platform compatibility
- Resource management with temporary file cleanup
- Parameter validation and input sanitization
- Separation of concerns with focused helper methods
The migration from the old
DocumentServicetoOsmoDocappears to have been executed well with improved async functionality and better configuration handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
OsmoDoc/Word/WordDocumentGenerator.cs (5)
33-48: Add null check for documentData parameter.The method validates
templateFilePathandoutputFilePathbut doesn't validate thedocumentDataparameter before accessing its properties on lines 47-48. This could result in null reference exceptions and compiler warnings.+ if (documentData == null) + { + throw new ArgumentNullException(nameof(documentData)); + } + List<ContentData> contentData = documentData.Placeholders; List<TableData> tablesData = documentData.TablesData;
126-129: Remove unnecessary try-catch block.The try-catch block doesn't add any value as it simply rethrows the exception without additional context or handling.
- catch (Exception) - { - throw; - }
137-144: Fix resource disposal and avoid unnecessary Task.Run.The FileStream is not properly disposed, and using
Task.Runfor synchronous I/O operations is not recommended for ASP.NET applications as it can lead to thread pool starvation.- private async static Task<XWPFDocument> GetXWPFDocument(string docFilePath) - { - return await Task.Run(() => - { - FileStream readStream = File.OpenRead(docFilePath); - return new XWPFDocument(readStream); - }); - } + private static Task<XWPFDocument> GetXWPFDocument(string docFilePath) + { + return Task.FromResult(() => + { + using FileStream readStream = File.OpenRead(docFilePath); + return new XWPFDocument(readStream); + }()); + }
151-166: Avoid unnecessary Task.Run for I/O operations.Using
Task.Runfor file I/O operations is not recommended as it adds unnecessary thread pool overhead. File I/O operations should use native async methods or be synchronous if no async alternative exists.- await Task.Run(() => - { - using (FileStream writeStream = File.Create(filePath)) - { - document.Write(writeStream); - } - }); + using (FileStream writeStream = File.Create(filePath)) + { + document.Write(writeStream); + }
309-322: Unsafe use of null-forgiving operators.The code uses null-forgiving operators (
!) without ensuring the values are actually non-null, which could lead to runtime exceptions.- if (docProperty != null && docProperty.Name != null && imagePlaceholders.ContainsKey(docProperty.Name!)) + if (docProperty?.Name != null && imagePlaceholders.ContainsKey(docProperty.Name)) { List<Blip> drawingBlips = drawing.Descendants<Blip>().ToList(); foreach (Blip blip in drawingBlips) { - if (blip.Embed == null) + if (blip.Embed?.Value == null) { continue; } - OpenXmlPart imagePart = mainDocumentPart!.GetPartById(blip.Embed!); + if (mainDocumentPart == null) continue; + OpenXmlPart imagePart = mainDocumentPart.GetPartById(blip.Embed.Value); - string imagePath = imagePlaceholders[docProperty.Name!]; + string imagePath = imagePlaceholders[docProperty.Name];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
OsmoDoc/Word/WordDocumentGenerator.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
…ve generated token to redis
…t.RequestAborted for automatic request cancellation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
OsmoDoc.API/Controllers/LoginController.cs (2)
42-43: Consider using structured logging.Instead of logging the exception message and stack trace separately, consider using structured logging with the exception object directly for better error tracking.
- this._logger.LogError(ex.Message); - this._logger.LogError(ex.StackTrace); + this._logger.LogError(ex, "Error occurred during login for email: {Email}", loginRequest?.Email);
65-66: Consider using structured logging for revoke method.Apply the same structured logging improvement here as well.
- this._logger.LogError(ex.Message); - this._logger.LogError(ex.StackTrace); + this._logger.LogError(ex, "Error occurred during token revocation");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.env.example(1 hunks)OsmoDoc.API/Controllers/LoginController.cs(1 hunks)OsmoDoc.API/Controllers/PdfController.cs(1 hunks)OsmoDoc.API/Controllers/WordController.cs(1 hunks)OsmoDoc.API/Helpers/CommonMethodsHelper.cs(1 hunks)OsmoDoc.API/Models/BaseResponse.cs(1 hunks)OsmoDoc.API/Models/LoginRequestDTO.cs(1 hunks)OsmoDoc.API/Models/PdfGenerationRequestDTO.cs(1 hunks)OsmoDoc.API/Models/RevokeTokenRequestDTO.cs(1 hunks)OsmoDoc.API/Models/WordGenerationRequestDTO.cs(1 hunks)OsmoDoc.API/OsmoDoc.API.csproj(1 hunks)OsmoDoc.API/Program.cs(1 hunks)OsmoDoc/OsmoDoc.csproj(1 hunks)OsmoDoc/Pdf/Models/ContentMetaData.cs(1 hunks)OsmoDoc/Services/Interfaces/IRedisTokenStoreService.cs(1 hunks)OsmoDoc/Services/RedisTokenStoreService.cs(1 hunks)OsmoDoc/Word/Models/ContentData.cs(1 hunks)OsmoDoc/Word/Models/DocumentData.cs(1 hunks)OsmoDoc/Word/Models/TableData.cs(1 hunks)OsmoDoc/Word/WordDocumentGenerator.cs(1 hunks)docker-compose.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- OsmoDoc.API/OsmoDoc.API.csproj
- OsmoDoc.API/Helpers/CommonMethodsHelper.cs
- OsmoDoc/Word/Models/TableData.cs
- OsmoDoc/Word/Models/DocumentData.cs
- OsmoDoc.API/Models/LoginRequestDTO.cs
- OsmoDoc/Word/Models/ContentData.cs
- OsmoDoc.API/Models/RevokeTokenRequestDTO.cs
- OsmoDoc.API/Models/PdfGenerationRequestDTO.cs
- OsmoDoc/Pdf/Models/ContentMetaData.cs
- OsmoDoc.API/Controllers/PdfController.cs
- OsmoDoc/OsmoDoc.csproj
- OsmoDoc.API/Models/BaseResponse.cs
- docker-compose.yaml
- OsmoDoc/Services/Interfaces/IRedisTokenStoreService.cs
- OsmoDoc/Services/RedisTokenStoreService.cs
- OsmoDoc.API/Controllers/WordController.cs
- OsmoDoc.API/Models/WordGenerationRequestDTO.cs
- OsmoDoc.API/Program.cs
- OsmoDoc/Word/WordDocumentGenerator.cs
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 4-4: [UnorderedKey] The REDIS_PORT key should go before the REDIS_URL key
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
.env.example (2)
1-1: Improved JWT key placeholder.The updated placeholder text clearly communicates the security requirement for a strong key with minimum 32 characters. This is a good improvement for security guidance.
3-4: Redis configuration variables added appropriately.The Redis URL and port variables are correctly configured for the new token storage functionality. The static analysis warning about key ordering is a false positive - environment variable order doesn't affect functionality in .env files.
Task Link
REST-1567
Description
Summary by CodeRabbit
New Features
Improvements
Refactor
Documentation
Chores