Skip to content

Fix obsolete GetRdfTypes usage in samples#608

Open
berezovskyi wants to merge 5 commits intomainfrom
fix/getrdftypes-deprecation-10586470375603891486
Open

Fix obsolete GetRdfTypes usage in samples#608
berezovskyi wants to merge 5 commits intomainfrom
fix/getrdftypes-deprecation-10586470375603891486

Conversation

@berezovskyi
Copy link
Member

@berezovskyi berezovskyi commented Feb 14, 2026

Replaced obsolete GetRdfTypes()[0] calls with GetTypes().First() in ETMSample.cs and EWMSample.cs.
This eliminates CS0618 warnings related to QmResource.GetRdfTypes and ChangeRequest.GetRdfTypes.


PR created automatically by Jules for task 10586470375603891486 started by @berezovskyi

Summary by CodeRabbit

  • Refactor

    • Added a public Types property and deprecated older get/set accessors for RDF types; examples updated to use it with no observable behavior change.
    • Generalized exception metadata to accept a broader member reference type.
    • Improved property comparison ordering with a stronger tie-breaker.
  • New Features

    • Resource shape generation now discovers and includes property-based annotations in addition to method-based ones.
  • Tests

    • Added tests for types serialization/roundtrip and resource-shape property discovery.

Replaced obsolete `GetRdfTypes()[0]` calls with `GetTypes().First()` in `ETMSample.cs` and `EWMSample.cs`.
This eliminates `CS0618` warnings related to `QmResource.GetRdfTypes` and `ChangeRequest.GetRdfTypes`.

Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 14, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replaces legacy GetTypes()/SetTypes() accessors with a new public Types property (marking old methods obsolete), updates resource classes and helpers to use the Types property, adjusts examples to use .Types.First() for RDF-type selection, and extends ResourceShapeFactory to support MemberInfo (properties and methods) when building resource shapes.

Changes

