Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add restrictions to Transmissions reference hierarchy #1310

Merged
merged 41 commits into from
Oct 25, 2024

Conversation

oskogstad
Copy link
Collaborator

@oskogstad oskogstad commented Oct 17, 2024

Description

This restricts transmissions reference hierarchies to:

  • only have a width of 1 (two separate transmissions cannot have the same relativeTransmissionId)
  • have a chain length/depth of max 100
  • not allow cyclical references (A => B => C => A)

Added tests for the command handlers making sure things are ready before using the new validator.
Added tests for general rule violations, varying depth/width etc.,

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

Summary by CodeRabbit

  • New Features

    • Introduced a method for validating reference hierarchies in collections, enhancing error handling for hierarchical structures.
    • Enhanced dialog creation and update processes to ensure proper transmission ID assignment and validation.
  • Bug Fixes

    • Improved validation logic to prevent depth, width, and cyclic reference violations in hierarchical data.
  • Tests

    • Added new unit tests to validate reference hierarchy functionality, including scenarios for depth, width, and circular reference violations.
    • Introduced integration tests for creating and updating transmissions within dialogs, ensuring robust error handling and validation.
    • Added tests for handling related transmissions with null IDs and ensuring valid updates.

@oskogstad oskogstad changed the title feat: Add restrictions to Transmissions reference hierarchy [DRAFT] feat: Add restrictions to Transmissions reference hierarchy Oct 17, 2024
Copy link
Contributor

coderabbitai bot commented Oct 24, 2024

📝 Walkthrough
📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces a new static class ReadOnlyCollectionExtensions containing the method ValidateReferenceHierarchy, which validates the hierarchical structure of collections of entities by checking for depth, cyclic references, and width violations. Additionally, it updates the CreateDialogCommand and UpdateDialogCommand handlers to incorporate this validation method. New tests are added to ensure the correctness of the validation logic, and the project file is updated to include the FluentAssertions package for enhanced testing capabilities.

Changes

File Change Summary
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ReadOnlyCollectionExtensions.cs Added ReadOnlyCollectionExtensions class with ValidateReferenceHierarchy method for validating entity reference hierarchies.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs Updated Handle method in CreateDialogCommandHandler to include validation of transmissions using ValidateReferenceHierarchy.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs Updated Handle method in UpdateDialogCommandHandler to validate transmissions using ValidateReferenceHierarchy, removing the VerifyTransmissionRelations method.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs Added test Can_Create_Related_Transmission_With_Null_Id and modified existing tests for transmission creation.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs Added UpdateTransmissionTests class with tests for updating transmissions.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs Added HierarchyTestNode class to facilitate hierarchical structure testing.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs Introduced IHierarchyTestNodeBuilder interface and HierarchyTestNodeBuilder class for building hierarchical test nodes.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Digdir.Domain.Dialogporten.Application.Unit.Tests.csproj Added FluentAssertions package reference.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs Added ValidateReferenceHierarchyTests class with unit tests for the ValidateReferenceHierarchy method.

Possibly related issues

  • Constrain RelatedTransmissionId hierarchy #1225: This issue addresses constraints on the relatedTransmissionId hierarchy, which aligns with the validation logic introduced in this PR. The new validation checks for circular references and depth limitations, which are key aspects of this issue.

Possibly related PRs

Suggested reviewers

  • elsand

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@oskogstad oskogstad changed the title [DRAFT] feat: Add restrictions to Transmissions reference hierarchy feat: Add restrictions to Transmissions reference hierarchy Oct 24, 2024
@oskogstad oskogstad marked this pull request as ready for review October 24, 2024 22:42
@oskogstad oskogstad requested a review from a team as a code owner October 24, 2024 22:42
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Outside diff range and nitpick comments (19)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (2)

8-9: Consider making Id setter private or adding validation.

The public setter for Id could allow invalid states. Consider making it private or adding validation to ensure it's never set to default(Guid).

-public Guid Id { get; set; }
+public Guid Id { get; private set; }

51-68: Enhance cyclic structure creation with validation and documentation.

