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

Proposal to refactor and simplify the Supabase.Fetcher module #51

Open
zoedsoupe opened this issue Jan 4, 2025 · 10 comments · May be fixed by #54
Open

Proposal to refactor and simplify the Supabase.Fetcher module #51

zoedsoupe opened this issue Jan 4, 2025 · 10 comments · May be fixed by #54
Labels
enhancement New feature or request

Comments

@zoedsoupe
Copy link
Collaborator

zoedsoupe commented Jan 4, 2025

Chore

Describe the chore

Objective: Enhance the Supabase.Fetcher module to provide a higher-level API that seamlessly integrates with Supabase.Client, reducing redundancy and improving error handling across various Supabase integration libraries.

Key Enhancements:

  1. Centralized Error Handling:

    • Implementation:
      Introduce a behaviour, Supabase.Error, to standardize error deserialization across different Supabase services. Each service can implement this behaviour to handle its specific error formats. Additionally, implement a unified Supabase.Error struct to encapsulate errors with contextual information such as the request path, arguments, and a semantic error atom.
    • Example:
      defmodule Supabase.Error do
         @type t :: %__MODULE__{
         	code: atom, 
         	message: String.t(), 
         	path: String.t(),
         	service: :database | :storage | :auth | :functions | :realtime,
         	args: map
         }
      
        defstruct [:code, :message, :path, :args, :service]
      
        @callback from_response(Finch.response(), context :: map) :: {:ok, t} | {:error, atom}
      end
      
      defmodule Supabase.Storage.Error do
        @behaviour Supabase.ErrorParser
      
        def from_response(%Finch.Response{body: body}, %{path: path, args: args}) do
          %{"code" => code, "message" => message} = body
      
          %Supabase.Error{
            code: String.to_existing_atom(code),
            message: message,
            path: path,
            args: args,
            service: :storage
          }
        end
      end
  2. Enhanced Integration with Supabase.Client:

    • Implementation:
      Refactor Supabase.Fetcher to accept a Supabase.Client struct, automatically applying necessary headers and base URLs. This approach eliminates repetitive code in integration libraries and ensures consistent request construction. Also implement a request builder approach, so we can build the request with in different ways depending on the service using it without coupling the Fetcher directly to these services, so we avoid code duplication.
    • Example:
      defmodule Supabase.Fetcher do
         def new(client) do
           # merge headers
           %Finch.Request{}
         end
      
         def with_method(builder, method) do
           %{builder | method: method}
         end
      
         def with_path(builder, path) do
       	  %{builder | path: path}
         end
      
         def with_body(builder, body) do
           %{builder | body: body}
         end
      
         def with_headers(builder, headers) do
           %{builder | headers: headers}
         end
      
         def with_opts(builder, opts) do
       	  %{builder | opts: opts}
         end
      end
  3. Simplified Usage in Integration Libraries:

    • Implementation:
      With the refactored Fetcher, integration libraries like storage-ex and postgrest-ex can perform API requests without manually managing headers or constructing URLs manually. This streamlines their codebases and ensures consistency across different services.
    • Example:
      defmodule Supabase.Storage do
        def list_buckets(client) do
          Supabase.Fetcher.new(client)
          |> Supabase.Fetcher.with_method(:get)
          |> Supabase.Fetcher.with_path("/bucket")
          |> then(fn
            {:ok, resp} -> parse_buckets(resp)
            {:error, resp} -> Supabase.Storage.Error.from_response(resp)
          end)
        end
      
        defp parse_buckets(response_body) do
          # Parse the response body into a list of Bucket structs
        end
      end

Benefits:

  • Consistency: Standardized error parsing across services ensures uniform error handling.
  • Reduced Redundancy: Automatic application of client headers and base URLs minimizes repetitive code.
  • Improved Developer Experience: Integration libraries can focus on core functionalities without dealing with low-level HTTP details.

Additional context

This refactor aims to align the Elixir Supabase client with best practices observed in other language implementations within the Supabase ecosystem, promoting a cohesive developer experience across different platforms.

For reference, Supabase's documentation provides insights into error codes and handling mechanisms:

These resources can guide the implementation of the SupabaseErrorParser protocol for different services.

@filipecabaco
Copy link

Love the base ideas! I have some feedback regarding point 2.

I'm wondering if we should make Supabase.Fetcher a behaviour so we can implement multiple backends (Finch, Req, etc) similar to exaws

https://github.com/ex-aws/ex_aws/blob/main/lib/ex_aws/request/req.ex

This would also impact point 3 where we could have this abstracted by the behaviour (with_method, with_path, etc are callbacks that can be used by Fetcher to build a request regardless of backend chosen)

@zoedsoupe
Copy link
Collaborator Author

for sure! actually this probably will be the ideal future idea. currently there's a Supabase.FetcherBehviour but callbacks are couple with the Finch.Response structure, but i think this can easily be extended to be a generic behaviour for different http clients

on #54 we can take a look on how it should look like to have this refactor and i can easily extend it to be more generic since the Supabase.Fetcher module already manages it own structure to then build the Finch request.

although i would say it isn't worthy to implement new http clients backends for now and only extract the Finch impl to its own module, since the majority of the lib users already uses a finch-based client like req or even mint, and also i think we could focus on making the adjacent integrations (storage, auth-ex) more stable for now and then extend with kinda "optional" http clients implementations, wdyt?

@grdsdev
Copy link

grdsdev commented Jan 6, 2025

although i would say it isn't worthy to implement new http clients backends for now and only extract the Finch impl to its own module, since the majority of the lib users already uses a finch-based client like req or even mint, and also i think we could focus on making the adjacent integrations (storage, auth-ex) more stable for now and then extend with kinda "optional" http clients implementations, wdyt?

I strongly agree with this.

@filipecabaco
Copy link

it isn't worthy to implement new http clients backends for now and only extract the Finch impl to its own module

also fully agree, we can just setup things up for the future and this abstraction will also make other changes more future ready 👍

@zoedsoupe
Copy link
Collaborator Author

@filipecabaco wdyt? #55

also, could I ask for a general review for open PRs, kinda there's a merge queue since one depends of another, so it will help the review if some more basic ones are merged.

@filipecabaco
Copy link

Changes look good 👍 do have some notes on readability but we can tackle it later or even automate it with credo 👍

@filipecabaco
Copy link

approved the PR, really awesome change that really feels like a great step forward ❤️

@zoedsoupe
Copy link
Collaborator Author

do have some notes on readability but we can tackle it later or even automate it with credo

what's on your mind? I'm basically using the default options for credo, does supabase have custom credo rules for elixir projects?

@zoedsoupe zoedsoupe linked a pull request Jan 7, 2025 that will close this issue
@filipecabaco
Copy link

mainly code readability: with into pipe, function extraction, etc small stuff that we can tackle and use credo strict.

at supabase no but it's something i want do bring soon as currently we're breaking a lot of rules 😰

again, not for today 😄

@zoedsoupe
Copy link
Collaborator Author

understandable!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants