Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
|
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:
📝 WalkthroughWalkthroughThis PR adds hardware unit tracking across models, persistence, services, RabbitMQ, HTTP APIs, security middleware, administration UI, and queued location processing. It also adds idempotent location writes, capability-path redaction, credential lifecycle management, retry/dead-letter handling, and broader authorization checks. ChangesUnit tracking platform
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> CreateTracker( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialProvisionData>> CreateCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialProvisionData>> RotateCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingCredentialData>> RevokeCredential( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> DisableTracker( |
| [ProducesResponseType(StatusCodes.Status403Forbidden)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Unit_Update)] | ||
| public async Task<ActionResult<UnitTrackingDeviceData>> RebindTracker( |
| [ProducesResponseType(typeof(UnitTrackingIngressErrorResponse), StatusCodes.Status422UnprocessableEntity)] | ||
| [ProducesResponseType(StatusCodes.Status429TooManyRequests)] | ||
| [ProducesResponseType(StatusCodes.Status503ServiceUnavailable)] | ||
| public async Task<IActionResult> PostPositions(string unitTrackingDeviceId) |
| [ProducesResponseType(typeof(UnitTrackingIngressErrorResponse), StatusCodes.Status422UnprocessableEntity)] | ||
| [ProducesResponseType(StatusCodes.Status429TooManyRequests)] | ||
| [ProducesResponseType(StatusCodes.Status503ServiceUnavailable)] | ||
| public async Task<IActionResult> PostCapability(string capabilityToken) |
| !ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return NotFound(); | ||
|
|
||
| if (model == null || string.IsNullOrWhiteSpace(model.UnitTrackingDeviceId)) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/DepartmentSettingsService.cs (1)
48-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCache invalidated before the write lands — reintroduces stale-cache window.
InvalidateSettingCacheAsyncruns beforeSaveOrUpdateAsyncpersists the change (Line 51 vs. Lines 60/65). A reader hitting the cache miss in that window re-populates it with the pre-update value via the fallback function, and since these settings useLongCacheLength(14 days), the stale value can survive for up to 14 days after the update. Invalidate after the write succeeds instead.🛠️ Proposed fix: invalidate after the write succeeds
public async Task<DepartmentSetting> SaveOrUpdateSettingAsync(int departmentId, string setting, DepartmentSettingTypes type, CancellationToken cancellationToken = default(CancellationToken)) { var savedSetting = await GetSettingByDepartmentIdType(departmentId, type); - await InvalidateSettingCacheAsync(departmentId, type); + DepartmentSetting result; if (savedSetting == null) { DepartmentSetting newSetting = new DepartmentSetting(); newSetting.DepartmentId = departmentId; newSetting.Setting = setting; newSetting.SettingType = (int)type; - return await _departmentSettingsRepository.SaveOrUpdateAsync(newSetting, cancellationToken); + result = await _departmentSettingsRepository.SaveOrUpdateAsync(newSetting, cancellationToken); } else { savedSetting.Setting = setting; - return await _departmentSettingsRepository.SaveOrUpdateAsync(savedSetting, cancellationToken); + result = await _departmentSettingsRepository.SaveOrUpdateAsync(savedSetting, cancellationToken); } - return null; + await InvalidateSettingCacheAsync(departmentId, type); + return result; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DepartmentSettingsService.cs` around lines 48 - 69, Update SaveOrUpdateSettingAsync so InvalidateSettingCacheAsync runs only after SaveOrUpdateAsync completes successfully for both the new-setting and existing-setting branches. Preserve the saved result, invalidate using departmentId and type after persistence, then return the result; remove the current pre-write invalidation.
🧹 Nitpick comments (21)
Web/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
[EnumDataType]over hardcoded[Range(1,4)]for enum-backed field.Hardcoding the valid range ties validation to the current
UnitTrackingAuthModemember count/numbering; it will silently misvalidate if the enum changes.♻️ Suggested fix
- [Range(1, 4)] + [EnumDataType(typeof(UnitTrackingAuthMode))] public int AuthMode { get; set; } = (int)UnitTrackingAuthMode.Bearer;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs` around lines 79 - 80, Replace the hardcoded Range validation on AuthMode with EnumDataType targeting UnitTrackingAuthMode, preserving the existing Bearer default and enum-backed validation behavior.Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml (1)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded adapter key literal in view.
"resgrid-json-v1"is duplicated as a magic string here rather than referenced from a shared constant (e.g., onUnitTrackingCatalogProfileor a dedicated constants class), risking silent drift if the key changes elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml` at line 169, Replace the hardcoded "resgrid-json-v1" comparison in the Details view with the shared constant exposed by UnitTrackingCatalogProfile or the established adapter-key constants class. Preserve the existing case-insensitive comparison and preview condition while ensuring the value comes from the single canonical definition.Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
data-identifier-requiredattribute is seeded but unused client-side.The profile
<option>carriesdata-identifier-required, butupdateProfileHelp()never reads it, so the UI never surfaces the identifier requirement before submit even though the server enforces it (ValidateProfilein the controller). Consider wiring this up to toggle a "required" hint/marker on the Device Identifier field when the selected profile requires one, or remove the unused attribute.Also applies to: 79-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml` around lines 19 - 30, The profile option’s data-identifier-required attribute is unused by updateProfileHelp(), so the UI does not reflect the server-side requirement. Update updateProfileHelp() and the Device Identifier field markup to read the selected option’s flag and show or hide an appropriate required hint/marker; preserve the existing behavior when the profile does not require an identifier.Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs (2)
489-551: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPreview validation duplicates (and may diverge from) the production JSON parser.
ValidatePreviewJsonre-implements structural checks (size/depth limits, duplicate-key handling, positions array shape, per-position field validation) independently of the ingestion-side parser (UnitTrackingJsonPayloadParser, per the PR stack outline). If the two diverge, this preview tool could report success for payloads the real ingestion endpoint would reject, or vice versa, misleading admins testing device setups.Consider extracting/reusing the shared parsing logic (e.g., expose a
TryParse/Validatemethod on the ingress parser) so both paths stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs` around lines 489 - 551, The ValidatePreviewJson method duplicates ingestion validation; expose and reuse a shared TryParse or Validate entry point from UnitTrackingJsonPayloadParser for preview requests. Move or centralize size, depth, duplicate-key, positions-shape, and coordinate checks there, then have ValidatePreviewJson consume its result and preserve the existing error key and position-count behavior.
609-647: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated field-mapping between
BuildDeviceandApplyUpdate.Both methods map the same set of profile/model fields onto a
UnitTrackingDevice; consider a single mapper that both call (withIsEnabledsupplied by the caller) to avoid drift when new fields are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs` around lines 609 - 647, Consolidate the duplicated field assignments in BuildDevice and ApplyUpdate into one shared UnitTrackingDevice mapper, accepting IsEnabled from each caller so creation remains enabled while updates use model.IsEnabled. Have both methods reuse this mapper and preserve all existing model/profile field mappings.Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs (1)
132-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
[Range(1, 4)]duplicates enum knowledge.
UnitTrackingDevicesController.CreateCredentialalready validates withEnum.IsDefinedplus the profile'sSupportedAuthModes. Hardcoding the upper bound means the nextUnitTrackingAuthModevalue is rejected with a 400 that no one will trace back to this attribute.♻️ Rely on the enum-based validation already in the controller
- [Range(1, 4)] public int AuthMode { get; set; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs` around lines 132 - 133, Remove the hardcoded [Range(1, 4)] validation from the AuthMode property in the unit-tracking admin model, leaving validation to UnitTrackingDevicesController.CreateCredential’s Enum.IsDefined and SupportedAuthModes checks so future enum values are accepted when supported.Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs (1)
84-102: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnsupported
Authorizationscheme short-circuits custom-header credentials.If any
Authorizationheader is present with a scheme other than Bearer/Basic (some trackers or intermediary proxies inject one), Line 101 returnsnulland the CustomHeader credential path is never evaluated, so an otherwise valid custom-header device is rejected. Consider falling through to the custom-header lookup instead of returning early.♻️ Fall through to custom-header lookup
if (authorization.StartsWith("Basic ", StringComparison.OrdinalIgnoreCase)) return ParseBasic(authorization.Substring("Basic ".Length).Trim()); - - return null; }Note: with this change the
MaximumAuthorizationHeaderLengthearly return should also fall through rather than reject outright, or be kept as-is if strict rejection of oversized headers is intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs` around lines 84 - 102, Update the Authorization parsing branch in the authentication method so unsupported schemes fall through to the existing custom-header credential lookup instead of returning null. Apply the same fall-through behavior to the MaximumAuthorizationHeaderLength check unless strict rejection of oversized headers is explicitly required; preserve the Bearer, Basic, and empty-token handling.Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs (1)
30-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the repository’s required dependency-resolution pattern.
This new controller constructor injects four services directly. Use
Bootstrapper.GetKernel().Resolve<T>()explicitly instead, per the repository rule.As per coding guidelines, “Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs` around lines 30 - 40, Update the UnitTrackingIngressController constructor to remove the four service parameters and resolve UnitTrackingHttpAuthenticationService, UnitTrackingJsonPayloadParser, UnitTrackingRateLimiter, and IUnitTrackingIngressService through Bootstrapper.GetKernel().Resolve<T>(), assigning each result to the corresponding fields.Source: Coding guidelines
Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs (1)
10-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not expose mutable singleton catalog entries.
UnitTrackingCatalogServicereturns its staticProfilesinstances directly, whileUnitTrackingCatalogProfileis initialized through mutable properties. A consumer can alter shared profile state, including selection/auth configuration, for subsequent requests. Return immutable profiles or defensive copies.As per coding guidelines, “Prefer functional patterns and immutable data where appropriate in C#.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs` around lines 10 - 15, Update UnitTrackingCatalogService methods GetProfilesAsync and GetProfileAsync so they never return the shared static Profiles instances directly. Return immutable profile values or defensive copies with independently owned mutable properties, preserving the existing profile data and lookup behavior while preventing consumers from modifying catalog state across requests.Source: Coding guidelines
Core/Resgrid.Services/UnitLocationSourceResolver.cs (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNaming deviates from the service convention for this project.
IUnitLocationSourceResolver/UnitLocationSourceResolverlives inCore/Resgrid.Servicesand is registered like other services, but does not use theI{Name}Service/{Name}Servicepair (e.g.IUnitLocationSourceService). Low priority given the DI/registration churn a rename implies.As per coding guidelines, "Service interface names must follow the pattern
I{Name}Serviceand implementations must be named{Name}Service".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitLocationSourceResolver.cs` around lines 10 - 17, Rename the resolver symbols to follow the service convention: use IUnitLocationSourceService and UnitLocationSourceService instead of IUnitLocationSourceResolver and UnitLocationSourceResolver. Update the constructor, implemented interface, dependency-injection registration, and all references consistently while preserving behavior.Source: Coding guidelines
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs (2)
231-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated validity predicate.
The credential window check in the
UnitTrackingAuthenticationResultoverload is identical to theUnitTrackingCredentialoverload; delegate to avoid the two drifting.♻️ Proposed refactor
private static bool IsActive(UnitTrackingAuthenticationResult result, DateTime utcNow) { if (result?.Credential == null || !IsEnabled(result.Device)) return false; - return result.Credential.ValidFrom <= utcNow && - !result.Credential.RevokedOn.HasValue && - (!result.Credential.ExpiresOn.HasValue || result.Credential.ExpiresOn > utcNow); + return IsActive(result.Credential, utcNow); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs` around lines 231 - 247, Update the UnitTrackingAuthenticationResult overload of IsActive to retain its null-result and device-enabled checks, then delegate credential validity evaluation to the existing IsActive(UnitTrackingCredential, DateTime) overload instead of duplicating the validity predicate.
252-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated credential-sanitization/key-normalization helpers across both tracking services.
SanitizeCredentialandNormalizeKeyare copied verbatim into two files; the shared root cause is the absence of one shared helper, so any futureUnitTrackingCredentialfield addition must be mirrored in both copies orSecretHashleaks / data is dropped from whichever copy is missed.
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs#L252-L272: extractSanitizeCredentialandNormalizeKeyinto a single shared internal static helper inCore/Resgrid.Servicesand call it here.Core/Resgrid.Services/UnitTrackingService.cs#L560-L577: delete the localSanitizeCredential(andNormalizeKeyat L707-L708) and call the shared helper instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs` around lines 252 - 272, Extract the duplicated SanitizeCredential and NormalizeKey logic into one shared internal static helper in Core/Resgrid.Services, then update Core/Resgrid.Services/UnitTrackingAuthenticationService.cs lines 252-272 to call it. Remove the local SanitizeCredential and NormalizeKey implementations from Core/Resgrid.Services/UnitTrackingService.cs lines 560-577 and 707-708, replacing their usages with the shared helper while preserving the existing sanitization and normalization behavior.Core/Resgrid.Model/Services/IUnitTrackingAuthenticationService.cs (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
bypassCacheparameter and consistentCancellationTokensurface.
GetEnabledDeviceByEndpointIdAsync,GetEnabledDeviceByProtocolIdentifierAsync, andGetActiveCredentialsForDeviceAsyncare cache-backed inUnitTrackingAuthenticationService, but callers can only get fresh data by calling theInvalidate*methods. AddingbypassCache = falsematches the service convention used elsewhere. Also,InvalidateCredentialAsync/InvalidateDeviceAsyncare the only async members without aCancellationToken.As per coding guidelines, "Service methods should include a
bypassCacheparameter (default:false) to allow callers to skip cache retrieval when fresh data is required".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Services/IUnitTrackingAuthenticationService.cs` around lines 24 - 29, Update the cache-backed methods GetEnabledDeviceByEndpointIdAsync, GetEnabledDeviceByProtocolIdentifierAsync, and GetActiveCredentialsForDeviceAsync to accept an optional bypassCache parameter defaulting to false, and propagate it through their implementations and cache lookup logic. Add optional CancellationToken parameters defaulting to default to InvalidateCredentialAsync and InvalidateDeviceAsync, updating implementations and call sites to preserve cancellation support.Source: Coding guidelines
Providers/Resgrid.Providers.Bus/EventAggregator.cs (1)
26-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAsync dispatch keys on the static type, unlike the sync path.
typeof(TMessage)resolves at compile time, so a caller holding the event in a base-class/interface/object-typed variable silently matches no listeners and the message is dropped without any signal._hub.Publish(sync path) dispatches on the runtime type, so the two APIs behave differently for the same call shape. Keying onmessage?.GetType()(with a null guard) removes the trap.♻️ Proposed change
public async Task SendMessageAsync<TMessage>(TMessage message) { - if (!_asyncListeners.TryGetValue(typeof(TMessage), out var listeners)) + if (message == null) + throw new ArgumentNullException(nameof(message)); + + if (!_asyncListeners.TryGetValue(message.GetType(), out var listeners)) return; await Task.WhenAll(listeners.Values.Select(listener => listener(message))); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus/EventAggregator.cs` around lines 26 - 32, Update SendMessageAsync<TMessage> to resolve the listener key from message.GetType() at runtime, with an appropriate null guard, instead of typeof(TMessage). Preserve the existing no-listener return and asynchronous listener dispatch behavior so async dispatch matches the sync path for base-class, interface, and object-typed references.Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs (1)
252-268: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBatch-wide publish timeout can leave the batch half-published.
publishTimeoutis created once andCancelAfterapplies to the whole loop, so with a large collection the laterBasicPublishAsynccalls can be cancelled after earlier ones were already confirmed. The method then returnsfalse, and the caller has no way to know how many messages landed — a retry re-publishes the entire batch. A per-message timeout (or returning the count/failure index) makes the contract honest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs` around lines 252 - 268, Change the publish loop in the outbound batch method so each message gets its own timeout token and timeout window when calling BasicPublishAsync, preventing one shared publishTimeout from expiring across the entire batch. Preserve the existing success return and cancellation behavior while ensuring a timeout applies only to the current message.Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs (3)
727-748: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate header-parsing switch.
This is the same numeric-header parse already inlined in
RetryQueueItem(Lines 759-778), now with clamping added — the two copies have already diverged. Extracting a singleTryGetIntHeader(headers, name)helper and using it from both keeps the semantics aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 727 - 748, Replace the duplicated numeric header parsing in GetRetryCount and RetryQueueItem with a shared TryGetIntHeader(headers, name) helper. Move the existing type handling and non-negative/int-range clamping into that helper, then have both callers use it while preserving a zero result for missing or unparseable headers.
663-671: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPermanent and transient failures are handled identically. The worker signals unrecoverable payload problems (missing
UnitId/DepartmentId, absent coordinates) with the same plain exceptions used for transient store/broker failures, and the consumer's catch cannot tell them apart — so a message that can never succeed still consumes the fullUnitLocationMaxRetryAttemptsbudget, and requeues in a tight loop whenever the republish itself fails.
Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs#L663-L671: catch a dedicated non-retryable exception type separately andBasicNackAsync(..., requeue: false)(or publish straight to the dead-letter queue) instead of the retry path.Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs#L15-L28: throw a distinct non-retryable exception type from the validation guards so the consumer can classify them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 663 - 671, The unit-location consumer must distinguish unrecoverable validation failures from retryable exceptions. In Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs lines 663-671, catch the dedicated non-retryable exception separately and acknowledge it with BasicNackAsync using requeue false, while preserving RetryOrDeadLetterUnitLocationAsync for other exceptions; in Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs lines 15-28, update the UnitId, DepartmentId, and coordinate validation guards to throw that distinct non-retryable exception type.
688-716: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse a confirm channel for retry publisher
RetryOrDeadLetterUnitLocationAsynccreates a new channel for every failed delivery, andRabbitConnection.CreateConnectioncurrently creates a new shared connection on failed initialization paths. Under failure bursts this generates one channel open/close per message, increasing broker-handshake load and exhausting connection channel capacity. Keep the retry publisher as a single lazy/channel-recoverable channel instead of per-message churn.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs` around lines 688 - 716, Update RetryOrDeadLetterUnitLocationAsync to reuse a single lazily initialized confirm channel for retry publishing instead of creating and disposing a channel per delivery. Back the channel with a shared connection, recover or recreate it when closed or publishing fails, and preserve the existing publish options, timeout, and retry behavior.Core/Resgrid.Services/UnitsService.cs (2)
545-565: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
cancellationTokenis now unused, and the Mongo new-vs-existing check is obscure.Neither
InsertAsync/UpdateAsyncnorSendMessageAsyncreceive the token, so callers passing one (e.g.UnitLocationQueueLogic) get no cancellation. Alsolocation.Id.Timestamp == 0is an indirect way of asking "is this ObjectId unset" —location.Id == ObjectId.Emptystates the intent directly.♻️ Proposed change
else { - if (location.Id.Timestamp == 0) - result = await _unitLocationsMongoRepository.Value.InsertAsync(location); + if (location.Id == ObjectId.Empty) + result = await _unitLocationsMongoRepository.Value.InsertAsync(location, cancellationToken); else - result = await _unitLocationsMongoRepository.Value.UpdateAsync(location); + result = await _unitLocationsMongoRepository.Value.UpdateAsync(location, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitsService.cs` around lines 545 - 565, Update AddUnitLocationAsync to propagate cancellationToken through the relevant repository and SendMessageAsync calls so caller cancellation is honored, including the UnitLocationQueueLogic path. Replace the Mongo insert/update condition based on location.Id.Timestamp with a direct comparison against ObjectId.Empty, preserving the existing insert behavior for unset IDs and update behavior for existing IDs.
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate the overlapping unit-location read/write repositories
SaveUnitLocationAsyncnow uses_unitLocationsMongoRepositoryfor writes, butGetLatestUnitLocationAsyncandGetLatestUnitLocationsAsyncstill rely on_unitLocationRepositoryfor MongoDB reads. Move the read paths under_unitLocationsMongoRepositoryor expose equivalent read methods on that abstraction so the same data source is not split between two abstractions.This also increases
UnitsServiceto 17 constructor parameters; per the C# guidelines, prefer resolving dependencies explicitly viaBootstrapper.GetKernel().Resolve<T>()rather than constructor injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitsService.cs` at line 25, Consolidate unit-location access in UnitsService by updating GetLatestUnitLocationAsync and GetLatestUnitLocationsAsync to use _unitLocationsMongoRepository, adding equivalent read methods there if needed, while preserving their current behavior. Remove the redundant _unitLocationRepository dependency and resolve any required repository explicitly through Bootstrapper.GetKernel().Resolve<T>() instead of increasing constructor injection.Source: Coding guidelines
Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs (1)
61-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThrowing here fails the caller's persistence path, not just the broadcast.
EventAggregator.SendMessageAsyncpropagates listener exceptions, and the only caller isUnitsService.AddUnitLocationAsync(Line 569), which invokes it after the location row is already written. So a transient topic-publish hiccup makesAddUnitLocationAsyncthrow, which makesUnitLocationQueueLogic.ProcessUnitLocationQueueItemthrow, which drives the whole message back through the retry/dead-letter pipeline and re-persists the location.If that at-least-once behavior is intentional it's workable given idempotent writes, but consider surfacing it distinctly (e.g. log + a dedicated broadcast-retry) so a realtime failure doesn't re-drive persistence and doesn't consume the location message's retry budget.
Also applies to: 613-620
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs` at line 61, Update the outbound broadcast handling around the event listeners at the referenced locations so EventAggregator.SendMessageAsync failures are caught and handled separately from the persistence call. Log the broadcast failure and route it through a dedicated broadcast retry mechanism, ensuring UnitsService.AddUnitLocationAsync and UnitLocationQueueLogic.ProcessUnitLocationQueueItem do not rethrow realtime delivery errors or consume the location message retry budget.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Config/ServiceBusConfig.cs`:
- Around line 53-55: Update the UnitLocationQueueV2Name,
UnitLocationRetryQueueV2Name, and UnitLocationDeadQueueV2Name definitions in
ServiceBusConfig to follow the existing `#if` DEBUG queue-name split, applying the
established test suffix or equivalent DEBUG-only isolation while preserving the
current release names.
In `@Core/Resgrid.Framework/SentryTransactionFilter.cs`:
- Around line 64-70: Update the transaction filtering logic around
RedactCapabilityPath so transaction.Name is also passed through the same
redaction before the status check. Preserve the existing Request.Url redaction
and ensure both fields are scrubbed, including 404 transactions whose names
derive from the raw request path.
In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs`:
- Around line 39-45: Update the token generation logic around
RandomNumberGenerator and keyPrefix so the stored/displayed prefix is generated
from separate random bytes rather than derived from encodedSecret. Encode the
independent prefix with the same base64url-safe normalization, then prepend it
to the token while preserving the existing rgtrk token format and full secret
entropy.
- Around line 195-205: Update the cached result handling in the credential
retrieval flow before the `cached.Where(...)` call to coalesce a null provider
result to an empty credential collection. Preserve the existing filtering and
list conversion for non-null results.
In `@Core/Resgrid.Services/UnitTrackingService.cs`:
- Around line 135-136: Update the edit flow containing the existing.IsEnabled
and device.IsEnabled check so requested field changes are applied before
handling the disable transition; do not return early to DisableDeviceAsync
before updating DisplayName, keys, identifiers, SourcePriority,
AllowedSourceCidrs, and FirmwareVersion. Preserve the disable behavior after the
updates, and expose the credential-revocation consequence through the existing
caller/UI notification or response mechanism.
- Line 102: Validate device.AllowedSourceCidrs before persisting it in the
device create/update flow, rather than only applying TrimToNull. Parse each
nonblank CIDR entry with TryParse, enforce valid prefix bounds, and require
canonical entries; reject malformed values before saving while preserving the
network policy’s blank-value behavior.
- Around line 681-697: Update IsHttpTokenCharacter to accept only ASCII letters
and digits, while retaining the existing RFC 9110 tchar punctuation set; replace
the Unicode-permissive char.IsLetterOrDigit check so CustomHeader validation in
the surrounding authentication configuration rejects characters such as é and
non-ASCII digits.
- Around line 633-660: Update CopyForRebind to preserve the source device’s
enabled state instead of hardcoding IsEnabled to true, and derive LastStatus
from that state using the same behavior as CreateDeviceAsync. Keep all other
copied properties unchanged.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitConnection.cs`:
- Around line 142-186: Isolate the UnitLocation V2 queue declaration and binding
block from the main setup flow by moving it after the unrelated queue
declarations and/or extracting it into a dedicated helper such as
DeclareUnitLocationV2QueuesAsync. Catch and log failures within that isolated
flow so a PRECONDITION_FAILED from the TTL arguments does not abort declarations
for PersonnelLoactionQueueName, EventingTopic, SecurityRefreshQueueName,
WorkflowQueueName, or ChatbotProcessingQueueName.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs`:
- Around line 188-194: Replace ASCII encoding with UTF-8 in both unit-location
publish paths: SendMessage at
Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs lines
188-194 and SendMessagesWithConfirmation at lines 259-265. Update each message
body encoding while preserving the existing publish behavior.
In `@Providers/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.cs`:
- Around line 42-48: Replace the separate department and unit foreign keys in
M0101_AddUnitTracking.cs and M0101_AddUnitTrackingPg.cs at lines 42-48 with a
composite foreign key linking UnitTrackingDevices.(DepartmentId, UnitId) to the
matching department-owned unit key (DepartmentId, UnitId), preserving the
existing table and constraint intent in both migrations.
In
`@Repositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.cs`:
- Around line 22-32: Replace constructor-injected collaborator parameters with
explicit Bootstrapper.GetKernel().Resolve<T>() calls in
UnitTrackingCredentialsRepository.cs (lines 22-32) and
UnitTrackingDevicesRepository.cs (lines 22-32), assigning each resolved
dependency to the existing fields. In DocumentDbRepository.cs (lines 13-18),
resolve the Mongo repository through the same service-locator pattern while
preserving its deferred initialization behavior.
In
`@Repositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.cs`:
- Around line 57-62: Update EnsureIndexesAsync and the cached _ensureIndexesTask
flow so a failed CreateIndexesAsync operation clears the cached task after its
await fails, allowing the next call to retry index initialization. Preserve the
existing lock and successful-task caching behavior.
In
`@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.cs`:
- Around line 232-238: Update ReadBoundedBodyAsync so the chunk allocation no
longer computes maximumBytes + 1; size the chunk using the bounded maximum
directly while retaining the existing total-byte limit enforcement.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitLocationController.cs`:
- Around line 103-104: Update the enqueue flow in EnqueueUnitLocationEventAsync
handling within the controller action so exceptions from queue submission are
caught and mapped to 503 ServiceUnavailable, matching the existing false-result
path. Ensure this handling occurs before the outer catch that returns 400, while
preserving the current response behavior for other failures.
In `@Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs`:
- Around line 98-99: Validate AllowedSourceCidrs at the API model boundary in
the relevant model properties, rejecting malformed CIDR entries such as invalid
prefixes or incomplete addresses instead of relying on
UnitTrackingNetworkPolicy.Contains. Preserve valid CIDR-list input and return a
clear validation error for any invalid entry, while retaining the existing
MaxLength constraint.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml`:
- Around line 137-154: Associate the Create Credential labels with their
controls by updating the labels in the authentication mode, custom header, and
basic username groups to use the corresponding asp-for bindings or matching for
attributes. Ensure they target CredentialInput.AuthMode,
CredentialInput.HeaderName, and CredentialInput.BasicUsername, while preserving
the existing label styling and text.
In `@Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs`:
- Around line 24-25: Update the UnitLocationQueueLogic handling so an unknown
IsValidFix value remains null rather than being coerced to true when persisted
to UnitsLocation. Preserve the existing invalid-fix early return, but add the
appropriate operational logging or metric before returning success so dropped
invalid fixes are observable.
- Around line 15-28: Restore try-catch handling around the validation logic in
the relevant method, retain the existing exception propagation for rejected
events, and call Resgrid.Framework.Logging.LogException() in the catch before
rethrowing. Re-add the Resgrid.Framework import required for the static logging
API, while preserving the current validation and retry/nack behavior.
---
Outside diff comments:
In `@Core/Resgrid.Services/DepartmentSettingsService.cs`:
- Around line 48-69: Update SaveOrUpdateSettingAsync so
InvalidateSettingCacheAsync runs only after SaveOrUpdateAsync completes
successfully for both the new-setting and existing-setting branches. Preserve
the saved result, invalidate using departmentId and type after persistence, then
return the result; remove the current pre-write invalidation.
---
Nitpick comments:
In `@Core/Resgrid.Model/Services/IUnitTrackingAuthenticationService.cs`:
- Around line 24-29: Update the cache-backed methods
GetEnabledDeviceByEndpointIdAsync, GetEnabledDeviceByProtocolIdentifierAsync,
and GetActiveCredentialsForDeviceAsync to accept an optional bypassCache
parameter defaulting to false, and propagate it through their implementations
and cache lookup logic. Add optional CancellationToken parameters defaulting to
default to InvalidateCredentialAsync and InvalidateDeviceAsync, updating
implementations and call sites to preserve cancellation support.
In `@Core/Resgrid.Model/Services/IUnitTrackingCatalogService.cs`:
- Around line 10-15: Update UnitTrackingCatalogService methods GetProfilesAsync
and GetProfileAsync so they never return the shared static Profiles instances
directly. Return immutable profile values or defensive copies with independently
owned mutable properties, preserving the existing profile data and lookup
behavior while preventing consumers from modifying catalog state across
requests.
In `@Core/Resgrid.Services/UnitLocationSourceResolver.cs`:
- Around line 10-17: Rename the resolver symbols to follow the service
convention: use IUnitLocationSourceService and UnitLocationSourceService instead
of IUnitLocationSourceResolver and UnitLocationSourceResolver. Update the
constructor, implemented interface, dependency-injection registration, and all
references consistently while preserving behavior.
In `@Core/Resgrid.Services/UnitsService.cs`:
- Around line 545-565: Update AddUnitLocationAsync to propagate
cancellationToken through the relevant repository and SendMessageAsync calls so
caller cancellation is honored, including the UnitLocationQueueLogic path.
Replace the Mongo insert/update condition based on location.Id.Timestamp with a
direct comparison against ObjectId.Empty, preserving the existing insert
behavior for unset IDs and update behavior for existing IDs.
- Line 25: Consolidate unit-location access in UnitsService by updating
GetLatestUnitLocationAsync and GetLatestUnitLocationsAsync to use
_unitLocationsMongoRepository, adding equivalent read methods there if needed,
while preserving their current behavior. Remove the redundant
_unitLocationRepository dependency and resolve any required repository
explicitly through Bootstrapper.GetKernel().Resolve<T>() instead of increasing
constructor injection.
In `@Core/Resgrid.Services/UnitTrackingAuthenticationService.cs`:
- Around line 231-247: Update the UnitTrackingAuthenticationResult overload of
IsActive to retain its null-result and device-enabled checks, then delegate
credential validity evaluation to the existing IsActive(UnitTrackingCredential,
DateTime) overload instead of duplicating the validity predicate.
- Around line 252-272: Extract the duplicated SanitizeCredential and
NormalizeKey logic into one shared internal static helper in
Core/Resgrid.Services, then update
Core/Resgrid.Services/UnitTrackingAuthenticationService.cs lines 252-272 to call
it. Remove the local SanitizeCredential and NormalizeKey implementations from
Core/Resgrid.Services/UnitTrackingService.cs lines 560-577 and 707-708,
replacing their usages with the shared helper while preserving the existing
sanitization and normalization behavior.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs`:
- Around line 727-748: Replace the duplicated numeric header parsing in
GetRetryCount and RetryQueueItem with a shared TryGetIntHeader(headers, name)
helper. Move the existing type handling and non-negative/int-range clamping into
that helper, then have both callers use it while preserving a zero result for
missing or unparseable headers.
- Around line 663-671: The unit-location consumer must distinguish unrecoverable
validation failures from retryable exceptions. In
Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs lines
663-671, catch the dedicated non-retryable exception separately and acknowledge
it with BasicNackAsync using requeue false, while preserving
RetryOrDeadLetterUnitLocationAsync for other exceptions; in
Workers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs lines 15-28,
update the UnitId, DepartmentId, and coordinate validation guards to throw that
distinct non-retryable exception type.
- Around line 688-716: Update RetryOrDeadLetterUnitLocationAsync to reuse a
single lazily initialized confirm channel for retry publishing instead of
creating and disposing a channel per delivery. Back the channel with a shared
connection, recover or recreate it when closed or publishing fails, and preserve
the existing publish options, timeout, and retry behavior.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.cs`:
- Around line 252-268: Change the publish loop in the outbound batch method so
each message gets its own timeout token and timeout window when calling
BasicPublishAsync, preventing one shared publishTimeout from expiring across the
entire batch. Preserve the existing success return and cancellation behavior
while ensuring a timeout applies only to the current message.
In `@Providers/Resgrid.Providers.Bus/EventAggregator.cs`:
- Around line 26-32: Update SendMessageAsync<TMessage> to resolve the listener
key from message.GetType() at runtime, with an appropriate null guard, instead
of typeof(TMessage). Preserve the existing no-listener return and asynchronous
listener dispatch behavior so async dispatch matches the sync path for
base-class, interface, and object-typed references.
In `@Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs`:
- Line 61: Update the outbound broadcast handling around the event listeners at
the referenced locations so EventAggregator.SendMessageAsync failures are caught
and handled separately from the persistence call. Log the broadcast failure and
route it through a dedicated broadcast retry mechanism, ensuring
UnitsService.AddUnitLocationAsync and
UnitLocationQueueLogic.ProcessUnitLocationQueueItem do not rethrow realtime
delivery errors or consume the location message retry budget.
In
`@Web/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.cs`:
- Around line 84-102: Update the Authorization parsing branch in the
authentication method so unsupported schemes fall through to the existing
custom-header credential lookup instead of returning null. Apply the same
fall-through behavior to the MaximumAuthorizationHeaderLength check unless
strict rejection of oversized headers is explicitly required; preserve the
Bearer, Basic, and empty-token handling.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.cs`:
- Around line 30-40: Update the UnitTrackingIngressController constructor to
remove the four service parameters and resolve
UnitTrackingHttpAuthenticationService, UnitTrackingJsonPayloadParser,
UnitTrackingRateLimiter, and IUnitTrackingIngressService through
Bootstrapper.GetKernel().Resolve<T>(), assigning each result to the
corresponding fields.
In `@Web/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.cs`:
- Around line 132-133: Remove the hardcoded [Range(1, 4)] validation from the
AuthMode property in the unit-tracking admin model, leaving validation to
UnitTrackingDevicesController.CreateCredential’s Enum.IsDefined and
SupportedAuthModes checks so future enum values are accepted when supported.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.cs`:
- Around line 489-551: The ValidatePreviewJson method duplicates ingestion
validation; expose and reuse a shared TryParse or Validate entry point from
UnitTrackingJsonPayloadParser for preview requests. Move or centralize size,
depth, duplicate-key, positions-shape, and coordinate checks there, then have
ValidatePreviewJson consume its result and preserve the existing error key and
position-count behavior.
- Around line 609-647: Consolidate the duplicated field assignments in
BuildDevice and ApplyUpdate into one shared UnitTrackingDevice mapper, accepting
IsEnabled from each caller so creation remains enabled while updates use
model.IsEnabled. Have both methods reuse this mapper and preserve all existing
model/profile field mappings.
In `@Web/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.cs`:
- Around line 79-80: Replace the hardcoded Range validation on AuthMode with
EnumDataType targeting UnitTrackingAuthMode, preserving the existing Bearer
default and enum-backed validation behavior.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtml`:
- Around line 19-30: The profile option’s data-identifier-required attribute is
unused by updateProfileHelp(), so the UI does not reflect the server-side
requirement. Update updateProfileHelp() and the Device Identifier field markup
to read the selected option’s flag and show or hide an appropriate required
hint/marker; preserve the existing behavior when the profile does not require an
identifier.
In `@Web/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtml`:
- Line 169: Replace the hardcoded "resgrid-json-v1" comparison in the Details
view with the shared constant exposed by UnitTrackingCatalogProfile or the
established adapter-key constants class. Preserve the existing case-insensitive
comparison and preview condition while ensuring the value comes from the single
canonical definition.
🪄 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 Plus
Run ID: 8d6e3773-5a91-4618-9a51-1c93ee77db66
⛔ Files ignored due to path filters (26)
Core/Resgrid.Localization/Areas/User/Units/Units.resxis excluded by!**/*.resxTests/Resgrid.Tests/Framework/SentryTransactionFilterTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Framework/UnitLocationEventSerializationTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Providers/EventAggregatorTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Providers/UnitLocationEventProviderTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Repositories/DocumentDbRepositoryTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Resgrid.Tests.csprojis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/DepartmentSettingsServiceUnitTrackingTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitLocationSourceResolverTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingAuthenticationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingEventIdServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingIdentifierServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingIngressServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/UnitTrackingStatusServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/CapabilityPathRedactionMiddlewareTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitLocationControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingDevicesControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingHttpAuthenticationServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingIngressControllerTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingJsonPayloadParserTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingNetworkPolicyTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/Services/UnitTrackingRateLimiterTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Web/User/UnitTrackingControllerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (94)
.coderabbit.yamlCore/Resgrid.Config/ServiceBusConfig.csCore/Resgrid.Config/UnitTrackingConfig.csCore/Resgrid.Framework/SentryTransactionFilter.csCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Events/UnitLocationEvent.csCore/Resgrid.Model/Providers/IEventAggregator.csCore/Resgrid.Model/Providers/IRabbitOutboundQueueProvider.csCore/Resgrid.Model/Providers/IUnitLocationEventProvider.csCore/Resgrid.Model/Repositories/IDocumentDbRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsDocRepository.csCore/Resgrid.Model/Repositories/IUnitLocationsMongoRepository.csCore/Resgrid.Model/Repositories/IUnitTrackingCredentialsRepository.csCore/Resgrid.Model/Repositories/IUnitTrackingDevicesRepository.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/IUnitLocationSourceResolver.csCore/Resgrid.Model/Services/IUnitTrackingAuthenticationService.csCore/Resgrid.Model/Services/IUnitTrackingCatalogService.csCore/Resgrid.Model/Services/IUnitTrackingEventIdService.csCore/Resgrid.Model/Services/IUnitTrackingIdentifierService.csCore/Resgrid.Model/Services/IUnitTrackingIngressService.csCore/Resgrid.Model/Services/IUnitTrackingService.csCore/Resgrid.Model/Services/IUnitTrackingStatusService.csCore/Resgrid.Model/Services/IUnitsService.csCore/Resgrid.Model/Tracking/AuthenticatedTrackingSource.csCore/Resgrid.Model/Tracking/CanonicalTrackingPosition.csCore/Resgrid.Model/Tracking/TrackingIngressResult.csCore/Resgrid.Model/Tracking/UnitTrackingCatalogProfile.csCore/Resgrid.Model/UnitLocationWriteResult.csCore/Resgrid.Model/UnitTrackingCredential.csCore/Resgrid.Model/UnitTrackingDevice.csCore/Resgrid.Model/UnitTrackingEnums.csCore/Resgrid.Model/UnitTrackingResults.csCore/Resgrid.Model/UnitsLocation.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/UnitLocationSourceResolver.csCore/Resgrid.Services/UnitTrackingAuthenticationService.csCore/Resgrid.Services/UnitTrackingCacheKeys.csCore/Resgrid.Services/UnitTrackingCatalogService.csCore/Resgrid.Services/UnitTrackingEventIdService.csCore/Resgrid.Services/UnitTrackingIdentifierService.csCore/Resgrid.Services/UnitTrackingIngressService.csCore/Resgrid.Services/UnitTrackingService.csCore/Resgrid.Services/UnitTrackingStatusService.csCore/Resgrid.Services/UnitsService.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitTopicProvider.csProviders/Resgrid.Providers.Bus/BusModule.csProviders/Resgrid.Providers.Bus/EventAggregator.csProviders/Resgrid.Providers.Bus/OutboundEventProvider.csProviders/Resgrid.Providers.Bus/UnitLocationEventProvider.csProviders/Resgrid.Providers.Migrations/Migrations/M0101_AddUnitTracking.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.csProviders/Resgrid.Providers.MigrationsPg/Sql/EF0003_PopulateDocDb.sqlRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingCredentialBySecretHashQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingCredentialsByDeviceQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingDeviceByProtocolIdentifierQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/UnitTracking/SelectUnitTrackingDevicesByUnitQuery.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingCredentialsRepository.csRepositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/DocumentDbRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/NoSqlDataModule.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsDocRepository.csRepositories/Resgrid.Repositories.NoSqlRepository/UnitLocationsMongoRepository.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingHttpAuthenticationService.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingJsonPayloadParser.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingNetworkPolicy.csWeb/Resgrid.Web.Services/ApplicationCore/UnitTracking/UnitTrackingRateLimiter.csWeb/Resgrid.Web.Services/Controllers/v4/UnitLocationController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingDevicesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitTrackingIngressController.csWeb/Resgrid.Web.Services/Middleware/CapabilityPathRedactionMiddleware.csWeb/Resgrid.Web.Services/Middleware/SentryEventProcessor.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingAdminModels.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingIngressResponse.csWeb/Resgrid.Web.Services/Models/v4/UnitTracking/UnitTrackingPositionInput.csWeb/Resgrid.Web.Services/Program.csWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/UnitTrackingController.csWeb/Resgrid.Web/Areas/User/Models/UnitTracking/UnitTrackingViewModels.csWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Credential.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Details.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/UnitTracking/_Editor.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtmlWorkers/Resgrid.Workers.Framework/Logic/UnitLocationQueueLogic.cs
| public static string UnitLocationQueueV2Name = "unitlocation-v2"; | ||
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | ||
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Isolate the v2 queues in DEBUG builds.
Lines 53-55 are outside the existing #if DEBUG split, so DEBUG and release builds use the same RabbitMQ queue names. Mirror the existing test suffix convention or verify that DEBUG always uses a completely isolated broker.
Proposed fix
+#if DEBUG
+ public static string UnitLocationQueueV2Name = "unitlocation-v2test";
+ public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retrytest";
+ public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.deadtest";
+#else
public static string UnitLocationQueueV2Name = "unitlocation-v2";
public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry";
public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead";
+#endif📝 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.
| public static string UnitLocationQueueV2Name = "unitlocation-v2"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; | |
| `#if` DEBUG | |
| public static string UnitLocationQueueV2Name = "unitlocation-v2test"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retrytest"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.deadtest"; | |
| `#else` | |
| public static string UnitLocationQueueV2Name = "unitlocation-v2"; | |
| public static string UnitLocationRetryQueueV2Name = "unitlocation-v2.retry"; | |
| public static string UnitLocationDeadQueueV2Name = "unitlocation-v2.dead"; | |
| `#endif` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Config/ServiceBusConfig.cs` around lines 53 - 55, Update the
UnitLocationQueueV2Name, UnitLocationRetryQueueV2Name, and
UnitLocationDeadQueueV2Name definitions in ServiceBusConfig to follow the
existing `#if` DEBUG queue-name split, applying the established test suffix or
equivalent DEBUG-only isolation while preserving the current release names.
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | ||
| return StatusCode(StatusCodes.Status503ServiceUnavailable); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return 503 when enqueueing throws.
A faulted EnqueueUnitLocationEventAsync task reaches the outer catch and becomes 400 BadRequest, so clients will not treat a queue outage as retryable.
Suggested handling
- if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location))
- return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ try
+ {
+ if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location))
+ return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ }
+ catch (Exception ex)
+ {
+ Logging.LogException(ex);
+ return StatusCode(StatusCodes.Status503ServiceUnavailable);
+ }📝 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.
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| try | |
| { | |
| if (!await _unitLocationEventProvider.EnqueueUnitLocationEventAsync(location)) | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| } | |
| catch (Exception ex) | |
| { | |
| Logging.LogException(ex); | |
| return StatusCode(StatusCodes.Status503ServiceUnavailable); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitLocationController.cs` around
lines 103 - 104, Update the enqueue flow in EnqueueUnitLocationEventAsync
handling within the controller action so exceptions from queue submission are
caught and mapped to 503 ServiceUnavailable, matching the existing false-result
path. Ensure this handling occurs before the outer catch that returns 400, while
preserving the current response behavior for other failures.
| public static bool Enabled = false; | ||
| public static bool HttpsIngressEnabled = false; | ||
| public static bool NativeGatewayEnabled = false; | ||
| public static string CredentialPepper = ""; |
There was a problem hiding this comment.
Mutable static fields, such as CredentialPepper, risk accidental runtime modification. Declare compile-time constant values using const or static readonly to ensure immutability and thread safety.
Kody rule violation: Use `readonly` or `const` for Immutable Data
Prompt for LLM
File Core/Resgrid.Config/UnitTrackingConfig.cs:
Line 8:
Mutable static fields, such as `CredentialPepper`, risk accidental runtime modification. Declare compile-time constant values using `const` or `static readonly` to ensure immutability and thread safety.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| public static string RedactCapabilityPath(string value) | ||
| { | ||
| const string prefix = "/api/v4/unit-trackers/c/"; |
There was a problem hiding this comment.
Decentralized string literals like route names violate reuse guidelines and complicate updates when paths change. Move this route-name constant to a centralized shared constants class, such as RouteConstants.UnitTrackerCapabilityPrefix, and reference it throughout the codebase.
Kody rule violation: Centralize string constants
Prompt for LLM
File Core/Resgrid.Framework/SentryTransactionFilter.cs:
Line 82:
Decentralized string literals like route names violate reuse guidelines and complicate updates when paths change. Move this route-name constant to a centralized shared constants class, such as `RouteConstants.UnitTrackerCapabilityPrefix`, and reference it throughout the codebase.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <returns>Returns the current IEventSubscriptionManager to allow for easy fluent additions.</returns> | ||
| Guid AddListener<T>(Action<T> listener); | ||
|
|
||
| Guid AddAsyncListener<T>(Func<T, Task> listener); |
There was a problem hiding this comment.
Unobserved task rejections occur when async listeners provided to AddAsyncListener<T> fault without a defined error-handling path. Add an error handler parameter, such as Action<Exception> onError, to allow deterministic error handling alongside the subscription.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File Core/Resgrid.Model/Providers/IEventAggregator.cs:
Line 29:
Unobserved task rejections occur when async listeners provided to `AddAsyncListener<T>` fault without a defined error-handling path. Add an error handler parameter, such as `Action<Exception> onError`, to allow deterministic error handling alongside the subscription.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Task<TrackingIngressResult> AcceptAsync( | ||
| AuthenticatedTrackingSource source, | ||
| IReadOnlyCollection<CanonicalTrackingPosition> positions, | ||
| CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Ambiguous async contract identified on AcceptAsync due to missing XML documentation describing resolution values, rejection conditions, or cancellation semantics. Add XML doc comments to detail the success result shape, faulted states, and OperationCanceledException propagation behavior.
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitTrackingIngressService.cs:
Line 10 to 13:
Ambiguous async contract identified on `AcceptAsync` due to missing XML documentation describing resolution values, rejection conditions, or cancellation semantics. Add XML doc comments to detail the success result shape, faulted states, and `OperationCanceledException` propagation behavior.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| Task<UnitTrackingDevice> GetDeviceByIdAsync(string deviceId, int departmentId); | ||
| Task<List<UnitTrackingDevice>> GetDevicesForDepartmentAsync(int departmentId); | ||
| Task<List<UnitTrackingDevice>> GetDevicesForUnitAsync(int departmentId, int unitId); | ||
| Task<List<UnitTrackingCredential>> GetCredentialsForDeviceAsync(string deviceId, int departmentId); |
There was a problem hiding this comment.
Mutable return types expose APIs to unintended side effects by allowing callers to modify the underlying collection. Change the GetCredentialsForDeviceAsync return type to Task<IReadOnlyList<UnitTrackingCredential>> to enforce immutability.
Kody rule violation: Use IReadOnlyList for immutable collections
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitTrackingService.cs:
Line 13:
Mutable return types expose APIs to unintended side effects by allowing callers to modify the underlying collection. Change the `GetCredentialsForDeviceAsync` return type to `Task<IReadOnlyList<UnitTrackingCredential>>` to enforce immutability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <returns>Task<UnitLocation>.</returns> | ||
| Task<UnitsLocation> AddUnitLocationAsync(UnitsLocation location, int departmentId, | ||
| /// <returns>The idempotent document-store write result.</returns> | ||
| Task<UnitLocationWriteResult> AddUnitLocationAsync(UnitsLocation location, int departmentId, |
There was a problem hiding this comment.
Breaking contract change identified by modifying the AddUnitLocationAsync return type from Task<UnitsLocation> to Task<UnitLocationWriteResult>. Document this modification with an explicit 'BREAKING CHANGE' section detailing migration steps, affected consumers, and a rollout plan.
Kody rule violation: Call out breaking changes explicitly
Prompt for LLM
File Core/Resgrid.Model/Services/IUnitsService.cs:
Line 308:
Breaking contract change identified by modifying the `AddUnitLocationAsync` return type from `Task<UnitsLocation>` to `Task<UnitLocationWriteResult>`. Document this modification with an explicit 'BREAKING CHANGE' section detailing migration steps, affected consumers, and a rollout plan.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public static UnitLocationWriteResult Duplicate(UnitsLocation location) | ||
| { | ||
| return new UnitLocationWriteResult | ||
| { | ||
| Status = UnitLocationWriteStatus.Duplicate, | ||
| Location = location | ||
| }; | ||
| } |
There was a problem hiding this comment.
Duplicate code sequences identified in the Duplicate and Inserted factory methods, which differ only by the Status enum value. Extract a private helper method accepting the status parameter to reduce maintenance burden and consolidate instantiation logic.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Model/UnitLocationWriteResult.cs:
Line 23 to 30:
Duplicate code sequences identified in the `Duplicate` and `Inserted` factory methods, which differ only by the `Status` enum value. Extract a private helper method accepting the status parameter to reduce maintenance burden and consolidate instantiation logic.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| LongCacheLength) | ||
| : await getSetting(); | ||
|
|
||
| return int.TryParse(value, out var seconds) ? Math.Max(1, seconds) : 180; |
There was a problem hiding this comment.
Magic numbers obscure intent and introduce maintenance risks when duplicated, such as 180 appearing as both an integer and a string literal. Extract named constants like DefaultStaleAfterSeconds and MinimumStaleAfterSeconds to clarify intent and centralize default values.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 974:
Magic numbers obscure intent and introduce maintenance risks when duplicated, such as `180` appearing as both an integer and a string literal. Extract named constants like `DefaultStaleAfterSeconds` and `MinimumStaleAfterSeconds` to clarify intent and centralize default values.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| public async Task<int> GetHardwareTrackingStaleAfterSecondsAsync(int departmentId, bool bypassCache = false) | ||
| { | ||
| async Task<string> getSetting() |
There was a problem hiding this comment.
Naming convention violation identified on the local function getSetting. Rename it to GetSetting to comply with .NET PascalCase requirements for methods.
Kody rule violation: Use proper naming conventions
Prompt for LLM
File Core/Resgrid.Services/DepartmentSettingsService.cs:
Line 959:
Naming convention violation identified on the local function `getSetting`. Rename it to `GetSetting` to comply with .NET PascalCase requirements for methods.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var latestPerSource = locations | ||
| .Where(location => | ||
| location != null && | ||
| location.DepartmentId == departmentId && | ||
| location.IsValidFix != false) | ||
| .GroupBy(location => new | ||
| { | ||
| location.SourceType, | ||
| SourceId = location.SourceId ?? string.Empty | ||
| }) | ||
| .Select(group => group | ||
| .OrderByDescending(location => location.Timestamp) | ||
| .ThenByDescending(location => location.ReceivedOn ?? location.Timestamp) | ||
| .First()) | ||
| .ToList(); |
There was a problem hiding this comment.
Complex LINQ chains obscure hidden grouping and latest-selection logic, complicating readability, debugging, and testing. Break the 15-line expression into named intermediate steps to materialize the list through discrete filtering, grouping, and selection operations.
Kody rule violation: Limit Lengthy LINQ Chains
Prompt for LLM
File Core/Resgrid.Services/UnitLocationSourceResolver.cs:
Line 30 to 44:
Complex LINQ chains obscure hidden grouping and latest-selection logic, complicating readability, debugging, and testing. Break the 15-line expression into named intermediate steps to materialize the list through discrete filtering, grouping, and selection operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return Invalid(receivedOn, "The tracking binding is not enabled."); | ||
|
|
||
| var device = source.Device; | ||
| var unit = await _unitsService.GetUnitByIdAsync(device.UnitId); |
There was a problem hiding this comment.
Unnecessary database queries occur because GetUnitByIdAsync executes before input validations complete. Move the positions.Count == 0 and batch-limit validations above the database call to allow invalid payloads to return early without a round-trip.
Kody rule violation: Order validations before database queries
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingIngressService.cs:
Line 57:
Unnecessary database queries occur because `GetUnitByIdAsync` executes before input validations complete. Move the `positions.Count == 0` and batch-limit validations above the database call to allow invalid payloads to return early without a round-trip.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var oldCacheIdentity = CopyCacheIdentity(existing); | ||
| var now = DateTime.UtcNow; | ||
|
|
||
| _unitOfWork.CreateOrGetConnection(); |
There was a problem hiding this comment.
Thread starvation risk identified due to synchronous connection-creation calls blocking execution inside an async method. Replace _unitOfWork.CreateOrGetConnection() with its async equivalent, such as await _unitOfWork.CreateOrGetConnectionAsync(), to improve throughput.
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 202:
Thread starvation risk identified due to synchronous connection-creation calls blocking execution inside an async method. Replace `_unitOfWork.CreateOrGetConnection()` with its async equivalent, such as `await _unitOfWork.CreateOrGetConnectionAsync()`, to improve throughput.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _devicesRepository.InsertAsync(replacement, cancellationToken); | ||
| _unitOfWork.CommitChanges(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Silent transaction failures hinder production diagnostics because the catch block rolls back and rethrows without structured logging. Catch a typed exception, log it with operational context including deviceId and departmentId, and then rethrow.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 218:
Silent transaction failures hinder production diagnostics because the catch block rolls back and rethrows without structured logging. Catch a typed exception, log it with operational context including `deviceId` and `departmentId`, and then rethrow.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _devicesRepository.InsertAsync(replacement, cancellationToken); | ||
| _unitOfWork.CommitChanges(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Indiscriminate exception handling masks transient database errors like timeouts and deadlocks, preventing retry policies from executing. Catch DbUpdateException to inspect for transient error codes and conditionally retry safe, idempotent operations.
Kody rule violation: Implement proper database error checking
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 218:
Indiscriminate exception handling masks transient database errors like timeouts and deadlocks, preventing retry policies from executing. Catch `DbUpdateException` to inspect for transient error codes and conditionally retry safe, idempotent operations.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| _eventAggregator.SendMessage(new AuditEvent | ||
| { | ||
| DepartmentId = departmentId, | ||
| UserId = userId, | ||
| Type = type, | ||
| Before = before == null ? null : JsonConvert.SerializeObject(before), | ||
| After = after == null ? null : JsonConvert.SerializeObject(after), | ||
| Successful = true, | ||
| ServerName = Environment.MachineName | ||
| }); |
There was a problem hiding this comment.
Incomplete audit trail fails security and compliance requirements due to missing tamper-evident fields. Populate timestamp, actor.role, resource.id, trace_id, ip, and user_agent in the AuditEvent to ensure immutable, verifiable logging.
Kody rule violation: Emit tamper-evident audit logs with required fields
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 495 to 504:
Incomplete audit trail fails security and compliance requirements due to missing tamper-evident fields. Populate `timestamp`, `actor.role`, `resource.id`, `trace_id`, `ip`, and `user_agent` in the `AuditEvent` to ensure immutable, verifiable logging.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var credential in credentials.Where(item => !item.RevokedOn.HasValue)) | ||
| { | ||
| credential.RevokedOn = revokedOn; | ||
| await _credentialsRepository.UpdateAsync(credential, cancellationToken); |
There was a problem hiding this comment.
N+1 query performance degradation identified due to issuing database updates per credential inside a foreach loop. Batch the update into a single bulk operation or use ExecuteUpdateAsync to minimize database round-trips.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/UnitTrackingService.cs:
Line 472:
N+1 query performance degradation identified due to issuing database updates per credential inside a foreach loop. Batch the update into a single bulk operation or use `ExecuteUpdateAsync` to minimize database round-trips.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var unitLocation = ObjectSerialization.Deserialize<UnitLocationEvent>(message) | ||
| ?? throw new InvalidOperationException("Unit location queue message could not be deserialized."); | ||
|
|
||
| await UnitLocationEventQueueReceived.Invoke(unitLocation); |
There was a problem hiding this comment.
NullReferenceException vulnerability identified where the public UnitLocationEventQueueReceived delegate is invoked without a null check. Capture the delegate into a local variable and verify it is non-null before invoking it to prevent runtime crashes.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs:
Line 660:
NullReferenceException vulnerability identified where the public `UnitLocationEventQueueReceived` delegate is invoked without a null check. Capture the delegate into a local variable and verify it is non-null before invoking it to prevent runtime crashes.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var listeners = _asyncListeners.GetOrAdd( | ||
| typeof(T), | ||
| _ => new ConcurrentDictionary<Guid, Func<object, Task>>()); | ||
| listeners[token] = message => listener((T)message); |
There was a problem hiding this comment.
InvalidCastException risk identified due to a direct cast on an object parameter without type verification. Replace the direct cast with pattern matching or the as operator to ensure safe type evaluation before listener invocation.
Kody rule violation: Use safe type casting with as operator
Prompt for LLM
File Providers/Resgrid.Providers.Bus/EventAggregator.cs:
Line 48:
InvalidCastException risk identified due to a direct cast on an object parameter without type verification. Replace the direct cast with pattern matching or the `as` operator to ensure safe type evaluation before listener invocation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message)) | ||
| throw new InvalidOperationException("Unable to publish the Unit location realtime update."); |
There was a problem hiding this comment.
Transient RabbitMQ realtime-publish failures in unitLocationUpdatedTopicHandler now propagate through UnitLocationQueueLogic.ProcessUnitLocationQueueItem into the queue consumer's retry path, requeuing already-persisted messages and causing duplicate inserts. Swallow or log the publish failure in the handler instead of throwing, or wrap the SendMessageAsync call in AddUnitLocationAsync with a try/catch to prevent realtime side-effects from failing the committed primary persistence operation.
if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message))
Logging.LogError("Unable to publish the Unit location realtime update for department {DepartmentId}.", message.DepartmentId);Prompt for LLM
File Providers/Resgrid.Providers.Bus/OutboundEventProvider.cs:
Line 618 to 619:
Transient RabbitMQ realtime-publish failures in `unitLocationUpdatedTopicHandler` now propagate through `UnitLocationQueueLogic.ProcessUnitLocationQueueItem` into the queue consumer's retry path, requeuing already-persisted messages and causing duplicate inserts. Swallow or log the publish failure in the handler instead of throwing, or wrap the `SendMessageAsync` call in `AddUnitLocationAsync` with a try/catch to prevent realtime side-effects from failing the committed primary persistence operation.
Suggested Code:
if (!await _rabbitTopicProvider.UnitLocationUpdatedChanged(message))
Logging.LogError("Unable to publish the Unit location realtime update for department {DepartmentId}.", message.DepartmentId);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| .WithColumn("headername").AsCustom("citext").Nullable() | ||
| .WithColumn("basicusername").AsCustom("citext").Nullable() | ||
| .WithColumn("keyprefix").AsCustom("citext").NotNullable() | ||
| .WithColumn("secrethash").AsCustom("citext").NotNullable() |
There was a problem hiding this comment.
Case-insensitive text declaration on the secrethash column breaks cryptographic hash comparisons and authentication logic. Use AsCustom("text") instead of AsCustom("citext") to enforce exact byte-level hash equality.
Kody rule violation: Optimize string column types
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs:
Line 90:
Case-insensitive text declaration on the `secrethash` column breaks cryptographic hash comparisons and authentication logic. Use `AsCustom("text")` instead of `AsCustom("citext")` to enforce exact byte-level hash equality.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| CREATE UNIQUE INDEX IF NOT EXISTS ux_unitlocations_eventid | ||
| ON public.unitlocations (eventid) | ||
| WHERE eventid IS NOT NULL; |
There was a problem hiding this comment.
Database locking risk identified because creating a unique index without CONCURRENTLY acquires an ACCESS EXCLUSIVE lock on the unitlocations table. Use CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS to prevent blocking reads and writes during index creation on large tables.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Sql/EF0003_PopulateDocDb.sql:
Line 30 to 32:
Database locking risk identified because creating a unique index without `CONCURRENTLY` acquires an `ACCESS EXCLUSIVE` lock on the `unitlocations` table. Use `CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS` to prevent blocking reads and writes during index creation on large tables.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var query = _queryFactory.GetQuery<SelectUnitTrackingDevicesByUnitQuery>(); | ||
|
|
||
| return await WithConnectionAsync(connection => | ||
| connection.QueryAsync<UnitTrackingDevice>(query, parameters, _unitOfWork.Transaction)); |
There was a problem hiding this comment.
NullReferenceException vulnerability identified by accessing _unitOfWork.Transaction without a null guard inside a Dapper lambda. Capture the transaction into a local variable using _unitOfWork?.Transaction before passing it to the lambda to align with existing null-safe fallback paths.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/UnitTrackingDevicesRepository.cs:
Line 44:
NullReferenceException vulnerability identified by accessing `_unitOfWork.Transaction` without a null guard inside a Dapper lambda. Capture the transaction into a local variable using `_unitOfWork?.Transaction` before passing it to the lambda to align with existing null-safe fallback paths.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| <ProjectReference Include="..\..\Providers\Resgrid.Providers.Messaging\Resgrid.Providers.Messaging.csproj" /> | ||
| <ProjectReference Include="..\..\Repositories\Resgrid.Repositories.DataRepository\Resgrid.Repositories.DataRepository.csproj" /> | ||
| <ProjectReference Include="..\..\Repositories\Resgrid.Repositories.NoSqlRepository\Resgrid.Repositories.NoSqlRepository.csproj" /> | ||
| <ProjectReference Include="..\..\Web\Resgrid.Web\Resgrid.Web.csproj" /> |
There was a problem hiding this comment.
Architecture boundary violation identified by coupling the general test project directly to the presentation layer (Resgrid.Web). Move the tested logic into a service or domain object, or relocate web-layer integration tests to a dedicated assembly to maintain proper separation of concerns.
Kody rule violation: Enforce architecture boundaries and layering rules
Prompt for LLM
File Tests/Resgrid.Tests/Resgrid.Tests.csproj:
Line 58:
Architecture boundary violation identified by coupling the general test project directly to the presentation layer (`Resgrid.Web`). Move the tested logic into a service or domain object, or relocate web-layer integration tests to a dedicated assembly to maintain proper separation of concerns.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Assert | ||
| profiles.Select(profile => profile.Key) | ||
| .Should().BeEquivalentTo("generic-https", "traccar-forwarder"); |
There was a problem hiding this comment.
Inline magic strings representing a closed set of profile keys are brittle and resist refactoring. Define a static class or enum, such as UnitTrackingProfileKeys, to reference named members instead of raw literals.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File Tests/Resgrid.Tests/Services/UnitTrackingCatalogServiceTests.cs:
Line 24:
Inline magic strings representing a closed set of profile keys are brittle and resist refactoring. Define a static class or enum, such as `UnitTrackingProfileKeys`, to reference named members instead of raw literals.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!Schema.Table("units").Index("uq_units_departmentid_unitid").Exists()) | ||
| { | ||
| Execute.Sql(@" | ||
| CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_units_departmentid_unitid | ||
| ON units (departmentid, unitid);"); | ||
| } | ||
|
|
||
| if (!Schema.Table("units").Constraint("uq_units_departmentid_unitid").Exists()) | ||
| { | ||
| Execute.Sql(@" | ||
| ALTER TABLE units | ||
| ADD CONSTRAINT uq_units_departmentid_unitid UNIQUE USING INDEX uq_units_departmentid_unitid;"); | ||
| } |
There was a problem hiding this comment.
The existence guard if (!Schema.Table("units").Index("uq_units_departmentid_unitid").Exists()) only checks index name, not validity, so a partially-failed CREATE INDEX CONCURRENTLY build leaves an INVALID index that breaks migration re-runs — ADD CONSTRAINT ... UNIQUE USING INDEX then fails on the invalid index. Drop any existing invalid index (e.g. DROP INDEX IF EXISTS uq_units_departmentid_unitid when pg_index.indisvalid is false) before recreating to satisfy the migration's partial-apply safety guarantee.
// Drop a previously-failed (INVALID) concurrent build before (re)creating, otherwise
// ADD CONSTRAINT ... USING INDEX below fails because the index is invalid.
Execute.Sql(@"
DROP INDEX IF EXISTS uq_units_departmentid_unitid;");
Execute.Sql(@"
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_units_departmentid_unitid
ON units (departmentid, unitid);");Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs:
Line 32 to 44:
The existence guard `if (!Schema.Table("units").Index("uq_units_departmentid_unitid").Exists())` only checks index name, not validity, so a partially-failed `CREATE INDEX CONCURRENTLY` build leaves an INVALID index that breaks migration re-runs — `ADD CONSTRAINT ... UNIQUE USING INDEX` then fails on the invalid index. Drop any existing invalid index (e.g. `DROP INDEX IF EXISTS uq_units_departmentid_unitid` when `pg_index.indisvalid` is false) before recreating to satisfy the migration's partial-apply safety guarantee.
Suggested Code:
// Drop a previously-failed (INVALID) concurrent build before (re)creating, otherwise
// ADD CONSTRAINT ... USING INDEX below fails because the index is invalid.
Execute.Sql(@"
DROP INDEX IF EXISTS uq_units_departmentid_unitid;");
Execute.Sql(@"
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS uq_units_departmentid_unitid
ON units (departmentid, unitid);");
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 (2)
Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (1)
1351-1352: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse POST plus antiforgery protection for number provisioning.
ProvisionDefaultNumberAsyncremains a state-changing[HttpGet]. A logged-in department administrator can be induced to visit an attacker-controlled page that triggers provisioning. Convert this action to POST and add[ValidateAntiForgeryToken].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 1351 - 1352, Update the ProvisionDefaultNumberAsync action to use [HttpPost] instead of [HttpGet], and add [ValidateAntiForgeryToken] alongside its existing authorization attribute. Preserve its parameters and provisioning behavior.Core/Resgrid.Services/DeleteService.cs (1)
207-233: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCheck-then-act race when guarding against duplicate deletion requests.
GetPendingDeleteDepartmentQueueItemAsync(existence check) andEnqueuePendingDeleteDepartmentAsync(enqueue) are not executed atomically. Two concurrentDeleteDepartmentcalls for the same department can both pass the existing-request check before either enqueues, producing duplicateDeleteDepartmentqueue items. SinceQueueService.GetAllPendingDeleteDepartmentQueueItemsAsync(used by the worker) returns every uncompleted item for a department, both duplicates would be processed independently — duplicate admin reminder emails and a redundant/racing execution attempt againstDeleteDepartmentAndUsersAsync(the second would likely throw and be swallowed, but this pollutes the audit/notification trail).Consider guarding this with a DB-level uniqueness constraint (e.g., unique index on
SourceId+QueueTypewhereCompletedOn IS NULL) or a distributed lock/transaction around the check-and-enqueue sequence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DeleteService.cs` around lines 207 - 233, Make the duplicate-deletion guard in DeleteDepartment atomic so concurrent requests cannot both pass GetPendingDeleteDepartmentQueueItemAsync and enqueue separate items through EnqueuePendingDeleteDepartmentAsync. Prefer enforcing this at the database/queue layer with a uniqueness constraint for active entries sharing department SourceId and queue type, or use an equivalent distributed lock/transaction around the check-and-enqueue operation; preserve the existing no-duplicate result and downstream notification behavior.
🧹 Nitpick comments (1)
Core/Resgrid.Services/DeleteService.cs (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNew dependency added via constructor injection instead of Service Locator.
IAuditLogsRepositoryis added as a 24th constructor parameter, following the class's existing pattern of classic constructor injection. As per coding guidelines, dependencies should be resolved viaBootstrapper.GetKernel().Resolve<T>()inside constructors rather than constructor injection, and the number of injected dependencies should be kept small. This class already deviates significantly from that guideline; consider resolving_auditLogsRepositoryvia the Service Locator pattern (or, longer-term, decomposing this god-class) instead of growing the constructor further.As per coding guidelines, "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small".Also applies to: 42-49, 73-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DeleteService.cs` at line 40, Update DeleteService construction to stop adding IAuditLogsRepository as a constructor-injected parameter; resolve it inside the constructor using Bootstrapper.GetKernel().Resolve<IAuditLogsRepository>() and assign the result to _auditLogsRepository. Remove the corresponding constructor parameter and preserve the existing audit repository usage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/DeleteService.cs`:
- Around line 214-224: Update the audit event construction in the
DeleteDepartment flow so AuditEvent.Successful reflects whether
EnqueuePendingDeleteDepartmentAsync returned a non-null result, matching the
existing result == null failure check. Preserve the current audit fields and
event dispatch behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 2316-2330: Update the cancellation flow around
CancelPendingDepartmentDeletionRequest so it does not publish a successful
DeleteDepartmentRequestedCancelled AuditEvent when cancelledItem is null. Move
or condition the _eventAggregator.SendMessage call so the success audit event is
emitted only after a non-null cancellation result confirms the state changed
successfully; preserve the existing audit details for successful cancellations.
In `@Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs`:
- Around line 192-213: Update ProcessAuditQueueItem’s DeleteDepartmentRequested
and DeleteDepartmentRequestedCancelled branches to handle null or malformed
QueueItem deserialization before dereferencing properties. Preserve the existing
audit data for valid payloads; otherwise set auditLog.Data to "No Data" or
return failure so the event is not acknowledged as successfully processed.
---
Outside diff comments:
In `@Core/Resgrid.Services/DeleteService.cs`:
- Around line 207-233: Make the duplicate-deletion guard in DeleteDepartment
atomic so concurrent requests cannot both pass
GetPendingDeleteDepartmentQueueItemAsync and enqueue separate items through
EnqueuePendingDeleteDepartmentAsync. Prefer enforcing this at the database/queue
layer with a uniqueness constraint for active entries sharing department
SourceId and queue type, or use an equivalent distributed lock/transaction
around the check-and-enqueue operation; preserve the existing no-duplicate
result and downstream notification behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 1351-1352: Update the ProvisionDefaultNumberAsync action to use
[HttpPost] instead of [HttpGet], and add [ValidateAntiForgeryToken] alongside
its existing authorization attribute. Preserve its parameters and provisioning
behavior.
---
Nitpick comments:
In `@Core/Resgrid.Services/DeleteService.cs`:
- Line 40: Update DeleteService construction to stop adding IAuditLogsRepository
as a constructor-injected parameter; resolve it inside the constructor using
Bootstrapper.GetKernel().Resolve<IAuditLogsRepository>() and assign the result
to _auditLogsRepository. Remove the corresponding constructor parameter and
preserve the existing audit repository usage.
🪄 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 Plus
Run ID: 2f66c955-1693-42a9-84fa-e67f31d61214
📒 Files selected for processing (8)
.github/workflows/dotnet.ymlCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/DeleteService.csCore/Resgrid.Services/QueueService.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWorkers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Core/Resgrid.Model/AuditLogTypes.cs
- Core/Resgrid.Services/AuditService.cs
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0101_AddUnitTrackingPg.cs
| await WriteDepartmentDeletionExecutedAuditLogAsync(item, cancellationToken); | ||
| Logging.LogInfo($"DeleteService::Executing pending department deletion for DepartmentId {item.SourceId}, requested by UserId {item.QueuedByUserId} on {item.QueuedOn:u}, scheduled for {item.ToBeCompletedOn:u}"); | ||
|
|
||
| var result = await _deleteRepository.DeleteDepartmentAndUsersAsync(int.Parse(item.SourceId)); | ||
|
|
||
| item.CompletedOn = DateTime.UtcNow; | ||
| item.Data = "Department deletion executed by the system."; | ||
| var result2 = await _queueService.UpdateQueueItem(item, cancellationToken); | ||
| } | ||
| catch (Exception e) |
There was a problem hiding this comment.
Infinite retry loop triggered in the department-deletion execution failure path because the handler returns Failure without setting item.CompletedOn, causing GetAllPendingDeleteDepartmentQueueItemsAsync to re-attempt past-due items indefinitely and generate duplicate AuditLog rows. In the catch block, set a terminal state by assigning item.CompletedOn and calling UpdateQueueItem to stop the item from re-queuing.
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", int.Parse(item.SourceId));
// Without a terminal state the past-due item is re-queued and re-tried every poll
// (and re-inserts an audit row each time), since the queue query no longer filters
// out past-due items.
item.CompletedOn = DateTime.UtcNow;
item.Data = $"Department deletion failed: {e.Message}";
await _queueService.UpdateQueueItem(item, cancellationToken);
return DeleteDepartmentResults.Failure;
}Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 258 to 267:
Infinite retry loop triggered in the department-deletion execution failure path because the handler returns Failure without setting item.CompletedOn, causing GetAllPendingDeleteDepartmentQueueItemsAsync to re-attempt past-due items indefinitely and generate duplicate AuditLog rows. In the catch block, set a terminal state by assigning item.CompletedOn and calling UpdateQueueItem to stop the item from re-queuing.
Suggested Code:
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", int.Parse(item.SourceId));
// Without a terminal state the past-due item is re-queued and re-tried every poll
// (and re-inserts an audit row each time), since the queue query no longer filters
// out past-due items.
item.CompletedOn = DateTime.UtcNow;
item.Data = $"Department deletion failed: {e.Message}";
await _queueService.UpdateQueueItem(item, cancellationToken);
return DeleteDepartmentResults.Failure;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (Exception ex) | ||
| { | ||
| // A single bad recipient must not block notifications to the remaining admins. | ||
| Logging.LogException(ex, $"DeleteService::Failed to send department deletion email to UserId {adminUserId} for DepartmentId {item.SourceId}"); |
There was a problem hiding this comment.
Unstructured logging violation in the error log passes identifiers inside an interpolated message string. Use a message template with named parameters, such as LogException(ex, "Failed to send deletion email", new { adminUserId, departmentId = item.SourceId }), to ensure queryability as required by Rule 3.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 344:
Unstructured logging violation in the error log passes identifiers inside an interpolated message string. Use a message template with named parameters, such as LogException(ex, "Failed to send deletion email", new { adminUserId, departmentId = item.SourceId }), to ensure queryability as required by Rule 3.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await WriteDepartmentDeletionExecutedAuditLogAsync(item, cancellationToken); | ||
| Logging.LogInfo($"DeleteService::Executing pending department deletion for DepartmentId {item.SourceId}, requested by UserId {item.QueuedByUserId} on {item.QueuedOn:u}, scheduled for {item.ToBeCompletedOn:u}"); | ||
|
|
||
| var result = await _deleteRepository.DeleteDepartmentAndUsersAsync(int.Parse(item.SourceId)); |
There was a problem hiding this comment.
FormatException risk from int.Parse on item.SourceId violates Rule 29 preferences for IO-derived string conversions. Use int.TryParse(item.SourceId, out var deptId) and handle the failure case explicitly.
Kody rule violation: Use TryParse for string conversions
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 261:
FormatException risk from int.Parse on item.SourceId violates Rule 29 preferences for IO-derived string conversions. Use int.TryParse(item.SourceId, out var deptId) and handle the failure case explicitly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| item.ReminderCount = 2; | ||
| reminderSent = true; | ||
| } | ||
| else if (now >= item.ToBeCompletedOn.Value.AddDays(-14) && item.ReminderCount < 1) |
There was a problem hiding this comment.
Inline magic numbers -14 and 1 violate Rule 10 requirements for named constants. Define constants such as FourteenDayReminderWindowDays = 14 and FirstReminderCount = 1 to eliminate magic numbers.
Kody rule violation: Replace magic numbers with named constants
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 293:
Inline magic numbers -14 and 1 violate Rule 10 requirements for named constants. Define constants such as FourteenDayReminderWindowDays = 14 and FirstReminderCount = 1 to eliminate magic numbers.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /* | ||
| * Deletion is scheduled for today (final reminder). | ||
| */ | ||
| await SendDeleteDepartmentReminderToAllAdminsAsync(item); |
There was a problem hiding this comment.
Code duplication across three branches repeats the send-reminder, set-count, and set-flag sequence. Rule 15 requires extracting duplicated statement sequences into a named function like SendReminderAndMark(item, level).
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 280:
Code duplication across three branches repeats the send-reminder, set-count, and set-flag sequence. Rule 15 requires extracting duplicated statement sequences into a named function like SendReminderAndMark(item, level).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<List<QueueItem>> GetAllPendingDeleteDepartmentQueueItemsAsync() | ||
| { | ||
| var allItems = await _queueItemsRepository.GetAllAsync(); | ||
|
|
||
| // No ToBeCompletedOn filter: filtering to future-dated items makes past-due requests | ||
| // unreachable, so the deletion in HandlePendingDepartmentDeletionRequestAsync would | ||
| // never execute. The handler decides between reminders and execution. | ||
| var depItems = allItems.Where(x => | ||
| x.ToBeCompletedOn > DateTime.UtcNow && x.QueueType == (int)QueueTypes.DeleteDepartment && x.CompletedOn == null).ToList(); | ||
| x.QueueType == (int)QueueTypes.DeleteDepartment && x.CompletedOn == null).ToList(); | ||
|
|
||
| return depItems; | ||
| } |
There was a problem hiding this comment.
Permanent stuck item loop identified in HandlePendingDepartmentDeletionRequestAsync where removing the ToBeCompletedOn filter causes past-due delete items to be re-queried indefinitely. The handler returns UnAuthorized without setting item.CompletedOn, so set item.CompletedOn, update item.Data, and call UpdateQueueItem to finalize the item when the authorization re-check fails.
// DeleteService.HandlePendingDepartmentDeletionRequestAsync — on auth failure for a due item, finalize it so it stops looping:
if (!await _authorizationService.CanUserDeleteDepartmentAsync(item.QueuedByUserId, int.Parse(item.SourceId)))
{
if (now >= item.ToBeCompletedOn.Value)
{
item.CompletedOn = DateTime.UtcNow;
item.Data = $"Department deletion abandoned: requesting user {item.QueuedByUserId} is no longer the managing user.";
await _queueService.UpdateQueueItem(item, cancellationToken);
}
return DeleteDepartmentResults.UnAuthorized;
}Prompt for LLM
File Core/Resgrid.Services/QueueService.cs:
Line 61 to 72:
Permanent stuck item loop identified in HandlePendingDepartmentDeletionRequestAsync where removing the ToBeCompletedOn filter causes past-due delete items to be re-queried indefinitely. The handler returns UnAuthorized without setting item.CompletedOn, so set item.CompletedOn, update item.Data, and call UpdateQueueItem to finalize the item when the authorization re-check fails.
Suggested Code:
// DeleteService.HandlePendingDepartmentDeletionRequestAsync — on auth failure for a due item, finalize it so it stops looping:
if (!await _authorizationService.CanUserDeleteDepartmentAsync(item.QueuedByUserId, int.Parse(item.SourceId)))
{
if (now >= item.ToBeCompletedOn.Value)
{
item.CompletedOn = DateTime.UtcNow;
item.Data = $"Department deletion abandoned: requesting user {item.QueuedByUserId} is no longer the managing user.";
await _queueService.UpdateQueueItem(item, cancellationToken);
}
return DeleteDepartmentResults.UnAuthorized;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // No ToBeCompletedOn filter: a past-due request that hasn't executed yet is still | ||
| // pending and must remain visible (and cancellable) until the worker completes it. | ||
| var depItem = allItems.Where(x => | ||
| x.SourceId == departmentId.ToString() && x.QueueType == (int)QueueTypes.DeleteDepartment && x.CompletedOn == null) |
There was a problem hiding this comment.
Duplicated LINQ predicate vulnerability exists in both GetPendingDeleteDepartmentQueueItemAsync (line 54) and GetAllPendingDeleteDepartmentQueueItemsAsync (line 69). Extract the shared predicate x.QueueType == (int)QueueTypes.DeleteDepartment && x.CompletedOn == null into a reusable Expression<Func<QueueItem, bool>> to prevent error-prone synchronous updates.
Kody rule violation: Extract common query logic
Prompt for LLM
File Core/Resgrid.Services/QueueService.cs:
Line 54:
Duplicated LINQ predicate vulnerability exists in both GetPendingDeleteDepartmentQueueItemAsync (line 54) and GetAllPendingDeleteDepartmentQueueItemsAsync (line 69). Extract the shared predicate `x.QueueType == (int)QueueTypes.DeleteDepartment && x.CompletedOn == null` into a reusable Expression<Func<QueueItem, bool>> to prevent error-prone synchronous updates.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var profile = await _userProfileService.GetProfileByUserIdAsync(UserId); | ||
|
|
||
| var cancelledItem = await _queueService.CancelPendingDepartmentDeletionRequest(DepartmentId, profile.FullName.AsFirstNameLastName, cancellationToken); |
There was a problem hiding this comment.
Unhandled exception vulnerability in the awaited CancelPendingDepartmentDeletionRequest call allows queue service failures to bypass audit event emission. Wrap the call in a try/catch block, log the error with DepartmentId and UserId context, and handle the failure explicitly.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs:
Line 2318:
Unhandled exception vulnerability in the awaited CancelPendingDepartmentDeletionRequest call allows queue service failures to bypass audit event emission. Wrap the call in a try/catch block, log the error with DepartmentId and UserId context, and handle the failure explicitly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var profile = await _userProfileService.GetProfileByUserIdAsync(UserId); | ||
|
|
||
| var cancelledItem = await _queueService.CancelPendingDepartmentDeletionRequest(DepartmentId, profile.FullName.AsFirstNameLastName, cancellationToken); |
There was a problem hiding this comment.
NullReferenceException risk identified when profile is dereferenced without a null check before accessing .FullName. Add a null check after fetching the profile via GetProfileByUserIdAsync to return NotFound or BadRequest before accessing its properties.
Kody rule violation: Add null checks to prevent NullReferenceException
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs:
Line 2318:
NullReferenceException risk identified when profile is dereferenced without a null check before accessing .FullName. Add a null check after fetching the profile via GetProfileByUserIdAsync to return NotFound or BadRequest before accessing its properties.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var profile = await _userProfileService.GetProfileByUserIdAsync(UserId); | ||
|
|
||
| var cancelledItem = await _queueService.CancelPendingDepartmentDeletionRequest(DepartmentId, profile.FullName.AsFirstNameLastName, cancellationToken); |
There was a problem hiding this comment.
Unhandled external service call vulnerability in the queue cancellation logic prevents infrastructure failures from being mapped to application-level errors. Wrap the call in a try/catch block, log the exception with DepartmentId context, and return an appropriate IActionResult.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs:
Line 2318:
Unhandled external service call vulnerability in the queue cancellation logic prevents infrastructure failures from being mapped to application-level errors. Wrap the call in a try/catch block, log the exception with DepartmentId context, and return an appropriate IActionResult.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!String.IsNullOrWhiteSpace(auditEvent.After)) | ||
| { | ||
| var deleteRequestedQueueItem = JsonConvert.DeserializeObject<QueueItem>(auditEvent.After); | ||
| auditLog.Data = $"QueueItemId: {deleteRequestedQueueItem.QueueItemId}; Scheduled deletion (UTC): {deleteRequestedQueueItem.ToBeCompletedOn:u}; RequestedByUserId: {deleteRequestedQueueItem.QueuedByUserId}"; |
There was a problem hiding this comment.
NullReferenceException risk triggered by accessing properties of deleteRequestedQueueItem without a null check. Null-check the JsonConvert.DeserializeObject result before dereferencing, such as if (deleteRequestedQueueItem == null) { auditLog.Data = "No Data"; break; }.
Kody rule violation: Add null checks before accessing properties
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:
Line 195:
NullReferenceException risk triggered by accessing properties of deleteRequestedQueueItem without a null check. Null-check the JsonConvert.DeserializeObject result before dereferencing, such as `if (deleteRequestedQueueItem == null) { auditLog.Data = "No Data"; break; }`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| auditLog.Data = "No Data"; | ||
| if (!String.IsNullOrWhiteSpace(auditEvent.After)) | ||
| { | ||
| var deleteRequestedQueueItem = JsonConvert.DeserializeObject<QueueItem>(auditEvent.After); |
There was a problem hiding this comment.
Unhandled JSON deserialization risk from directly parsing auditEvent.After allows malformed JSON to throw JsonReaderException or JsonSerializationException. Wrap the deserialization in a try/catch block or use JsonSerializer with error handling, and validate the parsed object before use.
Kody rule violation: Always validate JSON parsing
Prompt for LLM
File Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:
Line 194:
Unhandled JSON deserialization risk from directly parsing auditEvent.After allows malformed JSON to throw JsonReaderException or JsonSerializationException. Wrap the deserialization in a try/catch block or use JsonSerializer with error handling, and validate the parsed object before use.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
| // Set a terminal state: the pending-queue query no longer filters out past-due | ||
| // items, so without this the item is re-queued and re-tried every poll, writing | ||
| // a duplicate execution audit row and exception email each time. | ||
| await SetTerminalQueueItemStateAsync(item, $"Department deletion failed: {e.Message}", cancellationToken); |
There was a problem hiding this comment.
Improper terminal state assignment permanently abandons scheduled-and-past-due department deletions on any exception, including transient DB timeouts/deadlocks, because the 'executed' audit row is written at line 273 before DeleteDepartmentAndUsersAsync at line 276. Track a bounded retry/error count on the QueueItem and only call SetTerminalQueueItemStateAsync once a max-attempt threshold is exceeded, otherwise leave CompletedOn null so the next poll retries.
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", departmentId);
// Only abandon after a bounded number of attempts; a transient DB timeout/deadlock
// is recoverable and should not permanently abort a scheduled department deletion.
const int maxAttempts = 5;
item.Data = $"Department deletion attempt failed: {e.Message}";
if (!int.TryParse(item.AttemptCount?.ToString(), out var attempts))
attempts = 0;
attempts++;
if (attempts >= maxAttempts)
await SetTerminalQueueItemStateAsync(item, $"Department deletion permanently failed after {maxAttempts} attempts: {e.Message}", cancellationToken);
else
await _queueService.UpdateQueueItem(item, cancellationToken); // stays pending, retried next poll
return DeleteDepartmentResults.Failure;
}Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 290:
Improper terminal state assignment permanently abandons scheduled-and-past-due department deletions on any exception, including transient DB timeouts/deadlocks, because the 'executed' audit row is written at line 273 before `DeleteDepartmentAndUsersAsync` at line 276. Track a bounded retry/error count on the QueueItem and only call `SetTerminalQueueItemStateAsync` once a max-attempt threshold is exceeded, otherwise leave `CompletedOn` null so the next poll retries.
Suggested Code:
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", departmentId);
// Only abandon after a bounded number of attempts; a transient DB timeout/deadlock
// is recoverable and should not permanently abort a scheduled department deletion.
const int maxAttempts = 5;
item.Data = $"Department deletion attempt failed: {e.Message}";
if (!int.TryParse(item.AttemptCount?.ToString(), out var attempts))
attempts = 0;
attempts++;
if (attempts >= maxAttempts)
await SetTerminalQueueItemStateAsync(item, $"Department deletion permanently failed after {maxAttempts} attempts: {e.Message}", cancellationToken);
else
await _queueService.UpdateQueueItem(item, cancellationToken); // stays pending, retried next poll
return DeleteDepartmentResults.Failure;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /* | ||
| * You have a pending department deletion request, it is within 10 days out and we have no yet sent a reminder. | ||
| */ | ||
| Logging.LogError($"DeleteService::Pending department deletion QueueItemId {item.QueueItemId} has a malformed SourceId '{item.SourceId}'; setting a terminal state so it is not retried."); |
There was a problem hiding this comment.
Interpolated log message bakes identifiers into the message text, violating Rule 3 and preventing queryability by log-analysis tools. Pass a template with named placeholders and a structured payload, such as Logging.LogError("Pending department deletion has malformed SourceId", new { QueueItemId = item.QueueItemId, SourceId = item.SourceId });.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 239:
Interpolated log message bakes identifiers into the message text, violating Rule 3 and preventing queryability by log-analysis tools. Pass a template with named placeholders and a structured payload, such as `Logging.LogError("Pending department deletion has malformed SourceId", new { QueueItemId = item.QueueItemId, SourceId = item.SourceId });`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
| var now = DateTime.UtcNow; | ||
|
|
||
| if (!await _authorizationService.CanUserDeleteDepartmentAsync(item.QueuedByUserId, departmentId)) |
There was a problem hiding this comment.
Unguarded awaited operation allows an infrastructure failure in the authorization-service call to propagate and crash queue item processing, violating Rule 1 mandates. Wrap the call in a try/catch, log the context on failure, and return an appropriate result.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 250:
Unguarded awaited operation allows an infrastructure failure in the authorization-service call to propagate and crash queue item processing, violating Rule 1 mandates. Wrap the call in a try/catch, log the context on failure, and return an appropriate result.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| } | ||
| catch (Exception e) | ||
| { | ||
| Logging.LogException(e); |
There was a problem hiding this comment.
Audit-log duplication vulnerability in the bounded-retry catch block (lines 286-315) occurs when a committed delete (line 271) is followed by a failed persist at UpdateQueueItem (line 282). Implement a deleteSucceeded flag or isolate UpdateQueueItem(item) in a separate try block to bypass the deletion-retry path and prevent duplicate audit-log rows from WriteDepartmentDeletionExecutedAuditLogAsync.
var deleteSucceeded = false;
try
{
var result = await _deleteRepository.DeleteDepartmentAndUsersAsync(departmentId);
deleteSucceeded = true;
// ... audit log write (self-catching) ...
item.CompletedOn = DateTime.UtcNow;
item.Data = "Department deletion executed by the system.";
var result2 = await _queueService.UpdateQueueItem(item, cancellationToken);
}
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", departmentId);
if (deleteSucceeded)
{
// Delete committed but the queue-item persist failed; do NOT retry the deletion
// (it is already done) — just keep trying to persist the completed state.
try { await _queueService.UpdateQueueItem(item, cancellationToken); }
catch (Exception updateEx) { Logging.LogException(updateEx, $"DeleteService::Failed to persist completion for QueueItemId {item.QueueItemId}"); }
return DeleteDepartmentResults.NoFailure;
}
const int maxAttempts = 5;
item.AttemptCount += 1;
if (item.AttemptCount >= maxAttempts)
{
await SetTerminalQueueItemStateAsync(item, $"Department deletion permanently failed after {item.AttemptCount} attempts: {e.Message}", cancellationToken);
}
else
{
item.Data = $"Department deletion attempt {item.AttemptCount} failed: {e.Message}";
await _queueService.UpdateQueueItem(item, cancellationToken);
}
return DeleteDepartmentResults.Failure;
}Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 286:
Audit-log duplication vulnerability in the bounded-retry catch block (lines 286-315) occurs when a committed delete (line 271) is followed by a failed persist at `UpdateQueueItem` (line 282). Implement a `deleteSucceeded` flag or isolate `UpdateQueueItem(item)` in a separate try block to bypass the deletion-retry path and prevent duplicate audit-log rows from `WriteDepartmentDeletionExecutedAuditLogAsync`.
Suggested Code:
var deleteSucceeded = false;
try
{
var result = await _deleteRepository.DeleteDepartmentAndUsersAsync(departmentId);
deleteSucceeded = true;
// ... audit log write (self-catching) ...
item.CompletedOn = DateTime.UtcNow;
item.Data = "Department deletion executed by the system.";
var result2 = await _queueService.UpdateQueueItem(item, cancellationToken);
}
catch (Exception e)
{
Logging.LogException(e);
Logging.SendExceptionEmail(e, "DeleteDepartment", departmentId);
if (deleteSucceeded)
{
// Delete committed but the queue-item persist failed; do NOT retry the deletion
// (it is already done) — just keep trying to persist the completed state.
try { await _queueService.UpdateQueueItem(item, cancellationToken); }
catch (Exception updateEx) { Logging.LogException(updateEx, $"DeleteService::Failed to persist completion for QueueItemId {item.QueueItemId}"); }
return DeleteDepartmentResults.NoFailure;
}
const int maxAttempts = 5;
item.AttemptCount += 1;
if (item.AttemptCount >= maxAttempts)
{
await SetTerminalQueueItemStateAsync(item, $"Department deletion permanently failed after {item.AttemptCount} attempts: {e.Message}", cancellationToken);
}
else
{
item.Data = $"Department deletion attempt {item.AttemptCount} failed: {e.Message}";
await _queueService.UpdateQueueItem(item, cancellationToken);
}
return DeleteDepartmentResults.Failure;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Persisting the retry state failed; the item stays pending and is retried | ||
| // next poll regardless, but don't let this mask the original exception or | ||
| // abort the worker's processing of other pending items. | ||
| Logging.LogException(updateEx, $"DeleteService::Failed to persist retry state for department deletion QueueItemId {item.QueueItemId}"); |
There was a problem hiding this comment.
Unstructured logging in the error log embeds the operation name and QueueItemId in an interpolated message string, violating Rule [3] requirements for filterable, indexable logs. Pass structured parameters, such as new { op = "DeleteDepartment", queueItemId = item.QueueItemId, departmentId }, to the Logging.LogException method.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 313:
Unstructured logging in the error log embeds the operation name and `QueueItemId` in an interpolated message string, violating Rule [3] requirements for filterable, indexable logs. Pass structured parameters, such as `new { op = "DeleteDepartment", queueItemId = item.QueueItemId, departmentId }`, to the `Logging.LogException` method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public override void Down() | ||
| { | ||
|
|
||
| } |
There was a problem hiding this comment.
Irreversible database migration in M0102_AddAttemptCountToQueueItems.cs leaves the AttemptCount column orphaned during a rollback, violating Rule 126. Add Delete.Column("AttemptCount").FromTable("QueueItems") inside the Down() method to cleanly reverse the schema change made in Up().
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.Migrations/Migrations/M0102_AddAttemptCountToQueueItems.cs:
Line 15 to 18:
Irreversible database migration in M0102_AddAttemptCountToQueueItems.cs leaves the `AttemptCount` column orphaned during a rollback, violating Rule 126. Add `Delete.Column("AttemptCount").FromTable("QueueItems")` inside the `Down()` method to cleanly reverse the schema change made in `Up()`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
3 participants
Description
This PR implements comprehensive hardware GPS unit tracking support (RG-T127), enabling departments to bind physical hardware trackers or forwarding services to Units and receive live position data through authenticated HTTPS endpoints.
Key additions
Device & credential management
UnitTrackingDevicesandUnitTrackingCredentialsdata models with full database migrations (SQL Server and PostgreSQL)no-storecaching headers; stored secrets are never returned by APIsPosition ingestion pipeline
/api/v4/unit-trackers/{id}/positionsand capability-path variant) accepting single or batched JSON position payloadsLocation resolution and status
UnitLocationSourceResolverthat selects the freshest, highest-priority location across hardware and mobile-app sources, with configurable staleness thresholds and mobile fallback behaviorUnitTrackingStatusServicecomputing effective device status (Online, Stale, Error, Disabled, NeverSeen)UnitsLocationandUnitLocationEventdocuments with source metadata and rich telemetry (satellites, HDOP, battery, ignition, alarm codes, etc.)Web UI and API
User/UnitTracking) with views for listing, creating, editing, and managing tracker bindings, credentials, and a JSON preview tool (non-production only)UnitTrackingDevicesController) for programmatic lifecycle management with authorization scoped to Unit view/update permissionsSecurity and observability
Infrastructure improvements
Inserted/Duplicatestatus to suppress duplicate realtime eventsSummary by CodeRabbit