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

Optionally set schema per PostgrestQueryBuilder #441

Closed
wants to merge 1 commit into from

Conversation

bdotsamir
Copy link

@bdotsamir bdotsamir commented Jun 26, 2023

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
Copy link
Author

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
Copy link
Author

bdotsamir commented Jun 26, 2023

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

  • supabase.setSchema('other_schema').from('table')
  • supabase.from('other_schema', 'table') or vice versa
    • Maybe even overloading this function so if the user passes in only the table it selects it from public by default?

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

@bdotsamir
Copy link
Author

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
Copy link
Contributor

sjones6 commented Aug 5, 2023

Proposing an alternative implementation to solve this, but at the supabase-js level: supabase/supabase-js#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
Copy link
Member

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

@sjones6
Copy link
Contributor

sjones6 commented Aug 5, 2023

@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
Copy link
Member

soedirgo commented Aug 5, 2023

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
Copy link
Contributor

sjones6 commented Aug 5, 2023

@soedirgo , raised a new MR to implement the schema method on PostgrestClient per direction here: #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
Copy link
Member

soedirgo commented Aug 6, 2023

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

@soedirgo soedirgo closed this Aug 6, 2023
@bdotsamir
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

5 participants