supabase-community / postgrest-csharp

A C# Client library for Postgrest
https://supabase-community.github.io/postgrest-csharp/api/Postgrest.html
MIT License
117 stars 23 forks source link

Minor Unity Related Fixes #70

Closed wiverson closed 1 year ago

wiverson commented 1 year ago

What kind of change does this PR introduce?

  1. The Internal enum value is causing an error in Unity "Error CS0118 : 'Internal' is a namespace but is used like a variable". PR renames it to a semantically identical name that's less likely to cause a conflict.
  2. GetAssemblyVersion is used to set X-Client-Info for the requests if the header is not already set. This call to GetAssemblyVersion fails on Unity with a null reference. Replaced this with a Guid set for the app run - this will likely be more helpful for debugging (e.g. tracking down a choreography of requests in a logging solution).
acupofjose commented 1 year ago

Odd that GetAssemblyInfo fails in Unity - but the expectation is that the X-Client-Info header conforms to a particular schema (see here: https://github.com/search?q=org%3Asupabase-community+x-client-info&type=code)

You ought to be able to override it in the Headers when initializing a client:

_instance = new Supabase.Client(URL, PUBLIC_KEY, new Supabase.SupabaseOptions
{
  Headers = new Dictionary<string, string>
  {
    ["X-Client-Info"] = Guid.NewGuid().ToString()
  }
});
wiverson commented 1 year ago

I think GetAssemblyInfo fails for two potential reasons - first, when it's running in the Editor I think it's doing something interesting (bytecode off disk, caching in memory, etc). Second, when it's run through the ILL2CPP compiler it's all just native code and so technically there is no .NET assembly. This might be exacerbated by the fact that I'm using a degit version of the source in my project for testing/dev.

The schema is a bit loose, lol.

I just updated the commit to match closer to the other versions of the code (the specific GoTrue client info + assembly version) by default, while also supporting the Unity scenarios. I'm hoping to have the getting started / out-of-the-box scenario for a Unity dev to be as cake as possible, so either way I'd like a solution that "just works."

If you don't like the tracking session per app launch thing, the version that would match closer semantically would probably be something like:

if (!headers.ContainsKey("X-Client-Info"))
{
    try
    {
        // Default version to match other clients
        // https://github.com/search?q=org%3Asupabase-community+x-client-info&type=code
        headers.Add("X-Client-Info", $"postgrest-csharp/{Util.GetAssemblyVersion(typeof(Client))}");
    }
    catch (Exception)
    {
        // Fallback for when the version can't be found
        // e.g. running in the Unity Editor, ILL2CPP builds, etc.
        headers.Add("X-Client-Info", $"postgrest-csharp/0.0.1");
    }
}

...which looks like the express-js client version. Or perhaps just postgrest-csharp/unknown.

Then just doc the GUID insertion version instead. Just LMK - I prefer the out-of-box GUID for local debug but it's not that big of a deal.

acupofjose commented 1 year ago

I can dig the App launch thing as a fallback - I'll need to go implement that on the other child repos! Personally I'd place the blame on the ILL2CPP compiler. But regardless, that's a fix that works for me!

As always, thanks for your time @wiverson, it's much appreciated!