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

[DRAFT] feat: new client based on RestSharp #225

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions OSLC4Net_SDK/Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
<!-- depedencyManagement / convergence -->
<PackageVersion Include="System.Net.Http" Version="4.3.4" />
<PackageVersion Include="System.Text.RegularExpressions" Version="4.3.1" />
<PackageVersion Include="System.Text.Json" Version="9.0.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Downgrade System.Text.Json and related packages to .NET 8.0

The project is currently targeting .NET 8.0 and netstandard2.x, but uses .NET 9.0.0 preview packages. This mismatch could lead to compatibility issues. Recommend downgrading the following packages to their .NET 8.0 stable versions:

  • System.Text.Json
  • Microsoft.Extensions.Configuration.Json
  • Microsoft.Extensions.DependencyInjection
  • Microsoft.Extensions.DependencyInjection.Abstractions
  • Microsoft.Extensions.Hosting.Abstractions
  • Microsoft.Extensions.Logging.Abstractions
🔗 Analysis chain

Consider using stable version of System.Text.Json

Version 9.0.0 is a preview version as .NET 9 is not yet released. Consider using a stable version unless there's a specific need for preview features.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if other dependencies require .NET 9
rg "Version=\"9\.0\.0\"" OSLC4Net_SDK/Directory.Packages.props

Length of output: 592


Script:

#!/bin/bash
# Check target framework in project files
rg -l "TargetFramework" OSLC4Net_SDK/ | xargs cat | rg "TargetFramework"

# Check global.json if it exists
fd "global.json" OSLC4Net_SDK/ --exec cat {}

Length of output: 771

<!-- Main deps -->
<PackageVersion Include="dotNetRdf.Core" Version="3.3.0" />
<PackageVersion Include="Antlr3.Runtime" Version="3.5.1" />
<PackageVersion Include="RestSharp" Version="112.1.0" />
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

Fix invalid RestSharp version

The specified version 112.1.0 doesn't exist. The latest stable version is 110.2.0.

Apply this diff to fix the version:

-    <PackageVersion Include="RestSharp" Version="112.1.0" />
+    <PackageVersion Include="RestSharp" Version="110.2.0" />
📝 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
<PackageVersion Include="RestSharp" Version="112.1.0" />
<PackageVersion Include="RestSharp" Version="110.2.0" />

<!-- conneg support &c for System.Net.Http -->
<PackageVersion Include="Microsoft.AspNet.WebApi.Client" Version="6.0.0" />
<!-- Test deps -->
Expand Down
10 changes: 10 additions & 0 deletions OSLC4Net_SDK/OSLC4Net.Client.RestSharp/IOslcClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
using System;
using System.Threading.Tasks;
using OSLC4Net.Core.Model;

namespace OSLC4Net.Client.RestSharp;

public interface IOslcClient
{
Task<IOslcResponse<T>> GetResourceAsync<T>(Uri resource) where T : IResource, new();
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Error handling needs improvement in GetResourceAsync implementation

The current implementation has several gaps in error handling:

  1. The WrapOslcResponse method only handles successful responses (status code 200) and throws NotImplementedException for all other cases.
  2. While RestSharpResponseFactory.ResponseStatusFrom maps HTTP status codes to OSLC statuses, these aren't properly handled:
    • Authentication failures (401)
    • Authorization failures (403)
    • Client errors (400, 402-499)
    • Server errors (500+)
    • Network failures
  3. No specific exception types are defined for different error scenarios.

Recommendations:

  • Replace NotImplementedException with proper error handling for each status code
  • Add specific exception types for different failure scenarios
  • Handle network and deserialization errors
🔗 Analysis chain

Verify error handling in the implementation.

The interface method should have comprehensive error handling in its implementation for various scenarios:

  • Network failures
  • Authentication errors
  • Invalid resource formats
  • Server errors

Let's check the implementation:


Let me analyze the implementation further to verify error handling.


Let me check the RestSharpResponseFactory and OslcResourceResponseSharp implementations to complete the error handling analysis.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Look for error handling in the implementation
# Expected: Implementation should handle various error scenarios

# Search for the implementation class and error handling
rg -A 10 "class.*OslcClientSharpBase.*IOslcClient"
rg "catch|throw" --type csharp

Length of output: 34662


Script:

#!/bin/bash
# Look for the implementation of GetResourceAsync and related methods
ast-grep --pattern 'GetResourceAsync<T>(Uri resource) where T : IResource, new()'

# Look for WrapOslcResponse implementation since it's used in GetResourceAsync
rg -A 10 "WrapOslcResponse" --type csharp

Length of output: 1408


Script:

#!/bin/bash
# Look for RestSharpResponseFactory implementation
rg -A 10 "class RestSharpResponseFactory" --type csharp
rg -A 10 "ResponseStatusFrom" --type csharp

# Look for OslcResourceResponseSharp implementation
rg -A 10 "class OslcResourceResponseSharp" --type csharp

Length of output: 5579

}
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider expanding the interface for production readiness.

