supabase / postgrest-js

Isomorphic JavaScript client for PostgREST.
https://supabase.com
MIT License
965 stars 129 forks source link

Optionally set schema per PostgrestQueryBuilder #441

Closed bdotsamir closed 11 months ago

bdotsamir commented 1 year ago

What kind of change does this PR introduce?

This introduces a new method to the PostgrestQueryBuilder file that allows the developer to specify which schema they're targeting with the table specified in .from().

What is the current behavior?

Currently, there is no way to do this. If I want to have only one instance of supabase-js, I can only set its schema once- upon creation. If, somewhere else in the code, I'd like to perform an operation on a table that's in a schema other than the one I specified, I'm SOL (or: I have to create a new supabase-js instance on the fly and set its schema, then run the query, which is ugly).

Related issues:

What is the new behavior?

    supabase.from("test_table")
      .setSchema("test_schema") // ✨ :D
      // ^ I can now set the schema for this query.
      .select("*")
      .then(({ data, error }) => {
        console.log('test_schema.test_table data', data);
        console.log('test_schema.test_table error', error);
      });

    supabase.from("test_public_table")
      .select("*")
      // Note that it is optional, and does not affect other queries.
      .then(({ data, error }) => {
        console.log('public.test_public_table data', data);
        console.log('public.test_public_table error', error);
      });

Sample output: image

Additional context

Clearly, it's not just this class that needs to change. It's also the typings, and with it possibly updating the database definition generator to include all public schemas / all schemas the anon key has access to (via the project API settings, see image below). This then requires changing the way clients accept database typings (or, actually any place a Database typings file is accepted, e.g. createPages{Server,Browser}Client<Database>(), etc.). More to come later.

Also, when I created this new schema for testing purposes, I had to grant it access to the roles I wanted. It would be nice if this were an automated thing? Possibly even further scope creep, I'm not gonna push it too much.

image

By the way, the way that I tested this was not directly through new PostgrestClient(URL), I basically npm link'd the HELL out of everything. My development cycle looked like:

  1. Clone postgrest-js and supabase-js
  2. Create a test project (next.js in this case)
  3. Make the changes to postgrest-js, then npm link it so I can access this specific version of the package anywhere
  4. In supabase-js, run npm link @supabase/postgrest-js to override its dependency, instead pointing to my local copy
  5. In supabase-js, run npm link so I can access this specific overriden version anywhere
  6. In my testing project, run npm link @supabase/supabase-js
  7. Repeat steps 3 - 7.
bdotsamir commented 1 year ago

Additionally, if anybody would like to brainstorm this further (off this thread), I have a running forum post on the supabase discord. Feel free to ping me there, I'm Strange#0009.

bdotsamir commented 1 year ago

The grammar of this could be changed too, of course. It could look like:

Note to self: you have these changes in stashes on your local copy

bdotsamir commented 1 year ago

I am actually looking for feedback on this PR. It's in a deliberately "unfinished" state because I want to be sure it's been vetted by the project's members.

sjones6 commented 11 months ago

Proposing an alternative implementation to solve this, but at the supabase-js level: https://github.com/supabase/supabase-js/pull/828

Does not implement the suggestion to use something like .schema({ countries: "core", cities: "protected" }) to enable multi-schema joins in a single query.

steve-chavez commented 11 months ago

@sjones6 I suggest adding the schema() method to postgrest-js directly https://github.com/supabase/postgrest-js/issues/280#issuecomment-1666382213.

sjones6 commented 11 months ago

@sjones6 I suggest adding the schema() method to postgrest-js directly #280 (comment).

@steve-chavez To clarify, which class are you suggesting the addition to: PostgrestClient or PostgrestQueryBuilder?

Having a schema(schema: SchemaName) method on PostgrestClient doesn't seem to make as much sense since constructor accepts the schema and changing it for the whole PostgrestClient could have unexpected side-effects for other queries.

Without additionally adding a method to the supabase client to proxy to the postgrest-js class, it would have to be:

await supabase.from('objects').schema('storage').select();

and never

await supabase.schema('storage').from('objects')

It seems somewhat backwards to have the table first and then the schema, but not sure if that's a consideration at all?

soedirgo commented 11 months ago

I think it makes sense to add it to PostgrestClient, because otherwise you're not able to use .rpc() on other schemas.

changing it for the whole PostgrestClient could have unexpected side-effects for other queries

Agreed, .schema() call should leave the object untouched, and instead create a new object with the schema switched (i.e. basically your PR).

supabase.schema('storage').from('objects') feels more intuitive to me since schema is a level above tables. Makes the typings easier to implement too.

sjones6 commented 11 months ago

@soedirgo , raised a new MR to implement the schema method on PostgrestClient per direction here: https://github.com/supabase/postgrest-js/pull/455

I think then the other PR could be modified like so:

export default class SupabaseClient {
  /* snip */

  /**
   * Perform a query on a schema distinct from the default schema supplied via
   * the `options.db.schema` constructor parameter.
   *
   * The schema needs to be on the list of exposed schemas inside Supabase.
   *
   * @param schema - The name of the schema to query
   */
  schema(
    schema: string & keyof Database
  ): PostgrestClient<Database[typeof schema] extends GenericSchema ? Database[typeof schema] : any, any> {
    return this.rest.schema(schema);
  }
}

Which also makes it function similar to how .from works on the supabase client.

soedirgo commented 11 months ago

Have merged the other PR - thanks for this @bdotsamir!

bdotsamir commented 11 months ago

Whoops, sorry it took me a while to get to this. Thanks for letting me know 🫡