Cohort / File(s) Summary
Core model surface
OSLC4Net_SDK/OSLC4Net.Core/Model/AbstractResource.cs, OSLC4Net_SDK/OSLC4Net.Core/Model/AbstractResourceRecord.cs, OSLC4Net_SDK/OSLC4Net.Core/Model/IExtendedResource.cs
Added public Types property; marked GetTypes()/SetTypes(...) obsolete; exposed explicit interface routing and made Types settable publicly with OSLC metadata.
Resource classes (RDF accessor changes)
OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/* (e.g., ArchitectureLinkType.cs, ArchitectureResource.cs, AutomationPlan.cs, AutomationRequest.cs, AutomationResult.cs, ChangeRequest.cs, ParameterInstance.cs, QmResource.cs, RequirementBase.cs)
Replaced internal GetTypes()/SetTypes(...) usages inside GetRdfTypes()/SetRdfTypes(...) with direct Types access (Types.ToArray() / Types = ...). No signature changes.
Resource-shape generation
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs
Generalized property discovery to include PropertyInfo as well as MethodInfo by switching helpers to accept MemberInfo; added property-based name derivation, deduplication and validation logic; adjusted create/validation flows.
Exceptions (reflection metadata)
OSLC4Net_SDK/OSLC4Net.Core/Exceptions/* (OslcCoreInvalidOccursException.cs, OslcCoreInvalidPropertyTypeException.cs, OslcCoreInvalidRepresentationException.cs, OslcCoreInvalidValueTypeException.cs)
Generalized constructor parameter and public property types from MethodInfo to MemberInfo to align with MemberInfo-based resource-shape handling.
Helpers / RDF provider
OSLC4Net_SDK/OSLC4Net.Core.DotNetRdfProvider/DotNetRdfHelper.cs
Switched iterations and type lookups from GetTypes() to the new Types property for extended/any resource handling.
Change-management class
OSLC4Net_SDK/OSLC4Net.ChangeManagement/ChangeRequest.cs
GetRdfTypes/SetRdfTypes now use the Types collection directly instead of calling GetTypes()/SetTypes().
Examples
OSLC4Net_SDK/Examples/OSLC4Net.Client.Samples/ETMSample.cs, OSLC4Net_SDK/Examples/OSLC4Net.Client.Samples/EWMSample.cs
Updated creation-factory lookup call sites to select RDF type via .Types.First().ToString() instead of GetRdfTypes()[0].ToString() / GetTypes()[0].
Property ordering
OSLC4Net_SDK/OSLC4Net.Core/Model/Property.cs
Enhanced CompareTo to tie-break equal names by comparing propertyDefinition URIs deterministically.
Tests added/updated
OSLC4Net_SDK/Tests/.../ResourceShapePropertyAnnotationTests.cs, TypesPropertyRoundtripTests.cs, OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs
Added tests for Types property roundtrip and property-based resource-shape discovery; updated tests to accept discovery of both getters and C# properties and to assert presence/uniqueness of rdf:type property.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

xtra:smoke-test

Poem

🐰 I nudged the getters, gave them a hop,
Now Types stands proudly atop the crop.
From GetTypes() I leap to .Types so bright,
First() finds the path and the code feels light.
A carrot for tests — nibble, then commit!

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.16% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately reflects the primary objective: replacing obsolete GetRdfTypes usage in sample files (ETMSample.cs and EWMSample.cs) to eliminate CS0618 warnings.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/getrdftypes-deprecation-10586470375603891486

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
OSLC4Net_SDK/OSLC4Net.Core/Model/AbstractResourceRecord.cs (1)

24-24: Public property exposes concrete List<Uri> instead of an interface type.

Per the Microsoft Framework Design Guidelines, public properties should prefer IList<T>, ICollection<T>, or Collection<T> over List<T> to preserve encapsulation and allow future changes to the backing collection. That said, AbstractResource already uses the same List<Uri> type, so this is at least consistent within the codebase. Consider aligning both to IList<Uri> in a follow-up.

As per coding guidelines, "Review the C# code against the Microsoft Framework design guidelines" — specifically, CA1002: Do not expose generic lists.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Feb 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.88%. Comparing base (7962779) to head (1b60c4d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #608      +/-   ##
==========================================
+ Coverage   50.86%   51.88%   +1.02%     
==========================================
  Files         174      174              
  Lines       10202    10246      +44     
  Branches     1010     1019       +9     
==========================================
+ Hits         5189     5316     +127     
+ Misses       4756     4676      -80     
+ Partials      257      254       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Introduced `Types` property in `IExtendedResource` and implemented it in `AbstractResource` and `AbstractResourceRecord`.
- Marked `GetTypes` and `SetTypes` as `[Obsolete]` to encourage property usage.
- Updated `AbstractResource.Types` setter to create a new `List<Uri>` to ensure safe modification.
- Maintained `List<Uri>` return type in `AbstractResourceRecord.Types` via explicit interface implementation to avoid breaking changes.
- Replaced all usages of `GetTypes()` and `SetTypes()` with `.Types` property across the codebase, including samples and client resources.
- Eliminated `CS0618` warnings related to obsolete method calls.

Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
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

🤖 Fix all issues with AI agents
In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/ArchitectureLinkType.cs`:
- Around line 147-151: The Obsolete attribute messages contain a typo: change
"User" to "Use" in the Obsolete strings for the methods such as GetRdfTypes
(method name GetRdfTypes) and SetRdfTypes (method name SetRdfTypes) in
ArchitectureLinkType.cs and the corresponding obsolete attributes in other
resource files (AutomationPlan, AutomationRequest, AutomationResult,
ParameterInstance, QmResource, and both ChangeRequest files); update each
Obsolete("User GetTypes() or .Types instead") and Obsolete("User SetTypes() or
.Types instead") to Obsolete("Use GetTypes() or .Types instead") and
Obsolete("Use SetTypes() or .Types instead") respectively so the message reads
correctly.

In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/ArchitectureResource.cs`:
- Around line 236-239: The Obsolete attribute message on the SetRdfTypes method
has a typo; update the attribute string on the SetRdfTypes method in
ArchitectureResource (the [Obsolete("User SetTypes() or .Types instead")]
decoration) to read "Use SetTypes() or .Types instead" so the guidance
references "Use" not "User" while keeping the same target methods/properties
(SetTypes and Types).
- Around line 147-151: Replace the typo "User" with "Use" in the Obsolete
attribute messages across the resource classes; specifically update the Obsolete
strings on methods/properties like GetRdfTypes (returns Types.ToArray()), any
SetRdfTypes/SetTypes variants, and the corresponding GetTypes()/SetTypes()
messages so they read "Use GetTypes() or .Types instead" and "Use SetTypes() or
.Types instead" respectively; apply this change in the listed resource files and
the two occurrences per file (e.g., GetRdfTypes/GetTypes and
SetRdfTypes/SetTypes in AutomationRequest, ParameterInstance, RequirementBase,
QmResource, AutomationResult, ChangeRequest, AutomationPlan,
ArchitectureResource, ArchitectureLinkType).

In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/RequirementBase.cs`:
- Around line 390-394: The Obsolete attribute on the
RequirementBase.GetRdfTypes() method contains a typo ("User" should be "Use");
update the attribute string on GetRdfTypes() to read "[Obsolete(\"Use GetTypes()
or .Types instead\")]" so the message is correct and consistent with other
resource files.
🧹 Nitpick comments (2)
OSLC4Net_SDK/OSLC4Net.ChangeManagement/ChangeRequest.cs (1)

354-358: Inconsistent [Obsolete] attributes — missing guidance message.

Unlike all other resource files in this PR (which include "User GetTypes() or .Types instead"), the [Obsolete] attributes here on GetRdfTypes() (line 354) and SetRdfTypes() (line 716) have no message. Consider adding the same message (with the "Use" correction) so consumers see actionable guidance from the compiler warning.

The delegation to Types is correct.

Also applies to: 716-720

OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/RequirementBase.cs (1)

57-57: Remove the unused RdfTypes field.

The protected readonly ISet<Uri> RdfTypes field on line 57 is dead code. Both GetRdfTypes() and SetRdfTypes() (which are marked [Obsolete]) delegate to the inherited Types property instead, and the field is never referenced anywhere in the codebase. Removing it will eliminate confusion and improve clarity.

Comment on lines 147 to 151
[Obsolete("User GetTypes() or .Types instead")]
public Uri[] GetRdfTypes()
{
return GetTypes().ToArray();
return Types.ToArray();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in [Obsolete] message: "User" → "Use".

The obsolete attribute messages on lines 147 and 212 say "User GetTypes()..." and "User SetTypes()..." — should be "Use". This same typo is repeated across all resource files in this PR (AutomationPlan, AutomationRequest, AutomationResult, ParameterInstance, QmResource, and both ChangeRequest files).

Proposed fix (this file)
-    [Obsolete("User GetTypes() or .Types instead")]
+    [Obsolete("Use GetTypes() or .Types instead")]
     public Uri[] GetRdfTypes()
-    [Obsolete("User SetTypes() or .Types instead")]
+    [Obsolete("Use SetTypes() or .Types instead")]
     public void SetRdfTypes(Uri[] rdfTypes)
📝 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.

Suggested change
[Obsolete("User GetTypes() or .Types instead")]
public Uri[] GetRdfTypes()
{
return GetTypes().ToArray();
return Types.ToArray();
}
[Obsolete("Use GetTypes() or .Types instead")]
public Uri[] GetRdfTypes()
{
return Types.ToArray();
}
🤖 Prompt for AI Agents
In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/ArchitectureLinkType.cs` around
lines 147 - 151, The Obsolete attribute messages contain a typo: change "User"
to "Use" in the Obsolete strings for the methods such as GetRdfTypes (method
name GetRdfTypes) and SetRdfTypes (method name SetRdfTypes) in
ArchitectureLinkType.cs and the corresponding obsolete attributes in other
resource files (AutomationPlan, AutomationRequest, AutomationResult,
ParameterInstance, QmResource, and both ChangeRequest files); update each
Obsolete("User GetTypes() or .Types instead") and Obsolete("User SetTypes() or
.Types instead") to Obsolete("Use GetTypes() or .Types instead") and
Obsolete("Use SetTypes() or .Types instead") respectively so the message reads
correctly.

Comment on lines 147 to 151
[Obsolete("User GetTypes() or .Types instead")]
public Uri[] GetRdfTypes()
{
return GetTypes().ToArray();
return Types.ToArray();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n '"User [GS]etTypes' --type=cs

Repository: OSLC/oslc4net

Length of output: 2210


Fix typo in [Obsolete] messages: "User" → "Use".

Multiple resource files contain the typo "User GetTypes() or .Types instead" and "User SetTypes() or .Types instead" in their [Obsolete] attributes. The correct phrasing should use "Use" instead of "User".

This appears in 9 resource files with 18 total occurrences (2 per file):

  • AutomationRequest.cs (lines 163, 306)
  • ParameterInstance.cs (lines 95, 130)
  • RequirementBase.cs (lines 390, 611)
  • QmResource.cs (lines 90, 136)
  • AutomationResult.cs (lines 173, 363)
  • ChangeRequest.cs (lines 341, 696)
  • AutomationPlan.cs (lines 153, 245)
  • ArchitectureResource.cs (lines 147, 236)
  • ArchitectureLinkType.cs (lines 147, 212)
🤖 Prompt for AI Agents
In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/ArchitectureResource.cs` around
lines 147 - 151, Replace the typo "User" with "Use" in the Obsolete attribute
messages across the resource classes; specifically update the Obsolete strings
on methods/properties like GetRdfTypes (returns Types.ToArray()), any
SetRdfTypes/SetTypes variants, and the corresponding GetTypes()/SetTypes()
messages so they read "Use GetTypes() or .Types instead" and "Use SetTypes() or
.Types instead" respectively; apply this change in the listed resource files and
the two occurrences per file (e.g., GetRdfTypes/GetTypes and
SetRdfTypes/SetTypes in AutomationRequest, ParameterInstance, RequirementBase,
QmResource, AutomationResult, ChangeRequest, AutomationPlan,
ArchitectureResource, ArchitectureLinkType).

Comment on lines 236 to +239
[Obsolete("User SetTypes() or .Types instead")]
public void SetRdfTypes(Uri[] rdfTypes)
{
SetTypes(rdfTypes);
Types = rdfTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same typo in the setter's [Obsolete] message.

"User SetTypes() or .Types instead""Use SetTypes() or .Types instead".

🤖 Prompt for AI Agents
In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/ArchitectureResource.cs` around
lines 236 - 239, The Obsolete attribute message on the SetRdfTypes method has a
typo; update the attribute string on the SetRdfTypes method in
ArchitectureResource (the [Obsolete("User SetTypes() or .Types instead")]
decoration) to read "Use SetTypes() or .Types instead" so the guidance
references "Use" not "User" while keeping the same target methods/properties
(SetTypes and Types).

Comment on lines 390 to 394
[Obsolete("User GetTypes() or .Types instead")]
public Uri[] GetRdfTypes()
{
return GetTypes().ToArray();
return Types.ToArray();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same "User" → "Use" typo as in other resource files.

🤖 Prompt for AI Agents
In `@OSLC4Net_SDK/OSLC4Net.Client/Oslc/Resources/RequirementBase.cs` around lines
390 - 394, The Obsolete attribute on the RequirementBase.GetRdfTypes() method
contains a typo ("User" should be "Use"); update the attribute string on
GetRdfTypes() to read "[Obsolete(\"Use GetTypes() or .Types instead\")]" so the
message is correct and consistent with other resource files.

- Replaced `GetTypes()` and `SetTypes()` with `Types` property in `IExtendedResource`, `AbstractResource`, and `AbstractResourceRecord`.
- Moved OSLC metadata attributes (`[OslcDescription]`, `[OslcName]`, `[OslcPropertyDefinition]`, `[OslcTitle]`) from `GetTypes()` method to the new `Types` property.
- Updated `ResourceShapeFactory` to support inspecting both properties and methods for OSLC metadata, ensuring correct resource shape generation.
- Marked legacy `GetTypes()` and `SetTypes()` methods as `[Obsolete]`.
- Updated usage in `DotNetRdfHelper`, `ETMSample`, `EWMSample`, and generated resources to use `.Types`.
- Ensured `AbstractResource.Types` setter performs a safe list copy.
- Maintained `AbstractResourceRecord.Types` as `List<Uri>` via explicit interface implementation.

Co-authored-by: berezovskyi <64734+berezovskyi@users.noreply.github.com>
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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (1)

173-205: ⚠️ Potential issue | 🔴 Critical

GetComponentType and GetDefaultValueType will fail to compile — still expect MethodInfo instead of MemberInfo.

CreateProperty now accepts MemberInfo (line 173), but calls to helper methods pass this parameter without conversion:

  • Line 219: GetComponentType(resourceType, method, returnType) where method is MemberInfo, but GetComponentType (line 428) signature is (Type, MethodInfo, Type)
  • Line 248: GetDefaultValueType(resourceType, method, componentType) where method is MemberInfo, but GetDefaultValueType (line 394) signature is (Type, MethodInfo, Type)

Additionally, OslcCoreInvalidPropertyTypeException (line 21 in exceptions file) still expects MethodInfo method — this will also fail when called with MemberInfo from helper methods.

Update signatures for GetComponentType, GetDefaultValueType, and the exception constructor to accept MemberInfo instead of MethodInfo.

🧹 Nitpick comments (2)
OSLC4Net_SDK/OSLC4Net.Core/Model/AbstractResourceRecord.cs (1)

19-30: Consider using IList<Uri> instead of List<Uri> for the public property type.

The public Types property (Line 24) exposes the concrete List<Uri> type, while the interface (IExtendedResource) declares ICollection<Uri>. Per Microsoft Framework Design Guidelines, public APIs should generally prefer collection interfaces over concrete types to reduce coupling and allow implementation flexibility.

That said, the private set limits external mutation surface, and the explicit interface implementation on Lines 26-30 correctly bridges the types. This is a minor point.

Optional: use IList<Uri> for the public property
-    public List<Uri> Types { get; private set; } = new();
+    public IList<Uri> Types { get; private set; } = new List<Uri>();

As per coding guidelines, "Review the C# code against the Microsoft Framework design guidelines".

OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (1)

200-205: Duplicated MemberInfo return-type extraction — consider extracting a helper.

The same switch expression for deriving the return/property type from a MemberInfo appears at Lines 200-205 and Lines 473-478. A small helper method (e.g., GetMemberReturnType(MemberInfo)) would reduce duplication and make future maintenance easier. Note that DotNetRdfHelper.cs already has a similar GetBackingMemberType pattern.

♻️ Proposed helper extraction
+    private static Type GetMemberReturnType(MemberInfo member) => member switch
+    {
+        MethodInfo m => m.ReturnType,
+        PropertyInfo p => p.PropertyType,
+        _ => throw new ArgumentException("Unsupported member type", nameof(member))
+    };

Then replace both switch expressions:

-        var returnType = method switch
-        {
-            MethodInfo methodInfo => methodInfo.ReturnType,
-            PropertyInfo propertyInfo => propertyInfo.PropertyType,
-            _ => throw new ArgumentException("Unsupported member type", nameof(method))
-        };
+        var returnType = GetMemberReturnType(method);

As per coding guidelines, "Building Maintainable Software" by Joost Visser recommends avoiding code duplication (DRY principle).

Also applies to: 470-478

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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
OSLC4Net_SDK/Tests/OSLC4Net.Core.Tests/ResourceShapeFactoryTests.cs (1)

301-302: ⚠️ Potential issue | 🟡 Minor

Stale comment contradicts the new behavior.

Lines 301–302 state "ResourceShapeFactory only supports getter/setter methods, not direct properties" and "Direct property pattern is not supported by ResourceShapeFactory." This is no longer accurate — ResourceShapeFactory now iterates resourceType.GetProperties() and discovers property-based annotations.

Proposed fix
-    // Note: ResourceShapeFactory only supports getter/setter methods, not direct properties
-    // Direct property pattern is not supported by ResourceShapeFactory
+    // ResourceShapeFactory now supports both getter/setter methods and C# properties with OSLC annotations
🤖 Fix all issues with AI agents
In `@OSLC4Net_SDK/OSLC4Net.Core/Model/Property.cs`:
- Around line 62-72: The CompareTo method in class Property does not handle a
null argument and will throw on o.GetName(); update Property.CompareTo(Property
o) to first check if o is null and return a positive value (e.g., return 1) per
IComparable<T> contract, then proceed with the existing name and
propertyDefinition comparisons (using GetName() and o.propertyDefinition)
unchanged.

In
`@OSLC4Net_SDK/Tests/OSLC4Net.Core.DotNetRdfProviderTests/TypesPropertyRoundtripTests.cs`:
- Around line 52-68: The test Types_EmptyCollectionRoundtrips currently only
checks deserialization non-nullity; update it to assert the deserialized
ChangeRequest.Types collection is present and contains no extra user types (i.e.
only the resource shape's rdf:type). Specifically, in the
Types_EmptyCollectionRoundtrips test (method name
Types_EmptyCollectionRoundtrips) after calling
RdfHelpers.DeserializeAsync<ChangeRequest>, add assertions that
deserialized.Types is not null and that its Count equals the expected single
rdf:type (e.g. assert Has.Count.EqualTo(1) or equivalent) so the test verifies
no extra types were round-tripped for the ChangeRequest.Types property. Ensure
you reference the ChangeRequest.Types property when adding these assertions.
🧹 Nitpick comments (4)
OSLC4Net_SDK/OSLC4Net.Core/Exceptions/OslcCoreInvalidRepresentationException.cs (1)

19-27: Property name Method and error message text are misleading for non-method members.

Since the parameter type was widened from MethodInfo to MemberInfo, the MemberInfo could now represent a PropertyInfo. The property name Method and the error message fragment "for method {method.Name}" would be inaccurate in that case. Consider renaming the property to Member and adjusting the message string accordingly. This applies to all four exception classes in this PR.

♻️ Suggested rename
 public class OslcCoreInvalidRepresentationException(
     Type resourceType,
-    MemberInfo method,
+    MemberInfo member,
     Representation representation) : OslcCoreApplicationException(
-    $"OSLC1006: Invalid representation {RepresentationExtension.ToString(representation)} defined for method {method.Name} of class {resourceType.Name}")
+    $"OSLC1006: Invalid representation {RepresentationExtension.ToString(representation)} defined for member {member.Name} of class {resourceType.Name}")
 {
     public Type ResourceType { get; } = resourceType;
-    public MemberInfo Method { get; } = method;
+    public MemberInfo Member { get; } = member;
     public Representation Representation { get; } = representation;
 }

Note: renaming MethodMember is a breaking change for any code accessing the property. If backward compatibility is a concern, you could keep the old property as [Obsolete] and add the new one. As per coding guidelines, the Microsoft Framework Design Guidelines recommend that property names accurately describe their semantics.

OSLC4Net_SDK/OSLC4Net.Core/Model/ResourceShapeFactory.cs (2)

150-169: No setter validation for property-based definitions.

The method-based loop calls ValidateSetMethodExists (line 144) to ensure a matching setter exists. The new property loop doesn't perform any analogous check. A read-only C# property (get-only) annotated with [OslcPropertyDefinition] would be silently accepted here but could fail at deserialization time.

Consider adding a check for prop.CanWrite or prop.GetSetMethod() != null and throwing if the property isn't writable, or document that read-only properties are intentionally supported.


185-206: Repeated MemberInfo dispatch pattern.

The method switch { MethodInfo => ..., PropertyInfo => ... } pattern appears three times (lines 185, 201, 480). Consider extracting a small helper to reduce duplication:

Example helper
private static Type GetReturnType(MemberInfo member) => member switch
{
    MethodInfo m => m.ReturnType,
    PropertyInfo p => p.PropertyType,
    _ => throw new ArgumentException("Unsupported member type", nameof(member))
};

Then replace inline switches at lines 201–206 and 480–485 with GetReturnType(method).

OSLC4Net_SDK/Tests/OSLC4Net.Core.DotNetRdfProviderTests/TypesPropertyRoundtripTests.cs (1)

1-1: Unused using directive.

System.Net.Http.Headers does not appear to be directly referenced in this file.

Proposed fix
-using System.Net.Http.Headers;
 using OSLC4Net.ChangeManagement;

Comment on lines 62 to 72
public int CompareTo(Property o)
{
return name.CompareTo(o.GetName());
var nameComparison = string.Compare(name, o.GetName(), StringComparison.Ordinal);
if (nameComparison != 0)
{
return nameComparison;
}

return Uri.Compare(propertyDefinition, o.propertyDefinition,
UriComponents.AbsoluteUri, UriFormat.UriEscaped, StringComparison.Ordinal);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

CompareTo does not guard against a null argument.

Per the .NET IComparable<T> contract, CompareTo(null) should return a positive value. Currently, passing null will throw a NullReferenceException on o.GetName().

Proposed fix
 public int CompareTo(Property o)
 {
+    if (o is null) return 1;
+
     var nameComparison = string.Compare(name, o.GetName(), StringComparison.Ordinal);
📝 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.

Suggested change
public int CompareTo(Property o)
{
return name.CompareTo(o.GetName());
var nameComparison = string.Compare(name, o.GetName(), StringComparison.Ordinal);
if (nameComparison != 0)
{
return nameComparison;
}
return Uri.Compare(propertyDefinition, o.propertyDefinition,
UriComponents.AbsoluteUri, UriFormat.UriEscaped, StringComparison.Ordinal);
}
public int CompareTo(Property o)
{
if (o is null) return 1;
var nameComparison = string.Compare(name, o.GetName(), StringComparison.Ordinal);
if (nameComparison != 0)
{
return nameComparison;
}
return Uri.Compare(propertyDefinition, o.propertyDefinition,
UriComponents.AbsoluteUri, UriFormat.UriEscaped, StringComparison.Ordinal);
}
🤖 Prompt for AI Agents
In `@OSLC4Net_SDK/OSLC4Net.Core/Model/Property.cs` around lines 62 - 72, The
CompareTo method in class Property does not handle a null argument and will
throw on o.GetName(); update Property.CompareTo(Property o) to first check if o
is null and return a positive value (e.g., return 1) per IComparable<T>
contract, then proceed with the existing name and propertyDefinition comparisons
(using GetName() and o.propertyDefinition) unchanged.

Comment on lines +52 to +68
[Test]
public async Task Types_EmptyCollectionRoundtrips()
{
var cr = new ChangeRequest(new Uri("http://example.com/cr/3"));
// Don't set any types - should roundtrip as empty

var formatter = new RdfXmlMediaTypeFormatter();
var rdfXml = await RdfHelpers.SerializeAsync(formatter, cr,
OslcMediaType.APPLICATION_RDF_XML_TYPE);

var deserialized = await RdfHelpers.DeserializeAsync<ChangeRequest>(formatter, rdfXml,
OslcMediaType.APPLICATION_RDF_XML_TYPE);

await Assert.That(deserialized).IsNotNull();
// The ChangeRequest will have its own rdf:type from OslcResourceShape describes,
// but no extra types should appear beyond that
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Empty collection roundtrip test has no meaningful assertion on Types.

The test comment on lines 66–67 says "no extra types should appear beyond that," but there is no assertion verifying the Types collection on the deserialized object. The test only checks that deserialization didn't return null.

Proposed assertion
         await Assert.That(deserialized).IsNotNull();
         // The ChangeRequest will have its own rdf:type from OslcResourceShape describes,
         // but no extra types should appear beyond that
+        // Assert that no additional types were injected during roundtrip
+        await Assert.That(deserialized!.Types.Count).IsLessThanOrEqualTo(1);
🤖 Prompt for AI Agents
In
`@OSLC4Net_SDK/Tests/OSLC4Net.Core.DotNetRdfProviderTests/TypesPropertyRoundtripTests.cs`
around lines 52 - 68, The test Types_EmptyCollectionRoundtrips currently only
checks deserialization non-nullity; update it to assert the deserialized
ChangeRequest.Types collection is present and contains no extra user types (i.e.
only the resource shape's rdf:type). Specifically, in the
Types_EmptyCollectionRoundtrips test (method name
Types_EmptyCollectionRoundtrips) after calling
RdfHelpers.DeserializeAsync<ChangeRequest>, add assertions that
deserialized.Types is not null and that its Count equals the expected single
rdf:type (e.g. assert Has.Count.EqualTo(1) or equivalent) so the test verifies
no extra types were round-tripped for the ChangeRequest.Types property. Ensure
you reference the ChangeRequest.Types property when adding these assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant