feat: normalize deployments table #14366
Conversation
github-actions
bot
added
area/app-deploy
mirkoSekulic
force-pushed
the
feature/normalize-deployments
branch
3 times, most recently
from
38458ef to
edd74a9
Compare
github-actions
bot
added
the
quality/testing
mirkoSekulic
force-pushed
the
feature/normalize-deployments
branch
5 times, most recently
from
7e37d39 to
9192722
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14366 +/- ##
=======================================
Coverage 95.58% 95.58%
=======================================
Files 1869 1869
Lines 24237 24237
Branches 2786 2786
=======================================
Hits 23167 23167
Misses 814 814
Partials 256 256 ☔ View full report in Codecov by Sentry. |
mirkoSekulic
force-pushed
the
feature/normalize-deployments
branch
from
9192722 to
c9a23f1
Compare
mirkoSekulic
force-pushed
the
feature/normalize-deployments
branch
from
c9a23f1 to
913c90e
Compare
📝 WalkthroughWalkthroughThis pull request introduces significant changes to the Altinn Studio project's backend, focusing on database management and repository implementations. The modifications include adding a PgAdmin service to the Docker Compose configuration, updating migration scripts, and refactoring repository classes for deployments and releases. The changes aim to improve database schema management, enhance data normalization, and simplify repository implementations by renaming and restructuring existing classes. Changes
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
CodeRabbit Configuration File (
|
mirkoSekulic
force-pushed
the
feature/normalize-deployments
branch
from
95b929f to
08d45b7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 16
🔭 Outside diff range comments (3)
backend/src/Designer/Repository/ORMImplementation/Mappers/ReleaseMapper.cs (1)
Line range hint
20-33: Add null checks and error handling for JSON serialization.The method should validate input parameters and handle potential serialization failures.
Consider this implementation:
public static ReleaseDbModel MapToDbModel(ReleaseEntity releaseEntity) { + ArgumentNullException.ThrowIfNull(releaseEntity); + ArgumentNullException.ThrowIfNull(releaseEntity.Build); + return new ReleaseDbModel { Buildid = releaseEntity.Build.Id, Tagname = releaseEntity.TagName, Org = releaseEntity.Org, App = releaseEntity.App, Buildstatus = releaseEntity.Build.Status.ToEnumMemberAttributeValue(), Buildresult = releaseEntity.Build.Result.ToEnumMemberAttributeValue(), Created = releaseEntity.Created, - Entity = JsonSerializer.Serialize(releaseEntity, s_jsonOptions) + Entity = JsonSerializer.Serialize(releaseEntity, s_jsonOptions) + ?? throw new InvalidOperationException("Failed to serialize release entity") }; }backend/tests/Designer.Tests/DbIntegrationTests/DeploymentEntityRepository/GetSingleIntegrationTests.cs (1)
DateTime comparison patterns need standardization across test classes
The codebase shows inconsistent DateTime comparison patterns:
- DeploymentEntityRepository tests use 200ms tolerance
- ReleaseEntityRepository and AppScopesRepository tests use 100ms tolerance
- Some repository tests lack DateTime comparison entirely
Consider:
- Standardizing the timeout value across all tests (either 100ms or 200ms)
- Adding DateTime comparison to ReleaseEntityRepository tests that use BeEquivalentTo
- Moving the DateTime comparison configuration to a shared test utility
🔗 Analysis chain
Line range hint
1-31: Verify DateTime comparison consistency across test classes.Consider checking if other test classes inheriting from
DeploymentEntityIntegrationTestsBaseneed similar DateTime comparison updates for consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all test classes that might need similar DateTime comparison updates fd -e cs -p ".*Tests\.cs$" backend/tests/Designer.Tests/DbIntegrationTests/ --exec rg -l "Should\(\)\.BeEquivalentTo\(" {}Length of output: 775
Script:
#!/bin/bash # Search for DateTime comparison patterns in the identified test files rg -U "options.*Using.*DateTime.*BeCloseTo" backend/tests/Designer.Tests/DbIntegrationTests/Length of output: 94
Script:
#!/bin/bash # Get broader context of BeEquivalentTo usage to see DateTime handling rg -U "Should\(\)\.BeEquivalentTo\([^;]*\);" -A 5 backend/tests/Designer.Tests/DbIntegrationTests/Length of output: 14903
backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/GetBuildStatusAndResultsFilterIntegrationTests.cs (1)
Add test cases for string comparison edge cases
The current implementation uses case-sensitive string comparison and doesn't handle empty string filters specially. Add test cases to verify:
- Case sensitivity handling (uppercase/mixed-case status/result values)
- Empty string filters behavior
- String comparison edge cases
Example locations that need test coverage:
backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/GetBuildStatusAndResultsFilterIntegrationTests.csThe implementation in
ReleaseRepository.Get()usesstring.Equals()for comparison, which is case-sensitive by default, and empty strings would be compared literally without special handling.🔗 Analysis chain
Line range hint
1-143: Verify edge cases in test coverage.The test cases cover various combinations of build statuses and results, including null filters. Consider adding edge cases for:
- Empty string filters
- Case sensitivity handling
- Maximum length tag names
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for edge cases in other test files # Look for similar test files that might contain edge cases rg -l "BuildStatus|BuildResult" "backend/tests/Designer.Tests" # Check for existing empty string and case sensitivity tests rg -A 5 'string\.Empty|\.ToLower|\.ToUpper' "backend/tests/Designer.Tests"Length of output: 13019
Script:
#!/bin/bash # Check implementation of ToEnumMemberAttributeValue and Get method ast-grep --pattern 'ToEnumMemberAttributeValue()' backend/tests/Designer.Tests ast-grep --pattern 'public $_ Get($$$)' backend/src/Designer/Repository/ORMImplementation/ReleaseRepository.cs # Check enum definitions rg -A 5 'enum BuildStatus|enum BuildResult' backend/src/DesignerLength of output: 1406
Script:
#!/bin/bash # Find ToEnumMemberAttributeValue implementation rg -A 10 "ToEnumMemberAttributeValue" backend/src/Designer # Find ReleaseRepository Get method implementation rg -A 20 "public async Task<IEnumerable<Release>> Get\(" backend/src/DesignerLength of output: 7196
Script:
#!/bin/bash # Find the Get method implementation with filters rg -A 20 "public\s+async\s+Task<IEnumerable<ReleaseEntity>>\s+Get.*buildStatuses" backend/src/Designer # Also check for any string comparison or filter implementations rg -A 10 "Contains.*buildStatuses|Contains.*buildResults" backend/src/DesignerLength of output: 185
Script:
#!/bin/bash # Find the ReleaseRepository implementation cat backend/src/Designer/Repository/ORMImplementation/ReleaseRepository.cs # Backup search for any LINQ queries with Contains rg -A 10 "\.Where.*Contains.*Status|\.Where.*Contains.*Result" backend/src/DesignerLength of output: 3506
🧹 Nitpick comments (27)
backend/src/Designer/Repository/ORMImplementation/Mappers/ReleaseMapper.cs (2)
47-50: Add null check for the collection parameter.While the implementation is clean, it should validate the input collection.
Consider this implementation:
public static IEnumerable<ReleaseEntity> MapToModels(IEnumerable<ReleaseDbModel> releases) { + ArgumentNullException.ThrowIfNull(releases); return releases.Select(MapToModel); }
Line range hint
20-50: Consider the implications of storing full entities as JSON.The current approach of storing the complete entity as JSON in the database has several implications:
- Data duplication: Some fields are stored both as columns and within the JSON
- Query limitations: Cannot efficiently query data within the JSON
- Schema evolution: Changes to the entity structure might require careful migration of the JSON data
Consider normalizing the data structure further if these aspects become problematic in the future.
backend/src/Designer/Migrations/20250110192106_BuildsGrants.cs (1)
21-21: Fix whitespace formatting.Line 21 contains incorrect whitespace formatting as indicated by the pipeline failure:
[error] 21-21: Fix whitespace formatting. Replace 21 characters with '\n\s\s\s\s\s\s\s\s'
Adjust the indentation to align with the project's coding standards and resolve the pipeline error.
🧰 Tools
🪛 GitHub Actions: Dotnet format check
[error] 21-21: Fix whitespace formatting. Replace 21 characters with '\n\s\s\s\s\s\s\s\s'
backend/src/Designer/Migrations/SqlScripts/Builds/01-setup-grants.sql (1)
1-1: Review the scope of granted permissions for security best practices.Granting
SELECT,INSERT,UPDATE, andDELETEpermissions on all tables in thedesignerschema to thedesignerrole might not adhere to the principle of least privilege. Consider restricting permissions to only those necessary for the role's functionality to enhance security and reduce potential risks.compose.yaml (1)
193-204: Consider securing pgAdmin configurationWhile the addition of pgAdmin is valuable for managing the database normalization process, consider these security improvements:
- Move default credentials to environment variables
- Consider enabling server mode in non-production environments for multi-user support
pgadmin: image: dpage/pgadmin4 ports: - "81:80" environment: - PGADMIN_DEFAULT_EMAIL: ${PGADMIN_DEFAULT_EMAIL:[email protected]} - PGADMIN_DEFAULT_PASSWORD: ${PGADMIN_DEFAULT_PASSWORD:-admin} + PGADMIN_DEFAULT_EMAIL: ${PGADMIN_DEFAULT_EMAIL:?PGADMIN_DEFAULT_EMAIL is required} + PGADMIN_DEFAULT_PASSWORD: ${PGADMIN_DEFAULT_PASSWORD:?PGADMIN_DEFAULT_PASSWORD is required} PGADMIN_CONFIG_SERVER_MODE: 'False'README.md (1)
64-64: Fix grammatical error in pgAdmin descriptionThere's a grammatical error in the service description.
- `pgadmin` which is a administration and development platform for PostgreSQL. + `pgadmin` which is an administration and development platform for PostgreSQL.🧰 Tools
🪛 LanguageTool
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... migrations before exiting. -pgadminwhich is a administration and development pla...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[misspelling] ~64-~64: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...ns before exiting. -pgadminwhich is a administration and development platform...(EN_A_VS_AN)
backend/src/Designer/Migrations/20250107121538_NormalizeDeploymentsData.Designer.cs (3)
73-74: Optimize index strategy for org/app queries.Multiple tables (
app_scopes,deployments,releases) have similar indexes on(org, app). Consider:
- Adding
INCLUDEclauses for frequently accessed columns- Analyzing query patterns to determine if partial indexes would be beneficial
Also applies to: 176-177, 225-226
116-118: Review index coverage for common queries.The indexes on
BuildDbObjectand its relationship withDeploymentshould be reviewed:
- The unique index on
(ExternalId, BuildType)is good for lookups- Consider adding covering indexes for common query patterns involving
StatusandResultAlso applies to: 174-175
88-91: Consider using enum for BuildType and Status.The
BuildTypeandStatuscolumns inBuildDbObjectare stored as integer and varchar respectively. Consider:
- Using an enum for
BuildTypeto enforce valid values- Converting
Statusto an enum if the values are predeterminedAlso applies to: 108-111
backend/src/Designer/Migrations/DesignerdbContextModelSnapshot.cs (3)
76-117: Add XML documentation for BuildType enum values.The
BuildTypeproperty appears to be an enum. Consider adding XML documentation to explain the possible values and their meanings.Well-designed indexing strategy.
The indexing strategy is well thought out:
- Unique index on
(ExternalId, BuildType)prevents duplicate builds- Additional index on
BuildTypesupports efficient filtering
178-178: Consider normalizing the ReleaseDbModel in future iterations.The
ReleaseDbModelstructure is similar to the pre-normalizedDeploymentDbModel. Consider applying similar normalization patterns in future iterations:
- Extract build-related fields into the
BuildDbModel- Add a foreign key relationship to
BuildDbModel
Line range hint
1-235: Monitor query performance after normalization.The normalization of the deployments table introduces new relationships and indexes. Consider:
- Monitoring query performance during and after the migration
- Adding appropriate indexes if common query patterns show performance degradation
- Updating any existing queries to take advantage of the new structure
backend/src/Designer/Repository/ORMImplementation/Models/DeploymentDbModel.cs (1)
5-5: Consider adding validation attributes for the new string properties.The new string properties (
EnvName,CreatedBy) are marked as non-nullable but lack length constraints or validation attributes. Consider adding appropriate data annotations to ensure data integrity.+using System.ComponentModel.DataAnnotations; public partial class DeploymentDbModel { // ... existing properties ... + [Required] + [StringLength(50)] public string EnvName { get; set; } = null!; + [Required] + [StringLength(256)] public string CreatedBy { get; set; } = null!;Also applies to: 17-18, 23-24, 27-29
backend/src/Designer/Repository/ORMImplementation/Data/DesignerdbContext.cs (1)
21-24: Consider ordering configurations based on dependencies.Since there's a relationship between Deployments and Builds, consider ordering the configurations to ensure foreign key relationships are properly set up:
- BuildConfiguration (independent)
- DeploymentConfiguration (depends on Build)
- Others
backend/src/Designer/Repository/ORMImplementation/Models/AppScopesDbModel.cs (1)
5-5: LGTM! Consider adding class-level documentation.The rename to
AppScopesDbModelaligns well with the database model naming convention. While all properties are well-documented, consider adding class-level XML documentation to describe the purpose of this database model.+/// <summary> +/// Database model representing application scopes and their associated metadata. +/// </summary> public class AppScopesDbModelbackend/tests/Designer.Tests/DbIntegrationTests/DeploymentEntityRepository/CreateIntegrationTests.cs (1)
Line range hint
21-29: Consider adding more test cases for edge scenarios.While the happy path is well tested, consider adding test cases for:
- Attempting to create a deployment with a duplicate build ID
- Creating a deployment with invalid build data
- Creating a deployment with null build data
backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/UpdateIntegrationTests.cs (1)
Line range hint
23-38: Add test cases for concurrent updates.Consider adding test cases to verify:
- Optimistic concurrency control when two users update the same release
- Update behavior when the release is deleted by another user
- Update behavior when the build is modified by another process
Example test case for concurrent updates:
[Fact] public async Task Update_WhenConcurrentModification_ShouldThrowDbUpdateConcurrencyException() { // Arrange var repository = new ReleaseRepository(DbFixture.DbContext); var buildId = Guid.NewGuid(); var releaseEntity = EntityGenerationUtils.Release.GenerateReleaseEntity("ttd", buildId: buildId.ToString()); await PrepareEntityInDatabase(releaseEntity); // Simulate concurrent modification await using var concurrentContext = new DesignerDbContext(DbFixture.DbContextOptions); var concurrentRepo = new ReleaseRepository(concurrentContext); var concurrentEntity = await concurrentRepo.Get(releaseEntity.Org, releaseEntity.App); concurrentEntity.Build.Status = BuildStatus.Completed; await concurrentRepo.Update(concurrentEntity); // Act & Assert releaseEntity.Build.Status = BuildStatus.Failed; await Assert.ThrowsAsync<DbUpdateConcurrencyException>(() => repository.Update(releaseEntity)); }backend/src/Designer/Repository/ORMImplementation/Data/EntityConfigurations/BuildConfiguration.cs (1)
44-47: Consider removing redundant index on BuildType.The composite index on
(ExternalId, BuildType)already covers queries onBuildTypealone due to the leftmost prefix rule. The additional single-column index onBuildTypemight be redundant.- builder.HasIndex(b => b.BuildType);backend/tests/Designer.Tests/DbIntegrationTests/DeploymentEntityRepository/Utils/DeploymentEntityAsserts.cs (1)
22-26: Consider adding assertions for additional build properties.While the current assertions cover the essential build properties, consider adding assertions for other properties that might be added to
BuildDbModelin the future to ensure complete validation.buildDbModel.BuildType.Should().Be(BuildType.Deployment); +buildDbModel.Created.Should().BeCloseTo(deploymentEntity.Build.Created, TimeSpan.FromMilliseconds(100)); +buildDbModel.CreatedBy.Should().BeEquivalentTo(deploymentEntity.Build.CreatedBy);backend/src/Designer/Repository/ORMImplementation/Data/EntityConfigurations/DeploymentConfiguration.cs (1)
26-29: Consider adding a maximum length constraint for EnvName.To prevent potential data truncation issues, consider adding a maximum length constraint for the
EnvNamecolumn.builder.Property(e => e.EnvName) .HasColumnType("character varying") + .HasMaxLength(255) .HasColumnName("envname");backend/src/Designer/Repository/ORMImplementation/Mappers/DeploymentMapper.cs (1)
44-55: Consider adding defensive copying for dates.To prevent potential mutation of date values after mapping, consider creating defensive copies of the date properties.
Created = dbObject.Created, +Created = dbObject.Created.ToUniversalTime(),backend/src/Designer/Repository/ORMImplementation/DeploymentRepository.cs (2)
32-32: Consider adding index hints for query optimizationThe queries use
Include(d => d.Build)which may not utilize indexes effectively. Consider adding.TagWith()to provide query hints to the database engine for better performance.- var deploymentsQuery = _dbContext.Deployments.Include(d => d.Build).AsNoTracking().Where(x => x.Org == org && x.App == app); + var deploymentsQuery = _dbContext.Deployments + .TagWith("Use Index: IX_deployments_org_app") + .Include(d => d.Build) + .AsNoTracking() + .Where(x => x.Org == org && x.App == app);Also applies to: 46-46
52-54: Fix typo in property nameThere's a typo in the property name 'SequnceNo'.
- .Select(d => new { SequnceNo = d.Sequenceno, BuildId = d.Build.Id }) + .Select(d => new { SequenceNo = d.Sequenceno, BuildId = d.Build.Id })backend/tests/Designer.Tests/DbIntegrationTests/DeploymentEntityRepository/GetIntegrationTests.cs (1)
40-43: Consider parameterizing the DateTime tolerance valueThe hardcoded 200ms tolerance value should be extracted to a constant or configuration value for better maintainability.
+ private static readonly TimeSpan DateTimeComparisonTolerance = TimeSpan.FromMilliseconds(200); + result.Should().BeEquivalentTo(expectedEntities, options => options.Using<DateTime>(ctx => - ctx.Subject.Should().BeCloseTo(ctx.Expectation, TimeSpan.FromMilliseconds(200)) + ctx.Subject.Should().BeCloseTo(ctx.Expectation, DateTimeComparisonTolerance) ).WhenTypeIs<DateTime>());backend/src/Designer/Migrations/SqlScripts/DeploymentsNormalization/normalization.sql (3)
7-7: Document the significance of build_type constant.The constant
build_type := 0lacks documentation explaining its purpose and possible values. Consider adding a comment to clarify what this value represents in the builds table.+ -- build_type: 0 represents <explain the type> build_type CONSTANT INTEGER := 0;
9-14: Consider performance optimizations for large datasets.Processing all records in a single transaction without batching could lead to:
- Long-running transactions
- High memory usage
- Potential timeouts
Consider:
- Adding batching using
LIMITandOFFSET- Adding
ORDER BYfor deterministic processing- Adding a progress indicator for monitoring
Example implementation:
DECLARE batch_size CONSTANT INTEGER := 1000; offset_count INTEGER := 0; processed_count INTEGER := 0; BEGIN LOOP FOR rec IN SELECT sequenceno, entity::jsonb AS entity_data FROM designer.deployments ORDER BY sequenceno LIMIT batch_size OFFSET offset_count LOOP -- Your existing processing logic processed_count := processed_count + 1; END LOOP; EXIT WHEN processed_count < batch_size; offset_count := offset_count + batch_size; RAISE NOTICE 'Processed % records', offset_count; COMMIT; END LOOP; END;
34-36: Enhance error handling with specific error cases.The current generic error handling might mask specific issues that require different handling strategies.
Consider handling specific exceptions:
EXCEPTION WHEN unique_violation THEN RAISE NOTICE 'Duplicate build ID found for deployment %: %', rec.sequenceno, SQLERRM; WHEN not_null_violation THEN RAISE NOTICE 'Required field missing for deployment %: %', rec.sequenceno, SQLERRM; WHEN OTHERS THEN RAISE NOTICE 'Unexpected error processing deployment %: %', rec.sequenceno, SQLERRM;
🛑 Comments failed to post (16)
backend/src/Designer/Repository/ORMImplementation/Mappers/ReleaseMapper.cs (1)
42-45: 🛠️ Refactor suggestion
Add input validation and error handling for JSON deserialization.
The method should validate input and handle potential deserialization failures.
Consider this implementation:
public static ReleaseEntity MapToModel(ReleaseDbModel releaseDbModel) { + ArgumentNullException.ThrowIfNull(releaseDbModel); + ArgumentException.ThrowIfNullOrEmpty(releaseDbModel.Entity); + + try + { return JsonSerializer.Deserialize<ReleaseEntity>(releaseDbModel.Entity, s_jsonOptions) + ?? throw new InvalidOperationException("Failed to deserialize release entity"); + } + catch (JsonException ex) + { + throw new InvalidOperationException("Invalid release entity JSON format", ex); + } }📝 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 ReleaseEntity MapToModel(ReleaseDbModel releaseDbModel) { ArgumentNullException.ThrowIfNull(releaseDbModel); ArgumentException.ThrowIfNullOrEmpty(releaseDbModel.Entity); try { return JsonSerializer.Deserialize<ReleaseEntity>(releaseDbModel.Entity, s_jsonOptions) ?? throw new InvalidOperationException("Failed to deserialize release entity"); } catch (JsonException ex) { throw new InvalidOperationException("Invalid release entity JSON format", ex); } }backend/src/Designer/Migrations/20250110192106_BuildsGrants.cs (2)
2-2: 🛠️ Refactor suggestion
Remove unnecessary using directive.
The
usingdirectiveAltinn.Studio.Designer.Repository.ORMImplementation.Modelson line 2 is unnecessary as the code does not reference any types from this namespace. Removing it will clean up the code and eliminate the warning from the static analysis tools.🧰 Tools
🪛 GitHub Check: Format check
[warning] 2-2:
Using directive is unnecessary.🪛 GitHub Actions: Dotnet format check
[warning] 2-2: Using directive is unnecessary
19-22: 🛠️ Refactor suggestion
Implement the
Downmethod to support migration rollback.The
Downmethod is currently empty. To ensure that the migration can be rolled back if necessary, consider implementing the reverse of the changes made in theUpmethod. This is important for maintaining database integrity during rollbacks.🧰 Tools
🪛 GitHub Actions: Dotnet format check
[error] 21-21: Fix whitespace formatting. Replace 21 characters with '\n\s\s\s\s\s\s\s\s'
backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/GetSingleIntegrationTests.cs (1)
22-22: 💡 Codebase verification
Update MapToDbObject method to align with schema normalization.
The base class is compatible with the new
ReleaseRepositoryas it doesn't directly use any repository implementation. However, theMapToDbObjectmethod still references the ORM-specific modelAltinn.Studio.Designer.Repository.ORMImplementation.Models.ReleaseDbModel, which should be updated to align with the database normalization effort.🔗 Analysis chain
Verify base class compatibility with ReleaseRepository.
The change from
ORMReleaseRepositorytoReleaseRepositorylooks good and aligns with the database normalization effort. However, we should verify that the base classReleaseEntityIntegrationTestsBaseis compatible with the new repository type.Let's verify the base class implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if base class has been updated to work with new repository ast-grep --pattern 'class ReleaseEntityIntegrationTestsBase { $$$ }' # Check for any remaining references to ORMReleaseRepository rg "ORMReleaseRepository" -A 5Length of output: 103
Script:
#!/bin/bash # Find the base class file fd ReleaseEntityIntegrationTestsBase.cs # Look for any repository-related code in test files rg -t cs "class.*Repository" backend/tests/Designer.Tests/DbIntegrationTests --context 2 # Look for base class inheritance rg -t cs "ReleaseEntityIntegrationTestsBase" backend/tests/Designer.Tests/DbIntegrationTests --context 2Length of output: 6095
Script:
#!/bin/bash # View base class implementation cat backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/Base/ReleaseEntityIntegrationTestsBase.cs # Check for any repository type references in the base class directory rg -t cs "Repository" backend/tests/Designer.Tests/DbIntegrationTests/ReleaseEntityRepository/Base/ --context 2Length of output: 4364
backend/src/Designer/Migrations/20250107121305_AddBuidsTableAndDeploymentsColumns.Designer.cs (1)
15-16:
⚠️ Potential issueCorrect the typo in the class and migration name
The word "Buids" should be corrected to "Builds" in the class name, migration attribute, and filename to ensure consistent naming and avoid confusion.
Apply this diff to correct the typo:
- [Migration("20250107121305_AddBuidsTableAndDeploymentsColumns")] - partial class AddBuidsTableAndDeploymentsColumns + [Migration("20250107121305_AddBuildsTableAndDeploymentsColumns")] + partial class AddBuildsTableAndDeploymentsColumnsAlso, rename the filename to:
20250107121305_AddBuildsTableAndDeploymentsColumns.Designer.cs📝 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.[Migration("20250107121305_AddBuildsTableAndDeploymentsColumns")] partial class AddBuildsTableAndDeploymentsColumnsbackend/src/Designer/Repository/ORMImplementation/Models/BuildDbModel.cs (1)
9-13:
⚠️ Potential issueEnsure string properties are properly initialized or nullable
The properties
ExternalId,Status, andResultare declared as non-nullable strings but are not initialized. To prevent potential null reference exceptions, consider marking them as nullable or initializing them with default values.Apply this diff to adjust the nullability:
- public string ExternalId { get; set; } + public string? ExternalId { get; set; } - public string Status { get; set; } + public string? Status { get; set; } - public string Result { get; set; } + public string? Result { get; set; }📝 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 string? ExternalId { get; set; } public string? Status { get; set; } public string? Result { get; set; }backend/src/Designer/Repository/ORMImplementation/Models/DeploymentDbModel.cs (1)
27-29: 💡 Codebase verification
Missing delete behavior configuration for Build relationship
The relationship between Deployment and Build is configured as required, but the delete behavior is not explicitly specified. This could lead to orphaned records or runtime errors when related builds are deleted.
- Add explicit delete behavior in
DeploymentConfiguration.cs:builder.HasOne(d => d.Build) .WithMany() .HasForeignKey(d => d.InternalBuildId) .OnDelete(DeleteBehavior.Cascade) // or DeleteBehavior.Restrict .HasConstraintName("fk_deployments_builds_buildid");🔗 Analysis chain
Verify cascade delete behavior for Build relationship.
The
Buildnavigation property implies a relationship with the builds table. Ensure that the cascade delete behavior is properly configured in the correspondingEntityConfigurationclass to prevent orphaned records.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if cascade delete is configured in the entity configuration rg -A 10 "builder\.HasOne.*Build" backend/src/Designer/Repository/ORMImplementation/Data/EntityConfigurations/Length of output: 915
Script:
#!/bin/bash # Check for DbContext configuration and global cascade delete settings echo "=== Searching for DbContext configuration ===" rg -l "DbContext" backend/src/Designer/Repository/ORMImplementation/Data/ echo -e "\n=== Checking OnModelCreating and cascade delete configurations ===" rg -A 5 "OnModelCreating|DeleteBehavior" backend/src/Designer/Repository/ORMImplementation/Data/Length of output: 1563
backend/src/Designer/Migrations/20250107121538_NormalizeDeploymentsData.cs (1)
17-21:
⚠️ Potential issueImplement Down migration for safe rollbacks.
The empty Down migration could make it difficult to rollback changes if needed. Consider implementing the Down migration to restore the original state.
backend/src/Designer/Repository/ORMImplementation/Mappers/BuildMapper.cs (2)
20-29:
⚠️ Potential issueAdd null parameter validation.
The
MapToDbModelmethod should validate its input parameters.- public static BuildDbModel MapToDbModel(BuildEntity buildEntity, BuildType buildType) => - new() + public static BuildDbModel MapToDbModel(BuildEntity buildEntity, BuildType buildType) + { + ArgumentNullException.ThrowIfNull(buildEntity); + + return new() { ExternalId = buildEntity.Id, Status = buildEntity.Status.ToString(), Result = buildEntity.Result.ToString(), Started = buildEntity.Started, Finished = buildEntity.Finished, BuildType = buildType - }; + }; + }📝 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 BuildDbModel MapToDbModel(BuildEntity buildEntity, BuildType buildType) { ArgumentNullException.ThrowIfNull(buildEntity); return new() { ExternalId = buildEntity.Id, Status = buildEntity.Status.ToString(), Result = buildEntity.Result.ToString(), Started = buildEntity.Started, Finished = buildEntity.Finished, BuildType = buildType }; }
10-18:
⚠️ Potential issueAdd null parameter validation and safer enum parsing.
The
MapToModelmethod should validate the input parameter and handle potential enum parsing failures.- public static BuildEntity MapToModel(BuildDbModel buildDbModel) => - new() + public static BuildEntity MapToModel(BuildDbModel buildDbModel) + { + ArgumentNullException.ThrowIfNull(buildDbModel); + + if (!Enum.TryParse<BuildStatus>(buildDbModel.Status, true, out var status)) + throw new ArgumentException($"Invalid build status: {buildDbModel.Status}", nameof(buildDbModel)); + + if (!Enum.TryParse<BuildResult>(buildDbModel.Result, true, out var result)) + throw new ArgumentException($"Invalid build result: {buildDbModel.Result}", nameof(buildDbModel)); + + return new() { Id = buildDbModel.ExternalId, - Status = Enum.Parse<BuildStatus>(buildDbModel.Status, true), - Result = Enum.Parse<BuildResult>(buildDbModel.Result, true), + Status = status, + Result = result, Started = buildDbModel.Started?.DateTime, Finished = buildDbModel.Finished?.DateTime - }; + }; + }📝 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 BuildEntity MapToModel(BuildDbModel buildDbModel) { ArgumentNullException.ThrowIfNull(buildDbModel); if (!Enum.TryParse<BuildStatus>(buildDbModel.Status, true, out var status)) throw new ArgumentException($"Invalid build status: {buildDbModel.Status}", nameof(buildDbModel)); if (!Enum.TryParse<BuildResult>(buildDbModel.Result, true, out var result)) throw new ArgumentException($"Invalid build result: {buildDbModel.Result}", nameof(buildDbModel)); return new() { Id = buildDbModel.ExternalId, Status = status, Result = result, Started = buildDbModel.Started?.DateTime, Finished = buildDbModel.Finished?.DateTime }; }backend/src/Designer/Repository/ORMImplementation/Mappers/DeploymentMapper.cs (1)
20-34: 🛠️ Refactor suggestion
Consider adding null checks for required properties.
The mapping logic should validate that required properties like
Build,Org, andAppare not null before mapping.public static DeploymentDbModel MapToDbModel(DeploymentEntity deploymentEntity) { + ArgumentNullException.ThrowIfNull(deploymentEntity); + ArgumentNullException.ThrowIfNull(deploymentEntity.Build, nameof(deploymentEntity.Build)); return new DeploymentDbModel { Buildid = deploymentEntity.Build.Id,📝 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 DeploymentDbModel MapToDbModel(DeploymentEntity deploymentEntity) { ArgumentNullException.ThrowIfNull(deploymentEntity); ArgumentNullException.ThrowIfNull(deploymentEntity.Build, nameof(deploymentEntity.Build)); return new DeploymentDbModel { Buildid = deploymentEntity.Build.Id, Tagname = deploymentEntity.TagName, Org = deploymentEntity.Org, App = deploymentEntity.App, EnvName = deploymentEntity.EnvName, Buildresult = deploymentEntity.Build.Result.ToEnumMemberAttributeValue(), Created = deploymentEntity.Created.ToUniversalTime(), Entity = JsonSerializer.Serialize(deploymentEntity, s_jsonOptions), Build = BuildMapper.MapToDbModel(deploymentEntity.Build, BuildType.Deployment), }; }backend/tests/Designer.Tests/DbIntegrationTests/DeploymentEntityRepository/Base/DeploymentEntityIntegrationTestsBase.cs (1)
55-62: 🛠️ Refactor suggestion
Consider adding BuildType to the build mapping.
The
MapBuildToDbModelmethod should set theBuildTypeproperty to maintain consistency with the production mapper.private static Altinn.Studio.Designer.Repository.ORMImplementation.Models.BuildDbModel MapBuildToDbModel(BuildEntity buildEntity) => new() { ExternalId = buildEntity.Id, Status = buildEntity.Status.ToString(), Result = buildEntity.Result.ToString(), Started = buildEntity.Started, - Finished = buildEntity.Finished + Finished = buildEntity.Finished, + BuildType = BuildType.Deployment };📝 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.private static Altinn.Studio.Designer.Repository.ORMImplementation.Models.BuildDbModel MapBuildToDbModel(BuildEntity buildEntity) => new() { ExternalId = buildEntity.Id, Status = buildEntity.Status.ToString(), Result = buildEntity.Result.ToString(), Started = buildEntity.Started, Finished = buildEntity.Finished, BuildType = BuildType.Deployment };backend/src/Designer/Repository/ORMImplementation/DeploymentRepository.cs (1)
59-61: 🛠️ Refactor suggestion
Consider using transaction for multi-entity updates
When updating multiple related entities (Deployment and Build), consider wrapping the operations in a transaction to ensure data consistency.
+ await using var transaction = await _dbContext.Database.BeginTransactionAsync(); _dbContext.Entry(mappedDbObject).State = EntityState.Modified; _dbContext.Entry(mappedDbObject.Build).State = EntityState.Modified; - await _dbContext.SaveChangesAsync(); + try + { + await _dbContext.SaveChangesAsync(); + await transaction.CommitAsync(); + } + catch + { + await transaction.RollbackAsync(); + throw; + }📝 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.await using var transaction = await _dbContext.Database.BeginTransactionAsync(); _dbContext.Entry(mappedDbObject).State = EntityState.Modified; _dbContext.Entry(mappedDbObject.Build).State = EntityState.Modified; try { await _dbContext.SaveChangesAsync(); await transaction.CommitAsync(); } catch { await transaction.RollbackAsync(); throw; }backend/src/Designer/Migrations/20250107121305_AddBuidsTableAndDeploymentsColumns.cs (1)
74-81: 🛠️ Refactor suggestion
Add ON DELETE behavior to foreign key
The foreign key constraint should specify ON DELETE behavior to handle cascading deletes or prevent orphaned records.
migrationBuilder.AddForeignKey( name: "fk_deployments_builds_buildid", schema: "designer", table: "deployments", column: "internal_build_id", principalSchema: "designer", principalTable: "builds", - principalColumn: "id"); + principalColumn: "id", + onDelete: ReferentialAction.Restrict);📝 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.migrationBuilder.AddForeignKey( name: "fk_deployments_builds_buildid", schema: "designer", table: "deployments", column: "internal_build_id", principalSchema: "designer", principalTable: "builds", principalColumn: "id", onDelete: ReferentialAction.Restrict);backend/src/Designer/Migrations/SqlScripts/DeploymentsNormalization/normalization.sql (2)
17-26:
⚠️ Potential issueAdd validation for required fields.
The script assumes all required fields exist in the JSONB data. Missing or invalid data could lead to constraint violations or data integrity issues.
Add validation before insertion:
IF build_data IS NULL OR build_data->>'id' IS NULL THEN RAISE NOTICE 'Skipping deployment %: Missing build data', rec.sequenceno; CONTINUE; END IF; -- Validate other required fields IF build_data->>'status' IS NULL OR build_data->>'result' IS NULL THEN RAISE NOTICE 'Skipping deployment %: Missing status or result', rec.sequenceno; CONTINUE; END IF;
28-32: 🛠️ Refactor suggestion
Verify successful updates.
The script doesn't verify if the update was successful or handle potential constraint violations.
Add update verification:
UPDATE designer.deployments SET internal_build_id = new_build_id, created_by = entity_data->>'createdBy', envname = entity_data->>'envName' WHERE sequenceno = rec.sequenceno RETURNING sequenceno INTO updated_seq; IF updated_seq IS NULL THEN RAISE NOTICE 'Failed to update deployment %', rec.sequenceno; END IF;
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/Designer/Migrations/DesignerdbContextModelSnapshot.cs (2)
152-154: Consider making EnvName required.The EnvName field is nullable, which might lead to data consistency issues. Consider making it required if it's a crucial part of the deployment information.
Line range hint
1-238: Solid database normalization approach.The changes demonstrate a well-structured approach to normalizing the database schema:
- Introduction of a dedicated builds table
- Proper relationship modeling with foreign keys
- Consistent naming conventions
- Appropriate use of indexes
This will improve data integrity and query performance while maintaining backward compatibility during migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/Designer/Migrations/20250110192106_BuildsGrants.Designer.cs(1 hunks)backend/src/Designer/Migrations/20250110192106_BuildsGrants.cs(1 hunks)backend/src/Designer/Migrations/DesignerdbContextModelSnapshot.cs(5 hunks)backend/src/Designer/Migrations/SqlScripts/Builds/01-setup-grants.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/Designer/Migrations/SqlScripts/Builds/01-setup-grants.sql
- backend/src/Designer/Migrations/20250110192106_BuildsGrants.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: docker-build-push / Build and push docker image
- GitHub Check: Build environment and run e2e test
🔇 Additional comments (5)
backend/src/Designer/Migrations/DesignerdbContextModelSnapshot.cs (5)
Line range hint
25-75: LGTM! Well-structured entity definition.The AppScopesDbModel entity is well-designed with appropriate indexes and constraints. The rename from AppScopesDbObject follows better naming conventions.
227-235: LGTM! Well-defined relationship configuration.The relationship between BuildDbModel and DeploymentDbModel is properly configured with appropriate foreign key constraints and naming conventions.
76-117: Consider adding audit fields and verify the unique index design.The BuildDbModel entity is well-structured, but consider these improvements:
- Add audit fields (CreatedBy, Created, LastModifiedBy) for better traceability
- Verify if the unique index on (ExternalId, BuildType) is sufficient for your use case
Let's verify the uniqueness requirements:
✅ Verification successful
The unique index on (ExternalId, BuildType) is well-designed
The unique constraint is appropriate as it:
- Allows tracking deployment (0) and release (1) builds separately with the same ExternalId
- Prevents duplicate builds of the same type
- Supports the foreign key relationship with deployments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any other unique constraints in the codebase # that might conflict with or complement the current design # Look for unique constraint definitions rg -i "isunique|unique|key" --type cs # Look for BuildType enum definition to understand possible values ast-grep --pattern 'enum BuildType { $$$ }'Length of output: 73525
Script:
#!/bin/bash # Search for BuildType enum definition and usage rg -A 5 "enum BuildType" --type cs # Search for BuildDbModel usage to understand the business logic rg -A 5 "BuildDbModel" --type csLength of output: 8786
Line range hint
178-225: Ensure consistent data migration approach across tables.The ReleaseDbModel also contains an Entity field. Ensure the data migration strategy is consistent with the approach used for DeploymentDbModel.
Let's verify the consistency:
✅ Verification successful
Entity field implementation is consistent across models
The Entity field is implemented consistently in both ReleaseDbModel and DeploymentDbModel:
- Both use text column type
- Both are nullable
- Both use the same column name "entity"
- Both are handled similarly in the database schema and migrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare Entity field usage across models # Look for Entity field handling across models rg -B 2 -A 2 "Entity.*HasColumnType\(\"text\"\)" --type csLength of output: 58
Script:
#!/bin/bash # Search for model definitions and migrations # Find all model snapshot files fd ModelSnapshot # Search for Entity property definitions in models ast-grep --pattern 'Property<string>("Entity")' # Find all migration files fd Migrations.*cs$ --type f # Look for Entity column definitions with different formatting rg -B 2 -A 2 'Entity.*HasColumnName\("entity"\)' --type csLength of output: 227
Script:
#!/bin/bash # Examine the model snapshot file content cat backend/src/Designer/Migrations/DesignerdbContextModelSnapshot.cs | grep -A 30 "DeploymentDbModel\|ReleaseDbModel" # Look for migration files in the Designer directory fd --full-path backend/src/Designer/Migrations/ --type fLength of output: 5478
144-159: Ensure data migration for the Entity field.The Entity field appears to be retained while new fields are added. Ensure there's a data migration plan to:
- Extract data from the Entity field
- Populate the new fields (CreatedBy, EnvName, InternalBuildId)
- Clean up the Entity field once migration is complete
Let's check for migration scripts:
✅ Verification successful
Data migration is properly implemented.
The Entity field data is being correctly migrated through:
- Migration 20250107121305 adds the new columns
- Migration 20250107121538 executes normalization.sql which:
- Extracts data from Entity JSON
- Populates new columns (created_by, envname, internal_build_id)
- Includes per-record error handling
The Entity field is retained for backward compatibility. No further action required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for migration scripts handling the Entity field data # Search for migration files fd -e sql -e cs --full-path "Migrations" # Look for Entity field handling rg -A 5 "Entity" --type sql --type csLength of output: 78586
Script:
#!/bin/bash # Look for migration files and SQL scripts related to deployments normalization rg -A 10 "AddBuidsTableAndDeploymentsColumns|NormalizeDeploymentsData" --type cs # Check content of SQL normalization scripts cat backend/src/Designer/Migrations/SqlScripts/DeploymentsNormalization/normalization.sql # Look for Entity field usage in migrations rg -A 5 "Entity.*HasColumnType|\.Entity\(" --type cs backend/src/Designer/Migrations/Length of output: 32568
Description
Normalized the deployments table. Created scripts that create the builds table and migrate data from the entity column to the builds and additional tables in deployments
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Database Changes
Repository Improvements
Infrastructure