fix: openapi documentation #14458
fix: openapi documentation #14458
Conversation
…rn type for `CreateOrOverwriteOptionsList` in `OptionsController`.
Konrad-Simso
added
skip-releasenotes
📝 WalkthroughWalkthroughThe pull request introduces the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 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 (7)
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 (
|
github-actions
bot
added
area/text-editor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (5)
backend/src/Designer/Controllers/ConfigController.cs (1)
Line range hint
63-74: Consider using a strongly-typed model instead of dynamic.Using
dynamicfor the request body bypasses the model validation benefits provided by[ApiController]. Consider creating a dedicated DTO class for the service configuration.+ public class ServiceConfigurationDto + { + public string ServiceDescription { get; set; } + public string ServiceId { get; set; } + public string ServiceName { get; set; } + } - public async Task SetServiceConfig(string org, string app, [FromBody] dynamic serviceConfig) + public async Task<IActionResult> SetServiceConfig(string org, string app, [FromBody] ServiceConfigurationDto serviceConfig) { ServiceConfiguration serviceConfigurationObject = await _applicationMetadataService.GetAppMetadataConfigAsync(org, app); - serviceConfigurationObject.ServiceDescription = serviceConfig.serviceDescription.ToString(); - serviceConfigurationObject.ServiceId = serviceConfig.serviceId.ToString(); - serviceConfigurationObject.ServiceName = serviceConfig.serviceName.ToString(); + serviceConfigurationObject.ServiceDescription = serviceConfig.ServiceDescription; + serviceConfigurationObject.ServiceId = serviceConfig.ServiceId; + serviceConfigurationObject.ServiceName = serviceConfig.ServiceName;backend/src/Designer/Controllers/UserController.cs (1)
Line range hint
91-98: Add XML documentation for HasAccessToCreateRepository endpoint.This endpoint is missing XML documentation which is needed for OpenAPI documentation generation.
+ /// <summary> + /// Checks if the authenticated user has permission to create repositories in the specified organization. + /// </summary> + /// <param name="org">The organization name to check permissions for</param> + /// <returns>User's organization permissions</returns> [HttpGet] [Route("org-permissions/{org}")] public async Task<IActionResult> HasAccessToCreateRepository(string org)backend/src/Designer/Controllers/PolicyController.cs (1)
Line range hint
115-127: Return appropriate status codes for validation errors.The validation endpoints return 200 OK with validation errors in the response body. Consider:
- Return 400 Bad Request for validation errors
- Return 200 OK only when validation passes
- if (vpd.Errors.Count == 0) - { - vpd.Status = 200; - } - return Ok(vpd); + if (vpd.Errors.Count > 0) + { + return BadRequest(vpd); + } + return Ok(vpd);Also applies to: 129-146
backend/src/Designer/Controllers/PreviewController.cs (2)
Line range hint
825-829: Add safer version parsing to prevent potential exceptions.The current version parsing implementation could throw exceptions if the version string contains non-numeric values. Consider using
int.TryParsefor safer parsing.private bool IsValidSemVerVersion(string[] versionParts) { - return versionParts.Length >= 3 && Convert.ToInt32(versionParts[0]) >= 8; + return versionParts.Length >= 3 && + int.TryParse(versionParts[0], out int majorVersion) && + majorVersion >= 8; }
Line range hint
835-838: Add safer preview version parsing to prevent potential exceptions.Similar to the previous issue, use
int.TryParsefor safer parsing of the preview version number.private int GetPreviewVersion(string[] versionParts) { - return Convert.ToInt32(versionParts[3]); + return int.TryParse(versionParts[3], out int previewVersion) + ? previewVersion + : -1; // or another suitable default value }
🧹 Nitpick comments (8)
backend/src/Designer/Controllers/OptionsController.cs (1)
Line range hint
20-23: Consider adding rate limiting for API protection.While the controller has good security measures, consider adding rate limiting to protect against potential abuse, especially for resource-intensive operations like file uploads and list creation.
Consider using a rate limiting middleware or attribute:
[EnableRateLimiting("fixed")] [ApiController] [Authorize] [AutoValidateAntiforgeryToken]backend/src/Designer/Controllers/EnvironmentsController.cs (1)
Line range hint
35-38: Consider wrapping the response in ActionResult for better HTTP semantics.The endpoint should return
ActionResult<List<EnvironmentModel>>instead ofList<EnvironmentModel>to properly handle HTTP status codes and response types.- public async Task<List<EnvironmentModel>> Environments() + public async Task<ActionResult<List<EnvironmentModel>>> Environments() { - return await _environmentsService.GetEnvironments(); + return Ok(await _environmentsService.GetEnvironments()); }backend/src/Designer/Controllers/OrganizationController.cs (1)
Line range hint
35-39: Consider wrapping the response in ActionResult for better HTTP semantics.For consistency with other API endpoints and better HTTP status code handling, consider returning
ActionResult<List<Organization>>.- public async Task<List<Organization>> Organizations() + public async Task<ActionResult<List<Organization>>> Organizations() { List<Organization> orglist = await _giteaApi.GetUserOrganizations(); - return orglist ?? new List<Organization>(); + return Ok(orglist ?? new List<Organization>()); }backend/src/Designer/Controllers/ConfigController.cs (1)
Line range hint
20-20: Consider inheriting from ControllerBase instead of Controller.Since this is an API controller that doesn't need view support, it should inherit from
ControllerBaseinstead ofController.- public class ConfigController : Controller + public class ConfigController : ControllerBasebackend/src/Designer/Controllers/HomeController.cs (2)
Line range hint
19-23: Consider separating MVC and API concerns.The controller mixes MVC view rendering (
View()calls) with API endpoints. Consider splitting this into separate controllers:
HomeControllerfor MVC viewsHomeApiControllerfor API endpoints
20-22: Simplify route attributes to avoid duplication.The controller has multiple overlapping route patterns. Consider consolidating them:
-[Route("[action]/{id?}")] -[Route("[controller]/[action]/{id?}")] +[Route("[controller]")]backend/src/Designer/Controllers/PolicyController.cs (1)
Line range hint
83-89: Consider using appropriate HTTP methods.The endpoints support both PUT and POST for the same operations. According to HTTP semantics:
- Use PUT for updating existing resources
- Use POST for creating new resources
Consider choosing the most appropriate method based on the operation.Also applies to: 98-104
backend/src/Designer/Controllers/TextController.cs (1)
24-24: Consider deferring API enhancements for deprecated controllerWhile adding
[ApiController]is good practice, this controller is marked as obsolete. Consider prioritizing the migration toTextsControllerinstead of enhancing deprecated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/Designer/Controllers/AppDevelopmentController.cs(3 hunks)backend/src/Designer/Controllers/ConfigController.cs(1 hunks)backend/src/Designer/Controllers/DatamodelsController.cs(1 hunks)backend/src/Designer/Controllers/EnvironmentsController.cs(1 hunks)backend/src/Designer/Controllers/FeedbackFormController.cs(0 hunks)backend/src/Designer/Controllers/HomeController.cs(6 hunks)backend/src/Designer/Controllers/ImageController.cs(1 hunks)backend/src/Designer/Controllers/OptionsController.cs(1 hunks)backend/src/Designer/Controllers/OrganizationController.cs(1 hunks)backend/src/Designer/Controllers/PolicyController.cs(1 hunks)backend/src/Designer/Controllers/PreviewController.cs(1 hunks)backend/src/Designer/Controllers/ProcessModelingController.cs(1 hunks)backend/src/Designer/Controllers/RepositoryController.cs(1 hunks)backend/src/Designer/Controllers/TextController.cs(1 hunks)backend/src/Designer/Controllers/UserController.cs(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/Designer/Controllers/FeedbackFormController.cs
🔇 Additional comments (15)
backend/src/Designer/Controllers/OptionsController.cs (2)
Line range hint
20-23: Well-structured API controller setup with comprehensive security measures!The controller is properly configured with:
- [ApiController] for enhanced OpenAPI documentation
- [Authorize] for authentication
- [AutoValidateAntiforgeryToken] for CSRF protection
- Regex-validated route pattern
164-164: Verify impact of return type change on API consumers.The return type has been simplified from
Task<ActionResult<Dictionary<string, List<Option>>>>toTask<ActionResult<List<Option>>>. While this change makes the API more straightforward, it's a breaking change that could affect existing consumers.Let's check for any API consumers that might be affected:
✅ Verification successful
Return type change appears to be safe ✅
The simplified return type aligns with the existing service layer implementation, which already returns
Task<List<Option>>. No direct API consumers were found in the frontend codebase that would be affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of CreateOrOverwriteOptionsList in the codebase rg -l "CreateOrOverwriteOptionsList" --type cs # Search for potential API client implementations rg -l "options.*json" --type ts --type jsLength of output: 701
Script:
#!/bin/bash # Check backend service implementations echo "=== Service Interface ===" rg -A 5 "CreateOrOverwriteOptionsList" backend/src/Designer/Services/Interfaces/IOptionsService.cs echo "=== Service Implementation ===" rg -A 5 "CreateOrOverwriteOptionsList" backend/src/Designer/Services/Implementation/OptionsService.cs echo "=== Frontend API Call ===" rg -A 10 "options.*json" frontend/packages/shared/src/api/mutations.tsLength of output: 3432
backend/src/Designer/Controllers/EnvironmentsController.cs (1)
14-14: LGTM! ApiController attribute added correctly.The addition of
[ApiController]attribute aligns with the PR objectives and enables automatic model validation and OpenAPI documentation generation.backend/src/Designer/Controllers/OrganizationController.cs (1)
13-13: LGTM! ApiController attribute added correctly.The addition of
[ApiController]attribute aligns with the PR objectives and enables automatic model validation and OpenAPI documentation generation.backend/src/Designer/Controllers/ConfigController.cs (1)
16-16: LGTM! ApiController attribute added correctly.The addition of
[ApiController]attribute aligns with the PR objectives and enables automatic model validation and OpenAPI documentation generation.backend/src/Designer/Controllers/UserController.cs (1)
18-18: LGTM! ApiController attribute added correctly.The addition of
[ApiController]attribute aligns with the PR objectives and enables automatic model validation and OpenAPI documentation generation.backend/src/Designer/Controllers/ImageController.cs (1)
22-25: LGTM! Well-structured API controller.The addition of [ApiController] complements the existing well-structured API design, proper route patterns, and robust error handling.
backend/src/Designer/Controllers/ProcessModelingController.cs (1)
25-28: LGTM! Well-implemented API controller.The addition of [ApiController] enhances the already well-implemented controller with proper async/await patterns and cancellation token support.
backend/src/Designer/Controllers/PolicyController.cs (1)
14-16: LGTM! The addition of [ApiController] is appropriate.The attribute addition aligns with the PR objectives.
backend/src/Designer/Controllers/DatamodelsController.cs (1)
27-27: LGTM! Well-structured API controllerThe addition of
[ApiController]enhances the controller's capabilities with automatic model validation and improved request handling. The controller follows RESTful conventions with proper route and verb attributes.backend/src/Designer/Controllers/RepositoryController.cs (1)
30-30: LGTM! Enhanced API validationThe addition of
[ApiController]complements the existing route constraints and enhances request validation.backend/src/Designer/Controllers/AppDevelopmentController.cs (2)
27-27: LGTM! Enhanced API validationThe addition of
[ApiController]enables automatic model validation and improved request handling.
562-578: LGTM! Improved response handlingThe changes to
GetAppVersionmethod enhance response handling by:
- Properly defining the route with
[HttpGet]and[Route("app-version")]- Using
ActionResultfor better HTTP response control- Following ASP.NET Core best practices for API responses
backend/src/Designer/Controllers/PreviewController.cs (2)
43-44: LGTM! API attributes are correctly configured.The addition of
[ApiController]and[ApiExplorerSettings(IgnoreApi = true)]attributes aligns perfectly with the PR objectives. The controller is properly configured to be excluded from OpenAPI documentation while still benefiting from the API controller features.
Line range hint
49-54: LGTM! Modern C# constructor syntax.The constructor has been updated to use C# 12's primary constructor syntax, which reduces boilerplate while maintaining the same dependency injection functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
backend/src/Designer/Controllers/OptionsController.cs (1)
Line range hint
246-259: Enhance error handling in DeleteOptionsList method.Consider returning a 404 Not Found status when the options list doesn't exist, instead of proceeding with a 200 OK status. Also, consider using a proper response object instead of a string message.
Here's a suggested improvement:
- bool optionsListExists = await _optionsService.OptionsListExists(org, repo, developer, optionsListId, cancellationToken); - if (optionsListExists) - { - _optionsService.DeleteOptionsList(org, repo, developer, optionsListId); - } - - return Ok($"The options file {optionsListId}.json has been deleted."); + if (!await _optionsService.OptionsListExists(org, repo, developer, optionsListId, cancellationToken)) + { + return NotFound($"Options list '{optionsListId}' not found."); + } + + _optionsService.DeleteOptionsList(org, repo, developer, optionsListId); + return NoContent();backend/src/Designer/Controllers/ConfigController.cs (1)
Line range hint
16-20: Change base class to ControllerBase for API controllers.Since this is an API controller (as indicated by [ApiController]), it should inherit from ControllerBase instead of Controller. The Controller base class includes view-related functionality not needed in API controllers.
- public class ConfigController : Controller + public class ConfigController : ControllerBase
🧹 Nitpick comments (8)
backend/src/Designer/Controllers/OptionsController.cs (2)
Line range hint
21-22: Consider adding repository-level access control.While authentication and CSRF protection are properly implemented, consider adding explicit authorization checks to verify if the authenticated user has access rights to the specified organization and repository.
This could be implemented as a custom authorization filter or policy:
[Authorize] [AutoValidateAntiforgeryToken] [RepoAccessAuthorization] // New attribute to check org/repo access [Route("designer/api/{org}/{repo:regex(^(?!datamodels$)[[a-z]][[a-z0-9-]]{{1,28}}[[a-z0-9]]$)}/options")] public class OptionsController : ControllerBase
Line range hint
23-23: Consider extracting constants for better maintainability.The regex pattern in the route and the ".json" file extension are hardcoded in multiple places. Consider extracting these as constants for better maintainability.
Example implementation:
private const string RepoNamePattern = @"^(?!datamodels$)[[a-z]][[a-z0-9-]]{{1,28}}[[a-z0-9]]$"; private const string OptionsFileExtension = ".json"; [Route("designer/api/{org}/{repo:regex(" + RepoNamePattern + ")}/options")] public class OptionsController : ControllerBase { // ... public async Task<IActionResult> UploadFile(...) { string fileName = file.FileName.Replace(OptionsFileExtension, ""); // ... } }Also applies to: 223-223
backend/src/Designer/Controllers/EnvironmentsController.cs (1)
14-14: Add class-level [Route] attribute for OpenAPI documentation consistency.While adding [ApiController] is good for OpenAPI documentation, consider adding a class-level [Route] attribute to explicitly define the base route for all actions. This improves API documentation clarity and follows the pattern seen in other controllers.
[ApiController] [Authorize] [AutoValidateAntiforgeryToken] + [Route("designer/api/environments")]backend/src/Designer/Controllers/UserController.cs (1)
Line range hint
92-99: Add XML documentation for consistency.The HasAccessToCreateRepository method is missing XML documentation comments, unlike other methods in this controller.
+ /// <summary> + /// Checks if the authenticated user has permission to create repositories in the specified organization. + /// </summary> + /// <param name="org">The organization name to check permissions for</param> + /// <returns>ActionResult containing the user's organization permissions</returns> [HttpGet] [Route("org-permissions/{org}")] public async Task<IActionResult> HasAccessToCreateRepository(string org)backend/src/Designer/Controllers/HomeController.cs (2)
Line range hint
80-87: Consider adding XML documentation for the Index method.The method lacks XML documentation comments which are useful for OpenAPI documentation generation.
Add documentation like this:
+/// <summary> +/// Handles all non-designer routes and renders the studio root view +/// </summary> +/// <returns>The studio root view</returns> [HttpGet] [Route("/{*AllValues:regex(^(?!designer).*$)}")] public IActionResult Index()
Line range hint
94-101: Consider consolidating route attributes.The method has two route attributes that could be consolidated for better maintainability.
-[Route("/[controller]/[action]")] -[HttpGet] -[Route("/dashboard/{*AllValues}", Name = "DefaultLoggedIn")] +[HttpGet] +[Route("/[controller]/[action]", Name = "DefaultLoggedIn")] +[Route("/dashboard/{*AllValues}")]backend/src/Designer/Controllers/ProcessModelingController.cs (1)
Line range hint
41-49: Add XML documentation for GetProcessDefinition method.Missing XML documentation affects OpenAPI documentation quality.
Add documentation like this:
+/// <summary> +/// Retrieves the process definition for a given organization and repository +/// </summary> +/// <param name="org">The organization identifier</param> +/// <param name="repo">The repository identifier</param> +/// <returns>Process definition as a file stream</returns> [HttpGet("process-definition")] public FileStreamResult GetProcessDefinition(string org, string repo)backend/src/Designer/Controllers/AppDevelopmentController.cs (1)
562-578: Consider improving return type and documentation.
- Use
ActionResult<VersionResponse>as the return type for better type safety and OpenAPI documentation:-public ActionResult GetAppVersion(string org, string app) +public ActionResult<VersionResponse> GetAppVersion(string org, string app)
- Add XML documentation for parameters and return value:
/// <summary> /// Gets the backend and frontend versions of the app. /// </summary> /// <param name="org">Unique identifier of the organisation responsible for the app.</param> /// <param name="app">Application identifier which is unique within an organisation.</param> /// <returns>A <see cref="VersionResponse"/> containing the backend and frontend versions.</returns>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/Designer/Controllers/AppDevelopmentController.cs(3 hunks)backend/src/Designer/Controllers/ConfigController.cs(1 hunks)backend/src/Designer/Controllers/DatamodelsController.cs(1 hunks)backend/src/Designer/Controllers/EnvironmentsController.cs(1 hunks)backend/src/Designer/Controllers/FeedbackFormController.cs(0 hunks)backend/src/Designer/Controllers/HomeController.cs(6 hunks)backend/src/Designer/Controllers/ImageController.cs(1 hunks)backend/src/Designer/Controllers/OptionsController.cs(1 hunks)backend/src/Designer/Controllers/OrganizationController.cs(1 hunks)backend/src/Designer/Controllers/PolicyController.cs(1 hunks)backend/src/Designer/Controllers/PreviewController.cs(1 hunks)backend/src/Designer/Controllers/ProcessModelingController.cs(1 hunks)backend/src/Designer/Controllers/RepositoryController.cs(1 hunks)backend/src/Designer/Controllers/TextController.cs(1 hunks)backend/src/Designer/Controllers/UserController.cs(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/Designer/Controllers/FeedbackFormController.cs
🔇 Additional comments (14)
backend/src/Designer/Controllers/OptionsController.cs (2)
Line range hint
20-23: LGTM! Well-documented API controller setup.The addition of
[ApiController]attribute along with proper route constraints and authorization attributes provides a robust API endpoint configuration. The XML documentation on the controller and its methods will generate comprehensive OpenAPI/Swagger documentation.
164-164: LGTM! Improved response type consistency.The simplified return type
Task<ActionResult<List<Option>>>aligns better with the actual implementation and maintains consistency with other endpoints in the controller that return option lists.backend/src/Designer/Controllers/OrganizationController.cs (1)
13-13: LGTM! Proper API controller setup.The addition of [ApiController] attribute complements the existing setup with proper routing and security attributes.
backend/src/Designer/Controllers/UserController.cs (1)
18-18: LGTM! Proper API controller configuration.The addition of [ApiController] attribute is correct and complements the existing proper setup.
backend/src/Designer/Controllers/HomeController.cs (1)
Line range hint
19-23: LGTM! The ApiController attribute enhances OpenAPI documentation.The addition of
[ApiController]and route attributes is correct and aligns with the PR objectives.backend/src/Designer/Controllers/ImageController.cs (1)
22-25: LGTM! Well-documented controller with proper attributes.The addition of
[ApiController]complements the existing comprehensive XML documentation and security attributes.backend/src/Designer/Controllers/ProcessModelingController.cs (1)
25-28: LGTM! Proper attribute configuration.The addition of
[ApiController]with antiforgery token validation is correct.backend/src/Designer/Controllers/PolicyController.cs (2)
14-16: LGTM! Proper attribute configuration with route constraints.The addition of
[ApiController]with proper route constraints is correct.
Line range hint
82-85: Consider choosing between PUT and POST.The methods support both PUT and POST operations. According to REST principles:
- PUT is for updating an existing resource
- POST is for creating a new resource
Consider choosing one based on the primary operation.Also applies to: 102-105
backend/src/Designer/Controllers/DatamodelsController.cs (1)
27-27: LGTM! Appropriate use of [ApiController].The addition of
[ApiController]enhances the controller's capabilities with automatic model validation, which is particularly beneficial for data model operations.backend/src/Designer/Controllers/RepositoryController.cs (1)
30-30: LGTM! Well-placed [ApiController] attribute.The addition of
[ApiController]enhances the controller's capabilities with automatic model validation, which is valuable for repository operations and file management endpoints.backend/src/Designer/Controllers/PreviewController.cs (1)
43-44: LGTM! Appropriate attribute combination.The combination of attributes is well thought out:
[ApiController]provides consistent API behavior with other controllers[ApiExplorerSettings(IgnoreApi = true)]correctly excludes this preview-only controller from OpenAPI documentationbackend/src/Designer/Controllers/AppDevelopmentController.cs (2)
27-27: LGTM! Addition of[ApiController]enhances API behavior.The addition of
[ApiController]attribute enables automatic model validation, consistent error responses, and better OpenAPI documentation support.
562-564: LGTM! Explicit HTTP method and route definition.The addition of
[HttpGet]and[Route("app-version")]attributes improves OpenAPI documentation by explicitly defining the endpoint.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
backend/src/Designer/Controllers/ConfigController.cs (1)
Line range hint
63-72: Replace dynamic payload with strongly-typed model.Using
dynamictype for the request body reduces the effectiveness of OpenAPI documentation and bypasses compile-time type checking. Consider creating a dedicated DTO class for the service configuration.-public async Task SetServiceConfig(string org, string app, [FromBody] dynamic serviceConfig) +public async Task SetServiceConfig(string org, string app, [FromBody] ServiceConfigurationDto serviceConfig) { ServiceConfiguration serviceConfigurationObject = await _applicationMetadataService.GetAppMetadataConfigAsync(org, app); - serviceConfigurationObject.ServiceDescription = serviceConfig.serviceDescription.ToString(); - serviceConfigurationObject.ServiceId = serviceConfig.serviceId.ToString(); - serviceConfigurationObject.ServiceName = serviceConfig.serviceName.ToString(); + serviceConfigurationObject.ServiceDescription = serviceConfig.ServiceDescription; + serviceConfigurationObject.ServiceId = serviceConfig.ServiceId; + serviceConfigurationObject.ServiceName = serviceConfig.ServiceName;Add this DTO class:
public class ServiceConfigurationDto { public string ServiceDescription { get; set; } public string ServiceId { get; set; } public string ServiceName { get; set; } }
🧹 Nitpick comments (5)
backend/src/Designer/Controllers/EnvironmentsController.cs (1)
14-17: Consider adding a route prefix at the class level.For consistency with other controllers (e.g., OrganizationController), consider adding a route prefix at the class level instead of individual method routes.
[ApiController] [Authorize] [AutoValidateAntiforgeryToken] +[Route("designer/api/environments")] public class EnvironmentsController : ControllerBasebackend/src/Designer/Controllers/UserController.cs (1)
Line range hint
92-93: Add XML documentation for HasAccessToCreateRepository endpoint.For consistency with other endpoints and to improve OpenAPI documentation, add XML documentation for this endpoint.
+/// <summary> +/// Checks if the authenticated user has permission to create repositories in the specified organization. +/// </summary> +/// <param name="org">The organization name to check permissions for</param> +/// <returns>User's organization permissions</returns> [HttpGet] [Route("org-permissions/{org}")] public async Task<IActionResult> HasAccessToCreateRepository(string org)backend/src/Designer/Controllers/HomeController.cs (1)
95-96: Consider adding route order attribute.Multiple routes for the dashboard might cause ambiguity. Consider adding [Route] order to explicitly define precedence.
[HttpGet] +[Route("/dashboard", Order = 1)] [Route("/dashboard/{*AllValues}", Name = "DefaultLoggedIn")]backend/src/Designer/Controllers/PreviewController.cs (2)
Line range hint
391-407: Consider externalizing mock user data.The hardcoded mock user profile could be moved to a configuration file or mock data provider service, making it easier to maintain and modify test data.
- UserProfile userProfile = new() - { - UserId = 1024, - UserName = "previewUser", - PhoneNumber = "12345678", - Email = "[email protected]", - PartyId = PartyId, - Party = new(), - UserType = 0, - ProfileSettingPreference = new() { Language = "nb" } - }; + UserProfile userProfile = _mockDataProvider.GetMockUserProfile(PartyId);
Line range hint
1012-1039: Simplify version parsing logic.The version parsing logic could be simplified using Version.TryParse and a more straightforward comparison approach.
- private string GetMockedAltinnNugetBuildFromVersion(string version) - { - string[] versionParts = version.Split('.'); - if (!IsValidSemVerVersion(versionParts)) - { - return string.Empty; - } - - if (IsPreviewVersion(versionParts) && GetPreviewVersion(versionParts) < MINIMUM_PREVIEW_NUGET_VERSION) - { - return string.Empty; - } - - return MINIMUM_NUGET_VERSION; - } - - private bool IsValidSemVerVersion(string[] versionParts) - { - return versionParts.Length >= 3 && Convert.ToInt32(versionParts[0]) >= 8; - } - - private bool IsPreviewVersion(string[] versionParts) - { - return versionParts[2].Contains("-preview") && versionParts.Length == 4; - } - - private int GetPreviewVersion(string[] versionParts) - { - return Convert.ToInt32(versionParts[3]); - } + private string GetMockedAltinnNugetBuildFromVersion(string version) + { + if (!Version.TryParse(version.Split('-')[0], out Version parsedVersion)) + { + return string.Empty; + } + + if (parsedVersion.Major < 8) + { + return string.Empty; + } + + if (version.Contains("-preview")) + { + int previewVersion = int.Parse(version.Split('-')[1].Replace("preview", "")); + if (previewVersion < MINIMUM_PREVIEW_NUGET_VERSION) + { + return string.Empty; + } + } + + return MINIMUM_NUGET_VERSION; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/Designer/Controllers/AppDevelopmentController.cs(3 hunks)backend/src/Designer/Controllers/ConfigController.cs(1 hunks)backend/src/Designer/Controllers/DatamodelsController.cs(1 hunks)backend/src/Designer/Controllers/EnvironmentsController.cs(1 hunks)backend/src/Designer/Controllers/FeedbackFormController.cs(0 hunks)backend/src/Designer/Controllers/HomeController.cs(6 hunks)backend/src/Designer/Controllers/ImageController.cs(1 hunks)backend/src/Designer/Controllers/OptionsController.cs(1 hunks)backend/src/Designer/Controllers/OrganizationController.cs(1 hunks)backend/src/Designer/Controllers/PolicyController.cs(1 hunks)backend/src/Designer/Controllers/PreviewController.cs(1 hunks)backend/src/Designer/Controllers/ProcessModelingController.cs(1 hunks)backend/src/Designer/Controllers/RepositoryController.cs(1 hunks)backend/src/Designer/Controllers/TextController.cs(1 hunks)backend/src/Designer/Controllers/UserController.cs(1 hunks)
💤 Files with no reviewable changes (1)
- backend/src/Designer/Controllers/FeedbackFormController.cs
🔇 Additional comments (19)
backend/src/Designer/Controllers/OptionsController.cs (2)
Line range hint
20-20: LGTM! Addition of[ApiController]attribute enhances API behavior.The addition of
[ApiController]attribute enables automatic model validation, binding source parameter inference, and aligns with ASP.NET Core best practices.
164-164: Verify impact of simplified response type on API consumers.The change from
Dictionary<string, List<Option>>toList<Option>simplifies the response structure and aligns with other endpoints. However, this could be a breaking change for existing API consumers.Let's verify the usage of this endpoint in the frontend code:
✅ Verification successful
API response type change is safe and aligned with frontend implementation
The frontend code already expects and handles a direct
List<Option>response type. The simplified response structure matches the frontend's implementation inmutations.tsand type definitions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for API calls to CreateOrOverwriteOptionsList rg -l "CreateOrOverwriteOptionsList|optionsListId.*payload" --type ts --type js # Search for any dictionary-style access on the response rg "\.data\[.*\]\.options" --type ts --type jsLength of output: 176
Script:
#!/bin/bash # Check the implementation in mutations.ts cat frontend/packages/shared/src/api/mutations.ts # Search for type definitions related to options rg -A 5 "type.*Option" --type ts # Alternative search for response handling rg -A 3 "CreateOrOverwriteOptionsList.*then|CreateOrOverwriteOptionsList.*response" --type tsLength of output: 66170
backend/src/Designer/Controllers/EnvironmentsController.cs (1)
14-14: LGTM! ApiController attribute addition is correct.The addition of
[ApiController]attribute will enhance OpenAPI documentation generation and enable automatic model validation.backend/src/Designer/Controllers/OrganizationController.cs (1)
13-13: LGTM! ApiController attribute addition is correct.The addition of
[ApiController]attribute is properly placed and complements existing attributes.backend/src/Designer/Controllers/UserController.cs (1)
18-18: LGTM! ApiController attribute addition is correct.The addition of
[ApiController]attribute is properly placed and complements existing attributes.backend/src/Designer/Controllers/HomeController.cs (2)
60-63: LGTM! Route attributes properly define the landing pages.The [HttpGet] attribute and route patterns correctly define the default landing pages for both authenticated and non-authenticated users.
19-22: LGTM! Route patterns need verification.The addition of [ApiController] and route attributes enhances OpenAPI documentation. However, multiple route patterns at the class level need verification to ensure no conflicts.
Run this script to check for potential route conflicts:
✅ Verification successful
Route patterns are well-structured and non-conflicting
The multiple route patterns at class and method levels follow ASP.NET Core conventions and provide flexible routing without conflicts:
- Class-level routes offer standard action-based routing
- Method-level routes either use specific paths or follow the controller/action pattern
- StartPage's multiple routes intentionally provide different access paths
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential route conflicts in HomeController # Look for similar route patterns across all controllers rg -l 'Route\s*\(\s*\[action\]' rg -l 'Route\s*\(\s*\[controller\]/\[action\]'Length of output: 82
Script:
#!/bin/bash # First locate the HomeController file fd HomeController.cs # Once found, let's examine its content and other controllers # to understand route patterns better fd -e cs -x cat {} | grep -B2 -A2 "Route.*action"Length of output: 821
backend/src/Designer/Controllers/ImageController.cs (1)
22-25: LGTM! Comprehensive attribute configuration.The [ApiController] addition with [AutoValidateAntiforgeryToken] and regex-based route constraint provides robust API documentation and security.
backend/src/Designer/Controllers/ProcessModelingController.cs (1)
25-28: LGTM! Consistent attribute configuration.The attributes match the pattern used in ImageController, maintaining consistency across the codebase.
backend/src/Designer/Controllers/PolicyController.cs (1)
14-16: LGTM! Consistent with other controllers.The [ApiController] attribute and route pattern maintain consistency with ImageController and ProcessModelingController.
Let's verify the consistency of the regex pattern across all controllers:
✅ Verification successful
Regex pattern verified across controllers
The route pattern in PolicyController is consistent with the standard pattern used across most controllers in the codebase. The few variations found are intentional for specific routes with different requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of regex patterns in route attributes # Look for variations in the app regex pattern rg 'regex\(.*\[\[a-z\]\]' --type csLength of output: 7938
backend/src/Designer/Controllers/TextController.cs (2)
24-24: LGTM! Addition of[ApiController]enhances API behavior.The addition of
[ApiController]attribute enables automatic model validation and consistent error handling, aligning with ASP.NET Core best practices.
Line range hint
28-28: Consider migrating toTextsController.This controller is marked as obsolete. Plan to migrate clients to use
TextsControllerand eventually remove this controller to reduce technical debt.Let's verify the usage of this deprecated controller:
✅ Verification successful
Confirmed: TextController is marked as obsolete and should be migrated
The controller is explicitly marked as obsolete with the message: "TextController is deprecated, please use TextsController instead. Only in use until new texts format is implemented in apps."
- TextsController provides equivalent functionality with a cleaner API
- A
/convertendpoint is available in TextsController to assist with migration- Currently only test files reference this controller, suggesting limited internal usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to TextController in the codebase rg -l "TextController" --type csLength of output: 527
Script:
#!/bin/bash # Check if TextController is marked as obsolete and compare both controllers rg -A 5 "\[Obsolete" backend/src/Designer/Controllers/TextController.cs echo "=== TextController endpoints ===" ast-grep --pattern 'public $_ $_($$$) {$$$}' backend/src/Designer/Controllers/TextController.cs echo -e "\n=== TextsController endpoints ===" ast-grep --pattern 'public $_ $_($$$) {$$$}' backend/src/Designer/Controllers/TextsController.cs # Look for client usage beyond test files rg "TextController" --type cs -g '!**/test*/**' -g '!**/Test*/**' -g '!**/Controllers/**'Length of output: 896
Script:
#!/bin/bash # Check endpoints with HTTP attributes echo "=== TextController endpoints ===" rg "\[Http.*\]|public.*\(" -A 1 backend/src/Designer/Controllers/TextController.cs echo -e "\n=== TextsController endpoints ===" rg "\[Http.*\]|public.*\(" -A 1 backend/src/Designer/Controllers/TextsController.csLength of output: 2442
backend/src/Designer/Controllers/DatamodelsController.cs (1)
27-27: LGTM! Well-structured controller with proper API attributes.The addition of
[ApiController]attribute enhances the controller's capabilities. The controller follows good practices with proper route constraints, cancellation token support, and comprehensive documentation.backend/src/Designer/Controllers/RepositoryController.cs (1)
30-30: LGTM! Well-structured repository controller.The addition of
[ApiController]attribute enhances the controller's capabilities. The controller follows good practices with proper dependency injection and comprehensive documentation.backend/src/Designer/Controllers/AppDevelopmentController.cs (2)
27-27: LGTM! Addition of[ApiController]enhances API behavior.The addition of
[ApiController]attribute enables automatic model validation and consistent error handling, aligning with ASP.NET Core best practices.
562-578: LGTM! Improved GetAppVersion method with proper API design.The changes enhance the method by:
- Adding explicit HTTP verb and route attributes
- Using ActionResult for better HTTP response handling
backend/src/Designer/Controllers/PreviewController.cs (3)
43-44: LGTM! Attributes align with PR objectives.The added attributes serve two purposes:
[ApiController]enables API behavior conventions[ApiExplorerSettings(IgnoreApi = true)]correctly excludes this preview/development controller from OpenAPI documentation
Line range hint
391-391: TODO comment needs addressing.The comment indicates that actual test users should be used when tenor test users are available.
Would you like me to help create an issue to track this TODO item?
Line range hint
673-686: LGTM! Robust error handling implementation.The error handling across the controller is well-implemented with:
- Proper use of try-catch blocks
- Appropriate HTTP status codes
- Consistent handling of file not found scenarios
- Support for cancellation tokens
Also applies to: 711-724, 749-762, 787-800, 825-838
# Conflicts: # backend/src/Designer/Controllers/AppDevelopmentController.cs
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14458 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1891 1891
Lines 24583 24583
Branches 2823 2823
=======================================
Hits 23517 23517
Misses 805 805
Partials 261 261 ☔ View full report in Codecov by Sentry. |
…Settings(IgnoreApi = true)]`
Description
Updated backend controllers to include them in generation of documentation by OpenApi
Related Issue
Verification
Summary by CodeRabbit
API Improvements
[ApiController]attribute to multiple backend controllers to enhance request handling and validation.AppDevelopmentControllerwith a more flexible version endpoint response.OptionsControllerto simplify options list return type.Controller Enhancements
HomeController.[ApiExplorerSettings(IgnoreApi = true)]toPreviewController.