The method needs better documentation to explain the cycle creation pattern and should include comprehensive parameter validation.

+/// <summary>
+/// Creates a hierarchical structure with a cycle by connecting the last node back to an intermediate node.
+/// The resulting structure will be: node1 -> node2 -> ... -> nodeN -> node2
+/// </summary>
+/// <param name="depth">The depth of the hierarchy (must be at least 2 to create a cycle)</param>
+/// <param name="ids">Optional array of GUIDs to use for node IDs</param>
+/// <returns>Enumerable of nodes in the cyclic hierarchy</returns>
 public static IEnumerable<HierarchyTestNode> CreateCyclicDepth(int depth, params Guid[] ids)
 {
-    if (depth < 1)
+    if (depth < 2)
     {
-        yield break;
+        throw new ArgumentOutOfRangeException(nameof(depth), "Depth must be at least 2 to create a cycle");
     }
+    if (depth > 100)
+    {
+        throw new ArgumentOutOfRangeException(nameof(depth), "Depth cannot exceed 100");
+    }
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs (1)

66-82: Consider handling duplicate node IDs.

The AddRangeReturnFirst method could throw a Dictionary.Add exception if duplicate node IDs exist. Consider adding duplicate key detection or using a more explicit error message.

 private HierarchyTestNode AddRangeReturnFirst(IEnumerable<HierarchyTestNode> nodes)
 {
     using var nodeEnumerator = nodes.GetEnumerator();
     if (!nodeEnumerator.MoveNext())
     {
         throw new InvalidOperationException("Expected at least one node.");
     }

     var first = nodeEnumerator.Current;
-    _nodes.Add(first.Id, first);
+    if (!_nodes.TryAdd(first.Id, first))
+        throw new InvalidOperationException($"Duplicate node ID: {first.Id}");
     while (nodeEnumerator.MoveNext())
     {
         var current = nodeEnumerator.Current;
-        _nodes.Add(nodeEnumerator.Current.Id, nodeEnumerator.Current);
+        if (!_nodes.TryAdd(current.Id, current))
+            throw new InvalidOperationException($"Duplicate node ID: {current.Id}");
     }

     return first;
 }
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1)

104-125: Enhance test assertions for better coverage.

While the test verifies the basic success case, it could be strengthened with additional assertions to validate:

  1. The generated ID format (should be UUIDv7)
  2. The relationship between transmissions is maintained
  3. The reference hierarchy constraints are respected

Consider adding these assertions:

 // Assert
 response.TryPickT0(out var success, out _).Should().BeTrue();
 success.Should().NotBeNull();
+
+// Verify the generated ID
+var transmissionEntities = await Application.GetDbEntities<DialogTransmission>();
+transmissionEntities.Should().HaveCount(2);
+var firstTransmission = transmissionEntities.First(t => t.RelatedTransmissionId == transmissions[1].Id);
+firstTransmission.Id.Should().NotBe(default);
+firstTransmission.Id.Should().BeGreaterThan(Guid.Empty);
+
+// Verify the relationship is maintained
+firstTransmission.RelatedTransmissionId.Should().Be(transmissions[1].Id);
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs (3)

52-89: Add relationship structure validation.

While the test verifies null ID handling, it should also validate that:

  1. The relationship between transmissions is correctly established
  2. The related transmission maintains the correct hierarchy structure

Add these assertions:

 transmissionEntities.Should().HaveCount(2);
 transmissionEntities.Single(x => x.Id == newTransmission.Id).Should().NotBeNull();
+var relatedTransmission = transmissionEntities.Single(x => x.Id == newTransmission.Id);
+relatedTransmission.RelatedTransmissionId.Should().Be(existingTransmission.Id);

91-121: Enhance error validation.

The test should verify not just the presence of an error message but also the specific type of domain error to ensure the correct validation rule was triggered.

Consider adding:

 domainError.Should().NotBeNull();
+domainError.Should().BeOfType<TransmissionValidationError>();
 domainError.Errors.Should().Contain(e => e.ErrorMessage.Contains(existingTransmission.Id.ToString()!));

