Feature Request: add interfaces to all public classes

[HunteRoi]

Hello everyone!

I would firstly like to thank you for the work you have done to create this package.

As the title might suggest, I am here to ask something of you... Would it be possible to have your opinions on the abstraction layer that is missing in Supabase-csharp and its dependencies? I would have expected to be able to use the dependency injection with this package (in order to create my own wrapper around it and not naively inject a concrete class around in my projects).

Nonetheless thank you very much for the ground work on what proves to be the next great addition to the Open Source Community!

[acupofjose]

@HunteRoi thanks! Could you clarify what you’re wanting to do? I’m not sure I understand.

[HunteRoi]

Hey there! That's a quick response.

What I am willing to do is basically create a service that uses Supabase as a dependency. However I usually try to not register dependencies as concrete implementations but rather tend to use interfaces for it.

So far, I could write an AuthService that looks like the following:

public class AuthService : IAuthService
{
    private readonly Client _gotrueClient;

    public AuthService(Client gotrueClient)
    {
      _gotrueClient = gotrueClient ?? throw new ArgumentNullException(nameof(gotrueClient));
    }

    public Task<bool> SignIn(string email)
    {
        return _gotrueClient.SignIn(email);
    }
}

where I want to create something like:

public class AuthService : IAuthService
{
    private readonly IClient _gotrueClient;

    public AuthService(IClient gotrueClient)
    {
      _gotrueClient = gotrueClient ?? throw new ArgumentNullException(nameof(gotrueClient));
    }

    public Task<bool> SignIn(string email)
    {
        return _gotrueClient.SignIn(email);
    }
}

(notice the IClient instead of Client)

EDIT: of course, this changes imply to create interfaces for everything that is public (a quick way to generate interfaces on Visual Studio is to select the class then Ctrl + r + i by the way) and no matter the repository, which is why I am opening this issue here and not on supabase-community/gotrue-csharp or any other dependencies of this main package.

EDIT 2: this could also be the ground work of a Supabase.DependencyInjection package that would contain code for the default .NET ioc container, and for other well known ones such as AutoFac, NInject, and so on...

[acupofjose]

@HunteRoi I'm not at all opposed to implementing this - but I have to admit my ignorance, the DependencyInjection pattern isn't one that I'm particularly familiar with. Would you mind providing some projects to reference or a PR?

[HunteRoi]

The dependency injection (DI) software design pattern is often used in C# with the .NET framework.
It is considered the standard way of working with dependencies in your code in .NET. The idea of the design pattern is to rely on the inversion of control principle (the D of SOLID).

If you want to know more, I recommend looking into these sources:

• SOLID principles
• Dependency inversion
• Dependency injection in .NET
• .NET abstractions - it's not just about testing

For examples of it being used, I would consider looking into repositories that use the .NET framework or directly in the framework's code.
You could take a look at old repositories of mine in C# but taking into account that I haven't updated them (except for https://github.com/HunteRoi/cloud-filesystem which is still under reconstruction) I wouldn't go too deep.

Do you need some time to think about it or should I look into making a PR ?

[HunteRoi]

