feat: Add reverse indexes on Localizations and SearchTags #1971
Conversation
|
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe changes standardize the usage of constants by replacing fully qualified and aliased references with direct static imports. Multiple configuration files now use a new internal Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
oskogstad
changed the title
oskogstad
force-pushed
the
feat/add-reverse-indexes-for-localizations-and-searchtags
branch
3 times, most recently
from
37cb74b to
5a6f96c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250226142844_CreateGinIndexOnLocalizationsAndSearchTags.cs (1)
13-14: Consider centralizing PostgreSQL extension annotationThe string literal for the PostgreSQL extension annotation could be replaced with a constant from the new
Constantsclass for better maintainability.migrationBuilder.AlterDatabase() - .Annotation("Npgsql:PostgresExtension:pg_trgm", ",,"); + .Annotation(Constants.PostgreSqlExtensionPrefix + Constants.PostrgreSqlTrigram, ",,");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250226142844_CreateGinIndexOnLocalizationsAndSearchTags.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (8)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Dialogs/DialogEntityConfiguration.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Dialogs/DialogSearchTagConfiguration.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Localizations/LocalizationConfiguration.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Constants.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/IdempotentNotifications/NotificationAcknowledgement.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250226142844_CreateGinIndexOnLocalizationsAndSearchTags.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Interceptors/ConvertDomainEventsToOutboxMessagesInterceptor.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Constants.cs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Dry run deploy apps / Deploy service to test
- GitHub Check: Dry run deploy apps / Deploy graphql to test
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: Dry run deploy apps / Deploy web-api-eu to test
- GitHub Check: build / build-and-test
🔇 Additional comments (12)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Localizations/LocalizationConfiguration.cs (1)
14-18: Well-implemented text search optimization.The addition of a GIN index with trigram operators on the
Valueproperty is an excellent approach for enhancing text search capabilities on localization values. This will significantly improve performance for partial and fuzzy text searches, which is particularly valuable for localized content.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/IdempotentNotifications/NotificationAcknowledgement.cs (2)
4-4: Good coding style improvement.Using static imports for constants makes the code cleaner and easier to read.
21-21: Consistent use of direct constant reference.The direct reference to
DefaultMaxStringLengthwithout theConstants.prefix improves readability while maintaining the same functionality.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Dialogs/DialogEntityConfiguration.cs (2)
4-4: Good coding style improvement.Using static imports for constants makes the code cleaner and easier to read.
14-14: Consistent use of direct constant reference.The direct reference to
DefaultMaxStringLengthwithout theConstants.prefix improves readability while maintaining the same functionality.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Configurations/Dialogs/DialogSearchTagConfiguration.cs (3)
4-4: Good coding style improvement.Using static imports for constants makes the code cleaner and easier to read.
15-15: Consistent use of direct constant reference.The direct reference to
MaxSearchTagLengthwithout theConstants.prefix improves readability while maintaining the same functionality.
17-19: Well-implemented text search optimization.The addition of a GIN index with trigram operators on the
Valueproperty is an excellent approach for enhancing text search capabilities on search tags. This will significantly improve performance for partial and fuzzy text searches, which is crucial for search functionality.Note that while you're using static imports for domain constants,
Constants.GinandConstants.GinTrgmOpsare still fully qualified, suggesting they come from a different Constants class than the one being statically imported.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250226142844_CreateGinIndexOnLocalizationsAndSearchTags.cs (2)
1-46: GIN indexes implemented correctly for improved search performanceThe migration properly creates GIN indexes with trigram operations on the
Valuecolumns of bothLocalizationandDialogSearchTagtables, and adds the required PostgreSQL trigram extension. This will significantly improve text search performance, especially for queries using LIKE operations or fuzzy matching.The Up and Down methods are correctly implemented to allow bidirectional migration.
14-14:❓ Verification inconclusive
Verify the empty parameters in the extension annotation
The extension annotation has empty parameters (
",,"). Is this intentional? Consider documenting the purpose of these empty parameters or adjusting if necessary.
🏁 Script executed:
#!/bin/bash # Check how postgres extension annotations are implemented in other migrations rg -A 2 "PostgresExtension" --glob="*.cs" --glob="!*20250226142844_*"Length of output: 452
Clarify Usage of Empty Parameters in Extension Annotation
The migration file uses an annotation with empty parameters:
.Annotation("Npgsql:PostgresExtension:pg_trgm", ",,");This contrasts with other parts of the code (e.g., in
DialogDbContext.cswhere the extension is added via.HasPostgresExtension(Constants.PostrgreSqlTrigram)). Please verify that the empty parameter string is intentional. If so, adding a comment explaining the design decision would help future maintainers; otherwise, consider aligning this implementation with the pattern used elsewhere.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (2)
103-104: Improved clarity with fully qualified constant referencesExplicitly qualifying the constants with their namespace improves code clarity and prevents ambiguity when multiple
Constantsclasses exist.
115-117: Extension registration aligns with migration changesAdding the PostgreSQL trigram extension registration in the DbContext ensures consistency with the migration that creates the GIN indexes. This is a good practice that keeps the model definition aligned with the actual database state.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs
Outdated
Show resolved
Hide resolved
oskogstad
force-pushed
the
feat/add-reverse-indexes-for-localizations-and-searchtags
branch
from
6a4eb46 to
390f105
Compare
oskogstad
deleted the
feat/add-reverse-indexes-for-localizations-and-searchtags
branch
🤖 I have created a release *beep* *boop* --- ## [1.57.0](v1.56.1...v1.57.0) (2025-02-28) ### Features * Add reverse indexes on Localizations and SearchTags ([#1971](#1971)) ([7a506b3](7a506b3)) * **ci:** Enable PostreSQL extension pg_trgm ([#1984](#1984)) ([4dce375](4dce375)) ### Miscellaneous Chores * **sdk:** Add doc ([#1975](#1975)) ([37c9130](37c9130)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Co-authored-by: Bjørn Dybvik Langfors [email protected]
Co-authored-by: Magnus Sandgren [email protected]