1-134: Add missing test cases for reference hierarchy restrictions.

The current test file doesn't cover the main objectives of the PR. Please add the following test cases:

  1. Width Restriction Tests:

    • Test attempting to create two transmissions referencing the same transmission (should fail)
    • Test successful creation of a valid single-width chain
  2. Depth Limitation Tests:

    • Test creating a chain approaching the 100-depth limit
    • Test failure when exceeding the 100-depth limit
  3. Cyclical Reference Tests:

    • Test detection of direct cycles (A → A)
    • Test detection of indirect cycles (A → B → C → A)
    • Test valid non-cyclic chains

Consider organizing these tests into separate test classes or using test categories to maintain clarity:

  • UpdateTransmissionWidthTests
  • UpdateTransmissionDepthTests
  • UpdateTransmissionCyclicTests
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)

75-79: Enhance the comment to explain UUIDv7 requirement.

While the implementation is correct, the comment could be more specific about why UUIDv7 is required for hierarchy validation.

Consider updating the comment to:

-// Ensure transmissions have a UUIDv7 ID, needed for the transmission hierarchy validation.
+// Ensure transmissions have temporally-ordered UUIDv7 IDs to maintain a consistent reference hierarchy
+// and prevent potential ID collisions during validation.

81-86: Extract magic number to a constant or configuration value.

The maxDepth of 100 is hardcoded. Consider extracting this to a named constant or configuration value for better maintainability.

+private const int MaxTransmissionHierarchyDepth = 100;
+
 _domainContext.AddErrors(dialog.Transmissions.ValidateReferenceHierarchy(
     keySelector: x => x.Id,
     parentKeySelector: x => x.RelatedTransmissionId,
     propertyName: nameof(CreateDialogCommand.Transmissions),
-    maxDepth: 100,
+    maxDepth: MaxTransmissionHierarchyDepth,
     maxWidth: 1));
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs (7)

10-32: Consider adding edge cases for depth validation.

While the current test cases cover standard scenarios well, consider adding edge cases such as:

  • maxDepth = 0 (should fail)
  • maxDepth = int.MaxValue (potential overflow scenarios)

34-56: Consider documenting the width violation scenario.

The test would benefit from a comment explaining what constitutes a width violation (i.e., multiple nodes sharing the same parent) to make the test's purpose clearer to future maintainers.


58-78: Consider adding complex cyclic reference scenarios.

While the current tests cover basic cycles well, consider adding tests for:

  • Multiple independent cycles in the same hierarchy
  • Cycles with branches (A->B->C->A with D->B)

102-106: Consider using more specific assertion messages.

The current assertions could be more descriptive. Consider using custom failure messages:

-domainFailures.Should().ContainSingle(x => x.ErrorMessage.Contains("depth violation"));
+domainFailures.Should().ContainSingle(x => x.ErrorMessage.Contains("depth violation"), 
+    "should detect exactly one depth violation");

110-129: Consider adding boundary test cases for valid hierarchies.

Consider adding test cases for:

  • Maximum allowed depth with maximum allowed width
  • Single node hierarchy
  • Multiple parallel hierarchies

143-146: Enhance error message assertions.

Consider using more specific assertions for error messages:

-domainFailure.ErrorMessage.Should().Contain(node.ParentId.ToString());
-domainFailure.ErrorMessage.Should().Contain("reference violation");
+domainFailure.ErrorMessage.Should().Match(
+    $"*reference violation*{node.ParentId}*",
+    "error message should clearly indicate the invalid parent reference");

191-198: Add XML documentation for the SUT helper method.

Consider adding XML documentation to clarify the purpose and parameters of the helper method:

+/// <summary>
+/// Validates the reference hierarchy of test nodes against specified constraints.
+/// </summary>
+/// <param name="nodes">Collection of nodes to validate</param>
+/// <param name="maxDepth">Maximum allowed depth of the hierarchy</param>
+/// <param name="maxWidth">Maximum allowed width at any level</param>
+/// <returns>List of domain failures if any constraints are violated</returns>
private static List<DomainFailure> Sut(...)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ReadOnlyCollectionExtensions.cs (3)

70-73: Correct grammatical agreement in error message

In the error message on lines 71-73, the phrase "It, and all its referencing children is in violation" should be "are in violation" to match the plural subject "It, and all its referencing children."

Apply this diff:

             errors.Add(new DomainFailure(propertyName,
                 $"Hierarchy depth violation found. {type.Name} with the following " +
                 $"ids is at depth {maxDepthViolation}, exceeding the max allowed depth of {maxDepth}. " +
-                $"It, and all its referencing children is in violation of the depth constraint. {ids}."));
+                $"It, and all its referencing children are in violation of the depth constraint. {ids}."));

89-93: Fix typo in error message: "to many" should be "too many"

In the error message constructed on lines 91-93, the phrase "ids has to many referring" should be "ids has too many referring" to correct the typo.

Apply this diff:

             errors.Add(new DomainFailure(propertyName,
                 $"Hierarchy width violation found. '{type.Name}' with the following " +
-                $"ids has to many referring {type.Name}, exceeding the max " +
+                $"ids has too many referring {type.Name}, exceeding the max " +
                 $"allowed width of {maxWidth}: {ids}."));

108-118: Rename variable 'transmissionById' to 'entitiesById' for consistency

In the ToDepthByKey method, the variable transmissionById should be renamed to entitiesById to maintain consistency and clarity, as the method is generic and applies to any TEntity, not just transmissions.

Apply this diff:

 private static Dictionary<TKey, int> ToDepthByKey<TKey, TEntity>(
-    this Dictionary<TKey, TEntity> transmissionById,
+    this Dictionary<TKey, TEntity> entitiesById,
     Func<TEntity, TKey> keySelector,
     Func<TEntity, TKey?> parentKeySelector)
     where TKey : struct
 {
     var depthByKey = new Dictionary<TKey, int>();
     foreach (var (_, current) in entitiesById)
     {
         var breadCrumbs = new HashSet<TKey>();
-        GetDepth(current, transmissionById, keySelector, parentKeySelector, depthByKey, breadCrumbs);
+        GetDepth(current, entitiesById, keySelector, parentKeySelector, depthByKey, breadCrumbs);
     }

     return depthByKey;
 }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e31c08b and 680f3b0.

