Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
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:
📝 WalkthroughWalkthroughUses explicit SQL aliases for action-log queries, logs-and-clears Redis deserialize errors, adds year-parse fallback, guards shift calendar rendering, minor view tweaks, adds CanUserModifyDepartmentAsync and enforces it across controllers (IDOR mitigations), converts document delete to POST with antiforgery, and adds a document-delete claim helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client as "Client"
participant DeptCtrl as "DepartmentController"
participant AuthSvc as "AuthorizationService"
participant DeptRepo as "DepartmentRepository"
participant DB as "Database"
Client->>DeptCtrl: POST/PUT modify department (with antiforgery, form data)
DeptCtrl->>AuthSvc: CanUserModifyDepartmentAsync(userId, departmentId)
AuthSvc->>DeptRepo: GetDepartmentById(departmentId)
DeptRepo->>DB: SELECT Department WHERE Id=...
DB-->>DeptRepo: DepartmentRecord
DeptRepo-->>AuthSvc: Department object
AuthSvc-->>DeptCtrl: true/false
alt authorized
DeptCtrl->>DeptRepo: Validate ownership / apply server-authoritative fields
DeptRepo->>DB: UPDATE/DELETE...
DB-->>DeptRepo: OK
DeptCtrl-->>Client: 200 OK / redirect
else unauthorized
DeptCtrl-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs`:
- Around line 87-91: The query assigned to SelectPreviousActionLogsByUserQuery
can return any older row because it lacks deterministic ordering but is consumed
with FirstOrDefault(); update the SQL string in PostgreSqlConfiguration.cs
(SelectPreviousActionLogsByUserQuery) to order by a1.ActionLogId descending and
restrict to a single row (Postgres LIMIT 1) so it always returns the immediately
previous action log for the given user and ActionLogId.
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs`:
- Around line 85-89: The SQL in SelectPreviousActionLogsByUserQuery can return
multiple rows and an indeterminate "previous" row; update the query used by
ActionLogsRepository.GetPreviousActionLogAsync to return a single deterministic
row by selecting only the top row and ordering by the ActionLogId descending
(e.g., use SELECT TOP 1 for SQL Server and add ORDER BY a1.[ActionLogId] DESC)
so the repository's FirstOrDefault() always receives the most recent earlier log
for the given %USERID% and %ACTIONLOGID%.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eff148c3-3e47-4589-8127-2bed56aa4477
📒 Files selected for processing (3)
Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtml
Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs
Outdated
Show resolved
Hide resolved
Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs`:
- Around line 39-44: In the catch block that handles deserialization failures,
avoid double-prefixing the key: call Remove with the original cacheKey
(Remove(cacheKey)) instead of Remove(SetCacheKeyForEnv(cacheKey)), because
Remove already calls SetCacheKeyForEnv internally; update the catch block where
deserializeEx is handled (and referenced functions SetCacheKeyForEnv and Remove)
to pass the unmodified cacheKey so the actual stored entry is removed.
- Around line 115-120: In AzureRedisCacheProvider, the catch block is calling
RemoveAsync(SetCacheKeyForEnv(cacheKey)) which double-applies SetCacheKeyForEnv
(RemoveAsync already calls SetCacheKeyForEnv internally); change the call to
pass the original cacheKey (e.g., RemoveAsync(cacheKey)) so RemoveAsync can
apply SetCacheKeyForEnv exactly once and the corrupted cache entry is correctly
removed; keep Logging.LogException(deserializeEx) and null assignment for data
unchanged.
In `@Repositories/Resgrid.Repositories.DataRepository/LogsRepository.cs`:
- Line 204: The code in LogsRepository.cs currently substitutes any malformed
year string with DateTime.UtcNow.Year; change this so only null/whitespace falls
back to the current year and any non-empty, non-numeric year is rejected: update
the dynamicParameters.Add("Year", ...) logic in the method that builds query
parameters (the line with dynamicParameters.Add("Year", int.TryParse(year, out
var parsedYear) ? parsedYear : DateTime.UtcNow.Year)) to check if
string.IsNullOrWhiteSpace(year) then use DateTime.UtcNow.Year, else attempt
int.TryParse and if parsing fails throw an ArgumentException (or propagate a
validation error) so callers (e.g., LogsController/WorkLogsService) can surface
invalid input instead of silently using the current year.
In `@Web/Resgrid.Web/Areas/User/Controllers/ShiftsController.cs`:
- Around line 1089-1092: The endpoint currently returns shift data without
verifying department ownership; after retrieving shift via
_shiftsService.GetShiftByIdAsync(shiftId) add a guard that compares
shift.DepartmentId to the controller's DepartmentId (or equivalent property) and
if they do not match return Json(calendarItems); ensure this check is placed
alongside the existing null/empty Days guard so other departments cannot access
another department's shift data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fee94518-d80b-4630-971e-44607f825b02
📒 Files selected for processing (3)
Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.csRepositories/Resgrid.Repositories.DataRepository/LogsRepository.csWeb/Resgrid.Web/Areas/User/Controllers/ShiftsController.cs
| var dynamicParameters = new DynamicParametersExtension(); | ||
| dynamicParameters.Add("DepartmentId", departmentId); | ||
| dynamicParameters.Add("Year", int.Parse(year)); | ||
| dynamicParameters.Add("Year", int.TryParse(year, out var parsedYear) ? parsedYear : DateTime.UtcNow.Year); |
Contributor
There was a problem hiding this comment.
Don't substitute malformed year values with the current year.
Line 204 turns invalid input into a valid-but-wrong filter. Because Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs passes the query-string value through and Core/Resgrid.Services/WorkLogsService.cs does not validate it, a request like ?year=abcd will now return current-year logs instead of surfacing bad input. That is a correctness regression and will be very hard to spot from the UI.
Prefer failing validation upstream, or at minimum only apply a fallback for null/whitespace and reject malformed values explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Repositories/Resgrid.Repositories.DataRepository/LogsRepository.cs` at line
204, The code in LogsRepository.cs currently substitutes any malformed year
string with DateTime.UtcNow.Year; change this so only null/whitespace falls back
to the current year and any non-empty, non-numeric year is rejected: update the
dynamicParameters.Add("Year", ...) logic in the method that builds query
parameters (the line with dynamicParameters.Add("Year", int.TryParse(year, out
var parsedYear) ? parsedYear : DateTime.UtcNow.Year)) to check if
string.IsNullOrWhiteSpace(year) then use DateTime.UtcNow.Year, else attempt
int.TryParse and if parsing fails throw an ArgumentException (or propagate a
validation error) so callers (e.g., LogsController/WorkLogsService) can surface
invalid input instead of silently using the current year.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs (1)
718-743:⚠️ Potential issue | 🔴 CriticalKeep role updates inside the admin-only branch.
The new hardening protects group/admin flags, but
rolesis still applied for any caller who can reach this action. BecauseSetRolesForUserAsynconly receives the target user and role ids, this path has no actor context, and roles are permission-bearing here (for example,Providers/Resgrid.Providers.Claims/ClaimsLogic.cs:524-601grants document create/delete claims from role membership). A self-edit or group-admin post can still escalate privileges through this block.🛡️ Suggested fix
- if (form.ContainsKey("roles")) + if (callerIsDepartmentAdmin && form.ContainsKey("roles")) { var roles = form["roles"].ToString().Split(char.Parse(",")); if (roles.Any()) await _personnelRolesService.SetRolesForUserAsync(DepartmentId, model.UserId, roles, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs` around lines 718 - 743, The roles update block currently runs for any caller; restrict it to department admins by moving the form.ContainsKey("roles") handling inside the callerIsDepartmentAdmin branch (where group/admin flag changes are applied). Ensure the code that calls _personnelRolesService.SetRolesForUserAsync(DepartmentId, model.UserId, roles, cancellationToken) only executes when callerIsDepartmentAdmin is true (preserve parsing of form["roles"] to string[] and existing use of DepartmentId, model.UserId and cancellationToken). This prevents non-admins from invoking SetRolesForUserAsync and escalating privileges.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (3)
664-669:⚠️ Potential issue | 🟠 MajorDo not rotate department keys from
GETendpoints.These actions still mutate API/RSS keys on
GET, so a cross-site navigation can trigger unintended key rotation under an authenticated session. Convert them toPOSTand require an antiforgery token.🛡️ Suggested fix
-[HttpGet] +[HttpPost] +[ValidateAntiForgeryToken] [Authorize(Policy = ResgridResources.Department_Update)] public async Task<IActionResult> ProvisionApiKey(CancellationToken cancellationToken)Apply the same change to
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.ProvisionApiKeyAsync(CancellationToken)andWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.ProvisionActiveCallRssKey(CancellationToken), then update the callers to submit POSTs.Also applies to: 682-687, 696-701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 664 - 669, The ProvisionApiKey, ProvisionApiKeyAsync and ProvisionActiveCallRssKey controller actions currently rotate keys on an HTTP GET which is unsafe; change each action's routing attribute from [HttpGet] to [HttpPost] and add [ValidateAntiForgeryToken] (keeping existing [Authorize] checks) and then update any client callers/views/forms to submit POST requests (including antiforgery tokens) to these endpoints so key rotation occurs only via explicit POSTs.
1810-1815:⚠️ Potential issue | 🔴 CriticalUse the delete-specific authorization check here.
Core/Resgrid.Services.AuthorizationService.CanUserDeleteDepartmentAsync(string userId, int departmentId)already limits department deletion toManagingUserId. Replacing it withCore/Resgrid.Model.Services.IAuthorizationService.CanUserModifyDepartmentAsync(string userId, int departmentId)broadens deletion to any department admin, and this POST still has no antiforgery validation.🛡️ Suggested fix
[HttpPost] +[ValidateAntiForgeryToken] [Authorize(Policy = ResgridResources.Department_Update)] public async Task<IActionResult> DeleteDepartment(DeleteDepartmentView model, CancellationToken cancellationToken) { - if (!await _authorizationService.CanUserModifyDepartmentAsync(UserId, DepartmentId)) + if (!await _authorizationService.CanUserDeleteDepartmentAsync(UserId, DepartmentId)) return Unauthorized();The corresponding confirmation form should post an antiforgery token as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 1810 - 1815, The DeleteDepartment POST action uses CanUserModifyDepartmentAsync but should call CanUserDeleteDepartmentAsync to enforce deletion-only rights; update the authorization check in the DeleteDepartment method to await _authorizationService.CanUserDeleteDepartmentAsync(UserId, DepartmentId) instead of CanUserModifyDepartmentAsync, and add antiforgery protection by annotating the action with [ValidateAntiForgeryToken] (and ensure the confirmation form in the DeleteDepartment view includes `@Html.AntiForgeryToken`() or equivalent) so the POST requires a valid antiforgery token.
865-870:⚠️ Potential issue | 🟠 MajorThese configuration POSTs still lack request-forgery protection.
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.CallSettings(CallSettingsView, CancellationToken)andWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.SubmitSetupWizard(SetupWizardFormPayload, CancellationToken)both mutate department configuration, but neither validates an antiforgery token. The new authorization check does not protect against a forged browser request.🛡️ Suggested fix
[HttpPost] +[ValidateAntiForgeryToken] [Authorize(Policy = ResgridResources.Department_Update)] public async Task<IActionResult> CallSettings(CallSettingsView model, CancellationToken cancellationToken) [HttpPost] +[ValidateAntiForgeryToken] [Authorize(Policy = ResgridResources.Department_Update)] public async Task<IActionResult> SubmitSetupWizard([FromBody] SetupWizardFormPayload payload, CancellationToken cancellationToken)For the JSON setup-wizard POST, send the request token in a header from the client.
Also applies to: 1536-1541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 865 - 870, Both CallSettings(CallSettingsView, CancellationToken) and SubmitSetupWizard(SetupWizardFormPayload, CancellationToken) mutate department configuration but do not validate an antiforgery token; add antiforgery validation to these POST endpoints by invoking the IAntiforgery/ValidateRequestAsync flow (or decorating with [ValidateAntiForgeryToken] where applicable) so requests are checked for a valid token. For the JSON-based SubmitSetupWizard endpoint, accept the antiforgery token from a request header (e.g., X-CSRF-TOKEN) on the server side and validate it before performing any updates; ensure the client sends the token in that header when posting JSON. Ensure the checks occur before calling _authorizationService.CanUserModifyDepartmentAsync or performing any state changes in these methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web/Areas/User/Views/Documents/Index.cshtml`:
- Around line 79-80: The inline onsubmit handler is vulnerable because it
injects doc.Name directly into JavaScript; update the Documents view to
JavaScript-encode the document name before embedding it (or remove the dynamic
name from the inline handler and show a static confirmation text or use a
separate data-attribute + event handler). Locate the form with the onsubmit
attribute that references `@doc.Name` in Index.cshtml and replace the direct
interpolation with a safely encoded value (e.g., use a server-side JavaScript
string encoder to produce a safe literal) or move the confirmation logic into a
script that reads a data-name attribute and encodes/escapes it before using
confirm().
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 664-669: The ProvisionApiKey, ProvisionApiKeyAsync and
ProvisionActiveCallRssKey controller actions currently rotate keys on an HTTP
GET which is unsafe; change each action's routing attribute from [HttpGet] to
[HttpPost] and add [ValidateAntiForgeryToken] (keeping existing [Authorize]
checks) and then update any client callers/views/forms to submit POST requests
(including antiforgery tokens) to these endpoints so key rotation occurs only
via explicit POSTs.
- Around line 1810-1815: The DeleteDepartment POST action uses
CanUserModifyDepartmentAsync but should call CanUserDeleteDepartmentAsync to
enforce deletion-only rights; update the authorization check in the
DeleteDepartment method to await
_authorizationService.CanUserDeleteDepartmentAsync(UserId, DepartmentId) instead
of CanUserModifyDepartmentAsync, and add antiforgery protection by annotating
the action with [ValidateAntiForgeryToken] (and ensure the confirmation form in
the DeleteDepartment view includes `@Html.AntiForgeryToken`() or equivalent) so
the POST requires a valid antiforgery token.
- Around line 865-870: Both CallSettings(CallSettingsView, CancellationToken)
and SubmitSetupWizard(SetupWizardFormPayload, CancellationToken) mutate
department configuration but do not validate an antiforgery token; add
antiforgery validation to these POST endpoints by invoking the
IAntiforgery/ValidateRequestAsync flow (or decorating with
[ValidateAntiForgeryToken] where applicable) so requests are checked for a valid
token. For the JSON-based SubmitSetupWizard endpoint, accept the antiforgery
token from a request header (e.g., X-CSRF-TOKEN) on the server side and validate
it before performing any updates; ensure the client sends the token in that
header when posting JSON. Ensure the checks occur before calling
_authorizationService.CanUserModifyDepartmentAsync or performing any state
changes in these methods.
In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs`:
- Around line 718-743: The roles update block currently runs for any caller;
restrict it to department admins by moving the form.ContainsKey("roles")
handling inside the callerIsDepartmentAdmin branch (where group/admin flag
changes are applied). Ensure the code that calls
_personnelRolesService.SetRolesForUserAsync(DepartmentId, model.UserId, roles,
cancellationToken) only executes when callerIsDepartmentAdmin is true (preserve
parsing of form["roles"] to string[] and existing use of DepartmentId,
model.UserId and cancellationToken). This prevents non-admins from invoking
SetRolesForUserAsync and escalating privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51d4aa50-d3bf-441e-aa36-bae90814a565
📒 Files selected for processing (7)
Core/Resgrid.Model/Services/IAuthorizationService.csCore/Resgrid.Services/AuthorizationService.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DocumentsController.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Views/Documents/Index.cshtmlWeb/Resgrid.Web/Helpers/ClaimsAuthorizationHelper.cs
Comment on lines
+79
to
+80
| <form asp-controller="Documents" asp-action="DeleteDocument" asp-route-area="User" method="post" | ||
| onsubmit="return confirm('WARNING: This will permanently delete the document \'@doc.Name\'. Are you sure?');" |
Contributor
There was a problem hiding this comment.
JavaScript-encode doc.Name before putting it in onsubmit.
A document name with an apostrophe already breaks this handler, and a crafted name can turn this into stored script execution when an authorized user clicks Delete. Encode for JavaScript or remove the dynamic name from the inline handler.
🛡️ Suggested fix
- onsubmit="return confirm('WARNING: This will permanently delete the document \'@doc.Name\'. Are you sure?');"
+ onsubmit="return confirm('WARNING: This will permanently delete the document \'@System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(doc.Name)\'. Are you sure?');"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <form asp-controller="Documents" asp-action="DeleteDocument" asp-route-area="User" method="post" | |
| onsubmit="return confirm('WARNING: This will permanently delete the document \'@doc.Name\'. Are you sure?');" | |
| <form asp-controller="Documents" asp-action="DeleteDocument" asp-route-area="User" method="post" | |
| onsubmit="return confirm('WARNING: This will permanently delete the document \'@System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(doc.Name)\'. Are you sure?');" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Documents/Index.cshtml` around lines 79 -
80, The inline onsubmit handler is vulnerable because it injects doc.Name
directly into JavaScript; update the Documents view to JavaScript-encode the
document name before embedding it (or remove the dynamic name from the inline
handler and show a static confirmation text or use a separate data-attribute +
event handler). Locate the form with the onsubmit attribute that references
`@doc.Name` in Index.cshtml and replace the direct interpolation with a safely
encoded value (e.g., use a server-side JavaScript string encoder to produce a
safe literal) or move the confirmation logic into a script that reads a
data-name attribute and encodes/escapes it before using confirm().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/WorkflowService.cs`:
- Around line 174-184: The code treats a missing existing step as if it were an
update by fabricating CreatedOn and continuing; instead, when existing (result
of _stepRepository.GetByIdAsync(step.WorkflowStepId)) is null you should
short-circuit and surface a clear not-found/stale-object error (e.g., throw
KeyNotFoundException or return a not-found result) rather than setting
step.CreatedOn or step.CreatedByUserId and proceeding; remove the fallback
assignment for step.CreatedOn in the update path and only apply default
timestamps when creating new records, ensuring the update method fails fast for
missing existing entries.
In `@Repositories/Resgrid.Repositories.DataRepository/WorkflowRepository.cs`:
- Around line 143-147: DeleteWorkflowWithAllDependenciesAsync currently creates
its own connection and transaction via _connectionProvider.Create() and
BeginTransaction, which bypasses any ambient _unitOfWork; change the method to
enlist in the existing unit of work by reusing the connection and transaction
provided by _unitOfWork (e.g., use _unitOfWork.Connection and
_unitOfWork.Transaction or call the unit-of-work's enlist/BeginIfNeeded API)
instead of calling _connectionProvider.Create() and BeginTransaction so the
deletes participate in the caller's transaction scope and do not commit
independently.
- Around line 147-173: Before deleting child rows in the transaction started in
the method that builds dp (DynamicParametersExtension) and runs
deleteRunLogsQuery, deleteRunsQuery, deleteStepsQuery and then executes
deleteWorkflowSql, first lock the parent workflow row by issuing a SELECT for
that workflowId inside the same transaction: for PostgreSQL use "SELECT ... FOR
UPDATE" and for SQL Server use "SELECT ... WITH (UPDLOCK, HOLDLOCK)" (use
DataConfig.DatabaseType and _sqlConfiguration.ParameterNotation to build the
appropriate select and pass the same dp); do this lock-select immediately after
creating dp and before executing the child delete queries so the parent row is
reserved and prevents concurrent inserts, then continue with the existing child
deletes and the final deleteWorkflowSql, and add integration tests that
concurrently attempt WorkflowRun inserts and workflow deletes for both
PostgreSQL and SQL Server to verify no FK violations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5266a8a5-9647-4f17-837f-703734dc6b50
📒 Files selected for processing (3)
Core/Resgrid.Model/Repositories/IWorkflowRepository.csCore/Resgrid.Services/WorkflowService.csRepositories/Resgrid.Repositories.DataRepository/WorkflowRepository.cs
Repositories/Resgrid.Repositories.DataRepository/WorkflowRepository.cs
Outdated
Show resolved
Hide resolved
Repositories/Resgrid.Repositories.DataRepository/WorkflowRepository.cs
Outdated
Show resolved
Hide resolved
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Style
Bug Fixes / Reliability
Security / Improvement
UI
Improvement