While the current implementation is clean and follows C# conventions, consider adding the following essential features for a more robust client interface:

  1. Cancellation support via CancellationToken
  2. Additional HTTP methods (POST, PUT, DELETE)
  3. Authentication/authorization mechanism
  4. Resource cleanup pattern (implement IDisposable if managing connections)

Here's a suggested expansion of the interface:

 public interface IOslcClient
 {
-    Task<IOslcResponse<T>> GetResourceAsync<T>(Uri resource) where T : IResource, new();
+    Task<IOslcResponse<T>> GetResourceAsync<T>(
+        Uri resource,
+        CancellationToken cancellationToken = default) where T : IResource, new();
+    
+    Task<IOslcResponse<T>> CreateResourceAsync<T>(
+        Uri resource,
+        T content,
+        CancellationToken cancellationToken = default) where T : IResource, new();
+    
+    Task<IOslcResponse<T>> UpdateResourceAsync<T>(
+        Uri resource,
+        T content,
+        CancellationToken cancellationToken = default) where T : IResource, new();
+    
+    Task<IOslcResponse> DeleteResourceAsync(
+        Uri resource,
+        CancellationToken cancellationToken = default);
+    
+    void SetAuthentication(IOslcAuthentication authentication);
 }

Additional interface for authentication:

public interface IOslcAuthentication
{
    Task AuthenticateRequestAsync(IRestRequest request);
}

43 changes: 43 additions & 0 deletions OSLC4Net_SDK/OSLC4Net.Client.RestSharp/IOslcResponse.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
using System;
using OSLC4Net.Core.Model;

namespace OSLC4Net.Client.RestSharp;

/// <summary>
/// Interface for responses for OSLC resource requests.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IOslcResponse<T> where T : IResource
{
Uri Uri { get; }

OslcResponseStatus Status { get; }
}

public enum OslcResponseStatus
{
Success,
SuccessNoResource,
AuthnFault,
ClientNonAuthnFault,
ServerFault
}

/// <summary>
/// Interface for a successful OSLC resource request containing a resource in the response.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IOslcResourceResponse<T> : IOslcResponse<T> where T : IResource
{
T Resource { get; }
}


/// <summary>
/// Interface for a successful OSLC resource request containing a resource in the response.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IOslcAuthNeededResponse<T> : IOslcResponse<T> where T : IResource
{

}
Comment on lines +36 to +43
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

Fix documentation and consider interface design.

The interface has several issues:

  1. The documentation is incorrectly copied from IOslcResourceResponse
  2. The purpose of this empty interface should be documented
  3. Contains unnecessary blank line
 /// <summary>
-///     Interface for a successful OSLC resource request containing a resource in the response.
+///     Marker interface for OSLC responses that require authentication.
+///     This interface is used to distinguish responses that failed due to authentication issues,
+///     allowing specialized handling of authentication scenarios.
 /// </summary>
 /// <typeparam name="T"></typeparam>
 public interface IOslcAuthNeededResponse<T> : IOslcResponse<T> where T : IResource
 {
-    
 }