📒 Files selected for processing (9)
  • src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ReadOnlyCollectionExtensions.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (2 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Digdir.Domain.Dialogporten.Application.Unit.Tests.csproj (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs (1 hunks)
🔇 Additional comments (14)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Digdir.Domain.Dialogporten.Application.Unit.Tests.csproj (1)

13-13: LGTM! Good choice of testing package.

The addition of FluentAssertions is well-suited for testing the new reference hierarchy validation rules. This package will make the test assertions more readable and expressive, particularly when validating complex hierarchical structures and their constraints.

tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (2)

18-23: LGTM!

The constructor is well-implemented with proper parameter handling and encapsulation.


1-69: Verify test coverage for all PR requirements.

Let's ensure that all test scenarios for the PR requirements are properly covered.

✅ Verification successful

Test coverage is comprehensive and properly implemented

The test implementation thoroughly covers all requirements with appropriate test cases:

  • Depth restrictions tested with various limits (1, 10, 100)
  • Width restrictions tested with various limits (1, 10, 100)
  • Cycle detection tested for:
    • Simple cycles (self-references)
    • Complex cycles of varying lengths
    • Multiple cycles in different scenarios
  • Additional edge cases covered:
    • Invalid parent references
    • Empty hierarchies
    • Complex valid hierarchies
    • Multiple simultaneous violations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify test coverage for transmission reference hierarchy restrictions

# Test 1: Check for width restriction tests
echo "Checking for width restriction tests..."
rg -l "CreateWidth.*1.*" --type cs

# Test 2: Check for depth limit tests
echo "Checking for depth limit tests..."
rg -l "CreateDepth.*100.*" --type cs

# Test 3: Check for cycle detection tests
echo "Checking for cycle detection tests..."
rg -l "CreateCyclicDepth" --type cs

# Test 4: Check for actual test assertions
echo "Checking for test assertions..."
rg -l "Should.*Throw.*Exception" --type cs

Length of output: 777


Script:

#!/bin/bash
# Let's check the actual test implementations to verify coverage

# Check the validator test implementation
echo "=== Validator Test Implementation ==="
rg -A 10 "class ValidateReferenceHierarchyTests" tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs

# Check test scenarios in the validator tests
echo -e "\n=== Test Scenarios ==="
rg "public.*void.*Test" tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs

# Check usage of HierarchyTestNode in tests
echo -e "\n=== Usage of Test Helpers ==="
rg -A 5 "CreateWidth|CreateDepth|CreateCyclicDepth" tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs

Length of output: 1051


Script:

#!/bin/bash
# Let's try a different approach to check the test file content

# Get the full content of the test file
echo "=== Full Test File Content ==="
cat tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs

# Also check for any other test files that might be using these test helpers
echo -e "\n=== Other Test Files Using Hierarchy Test Helpers ==="
rg -l "HierarchyTestNode" tests/

Length of output: 7359

tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs (3)

3-11: Well-designed interface for testing hierarchy restrictions!

The interface provides a comprehensive and fluent API for building test scenarios that cover all the PR requirements:

  • Width restriction testing via AddWidth
  • Depth limitation testing via AddDepth
  • Cyclical reference testing via CreateNewCyclicHierarchy

13-17: Efficient data structure choice with modern C# features!

The use of a Dictionary provides O(1) lookups for node references, which is crucial for efficient builder operations. The collection expression syntax [] demonstrates good use of modern C# features.


18-31: Consider adding depth parameter validation.

While the factory methods provide clear entry points for creating different hierarchy types, they should validate the depth parameter to ensure it doesn't exceed the maximum allowed depth of 100 (as per PR objectives).

Consider adding validation:

 public static IHierarchyTestNodeBuilder CreateNewHierarchy(int depth, params Guid[] ids)
 {
+    if (depth > 100)
+        throw new ArgumentException("Depth cannot exceed 100", nameof(depth));
+    if (depth < 0)
+        throw new ArgumentException("Depth cannot be negative", nameof(depth));
     return new HierarchyTestNodeBuilder(HierarchyTestNode.CreateDepth(depth, ids: ids));
 }
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/UpdateTransmissionTests.cs (2)

1-16: LGTM! Well-structured test class setup.

The test class is properly organized with appropriate test dependencies and inheritance from ApplicationCollectionFixture.


17-50: Consider adding hierarchy validation test cases.

While this test covers basic transmission creation, it doesn't validate any of the new reference hierarchy restrictions (width=1, depth<=100, no cycles) that are the focus of this PR. Consider adding assertions to verify that:

  1. The transmission hierarchy width remains 1
  2. The depth is within limits
  3. No cyclic references are created
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)

15-15: LGTM: Import required for UUID generation.

The added import is necessary for the CreateVersion7IfDefault extension method used in transmission ID generation.


81-86: Verify error handling in the validation chain.

The validation errors are added to _domainContext but not immediately checked. Verify that these errors are properly propagated and handled in the save operation chain.

Let's check the error handling flow:

#!/bin/bash
# Description: Check how validation errors from _domainContext are handled
# in the save operation chain

# Search for usages of _domainContext.AddErrors
rg "_domainContext\.AddErrors" -A 5

# Search for SaveChangesAsync implementation
ast-grep --pattern $'SaveChangesAsync($$$) {
  $$$
}'

81-86: Consider performance monitoring for large transmission sets.

The hierarchy validation could be computationally expensive for large sets of transmissions. Consider adding performance monitoring to track validation times.

Let's check for existing large transmission sets:

tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs (2)

1-9: LGTM! Well-structured test class setup.

The class structure and imports follow testing best practices, with appropriate use of FluentAssertions for readable assertions.


1-198: Overall excellent test coverage and structure!

The test suite comprehensively covers all the reference hierarchy restrictions mentioned in the PR objectives:

  1. Width restriction (limit of 1)
  2. Depth limitation (up to 100)
  3. Cyclical reference prevention

The tests are well-structured, use appropriate testing patterns, and provide good coverage of edge cases.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (1)

104-104: Verify the impact of modifying transmission IDs

Assigning new IDs to transmission.Id may affect existing references to these transmissions. Ensure that updating the IDs here does not introduce inconsistencies or break relationships elsewhere in the system.

Would you like assistance in verifying the usage of these transmission IDs across the codebase?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (1)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (1)

45-62: Enhance cyclic creation method with validation and documentation.

While the method correctly creates cyclic references for testing purposes, consider these improvements:

  1. Add validation for maximum depth of 100
  2. Add XML documentation to explain the cyclic structure created
  3. Consider adding validation for null ids array

Add documentation and validation:

+/// <summary>
+/// Creates a cyclic reference hierarchy for testing validation.
+/// The resulting structure will have the last node pointing back to the current node,
+/// creating a cycle of the specified depth.
+/// </summary>
+/// <param name="depth">The depth of the hierarchy (must be between 1 and 100)</param>
+/// <param name="ids">Optional array of GUIDs for the nodes</param>
+/// <returns>Enumerable of nodes forming a cyclic hierarchy</returns>
 public static IEnumerable<HierarchyTestNode> CreateCyclicDepth(int depth, params Guid?[] ids)
 {
+    ArgumentNullException.ThrowIfNull(ids);
+    if (depth > 100)
+        throw new ArgumentOutOfRangeException(nameof(depth), "Depth exceeds maximum allowed (100)");
     if (depth < 1)
     {
         yield break;
     }
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 680f3b0 and 3d10f63.

📒 Files selected for processing (3)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Validators/ValidateReferenceHierarchyTests.cs
🔇 Additional comments (5)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/HierarchyTestNode.cs (3)

3-10: LGTM! Well-structured class design.

The class has a clean structure with proper encapsulation of the children collection and clear property relationships.


12-16: LGTM! Constructor implementation is correct.

The private constructor correctly handles optional parameters and maintains the parent-child relationship.


18-20: Parameter validation needed in helper methods.

The previous review comment about adding parameter validation for width and depth limits is still applicable.

tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Common/IHierarchyTestNodeBuilder.cs (2)

45-49: Previous comment about null check is still valid

The issue regarding potential KeyNotFoundException in the FromNode method is still present. Please address the previous review comment.


15-15: ⚠️ Potential issue

Correct the initialization of '_nodes' dictionary

The _nodes dictionary is incorrectly initialized using []. In C#, you should initialize it with new Dictionary<Guid, HierarchyTestNode>();.

Apply this diff to fix the initialization:

-private readonly Dictionary<Guid, HierarchyTestNode> _nodes = [];
+private readonly Dictionary<Guid, HierarchyTestNode> _nodes = new Dictionary<Guid, HierarchyTestNode>();

Likely invalid or redundant comment.

Copy link

Copy link
Member

@elsand elsand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good - readable even with generous use of generics, tests appear to have full coverage, and all my attempts to break it in the API have failed. Great work :)

@oskogstad oskogstad merged commit e3d53ca into main Oct 25, 2024
23 checks passed
@oskogstad oskogstad deleted the feat/restrict-related-transmission-tree branch October 25, 2024 10:56
arealmaas added a commit that referenced this pull request Oct 29, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.27.0](v1.26.3...v1.27.0)
(2024-10-29)


### Features

* Add restrictions to Transmissions reference hierarchy
([#1310](#1310))
([e3d53ca](e3d53ca))
* **graphql:** configure opentelemetry
([#1343](#1343))
([e31c08b](e31c08b))
* **infrastructure:** add availability test for apim
([#1327](#1327))
([1f9fa2b](1f9fa2b))
* **service:** configure opentelemetry
([#1342](#1342))
([513d5e4](513d5e4))
* **utils:** configure open telemetry tracing for masstransit in aspnet
package ([#1344](#1344))
([5ec3b84](5ec3b84))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Are Almaas <arealmaas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants