query: reject duplicate selected properties - #1010
berezovskyi wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: OSLC/oslc4net/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 12 |
| Duplication | -2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 54.64% 54.69% +0.04%
==========================================
Files 184 184
Lines 10979 10998 +19
Branches 1188 1194 +6
==========================================
+ Hits 6000 6015 +15
- Misses 4673 4675 +2
- Partials 306 308 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements validation to reject duplicate selected properties in OSLC queries and refactors the property parsing API. While the core logic for flat and nested properties is sound, there is a logic gap regarding nested wildcards (e.g., '*{a}, *{b}') which currently bypasses the duplicate check and is silently merged later in the processing pipeline.
Codacy analysis indicates the PR is up to standards, though complexity in QueryUtils.cs has increased significantly (+12) due to the new recursive validation logic. Notably, several acceptance criteria related to prefix URI hardening and the legacy API alias lack corresponding unit tests to verify failure cases.
About this PR
- The new prefix URI validation logic (line 62 in QueryUtils.cs) and the legacy alias 'parseProperties' lack specific unit tests to verify failure cases (missing brackets, empty URIs) or functional parity.
Test suggestions
- Reject duplicate top-level properties in oslc.select (e.g., 'p1,p1')
- Reject duplicate nested properties in oslc.select (e.g., 'p1{a},p1{b}')
- Reject duplicate properties in oslc.properties
- Throw ParseException for prefix URIs missing angle brackets
- Throw ParseException for empty prefix URIs (e.g., '<>')
- Verify 'parseProperties' legacy alias correctly calls 'ParseProperties'
- Ensure full coverage for recursive property validation in QueryUtils.cs
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Throw ParseException for prefix URIs missing angle brackets
2. Throw ParseException for empty prefix URIs (e.g., '<>')
3. Verify 'parseProperties' legacy alias correctly calls 'ParseProperties'
4. Ensure full coverage for recursive property validation in QueryUtils.cs
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| { | ||
| if (property is NestedProperty nestedProperty) | ||
| { | ||
| if (!property.IsWildcard && |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The validation for NestedProperty explicitly skips the duplicate identifier check when IsWildcard is true. This allows multiple nested wildcards (e.g., '*{a}, *{b}') to bypass validation and be silently merged later during inversion (lines 363-368), contradicting the goal of strictly rejecting all duplicate selections. Wildcards should be tracked in the selectedProperties set just like named properties to ensure uniqueness.
| ) => ParseProperties(propertiesExpression, prefixMap); | ||
|
|
||
| private static void | ||
| ValidateNoDuplicateProperties(Properties properties) |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This method introduces recursive property validation, contributing to a complexity increase of 12 for this file. As the coverage report is currently unavailable for this complex file, ensure that the new test cases in QueryBasicTest.cs provide full coverage for both flat and nested duplicate scenarios.
| GetPropertyName(Property property) | ||
| { | ||
| var propertyName = property.Identifier; | ||
| return propertyName.ns + propertyName.local; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Concatenating the namespace and local name without a delimiter can lead to false-positive duplicate detection if different combinations result in the same string (e.g., 'urn:a:' + 'bc' vs 'urn:ab:' + 'c'). Consider using a delimiter like '|' to ensure property identity is unique.
| ["rdf"] = "http://www.w3.org/1999/02/22-rdf-syntax-ns#", | ||
| ["xs"] = "http://www.w3.org/2001/XMLSchema" | ||
| ["dcterms"] = "http://purl.org/dc/terms/", // NOSONAR: RDF namespace identifier, not a network endpoint. | ||
| ["oslc"] = "http://open-services.net/ns/core#", // NOSONAR: RDF namespace identifier, not a network endpoint. |
There was a problem hiding this comment.
⚪ LOW RISK
This URI is a standard RDF namespace identifier, not a network endpoint. The developer's use of // NOSONAR correctly identifies this as a false positive, as changing it to 'https' would break the string identity required for RDF compatibility.
Summary
ParseExceptionduring select and properties parsing.ParsePropertiesAPI while retaining the legacy lower-case alias.Validation
OSLC4Net.FuzzingRelease build: 0 warnings, 0 errors.Follow-up to #1008 addressing its review comments.