📝 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
/// <summary>
/// Interface for a successful OSLC resource request containing a resource in the response.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IOslcAuthNeededResponse<T> : IOslcResponse<T> where T : IResource
{
}
/// <summary>
/// Marker interface for OSLC responses that require authentication.
/// This interface is used to distinguish responses that failed due to authentication issues,
/// allowing specialized handling of authentication scenarios.
/// </summary>
/// <typeparam name="T"></typeparam>
public interface IOslcAuthNeededResponse<T> : IOslcResponse<T> where T : IResource
{
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netstandard2.1</TargetFramework>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="RestSharp" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\OSLC4Net.Core.DotNetRdfProvider\OSLC4Net.Core.DotNetRdfProvider.csproj" />
</ItemGroup>

</Project>
152 changes: 152 additions & 0 deletions OSLC4Net_SDK/OSLC4Net.Client.RestSharp/OslcClientSharp.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
using System;
using System.Diagnostics;
using System.Threading.Tasks;
using OSLC4Net.Core.Model;
using RestSharp;
using RestSharp.Authenticators;

namespace OSLC4Net.Client.RestSharp;

/// <summary>
/// RestSharp based OSLC client
/// </summary>
// TODO: consider a base class for all RestSharp clients but without auth
public abstract class OslcClientSharpBase : IOslcClient
{
protected RestClient Client { get; set; } = null!;

public async Task<IOslcResponse<T>> GetResourceAsync<T>(Uri resourceUri)
where T : IResource, new()
{
var request = PrepareGetRequest<T>(resourceUri);

return WrapOslcResponse<T>(resourceUri, await Client.GetAsync(request));
}

private static IOslcResponse<T> WrapOslcResponse<T>(Uri resourceUri, RestResponse response)
where T : IResource, new()
{
if (RestSharpResponseFactory.ResponseStatusFrom(response) == OslcResponseStatus.Success)
{
return new OslcResourceResponseSharp<T>(resourceUri, response);
}

throw new NotImplementedException();
}
Comment on lines +26 to +35
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

Replace NotImplementedException with appropriate error handling

The WrapOslcResponse<T> method throws a NotImplementedException when the response status is not Success. Throwing NotImplementedException can be misleading, as it suggests that the method is incomplete.

Implement proper error handling by throwing an exception that accurately reflects the error condition. For example, you could throw an OslcClientException with details from the response.

Apply this diff to fix the issue:

private static IOslcResponse<T> WrapOslcResponse<T>(Uri resourceUri, RestResponse response)
    where T : IResource, new()
{
    if (RestSharpResponseFactory.ResponseStatusFrom(response) == OslcResponseStatus.Success)
    {
        return new OslcResourceResponseSharp<T>(resourceUri, response);
    }

-    throw new NotImplementedException();
+    var status = RestSharpResponseFactory.ResponseStatusFrom(response);
+    throw new OslcClientException($"Failed to retrieve resource from {resourceUri}. Status: {status}, HTTP Status Code: {response.StatusCode}, Message: {response.ErrorMessage}");
}
📝 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
private static IOslcResponse<T> WrapOslcResponse<T>(Uri resourceUri, RestResponse response)
where T : IResource, new()
{
if (RestSharpResponseFactory.ResponseStatusFrom(response) == OslcResponseStatus.Success)
{
return new OslcResourceResponseSharp<T>(resourceUri, response);
}
throw new NotImplementedException();
}
private static IOslcResponse<T> WrapOslcResponse<T>(Uri resourceUri, RestResponse response)
where T : IResource, new()
{
if (RestSharpResponseFactory.ResponseStatusFrom(response) == OslcResponseStatus.Success)
{
return new OslcResourceResponseSharp<T>(resourceUri, response);
}
var status = RestSharpResponseFactory.ResponseStatusFrom(response);
throw new OslcClientException($"Failed to retrieve resource from {resourceUri}. Status: {status}, HTTP Status Code: {response.StatusCode}, Message: {response.ErrorMessage}");
}


private static RestRequest PrepareGetRequest<T>(Uri resource) where T : IResource
{
return new RestRequest(resource);
}
}

// no auth
public abstract class OslcClientSharpPublic : OslcClientSharpBase
{
/// <inheritdoc />
protected OslcClientSharpPublic(RestClient? client = null)
{
Client = client ?? new RestClient();
}
}

public abstract class OslcClientSharpBasic : OslcClientSharpBase
{
public OslcClientSharpBasic(string username, string password, RestClientOptions? options = null)
{
var restClientOptions = options ?? new RestClientOptions();
restClientOptions.Authenticator = new HttpBasicAuthenticator(username, password);

Client = new RestClient(restClientOptions);
}
}

// TODO: implement an ASP.NET Core server to test the dance
public abstract class OslcClientSharpOauth10a : OslcClientSharpBase
{
public OslcClientSharpOauth10a(string key, string secret, string accessToken,
string accessTokenSecret, RestClientOptions? options = null)
{
var restClientOptions = options ?? new RestClientOptions
{
Authenticator = OAuth1Authenticator.ForProtectedResource(key, secret,
accessToken, accessTokenSecret)
};

Client = new RestClient(restClientOptions);
}
}

public interface IResponseSharp
{
public RestResponse RestSharpResponse { get; }
}

internal class ResponseSharpMixin<T> : IResponseSharp where T : IResource, new()
{
// TODO: implement IRestSerializer based on our MediaFormatter
// https://restsharp.dev/docs/advanced/serialization#custom
private readonly Lazy<T> _resourceDeserialized = new(() => new T());

public OslcResponseStatus Status =>
RestSharpResponseFactory.ResponseStatusFrom(RestSharpResponse);

/// <inheritdoc />
public RestResponse RestSharpResponse { get; }

public T DeserializeResource()
{
Debug.Assert(_resourceDeserialized.Value != null, "_resourceDeserialized.Value != null");
return _resourceDeserialized.Value;
}
Comment on lines +89 to +101
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

Ensure proper deserialization of resources

In ResponseSharpMixin<T>, the _resourceDeserialized is initialized with new T(), which means it returns a new default instance instead of deserializing the actual response content. This will lead to incorrect resource handling.

Modify the code to deserialize the resource from the RestSharpResponse. Here's how you can adjust it:

-private readonly Lazy<T> _resourceDeserialized = new(() => new T());
+private readonly Lazy<T> _resourceDeserialized;

+public ResponseSharpMixin(RestResponse restSharpResponse)
+{
+    RestSharpResponse = restSharpResponse;
+    _resourceDeserialized = new Lazy<T>(() => DeserializeFromResponse(restSharpResponse));
+}

+private T DeserializeFromResponse(RestResponse response)
+{
+    // Implement deserialization logic using the custom serializer
+    // For example:
+    // return YourCustomSerializer.Deserialize<T>(response.Content);
+    throw new NotImplementedException();
+}

public T DeserializeResource()
{
    Debug.Assert(_resourceDeserialized.Value != null, "_resourceDeserialized.Value != null");
    return _resourceDeserialized.Value;
}

Would you like help in implementing the DeserializeFromResponse method?

Committable suggestion skipped: line range outside the PR's diff.


public static ResponseSharpMixin<TT> From<TT>(RestResponse restSharpResponse)
where TT : IResource, new()
{
throw new NotImplementedException();
}
Comment on lines +103 to +107
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

Implement the From<TT> factory method

The static method From<TT> in ResponseSharpMixin<T> currently throws NotImplementedException. This method is essential for creating instances of ResponseSharpMixin<T> from a RestResponse.

Implement the method to return a new instance of ResponseSharpMixin<TT>:

public static ResponseSharpMixin<TT> From<TT>(RestResponse restSharpResponse)
    where TT : IResource, new()
{
-    throw new NotImplementedException();
+    return new ResponseSharpMixin<TT>(restSharpResponse);
}

Ensure the constructor ResponseSharpMixin<T>(RestResponse restSharpResponse) is properly defined.

Committable suggestion skipped: line range outside the PR's diff.

}

public class OslcResourceResponseSharp<T> : IResponseSharp, IOslcResourceResponse<T>
where T : IResource, new()
{
public OslcResourceResponseSharp(Uri uri, RestResponse restSharpResponse)
{
Uri = uri;
Mixin = ResponseSharpMixin<T>.From<T>(restSharpResponse);
}

private ResponseSharpMixin<T> Mixin { get; }

public Uri Uri { get; }

// TODO
/// <inheritdoc />
public OslcResponseStatus Status => Mixin.Status;

public T Resource => Mixin.DeserializeResource();

public RestResponse RestSharpResponse => Mixin.RestSharpResponse;
}
Comment on lines +110 to +130
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

Fix instantiation in OslcResourceResponseSharp<T>

In the constructor of OslcResourceResponseSharp<T>, you are calling ResponseSharpMixin<T>.From<T>(restSharpResponse), but since From<TT> is not implemented, this will result in a NotImplementedException.

After implementing the From<TT> method as previously suggested, ensure that OslcResourceResponseSharp<T> correctly initializes its mixin.

Additionally, verify that all methods in OslcResourceResponseSharp<T> delegate appropriately to the mixin.

Would you like assistance in reviewing and adjusting the implementation?


public class RestSharpResponseFactory
{
public static OslcResponseStatus ResponseStatusFrom(RestResponse response)
{
return (int)response.StatusCode switch
{
> 0 and < 200 => OslcResponseStatus.SuccessNoResource,
200 => OslcResponseStatus.Success,
// TODO: introduce "technical"/intermediate statuses
> 200 and < 400 => OslcResponseStatus.SuccessNoResource,
400 or (>= 402 and < 500) => OslcResponseStatus.ClientNonAuthnFault,
// 403 is an authz fault, need to think what to do with it. In theory, nothing,
// because a non-logged in used should get 401 and a logged in has nothing
// to do other than to ask for access.
// But maybe worth introducing OslcResponseStatus.AuthzFault
401 => OslcResponseStatus.AuthnFault,
>= 500 and <= 599 => OslcResponseStatus.ServerFault,
_ => throw new ArgumentOutOfRangeException()
};
}
}
Comment on lines +132 to +152
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle unanticipated HTTP status codes gracefully

In RestSharpResponseFactory.ResponseStatusFrom, if the StatusCode does not match any of the specified cases, an ArgumentOutOfRangeException is thrown. This can cause the application to crash on unexpected status codes.

Consider handling unknown status codes by returning a default or unknown status, or by logging a warning. Here's a possible adjustment:

>= 500 and <= 599 => OslcResponseStatus.ServerFault,
-    _ => throw new ArgumentOutOfRangeException()
+    _ => OslcResponseStatus.UnknownError

Additionally, since there are TODO comments about handling specific statuses like 403, you might want to add explicit cases for them.

Would you like help in refining the status code handling logic?

Committable suggestion skipped: line range outside the PR's diff.

Loading
Loading