Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 14 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Postgres-based document storage support for personnel and unit locations alongside existing Mongo repositories, introducing conditional routing in services based on Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Service as UnitsService/<br/>UsersService
participant Config as Config.<br/>DataConfig
participant PgRepo as Postgres<br/>Repository
participant MongoRepo as Mongo<br/>Repository (Lazy)
participant EventAgg as IEventAggregator
Client->>Service: AddUnitLocation/<br/>SavePersonnelLocation
Service->>Config: Check DocDatabaseType
alt DocDatabaseType == Postgres
Service->>PgRepo: location.PgId is blank?
alt PgId blank
PgRepo->>PgRepo: InsertAsync(location)
PgRepo-->>Service: recordId
else PgId exists
PgRepo->>PgRepo: UpdateAsync(location)
PgRepo-->>Service: location with PgId
end
Service->>Service: Set event.RecordId = location.GetId()
else DocDatabaseType != Postgres
Service->>MongoRepo: InsertAsync/ReplaceAsync(location)
MongoRepo-->>Service: result
Service->>Service: Set event.RecordId = location.GetId()
end
Service->>EventAgg: Publish UnitLocationUpdatedEvent/<br/>PersonnelLocationUpdatedEvent
EventAgg-->>Client: Event published
sequenceDiagram
actor Client
participant Service as UnitsService/<br/>UsersService
participant Config as Config.<br/>DataConfig
participant PgRepo as Postgres<br/>Repository
participant MongoRepo as Mongo<br/>Repository (Lazy)
Client->>Service: GetLatestUnitLocations/<br/>GetLatestLocationsForDepartmentPersonnel
Service->>Config: Check DocDatabaseType
alt DocDatabaseType == Postgres
Service->>PgRepo: Query Postgres repository
PgRepo-->>Service: Personnel/Unit locations
else DocDatabaseType != Postgres
Service->>MongoRepo: Query via .Value (lazy init)
MongoRepo-->>Service: Personnel/Unit locations
end
Service-->>Client: Return locations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.cs (1)
58-77:⚠️ Potential issue | 🟡 MinorHeads-up: the
.Any()guard now activates a previously-dead fallback path.Previously,
unitLocationsData != nullwas almost always true (Dapper returns an emptyIEnumerable, not null) so theelsebranch was unreachable. With the new&& unitLocationsData.Any(), a miss onoidnow falls through toWHERE ul.id = {id}. Ifidis the typical Mongo ObjectId / UUID string and theidcolumn isbigint, Postgres will throw aninvalid input syntaxerror rather than gracefully returning null. Worth confirming theidcolumn type and what shapes ofidare passed in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.cs` around lines 58 - 77, GetByIdAsync currently uses raw-interpolated SQL and uses unitLocationsData.Any() which makes the fallback to the second query execute for non-matching oids and causes Postgres to try to parse string ids as bigints; fix by parameterizing both queries and only executing the second "WHERE ul.id = `@id`" query if the incoming id can be safely converted to the id column type (e.g., long) — use long.TryParse(id, out var idVal) and call connection.QueryAsync<UnitsLocation>(sql, new { oid = id }) for the oid lookup and connection.QueryAsync<UnitsLocation>(sql, new { id = idVal }) for the numeric id lookup so you avoid SQL injection and invalid input syntax errors while preserving the original fallback behavior in GetByIdAsync.
🧹 Nitpick comments (3)
Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs (1)
826-832:PaddleProcessingaction and view are now dead/orphaned code.With the Paddle success URLs in
Index.cshtmlandSelectRegistrationPlan.cshtmlupdated to point directly at/User/Subscription/Processing, thisPaddleProcessingaction is no longer reachable from the checkout flow, andViews/Subscription/PaddleProcessing.cshtmlis orphaned (the action renders the"Processing"view instead).A few cleanup options worth considering:
- Delete
PaddleProcessingandViews/Subscription/PaddleProcessing.cshtmlentirely if no in-flight Paddle checkouts or external configurations still reference the old URL.- If you need to keep the old URL alive for backward compatibility, convert it to a thin redirect instead of duplicating the logic:
♻️ Suggested simplification
- public async Task<IActionResult> PaddleProcessing(int planId) - { - ProcessingView model = new ProcessingView(); - model.PlanId = planId; - - return View("Processing", model); - } + public IActionResult PaddleProcessing(int planId) + => RedirectToAction(nameof(Processing), new { planId });Note: The method is declared
asyncbut contains noawait, which will produce a CS1998 compiler warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs` around lines 826 - 832, The PaddleProcessing action (PaddleProcessing) and the view Views/Subscription/PaddleProcessing.cshtml are now orphaned because checkout URLs point to /User/Subscription/Processing; either delete the PaddleProcessing method and the PaddleProcessing.cshtml view if nothing external references the old URL, or convert PaddleProcessing into a thin redirect to the existing Processing endpoint (e.g., return RedirectToAction("Processing", new { planId })) to preserve backward compatibility; also remove the unnecessary async modifier (no await) to eliminate the CS1998 warning when you modify the method.Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.cs (2)
17-32: Recommend marking the fixture[NonParallelizable].
DataConfig.DocDatabaseTypeis a static field. The[SetUp]/[TearDown]save/restore protects you under NUnit's default serial execution, but if any assembly-level or fixture-level[Parallelizable]is introduced (now or later), tests in this fixture can race with each other and with any other fixture that readsDocDatabaseType, producing flaky failures. Adding[NonParallelizable]to the fixture (or to each test) makes the contract explicit.♻️ Proposed change
[TestFixture] +[NonParallelizable] public class DocumentDatabaseProviderSelectionTests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.cs` around lines 17 - 32, The fixture DocumentDatabaseProviderSelectionTests mutates the static DataConfig.DocDatabaseType in SetUp/TearDown which can race under parallel test execution; add the NUnit [NonParallelizable] attribute to the DocumentDatabaseProviderSelectionTests class declaration so the whole fixture runs serially and avoids concurrent access to DataConfig.DocDatabaseType.
59-95: Consider adding coverage for theUpdateAsyncbranch.Both
AddUnitLocationAsync_should_publish_postgres_record_id_*andSavePersonnelLocationAsync_should_publish_postgres_record_id_*only exercise the insert branch (input has noPgId). The update branch (PgIdalready set →UpdateAsyncis called and its return value flows into the published event'sRecordId) is untested, which leaves the newIUnitLocationsDocRepository.UpdateAsync/IPersonnelLocationsDocRepository.UpdateAsyncpaths uncovered.Also applies to: 121-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.cs` around lines 59 - 95, Add tests that exercise the UpdateAsync branch by providing inputs with an existing PgId and mocking the repository UpdateAsync to return a UnitsLocation/PersonnelLocation whose PgId is the value that should be published; for example, in the AddUnitLocationAsync test set location.PgId = "314", Setup unitLocationsDocRepository.UpdateAsync(It.IsAny<UnitsLocation>()) to return a UnitsLocation with PgId = "updated-314", call service.AddUnitLocationAsync, Verify UpdateAsync was called once (instead of InsertAsync) and assert the published UnitLocationUpdatedEvent.RecordId equals the returned PgId; do the analogous change for SavePersonnelLocationAsync to cover IPersonnelLocationsDocRepository.UpdateAsync and the corresponding PersonnelLocationUpdatedEvent.
🤖 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.NoSqlRepository/PersonnelLocationsDocRepository.cs`:
- Line 49: The DISTINCT ON query in PersonnelLocationsDocRepository (used in the
unitLocationsData fetch) and the GetByIdAsync fallback are vulnerable because
they interpolate departmentId/id directly and rely on ORDER BY/.Any() behavior
that can trigger a SQL branch with non-numeric id values; change the raw
interpolated SQL to use parameterized queries (pass departmentId and id as
parameters to QueryAsync) and preserve the DISTINCT ON ... ORDER BY userid,
timestamp DESC ordering, and update GetByIdAsync to validate/normalize the id
type before executing the fallback (or use a parameterized WHERE ul.id = `@id` so
Postgres will error safely), ensuring the .Any() guard logic still prevents
unnecessary fallbacks but no longer injects or mis-types the id/departmentId
values.
- Around line 105-116: The UpdateAsync in PersonnelLocationsDocRepository
currently builds an interpolated SQL string (uses
JsonConvert.SerializeObject(location), location.UserId and location.PgId) which
opens SQL-injection and quote-escaping bugs and silently no-ops when PgId is
blank; change UpdateAsync to use a parameterized Dapper call (pass departmentId,
userId, dataJson and id as parameters rather than string-interpolating),
serialize the location to JSON into a parameter (dont inline the JSON), validate
location.PgId (throw ArgumentException/InvalidOperationException if missing)
instead of returning silently, and apply the same parameterization fixes to
InsertAsync for userid and data fields.
In
`@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.cs`:
- Around line 105-116: UpdateAsync currently builds SQL via string interpolation
which allows SQL injection and will break on single quotes in the serialized
JSON; change UpdateAsync to use Dapper parameterization: pass the serialized
JSON (or better, the object) as a parameter and set the column using a parameter
(e.g., data = `@data`::jsonb) and use a parameter for id (WHERE id = `@id`) instead
of inlining location.PgId and JsonConvert.SerializeObject(location). Also handle
blank/null location.PgId explicitly (either throw ArgumentException or return
null) instead of silently no-op so callers know the update was skipped; apply
the same parameterized pattern to InsertAsync (and ideally other Get* methods)
to conform to the repository Dapper pattern.
---
Outside diff comments:
In
`@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.cs`:
- Around line 58-77: GetByIdAsync currently uses raw-interpolated SQL and uses
unitLocationsData.Any() which makes the fallback to the second query execute for
non-matching oids and causes Postgres to try to parse string ids as bigints; fix
by parameterizing both queries and only executing the second "WHERE ul.id = `@id`"
query if the incoming id can be safely converted to the id column type (e.g.,
long) — use long.TryParse(id, out var idVal) and call
connection.QueryAsync<UnitsLocation>(sql, new { oid = id }) for the oid lookup
and connection.QueryAsync<UnitsLocation>(sql, new { id = idVal }) for the
numeric id lookup so you avoid SQL injection and invalid input syntax errors
while preserving the original fallback behavior in GetByIdAsync.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.cs`:
- Around line 17-32: The fixture DocumentDatabaseProviderSelectionTests mutates
the static DataConfig.DocDatabaseType in SetUp/TearDown which can race under
parallel test execution; add the NUnit [NonParallelizable] attribute to the
DocumentDatabaseProviderSelectionTests class declaration so the whole fixture
runs serially and avoids concurrent access to DataConfig.DocDatabaseType.
- Around line 59-95: Add tests that exercise the UpdateAsync branch by providing
inputs with an existing PgId and mocking the repository UpdateAsync to return a
UnitsLocation/PersonnelLocation whose PgId is the value that should be
published; for example, in the AddUnitLocationAsync test set location.PgId =
"314", Setup unitLocationsDocRepository.UpdateAsync(It.IsAny<UnitsLocation>())
to return a UnitsLocation with PgId = "updated-314", call
service.AddUnitLocationAsync, Verify UpdateAsync was called once (instead of
InsertAsync) and assert the published UnitLocationUpdatedEvent.RecordId equals
the returned PgId; do the analogous change for SavePersonnelLocationAsync to
cover IPersonnelLocationsDocRepository.UpdateAsync and the corresponding
PersonnelLocationUpdatedEvent.
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs`:
- Around line 826-832: The PaddleProcessing action (PaddleProcessing) and the
view Views/Subscription/PaddleProcessing.cshtml are now orphaned because
checkout URLs point to /User/Subscription/Processing; either delete the
PaddleProcessing method and the PaddleProcessing.cshtml view if nothing external
references the old URL, or convert PaddleProcessing into a thin redirect to the
existing Processing endpoint (e.g., return RedirectToAction("Processing", new {
planId })) to preserve backward compatibility; also remove the unnecessary async
modifier (no await) to eliminate the CS1998 warning when you modify the method.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7e85061-48bd-4e66-9c6d-73771fe351a2
📒 Files selected for processing (11)
Core/Resgrid.Model/Repositories/IPersonnelLocationsDocRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsDocRepository.csCore/Resgrid.Services/MappingService.csCore/Resgrid.Services/UnitsService.csCore/Resgrid.Services/UsersService.csRepositories/Resgrid.Repositories.NoSqlRepository/PersonnelLocationsDocRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.csTests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.csWeb/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml
Member
Author
|
Approve |
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
New Features
Bug Fixes
Tests