As an addition and because I now see that I did not mention it before: I need interfaces in order to unit test my components and this cannot be done with concrete implementations (because it wouldn't be unit tests anymore but more like integration tests).

[TestFixture]
internal class AuthServiceTests
{
-  private Mock<Client> GotrueClientMock { get; set; }
-  // ^ wrong because I can't mock an implementation
+  private Mock<IClient> GotrueClientMock { get; set; }
+  // ^ right because I can mock the behaviour of the public methods and getters/setters

  private AuthService AuthService { get; set; }

  // skipping initialization for the sake of simplicity 

  [Test]
  public void SignIn_Should_ThrowArgumentNullException_When_EmailIsNull()
  {
    ArgumentNullException expected = new();
    string email = null;
    this.GotrueClientMock.Setup(client => client.SignIn(email)).Throws(expected).Verifiable();
    // ^ calling Setup wouldn't throw if the mock was using an abstraction class instead of implementation's

    var actual = Assert.Throws<ArgumentNullException>(() => this.AuthService.SignIn(email));

    Assert.AreSame(expected, actual);
  }
}

Moq
Unit testing

[acupofjose]

@HunteRoi thanks for being so thorough with your responses! This looks like a great thing to implement for the main library and its children.

It does look like it will significantly change the way the API currently functions - I'm assuming that implementing DI will restructure the entire project, correct? As in, we won't be able to maintain compatibility with the current API after the changes?

If you have the time to do a PR on one of the client libraries that would be much appreciated. The easiest to implement would likely be functions-csharp but if you're up to go all out, gotrue-csharp would be great.

Since this is a little out of my wheelhouse, having something to reference (that is done correctly!) would be most helpful.

[HunteRoi]

@acupofjose Like you said, the changes on each repository would represent a huge breaking change.

To summarize everything so far: I have created this issue for a need of myself (ie. add an abstraction layer to inject dependencies thus allowing me to unit test my components). However, it seems like there is a lot to do and unfortunately I do not have enough time to do it on my own...

For example, I have taken a look at functions-csharp and its Client class. The current API is a bunch of static methods and its dependencies are highly coupled with it. For a rework of it, I would suggest something belong the lines of:

// IClient.cs
namespace Supabase.Functions;
public interface IClient
{
    Task<HttpConent> RawInvoke(string url, string token = null, InvokeFunctionOptions options = null);
    Task<string> Invoke(string url, string token = null, InvokeFunctionOptions options = null);
    Task<T> Invoke<T>(string url, string token = null, InvokeFunctionOptions options = null);
}

// Client.cs
namespace Supabase.Functions;
public class Client : IClient
{
    private readonly IHttpClientFactory _httpClientFactory;

    public Client (IHttpClientFactory httpClientFactory)
    {
        _httpClientFactory = httpClientFactory ?? throw new ArgumentNullException(nameof(httpClientFactory));
    }
    
    public async Task<HttpConent> RawInvoke(string url, string token = null, InvokeFunctionOptions options = null)
        => (await HandleRequest(url, token, options)).Content;
    
    public Task<string> Invoke(string url, string token = null, InvokeFunctionOptions options = null)
    {
    	return Invoke<string>(url, token, options);
    }
    
    public async Task<T> Invoke<T>(string url, string token = null, InvokeFunctionOptions options = null)
    {
    	var response = await HandleRequest(url, token, options);
        
        var content = await response.Content.ReadAsStringAsync();
        
        return typeof(T) == content.GetType() ? content : JsonConvert.DeserializeObject<T>(content);
    }
    
    private async Task<HttpResponse> HandleRequest(string url, string token = null, InvokeFunctionOptions options = null)
    {
    	if (url == null) throw new ArgumentNullException(nameof(url));
        if (string.IsNullOrWhiteSpace(url)) throw new ArgumentException("Cannot be empty or whitespaces", nameof(url));
        
        options ??= new InvokeFunctionOptions();
        
        if (!string.IsNullOrEmpty(token)) 
        {
            options.Headers["Authorization"] = $"Bearer {token}";
        }
        options.Headers["X-Client-Info"] = Util.GetAssemblyVersion();
        
        var client = _httpClientFactory.CreateClient();
        var builder = new UriBuilder(url);
        var query = HttpUtility.ParseQueryString(builder.Query);
        builder.Query = query.ToString();
        
        using var requestMessage = new HttpRequestMessage(HttpMethod.Post, builder.Uri);
        requestMessage.Content = new StringContent(JsonConvert.SerializeObject(options.Body), Encoding.UTF8, "applicatin/json");
        if (options.Headers != null)
        {
            foreach(var headerKeyPerValue in options.Headers)
            {
                requestMessage.Headers.TryAddWithoutValidation(headerKeyPerValue.key, headerKeyPerValue.Value);
            }
        }
        
        var response = await client.SendAsync(requestMessage);
        if (!response.IsSuccessStatusCode || response.Headers.Contains("X-Relay-Error"))
        {
            var content = await response.Content.ReadAsStringAsync();
            var errorResponse = new ErrorResponse { Content = content, Message = content };
            throw new RequestException(response, errorResponse);
        }
        return response;
    }
}

We could argue that it still depends on JsonConvert from Newtonsoft.Json and HttpUtility from System.Web and Util from the same assembly as those are also static instances called within the HandleRequest method. I do agree with this but I have no other mean but to let them be as a replacement of them would require more and more work.

[acupofjose]

Thanks for taking the time to look at this @HunteRoi! I've been poking around with gotrue-csharp to see what sort of changes would need to be made while keeping the API as non-breaking as possible.

As to the static functions - this was in response to #7 where there was a request for accessing stateless functionality.

My (incomplete, working) proposal:

• We would provide interfaces for each of the public classes and changes all of the return types (where applicable) to be interfaces as well.
• Additional optional Client constructors would be made available where large class interfaces could be supplied.

For example:

public class Client : IGotrueClient 
{  
  internal IGotrueApi api;

  // new, optional constructor
  public Client(string url, string apiKey, IGotrueApi gotrueApi = null) 
  {
     //...
     if (api != null) 
     {
       api = gotrueApi;
     }
     else 
     { 
       api = new Api(url, apiKey);
    }
  }
}

• By default, the implemented client would be used, but could be overridden by a developer's own client that respects the interface contract.
• As for the various return types on a class, for example BaseResponse in the gotrue-csharp repo, I think we could handle this through supplying generic type constraints that specify the need for an IBaseResponse.

Unfortunately, this means that the libraries would be sort of straddling both worlds, which is not ideal. I'm curious as to your thoughts on that approach though?

[acupofjose]

@HunteRoi I've created a PR on the gotrue-csharp repo for you to review. The docs are currently placeholders as the plugin that was suggested requires (AFAIK) the pro version to transfer comments from the methods to the interfaces.

[HunteRoi]

@HunteRoi I've created a PR on the gotrue-csharp repo for you to review. The docs are currently placeholders as the plugin that was suggested requires (AFAIK) the pro version to transfer comments from the methods to the interfaces.

I will take a look at it! Regarding the pro version, you are probably right. If so, I am sorry for the mislead. Anyway thank you for your time! Give me some time to review the changes. I have a tough week, I am not sure I can free up a lot of time but I will try my best!

[acupofjose]

No rush, good luck out there!

[kamyker]

What I am willing to do is basically create a service that uses Supabase as a dependency. However I usually try to not register dependencies as concrete implementations but rather tend to use interfaces for it.

My 2 cents: I always avoid doing that if type isn't meant to be replaced as it makes everything more complex. The worst is trying to find concrete interface implementation between multiple classes.

Imo avoid adding interfaces if they are not needed.