Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds end-to-end UTF-8 text sanitization for PostgreSQL safety. It introduces a configurable sanitizer utility that removes forbidden characters and repairs encoding anomalies, integrates sanitization at data layer write points, provides a keyset-paginated repository for discovering and cleaning text columns across the database, and implements a scheduled nightly worker and on-demand CLI command for batch cleanup. Existing migrations are made idempotent to prevent re-run failures. ChangesUTF-8 Sanitization and Cleanup Worker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0074_AddIsPermanentFailureToWeatherAlertSourcesPg.cs (1)
19-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Down()should be existence-guarded to match renumber-safeUp().Because
Up()may skip when the column already exists, unconditionalDown()can dropweatheralertsources.ispermanentfailureeven when migration 74 did not add it.🤖 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.MigrationsPg/Migrations/M0074_AddIsPermanentFailureToWeatherAlertSourcesPg.cs` around lines 19 - 21, Wrap the unconditional deletion in Down() with a presence check that mirrors Up(): before calling Delete.Column("ispermanentfailure").FromTable("weatheralertsources"), check Schema.Table("weatheralertsources").Column("ispermanentfailure").Exists() and only delete when true so Down() will no-op when the column wasn't added by this migration; update the Down() method accordingly to perform the guarded deletion.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0070_ChatbotDepartmentConfigColumnsPg.cs (1)
37-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Down()is not idempotency-safe relative to guardedUp().
Up()can legitimately skip column creation, butDown()always drops all columns. On databases where these columns pre-existed (e.g., renumber/partial-history path), rollback of migration 70 can remove schema it did not introduce.Suggested hardening
public override void Down() { - Delete.Column("LlmApiEndpoint".ToLower()) - .Column("LlmApiKey".ToLower()) - .Column("LlmModelName".ToLower()) - .Column("MessagesPerUserPerMinute".ToLower()) - .Column("MessagesPerDepartmentPerMinute".ToLower()) - .Column("RequireLinkingConfirmation".ToLower()) - .Column("ProactiveNotificationsEnabled".ToLower()) - .FromTable("ChatbotDepartmentConfigs".ToLower()); + var table = "ChatbotDepartmentConfigs".ToLower(); + if (Schema.Table(table).Column("LlmApiEndpoint".ToLower()).Exists()) + Delete.Column("LlmApiEndpoint".ToLower()).FromTable(table); + if (Schema.Table(table).Column("LlmApiKey".ToLower()).Exists()) + Delete.Column("LlmApiKey".ToLower()).FromTable(table); + if (Schema.Table(table).Column("LlmModelName".ToLower()).Exists()) + Delete.Column("LlmModelName".ToLower()).FromTable(table); + if (Schema.Table(table).Column("MessagesPerUserPerMinute".ToLower()).Exists()) + Delete.Column("MessagesPerUserPerMinute".ToLower()).FromTable(table); + if (Schema.Table(table).Column("MessagesPerDepartmentPerMinute".ToLower()).Exists()) + Delete.Column("MessagesPerDepartmentPerMinute".ToLower()).FromTable(table); + if (Schema.Table(table).Column("RequireLinkingConfirmation".ToLower()).Exists()) + Delete.Column("RequireLinkingConfirmation".ToLower()).FromTable(table); + if (Schema.Table(table).Column("ProactiveNotificationsEnabled".ToLower()).Exists()) + Delete.Column("ProactiveNotificationsEnabled".ToLower()).FromTable(table); }🤖 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.MigrationsPg/Migrations/M0070_ChatbotDepartmentConfigColumnsPg.cs` around lines 37 - 46, The Down() method in M0070_ChatbotDepartmentConfigColumnsPg unconditionally drops columns that Up() may have skipped, risking removal of pre-existing columns; modify Down() to only drop each column if it exists. For each column name used currently ("LlmApiEndpoint", "LlmApiKey", "LlmModelName", "MessagesPerUserPerMinute", "MessagesPerDepartmentPerMinute", "RequireLinkingConfirmation", "ProactiveNotificationsEnabled") wrap the Delete.Column(...).FromTable(...) calls in an existence check (e.g., use Schema.Table("ChatbotDepartmentConfigs".ToLower()).Column(columnName.ToLower()).Exists() or your repository's ColumnExists helper) and only call Delete when the check returns true; keep this logic inside the Down() method of class M0070_ChatbotDepartmentConfigColumnsPg.Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs (1)
153-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback can drop objects not created by this migration run.
Down()is unconditional whileUp()may no-op for already-existing tables. In renumber/partial-history scenarios, rolling back migration 71 can delete pre-existing feature-flag tables and data.🤖 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.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs` around lines 153 - 170, The Down() method unconditionally drops feature-flag tables and FKs which can remove objects that pre-existed this migration; update Down() in M0071_AddingFeatureTogglesPg to check existence before removing: for each Delete.Table("FeatureFlags".ToLower()) and each Delete.ForeignKey("FK_FeatureFlagUsages_Departments".ToLower()) / FK names used, wrap the delete in a conditional that verifies the table or constraint exists (e.g., use Schema.Table(...).Exists() or the migration framework's IfExists helpers) so rollbacks only remove objects created by this migration.Repositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.cs (1)
237-243:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis only fixes one Dapper path in
IdentityUserRepository.Because this repository bypasses
RepositoryBase<T>, sanitizingInsertAsyncalone still leavesUpdateAsyncplus raw string-bearing calls likeInsertClaimsAsync,InsertLoginInfoAsync,SetTokenAsync, andUpdateClaimAsyncsending unsanitized values straight to Dapper. PostgreSQL failures will still leak through those paths.🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.cs` around lines 237 - 243, The InsertAsync path added Utf8WriteGuard.Sanitize(user) but other Dapper paths in IdentityUserRepository still pass raw strings to the DB; update all methods that build DynamicParameters or send raw string values—specifically UpdateAsync, InsertClaimsAsync, InsertLoginInfoAsync, SetTokenAsync, UpdateClaimAsync (and any helper methods used to build queries/parameters)—to sanitize inputs before creating DynamicParameters or executing queries (e.g., call Utf8WriteGuard.Sanitize(user) or Utf8WriteGuard.Sanitize for individual string fields), or factor a small private helper (SanitizeUserFields or SanitizeParameters) and call it from each of those methods so all Dapper calls use sanitized values.
🧹 Nitpick comments (2)
Workers/Resgrid.Workers.Console/Program.cs (1)
426-428: ⚡ Quick winConsider logging when the configured hour is out of range.
The hour is silently clamped to 4 when
SystemBehaviorConfig.Utf8CleanupHourUtcis outside 0–23. Consider logging a warning so operators know the configured value was invalid and the default is being used.📝 Suggested enhancement
if (SystemBehaviorConfig.Utf8CleanupEnabled) { - var utf8CleanupHour = SystemBehaviorConfig.Utf8CleanupHourUtc >= 0 && SystemBehaviorConfig.Utf8CleanupHourUtc <= 23 - ? SystemBehaviorConfig.Utf8CleanupHourUtc - : 4; + var configuredHour = SystemBehaviorConfig.Utf8CleanupHourUtc; + var utf8CleanupHour = configuredHour >= 0 && configuredHour <= 23 + ? configuredHour + : 4; + + if (configuredHour < 0 || configuredHour > 23) + { + _logger.Log(LogLevel.Warning, + $"Configured UTF-8 cleanup hour ({configuredHour}) is out of range [0-23]. Defaulting to hour 4 UTC."); + } _logger.Log(LogLevel.Information, "Scheduling UTF-8 Data Cleanup");🤖 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 `@Workers/Resgrid.Workers.Console/Program.cs` around lines 426 - 428, The code silently clamps SystemBehaviorConfig.Utf8CleanupHourUtc into utf8CleanupHour defaulting to 4 when out of range; update the block around utf8CleanupHour to log a warning when SystemBehaviorConfig.Utf8CleanupHourUtc is <0 or >23 so operators know the configured value was invalid and the default is used—use the existing application logger (e.g., processLogger or the project's logger instance) to emit a clear warning that includes the invalid value and the fallback hour, then keep assigning utf8CleanupHour as currently implemented.Repositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.cs (1)
34-37: ⚡ Quick winConstructor should resolve dependency via
Bootstrapper.GetKernel().Resolve<T>()to match project conventionThis repository currently uses constructor injection for
IConnectionProvider. Please align with the repository-wide service-locator constructor pattern for consistency.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 `@Repositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.cs` around lines 34 - 37, Change the constructor of Utf8MaintenanceRepository to stop taking an IConnectionProvider parameter and instead resolve the dependency inside the constructor using Bootstrapper.GetKernel().Resolve<IConnectionProvider>(), assigning the result to the existing _connectionProvider field; update any constructor signature references and callers accordingly so the repository uses the project’s service-locator pattern rather than constructor injection.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.Framework/Text/Utf8Sanitizer.cs`:
- Around line 151-152: The sanitizer currently treats the entire C1 control
range (U+0080–U+009F) as dirty in Utf8Sanitizer (see the check using
C1RangeStart/C1RangeEnd), which can corrupt valid UTF-8; change the default
safety path to stop classifying C1 controls as unsafe—only mark NUL and unpaired
surrogates as dirty by default—and make the CP-1252 mojibake repair logic opt-in
(e.g., add a boolean flag or separate method to enable CP1252 remapping). Update
the analogous checks referenced around the other occurrences (the block at lines
181–187) to follow the same pattern so CP-1252 handling is only applied when
explicitly requested.
In `@Core/Resgrid.Model/Repositories/IUtf8MaintenanceRepository.cs`:
- Around line 26-27: The cursor-as-string breaks keyset queries for non-text PK
types; update the contract by restricting Utf8 text maintenance to tables whose
primary key column type is text/citext and enforce this in
GetTextColumnTargetsAsync (reject/skip targets with PK types like uuid/int), and
document/reflect that Utf8TextRow.Key and
IUtf8MaintenanceRepository.GetRowBatchAsync expect a text PK; also add
validation where repositories are selected so queries using WHERE {pk} >
`@lastKey` or WHERE {pk} = `@rg_key` only run against confirmed text PKs.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`:
- Around line 14-150: The migration currently skips creating foreign keys and
indexes when a table already exists, which leaves partial schema state if a
prior run created the table but failed before adding constraints; update
M0071_AddingFeatureTogglesPg.cs to separate table creation from constraint/index
creation: for each logical group (FeatureFlags, FeatureFlagOverrides,
FeatureFlagTargetingRules, FeatureFlagPrerequisites, FeatureFlagUsages) always
run checks that create missing FKs and Indexes even when the table
exists—specifically ensure Create.ForeignKey calls for
FK_FeatureFlagOverrides_FeatureFlags, FK_FeatureFlagOverrides_Departments,
FK_FeatureFlagTargetingRules_FeatureFlags,
FK_FeatureFlagPrerequisites_FeatureFlags,
FK_FeatureFlagPrerequisites_RequiredFeatureFlag,
FK_FeatureFlagUsages_FeatureFlags, FK_FeatureFlagUsages_Departments and
Create.Index calls for UX_FeatureFlags_FlagKey,
UX_FeatureFlagOverrides_Flag_Department, IX_FeatureFlagTargetingRules_Flag,
IX_FeatureFlagPrerequisites_Flag, IX_FeatureFlagUsages_Flag_Date are executed
conditionally (e.g., if the FK/index does not exist) regardless of whether
Create.Table was skipped so rerunning the migration repairs missing
constraints/indexes.
In
`@Repositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.cs`:
- Around line 292-310: The current UPDATE-then-INSERT in
Utf8MaintenanceRepository (using updateSql/insertSql, ProgressTable and the
'progress' parameter) is race-prone; replace it with a single atomic upsert or
wrap in a transactional lock: for PostgreSQL use "INSERT ... ON
CONFLICT(tablename) DO UPDATE SET ..." (merge the
lastprocessedkey/lastcompletedutc/rowsscanned/rowsfixed/updatedonutc), for SQL
Server use a MERGE statement or perform the UPDATE/INSERT inside a transaction
with an appropriate lock hint (e.g. UPDLOCK/HOLDLOCK on the ProgressTable) so
only one worker can insert when no row exists; keep using
_connectionProvider.Create() and ExecuteAsync/CommandDefinition but execute the
single upsert command (or the transaction-wrapped sequence) instead of the
current affected==0 insert branch.
In `@Tools/Resgrid.Console/Commands/CleanUtf8Command.cs`:
- Around line 39-43: Replace the manual logger.LogError calls in the catch block
of CleanUtf8Command (the catch in CleanUtf8Command.cs where it currently logs
"There was an error running the UTF-8 data cleanup..." and
logger.LogError(ex.ToString())) with a call to
Resgrid.Framework.Logging.LogException(ex, "There was an error running the UTF-8
data cleanup") so the exception is logged with caller file/member/line metadata;
remove the two logger.LogError calls and ensure the LogException invocation is
used instead (and add the optional extraMessage string exactly as above).
In `@Workers/Resgrid.Workers.Console/Tasks/Utf8CleanupTask.cs`:
- Line 16: Change the public mutable logger field to a private readonly field by
updating the declaration of _logger to private readonly ILogger _logger; and
ensure the Utf8CleanupTask constructor assigns this._logger so the field is
initialized once; this preserves encapsulation and prevents external
modification of the ILogger instance.
---
Outside diff comments:
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0070_ChatbotDepartmentConfigColumnsPg.cs`:
- Around line 37-46: The Down() method in M0070_ChatbotDepartmentConfigColumnsPg
unconditionally drops columns that Up() may have skipped, risking removal of
pre-existing columns; modify Down() to only drop each column if it exists. For
each column name used currently ("LlmApiEndpoint", "LlmApiKey", "LlmModelName",
"MessagesPerUserPerMinute", "MessagesPerDepartmentPerMinute",
"RequireLinkingConfirmation", "ProactiveNotificationsEnabled") wrap the
Delete.Column(...).FromTable(...) calls in an existence check (e.g., use
Schema.Table("ChatbotDepartmentConfigs".ToLower()).Column(columnName.ToLower()).Exists()
or your repository's ColumnExists helper) and only call Delete when the check
returns true; keep this logic inside the Down() method of class
M0070_ChatbotDepartmentConfigColumnsPg.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`:
- Around line 153-170: The Down() method unconditionally drops feature-flag
tables and FKs which can remove objects that pre-existed this migration; update
Down() in M0071_AddingFeatureTogglesPg to check existence before removing: for
each Delete.Table("FeatureFlags".ToLower()) and each
Delete.ForeignKey("FK_FeatureFlagUsages_Departments".ToLower()) / FK names used,
wrap the delete in a conditional that verifies the table or constraint exists
(e.g., use Schema.Table(...).Exists() or the migration framework's IfExists
helpers) so rollbacks only remove objects created by this migration.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0074_AddIsPermanentFailureToWeatherAlertSourcesPg.cs`:
- Around line 19-21: Wrap the unconditional deletion in Down() with a presence
check that mirrors Up(): before calling
Delete.Column("ispermanentfailure").FromTable("weatheralertsources"), check
Schema.Table("weatheralertsources").Column("ispermanentfailure").Exists() and
only delete when true so Down() will no-op when the column wasn't added by this
migration; update the Down() method accordingly to perform the guarded deletion.
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.cs`:
- Around line 237-243: The InsertAsync path added Utf8WriteGuard.Sanitize(user)
but other Dapper paths in IdentityUserRepository still pass raw strings to the
DB; update all methods that build DynamicParameters or send raw string
values—specifically UpdateAsync, InsertClaimsAsync, InsertLoginInfoAsync,
SetTokenAsync, UpdateClaimAsync (and any helper methods used to build
queries/parameters)—to sanitize inputs before creating DynamicParameters or
executing queries (e.g., call Utf8WriteGuard.Sanitize(user) or
Utf8WriteGuard.Sanitize for individual string fields), or factor a small private
helper (SanitizeUserFields or SanitizeParameters) and call it from each of those
methods so all Dapper calls use sanitized values.
---
Nitpick comments:
In
`@Repositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.cs`:
- Around line 34-37: Change the constructor of Utf8MaintenanceRepository to stop
taking an IConnectionProvider parameter and instead resolve the dependency
inside the constructor using
Bootstrapper.GetKernel().Resolve<IConnectionProvider>(), assigning the result to
the existing _connectionProvider field; update any constructor signature
references and callers accordingly so the repository uses the project’s
service-locator pattern rather than constructor injection.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Around line 426-428: The code silently clamps
SystemBehaviorConfig.Utf8CleanupHourUtc into utf8CleanupHour defaulting to 4
when out of range; update the block around utf8CleanupHour to log a warning when
SystemBehaviorConfig.Utf8CleanupHourUtc is <0 or >23 so operators know the
configured value was invalid and the default is used—use the existing
application logger (e.g., processLogger or the project's logger instance) to
emit a clear warning that includes the invalid value and the fallback hour, then
keep assigning utf8CleanupHour as currently implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69197c57-ead8-4d9a-a7c9-fdf41ed1d905
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Framework/Utf8SanitizerTests.csis excluded by!**/Tests/**
📒 Files selected for processing (32)
Core/Resgrid.Config/SystemBehaviorConfig.csCore/Resgrid.Framework/Text/Utf8Sanitizer.csCore/Resgrid.Model/Repositories/IUtf8MaintenanceRepository.csProviders/Resgrid.Providers.Migrations/Migrations/M0068_ChatbotTables.csProviders/Resgrid.Providers.Migrations/Migrations/M0069_ChatbotLinkingCodes.csProviders/Resgrid.Providers.Migrations/Migrations/M0070_ChatbotDepartmentConfigColumns.csProviders/Resgrid.Providers.Migrations/Migrations/M0071_AddingFeatureToggles.csProviders/Resgrid.Providers.Migrations/Migrations/M0072_AddingChatbotTwilioFeatureFlag.csProviders/Resgrid.Providers.Migrations/Migrations/M0074_AddIsPermanentFailureToWeatherAlertSources.csProviders/Resgrid.Providers.Migrations/Migrations/M0075_AddUtf8CleanupProgress.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0068_ChatbotTablesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0069_ChatbotLinkingCodesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0070_ChatbotDepartmentConfigColumnsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0072_AddingChatbotTwilioFeatureFlagPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0074_AddIsPermanentFailureToWeatherAlertSourcesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0075_AddUtf8CleanupProgressPg.csRepositories/Resgrid.Repositories.DataRepository/Extensions/DynamicParametersExtension.csRepositories/Resgrid.Repositories.DataRepository/IdentityRoleRepository.csRepositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.csRepositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/RepositoryBase.csRepositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.csRepositories/Resgrid.Repositories.DataRepository/Utf8WriteGuard.csTools/Resgrid.Console/Commands/CleanUtf8Command.csTools/Resgrid.Console/Program.csTools/Resgrid.Console/Services/ApplicationHostedService.csWorkers/Resgrid.Workers.Console/Commands/Utf8CleanupCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/Utf8CleanupTask.csWorkers/Resgrid.Workers.Framework/Logic/Utf8CleanupLogic.cs
Comment on lines
+151
to
+152
| if (c >= C1RangeStart && c <= C1RangeEnd) | ||
| return true; // C1 range: candidate for Windows-1252 mojibake repair. |
Contributor
There was a problem hiding this comment.
Don't rewrite C1 controls in the default safety path.
U+0080–U+009F are valid Unicode/UTF-8 and PostgreSQL will accept them. Treating that whole range as "dirty" here and remapping or dropping it silently corrupts legitimate data, even when the caller only wants storage safety. The CP-1252 mojibake heuristic needs to be opt-in; the always-on path should stay limited to NULs and unpaired surrogates.
Also applies to: 181-187
🤖 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.Framework/Text/Utf8Sanitizer.cs` around lines 151 - 152, The
sanitizer currently treats the entire C1 control range (U+0080–U+009F) as dirty
in Utf8Sanitizer (see the check using C1RangeStart/C1RangeEnd), which can
corrupt valid UTF-8; change the default safety path to stop classifying C1
controls as unsafe—only mark NUL and unpaired surrogates as dirty by default—and
make the CP-1252 mojibake repair logic opt-in (e.g., add a boolean flag or
separate method to enable CP1252 remapping). Update the analogous checks
referenced around the other occurrences (the block at lines 181–187) to follow
the same pattern so CP-1252 handling is only applied when explicitly requested.
Comment on lines
+14
to
+150
| if (!Schema.Table("FeatureFlags".ToLower()).Exists()) | ||
| { | ||
| Create.Table("FeatureFlags".ToLower()) | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FlagKey".ToLower()).AsCustom("citext").NotNullable() | ||
| .WithColumn("Name".ToLower()).AsCustom("citext").NotNullable() | ||
| .WithColumn("Description".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("Category".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("Tags".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("FlagType".ToLower()).AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("IsEnabledGlobally".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("DefaultValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("OffValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("RolloutPercentage".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("MinimumPlanType".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("Environment".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("EnableOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("DisableOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("IsArchived".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("IsPermanent".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("LastEvaluatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("CreatedOn".ToLower()).AsDateTime2().NotNullable().WithDefault(SystemMethods.CurrentUTCDateTime) | ||
| .WithColumn("CreatedByUserId".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("UpdatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("UpdatedByUserId".ToLower()).AsCustom("citext").Nullable(); | ||
|
|
||
| Create.Index("UX_FeatureFlags_FlagKey".ToLower()) | ||
| .OnTable("FeatureFlags".ToLower()) | ||
| .OnColumn("FlagKey".ToLower()).Ascending() | ||
| .WithOptions().Unique(); | ||
| } | ||
|
|
||
| // FeatureFlagOverrides - per-department override of a flag's value. | ||
| Create.Table("FeatureFlagOverrides".ToLower()) | ||
| .WithColumn("FeatureFlagOverrideId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("IsEnabled".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("FlagValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("Reason".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("ExpiresOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("CreatedOn".ToLower()).AsDateTime2().NotNullable().WithDefault(SystemMethods.CurrentUTCDateTime) | ||
| .WithColumn("CreatedByUserId".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("UpdatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("UpdatedByUserId".ToLower()).AsCustom("citext").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagOverrides_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagOverrides".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagOverrides_Departments".ToLower()) | ||
| .FromTable("FeatureFlagOverrides".ToLower()).ForeignColumn("DepartmentId".ToLower()) | ||
| .ToTable("Departments".ToLower()).PrimaryColumn("DepartmentId".ToLower()); | ||
|
|
||
| Create.Index("UX_FeatureFlagOverrides_Flag_Department".ToLower()) | ||
| .OnTable("FeatureFlagOverrides".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | ||
| .OnColumn("DepartmentId".ToLower()).Ascending() | ||
| .WithOptions().Unique(); | ||
| if (!Schema.Table("FeatureFlagOverrides".ToLower()).Exists()) | ||
| { | ||
| Create.Table("FeatureFlagOverrides".ToLower()) | ||
| .WithColumn("FeatureFlagOverrideId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("IsEnabled".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("FlagValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("Reason".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("ExpiresOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("CreatedOn".ToLower()).AsDateTime2().NotNullable().WithDefault(SystemMethods.CurrentUTCDateTime) | ||
| .WithColumn("CreatedByUserId".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("UpdatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("UpdatedByUserId".ToLower()).AsCustom("citext").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagOverrides_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagOverrides".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagOverrides_Departments".ToLower()) | ||
| .FromTable("FeatureFlagOverrides".ToLower()).ForeignColumn("DepartmentId".ToLower()) | ||
| .ToTable("Departments".ToLower()).PrimaryColumn("DepartmentId".ToLower()); | ||
|
|
||
| Create.Index("UX_FeatureFlagOverrides_Flag_Department".ToLower()) | ||
| .OnTable("FeatureFlagOverrides".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | ||
| .OnColumn("DepartmentId".ToLower()).Ascending() | ||
| .WithOptions().Unique(); | ||
| } | ||
|
|
||
| // FeatureFlagTargetingRules - attribute/segment targeting rules. | ||
| Create.Table("FeatureFlagTargetingRules".ToLower()) | ||
| .WithColumn("FeatureFlagTargetingRuleId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("Priority".ToLower()).AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("AttributeType".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("OperatorType".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("ComparisonValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("ResultEnabled".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("ResultValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("RolloutPercentage".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("CreatedOn".ToLower()).AsDateTime2().NotNullable().WithDefault(SystemMethods.CurrentUTCDateTime) | ||
| .WithColumn("CreatedByUserId".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("UpdatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("UpdatedByUserId".ToLower()).AsCustom("citext").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagTargetingRules_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagTargetingRules".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagTargetingRules_Flag".ToLower()) | ||
| .OnTable("FeatureFlagTargetingRules".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending(); | ||
| if (!Schema.Table("FeatureFlagTargetingRules".ToLower()).Exists()) | ||
| { | ||
| Create.Table("FeatureFlagTargetingRules".ToLower()) | ||
| .WithColumn("FeatureFlagTargetingRuleId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("Priority".ToLower()).AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("AttributeType".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("OperatorType".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("ComparisonValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("ResultEnabled".ToLower()).AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("ResultValue".ToLower()).AsCustom("text").Nullable() | ||
| .WithColumn("RolloutPercentage".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("CreatedOn".ToLower()).AsDateTime2().NotNullable().WithDefault(SystemMethods.CurrentUTCDateTime) | ||
| .WithColumn("CreatedByUserId".ToLower()).AsCustom("citext").Nullable() | ||
| .WithColumn("UpdatedOn".ToLower()).AsDateTime2().Nullable() | ||
| .WithColumn("UpdatedByUserId".ToLower()).AsCustom("citext").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagTargetingRules_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagTargetingRules".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagTargetingRules_Flag".ToLower()) | ||
| .OnTable("FeatureFlagTargetingRules".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending(); | ||
| } | ||
|
|
||
| // FeatureFlagPrerequisites - flag dependency edges. | ||
| Create.Table("FeatureFlagPrerequisites".ToLower()) | ||
| .WithColumn("FeatureFlagPrerequisiteId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("RequiredFeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("RequiredValue".ToLower()).AsCustom("text").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagPrerequisites_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagPrerequisites".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagPrerequisites_RequiredFeatureFlag".ToLower()) | ||
| .FromTable("FeatureFlagPrerequisites".ToLower()).ForeignColumn("RequiredFeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagPrerequisites_Flag".ToLower()) | ||
| .OnTable("FeatureFlagPrerequisites".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending(); | ||
| if (!Schema.Table("FeatureFlagPrerequisites".ToLower()).Exists()) | ||
| { | ||
| Create.Table("FeatureFlagPrerequisites".ToLower()) | ||
| .WithColumn("FeatureFlagPrerequisiteId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("RequiredFeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("RequiredValue".ToLower()).AsCustom("text").Nullable(); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagPrerequisites_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagPrerequisites".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagPrerequisites_RequiredFeatureFlag".ToLower()) | ||
| .FromTable("FeatureFlagPrerequisites".ToLower()).ForeignColumn("RequiredFeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagPrerequisites_Flag".ToLower()) | ||
| .OnTable("FeatureFlagPrerequisites".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending(); | ||
| } | ||
|
|
||
| // FeatureFlagUsages - aggregated daily evaluation counts (append-only flushes). | ||
| Create.Table("FeatureFlagUsages".ToLower()) | ||
| .WithColumn("FeatureFlagUsageId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("UsageDate".ToLower()).AsDateTime2().NotNullable() | ||
| .WithColumn("EvaluationCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("EnabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("DisabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagUsages_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagUsages".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagUsages_Departments".ToLower()) | ||
| .FromTable("FeatureFlagUsages".ToLower()).ForeignColumn("DepartmentId".ToLower()) | ||
| .ToTable("Departments".ToLower()).PrimaryColumn("DepartmentId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) | ||
| .OnTable("FeatureFlagUsages".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | ||
| .OnColumn("UsageDate".ToLower()).Ascending(); | ||
| if (!Schema.Table("FeatureFlagUsages".ToLower()).Exists()) | ||
| { | ||
| Create.Table("FeatureFlagUsages".ToLower()) | ||
| .WithColumn("FeatureFlagUsageId".ToLower()).AsInt32().NotNullable().PrimaryKey().Identity() | ||
| .WithColumn("FeatureFlagId".ToLower()).AsInt32().NotNullable() | ||
| .WithColumn("DepartmentId".ToLower()).AsInt32().Nullable() | ||
| .WithColumn("UsageDate".ToLower()).AsDateTime2().NotNullable() | ||
| .WithColumn("EvaluationCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("EnabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("DisabledCount".ToLower()).AsInt64().NotNullable().WithDefaultValue(0); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagUsages_FeatureFlags".ToLower()) | ||
| .FromTable("FeatureFlagUsages".ToLower()).ForeignColumn("FeatureFlagId".ToLower()) | ||
| .ToTable("FeatureFlags".ToLower()).PrimaryColumn("FeatureFlagId".ToLower()); | ||
|
|
||
| Create.ForeignKey("FK_FeatureFlagUsages_Departments".ToLower()) | ||
| .FromTable("FeatureFlagUsages".ToLower()).ForeignColumn("DepartmentId".ToLower()) | ||
| .ToTable("Departments".ToLower()).PrimaryColumn("DepartmentId".ToLower()); | ||
|
|
||
| Create.Index("IX_FeatureFlagUsages_Flag_Date".ToLower()) | ||
| .OnTable("FeatureFlagUsages".ToLower()) | ||
| .OnColumn("FeatureFlagId".ToLower()).Ascending() | ||
| .OnColumn("UsageDate".ToLower()).Ascending(); | ||
| } |
Contributor
There was a problem hiding this comment.
Table-level guards don’t heal partial schema state (missing FK/index).
Each block skips all FK/index creation when the table already exists. If a prior run created the table but failed before adding constraints/indexes, reruns won’t repair the schema, leaving missing uniqueness/referential guarantees.
🤖 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.MigrationsPg/Migrations/M0071_AddingFeatureTogglesPg.cs`
around lines 14 - 150, The migration currently skips creating foreign keys and
indexes when a table already exists, which leaves partial schema state if a
prior run created the table but failed before adding constraints; update
M0071_AddingFeatureTogglesPg.cs to separate table creation from constraint/index
creation: for each logical group (FeatureFlags, FeatureFlagOverrides,
FeatureFlagTargetingRules, FeatureFlagPrerequisites, FeatureFlagUsages) always
run checks that create missing FKs and Indexes even when the table
exists—specifically ensure Create.ForeignKey calls for
FK_FeatureFlagOverrides_FeatureFlags, FK_FeatureFlagOverrides_Departments,
FK_FeatureFlagTargetingRules_FeatureFlags,
FK_FeatureFlagPrerequisites_FeatureFlags,
FK_FeatureFlagPrerequisites_RequiredFeatureFlag,
FK_FeatureFlagUsages_FeatureFlags, FK_FeatureFlagUsages_Departments and
Create.Index calls for UX_FeatureFlags_FlagKey,
UX_FeatureFlagOverrides_Flag_Department, IX_FeatureFlagTargetingRules_Flag,
IX_FeatureFlagPrerequisites_Flag, IX_FeatureFlagUsages_Flag_Date are executed
conditionally (e.g., if the FK/index does not exist) regardless of whether
Create.Table was skipped so rerunning the migration repairs missing
constraints/indexes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 116-117: In IdentityRepository, update culture-sensitive
normalization by replacing newUsername.ToUpper() and newEmail?.ToUpper() with
newUsername.ToUpperInvariant() and newEmail?.ToUpperInvariant() respectively
(leave the existing SQL WHERE predicates as-is); locate the normalization usages
in the method that executes the UPDATE on public.aspnetusers (the db.Execute
call that sets username/normalizedusername and the similar update for
email/normalizedemail) and change the ToUpper calls to ToUpperInvariant to avoid
locale-dependent mismatches.
- Around line 93-95: The query in IdentityRepository is using LOWER(email) and
LOWER(username) which is unnecessary and can prevent index use on PostgreSQL
citext columns; remove the LOWER(...) wrappers and pass the raw parameter (rely
on citext to do case-insensitive comparisons) and ensure any username
normalization uses culture-invariant casing (replace ToUpper with
ToUpperInvariant when building newUsername). Update the methods that reference
LOWER(email) / LOWER(username) and the username normalization logic so
parameters are normalized with ToUpperInvariant() instead of ToUpper(), keeping
comparisons simple (e.g., WHERE email = `@email` / WHERE username = `@username`) to
preserve index 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
Run ID: eaf3a2ad-10a2-4e64-9dc4-4f3b947e01c5
📒 Files selected for processing (6)
Core/Resgrid.Model/Repositories/IUtf8MaintenanceRepository.csCore/Resgrid.Services/UsersService.csRepositories/Resgrid.Repositories.DataRepository/IdentityRepository.csRepositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.csTools/Resgrid.Console/Commands/CleanUtf8Command.csWorkers/Resgrid.Workers.Console/Tasks/Utf8CleanupTask.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Tools/Resgrid.Console/Commands/CleanUtf8Command.cs
- Workers/Resgrid.Workers.Console/Tasks/Utf8CleanupTask.cs
- Core/Resgrid.Model/Repositories/IUtf8MaintenanceRepository.cs
- Repositories/Resgrid.Repositories.DataRepository/Utf8MaintenanceRepository.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Line 143: The SQL Server branch in IdentityRepository.cs uses
newEmail.ToUpperInvariant() which will throw if newEmail is null; make it
consistent with the PostgreSQL branch by using null-safe conversion
(newEmail?.ToUpperInvariant()) when building the anonymous parameter for the
db.Execute call that updates AspNetUsers (the line with db.Execute($"UPDATE
[AspNetUsers] SET [Email] = `@newEmail`, [NormalizedEmail] = `@newEmailUpper` WHERE
Id = `@userId`", new { ... })). Ensure the parameter key remains newEmailUpper and
that the resulting value is null when newEmail is null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69564e86-35e9-4054-8c23-39447f16f2f1
📒 Files selected for processing (1)
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores