fix: CreateResourceShape TYPE_TO_VALUE_TYPE KeyNotFound #389
Conversation
WalkthroughAdds a new test project to the solution, introduces ResourceShapeFactory unit tests, and extends ResourceShapeFactory's type-to-value-type mapping to treat ICollection<Uri>, IEnumerable<Uri>, and ISet<Uri> as ValueType.Resource. Changes
Sequence Diagram(s)(omitted — changes are static type-to-value-type mapping and tests; no new runtime control-flow introduced) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (3)
8-9: Use English for all code comments.The XML documentation comment is in Chinese. According to Microsoft Framework design guidelines, code comments should be in English for consistency and maintainability.
/// <summary> -/// 测试ResourceShapeFactory.CreateResourceShape方法 +/// Tests for the ResourceShapeFactory.CreateResourceShape method /// </summary>
91-91: Use English for all code comments.The inline comment is in Chinese. Keep all comments in English for consistency.
- // ResourceShapeFactory只处理以Get或Is开头的方法，Requirement类只有从AbstractResourceRecord继承的GetTypes()方法 + // ResourceShapeFactory only processes methods starting with Get or Is, Requirement class only has GetTypes() method inherited from AbstractResourceRecord
10-15: Consider using more descriptive constant names.The constants could be more descriptive to better reflect their purpose in the OSLC context.
- private const string BaseUri = "http://example.com"; - private const string ResourceShapesPath = "resourceShapes"; - private const string ResourceShapePath = "requirement"; + private const string TestBaseUri = "http://example.com"; + private const string OslcResourceShapesPath = "resourceShapes"; + private const string RequirementResourceShapePath = "requirement";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
OSLC4Net_SDK/OSLC4Net.Core.sln(3 hunks)OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs(1 hunks)OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/OSLC4Net.Core.Tests.csproj(1 hunks)OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
⚙️ CodeRabbit Configuration File
**/*.cs: Review the C# code against the Microsoft Framework design guidelines as well as 10 guidelines
from "Building Maintainable Software" by Joost Visser. Point out deviations and improvements.
Files:
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.csOSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs
🧬 Code Graph Analysis (2)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (1)
OSLC4Net_SDK/OSLC4Net.Core/Model/ValueType.cs (2)
ValueType(69-82)ValueType(84-87)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (3)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (3)
ResourceShapeFactory(26-511)ResourceShapeFactory(38-57)ResourceShapeFactory(59-61)OSLC4Net_SDK/OSLC4Net.Domains.RequirementsManagement/Constants.cs (1)
RM(27-77)OSLC4Net_SDK/OSLC4Net.Core/Model/TypeFactory.cs (1)
GetName(41-47)
🔇 Additional comments (11)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (1)
56-56: LGTM! The mapping addition is correct and addresses the KeyNotFound issue.The addition of
TYPE_TO_VALUE_TYPE[typeof(ICollection<Uri>)] = ValueType.Resource;properly maps URI collections to resource value types. This aligns with the existing pattern whereUriis mapped toValueType.Resourceon line 55, and extends that mapping to collections of URIs, which is semantically correct for OSLC resource relationships.OSLC4Net_SDK/OSLC4Net.Core.sln (3)
45-46: LGTM! New test project properly added to the solution.The new test project is correctly defined with the appropriate GUID and path structure following the established patterns in the solution.
217-228: LGTM! Complete configuration coverage for the new test project.All solution configurations (Debug/Release across Any CPU, Mixed Platforms, and x86) are properly configured for the new test project, ensuring it builds correctly in all scenarios.
241-241: LGTM! Test project properly organized under Tests folder.The NestedProjects section correctly places the new test project under the Tests solution folder, maintaining the solution's organizational structure.
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/OSLC4Net.Core.Tests.csproj (1)
1-30: LGTM! Well-structured test project configuration.The project file follows .NET 8.0 best practices with:
- Proper targeting (
net8.0)- Appropriate project references to both core library and domain-specific components
- Comprehensive test package dependencies including xUnit v3, NSubstitute for mocking, and code coverage tools
- Correct asset configurations for analyzers and build-time dependencies
The package selection demonstrates good testing practices with modern tooling.
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (6)
16-33: LGTM! Well-structured basic validation test.The test follows the AAA pattern correctly and validates the fundamental functionality of resource shape creation including proper URI construction.
35-51: LGTM! Title validation test is well-implemented.The test properly verifies that the resource shape has the expected title as configured in the Requirement type's attributes.
53-71: LGTM! Describes validation test correctly checks domain URI.The test properly validates that the resource shape describes the correct OSLC domain URI for requirements management, using proper string comparison.
73-94: LGTM! Properties validation test appropriately checks expected properties.The test correctly validates that properties are generated and specifically checks for the "type" property, which is expected from the inheritance hierarchy.
96-118: LGTM! Detailed type property validation is thorough.The test provides comprehensive validation of the type property including its name, value type, occurrence, and RDF property definition URI, ensuring proper OSLC compliance.
120-138: LGTM! Getter method filtering validation is appropriate.The test correctly verifies that only getter methods are processed by the ResourceShapeFactory, confirming the expected behavior of filtering out setter methods and other non-getter methods.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 49.55% 50.76% +1.20%
==========================================
Files 166 166
Lines 15388 15391 +3
Branches 1024 1024
==========================================
+ Hits 7626 7813 +187
+ Misses 7479 7271 -208
- Partials 283 307 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the PR, @ZUOXIANGE! I am away from a computer for a week or so - reviewing from phone. First, a question: do the tests fail if we comment out your new mapping entry? I did not see Uri collections tested explicitly. Second, I think it would be better to map all collection types in a consistent way - did you check how other backing types than ICollection are handled? E.g. Third, I started adding support in #322 for props like ImplementedBy instead of GetImplementedBy and SetImplementedBy. I think I missed to update resource shape handling for that. Would be nice to handle that too but we can file a separate issue for that if it is too much work in this PR. Finally, we want to be careful with mapping ICollection to Uri* because there are other ways in RDF for representing collections other than repeating a prop multiple times (see https://ontola.io/blog/ordered-data-in-rdf) but we don't need to worry about this now unless you are modifying code that handles that conversion. P.S. Please update XMLDOC comments to be in English only - see good suggestions from Coderabbit for that under Nitpick Comments. |
…rable and ISet types
|
While creating the ResourceShape for Requirements, I encountered a KeyNotFoundException that required immediate attention to keep our development pipeline unblocked. I've implemented a temporary fix by adding the necessary code to resolve this specific scenario. I'd greatly appreciate your expertise in reviewing these changes to ensure they align with the project's architectural standards. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (6)
1-5: Organize imports according to C# conventions.The using directives should be organized with
Systemnamespaces first, followed by third-party libraries, then project namespaces. Additionally, there's a missingusing System.Linq;import that's required for LINQ operations used throughout the file.+using System; +using System.Linq; using OSLC4Net.Core.Model; using OSLC4Net.Core.Attribute; using OSLC4Net.Domains.RequirementsManagement; using Xunit;
14-31: Simplify test to focus on single responsibility.This test validates multiple aspects of the resource shape (non-null checks and URI construction). Consider splitting into focused tests or removing redundant assertions since the URI construction is already validated separately.
[Fact] -public void CreateResourceShape_WithRequirementType_ShouldReturnValidResourceShape() +public void CreateResourceShape_WithRequirementType_ShouldReturnNonNullResourceShape() { // Arrange var resourceType = typeof(Requirement); // Act var resourceShape = ResourceShapeFactory.CreateResourceShape( BaseUri, ResourceShapesPath, ResourceShapePath, resourceType); // Assert Assert.NotNull(resourceShape); - Assert.NotNull(resourceShape.GetAbout()); - Assert.Equal($"{BaseUri}/{ResourceShapesPath}/{ResourceShapePath}", resourceShape.GetAbout().ToString()); }
133-135: Test assertion may be too brittle.Using
Assert.Single(properties)assumes exactly one property will always be present. This could break if theRequirementclass gains additional OSLC properties. Consider using a more specific assertion.-Assert.Single(properties); -Assert.Equal("type", properties[0].GetName()); +var typeProperties = properties.Where(p => p.GetName() == "type").ToList(); +Assert.Single(typeProperties); +Assert.Equal("type", typeProperties[0].GetName());
298-332: Clean up commented code or document test intent.This test contains extensive commented-out assertions which makes it unclear whether this is work-in-progress or intentionally demonstrates unsupported functionality. The comment on Line 298-299 suggests this is intentional, but the commented code should either be removed or properly documented.
// Note: ResourceShapeFactory only supports getter/setter methods, not direct properties -// Direct property pattern is not supported by ResourceShapeFactory +// This test verifies that direct property patterns are NOT supported by ResourceShapeFactory [Fact] -public void CreateResourceShape_WithISetUriProperty_ShouldMapToResourceValueType() +public void CreateResourceShape_WithISetUriDirectProperty_ShouldNotMapProperty() { // Arrange var resourceType = typeof(TestResourceWithISetUri); // Act var resourceShape = ResourceShapeFactory.CreateResourceShape( BaseUri, ResourceShapesPath, ResourceShapePath, resourceType); // Assert + // Direct properties are not supported - only getter/setter method patterns var properties = resourceShape.GetProperties(); var uriSetProperty = properties.FirstOrDefault(p => p.GetName() == "uriSet"); Assert.Null(uriSetProperty); - //Assert.Equal("uriSet", uriSetProperty.GetName()); - - //var actualValueType = uriSetProperty.GetValueType(); - //var actualOccurs = uriSetProperty.GetOccurs(); - - //Assert.NotNull(actualValueType); - //Assert.NotNull(actualOccurs); - - //var expectedValueTypeUri = new Uri(ValueTypeExtension.ToString(OSLC4Net.Core.Model.ValueType.Resource)); - //var expectedOccursUri = new Uri(OccursExtension.ToString(OSLC4Net.Core.Model.Occurs.ZeroOrMany)); - - //Assert.Equal(expectedValueTypeUri, actualValueType); - //Assert.Equal(expectedOccursUri, actualOccurs); - //Assert.Equal("http://example.com/uriSet", uriSetProperty.GetPropertyDefinition()?.ToString()); }
392-392: Remove unnecessary 'this' qualifier.The
thisqualifier is redundant when there's no naming conflict.-this._uriCollection = uriCollection; +_uriCollection = uriCollection;
421-421: Use more readable array initialization.The current initialization
new Uri[0]can be replaced with the more readableArray.Empty<Uri>()for better intent clarity.-private Uri[] _uriArray = new Uri[0]; +private Uri[] _uriArray = Array.Empty<Uri>();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs(1 hunks)OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
⚙️ CodeRabbit Configuration File
**/*.cs: Review the C# code against the Microsoft Framework design guidelines as well as 10 guidelines
from "Building Maintainable Software" by Joost Visser. Point out deviations and improvements.
Files:
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs
🧬 Code Graph Analysis (1)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (4)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (5)
ResourceShapeFactory(26-513)ResourceShapeFactory(38-59)ResourceShapeFactory(61-63)ValueType(349-359)Occurs(371-381)OSLC4Net_SDK/OSLC4Net.Domains.RequirementsManagement/Constants.cs (1)
RM(27-77)OSLC4Net_SDK/OSLC4Net.Core/Model/ValueType.cs (1)
ValueTypeExtension(59-88)OSLC4Net_SDK/OSLC4Net.Core/Model/Occurs.cs (1)
OccursExtension(38-67)
🔇 Additional comments (5)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (5)
8-13: LGTM! Well-structured test class.The class structure follows good testing practices with clear constants and descriptive naming conventions.
68-68: Use StringComparison.Ordinal consistently.Good practice using
StringComparison.Ordinalfor URI comparisons to avoid culture-specific issues.
187-194: Excellent pattern for URI comparison validation.This test properly handles the fact that
GetValueType()andGetOccurs()return URIs by using the extension methods to get expected URI values for comparison. This pattern should be applied consistently across all similar tests.
337-362: Verify correct OSLC attribute usage pattern.The test class uses proper OSLC attributes and follows the getter/setter pattern correctly. The implementation properly handles null arrays and uses
ISet<Uri>internally while exposingUri[]externally, which is a common pattern for OSLC properties.
138-159: ResourceShapeFactory: Collection type mappings validated
- Confirmed that ResourceShapeFactory registers
ICollection<Uri>,IEnumerable<Uri>, andISet<Uri>in itsTYPE_TO_VALUE_TYPEdictionary, all mapping toValueType.Resource.- ResourceShapeFactoryTests include coverage for each of these collection types—verifying both the mapped value type and the occurrences setting.
No further action needed.
| Assert.NotNull(properties); | ||
| Assert.NotEmpty(properties); | ||
|
|
||
| var propertyNames = properties.Select(p => p.GetName()).ToList(); |
There was a problem hiding this comment.
Missing using directive causes compilation error.
The ToList() method requires using System.Linq; which is missing from the imports.
Add the missing using directive at the top of the file:
+using System.Linq;🤖 Prompt for AI Agents
In OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs around
line 89, the call to ToList() on the LINQ projection requires the System.Linq
namespace which is not imported; add a using System.Linq; directive to the top
of the file alongside the other using statements so the extension methods like
Select and ToList are available and the file compiles.
2 participants
Summary by CodeRabbit
New Features
Tests
Chores