supabase / supabase-js

An isomorphic Javascript client for Supabase. Query your Supabase database, subscribe to realtime events, upload and download files, browse typescript examples, invoke postgres functions via rpc, invoke supabase edge functions, query pgvector.
https://supabase.com
MIT License
2.86k stars 220 forks source link

Embedded resource types ignore `!inner` / `is not null` #943

Open gwax opened 4 months ago

gwax commented 4 months ago

Bug report

Describe the bug

When performing a many-to-one join in supabase using either !inner or .not('reference', 'is', null), the returned type is T | null but should be T.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

Taking the example tables:

create table countries (
  "id" serial primary key,
  "name" text
);

create table cities (
  "id" serial primary key,
  "name" text,
  "country_id" int references "countries"
);

and then create a number of queries against supabase-js using Typescript:

const query1 = await supabase.from('cities').select(`name, countries (name)`);
const query2 = await supabase.from('cities').select(`name, countries!inner (name)`);
const query3 = await supabase.from('cities').select(`name, countries (name)`).not('countries', 'is', null);
const query4 = await supabase.from('cities').select(`name, countries!inner (name)`).not('countries', 'is', null);

Expected behavior

Resulting typescript types should be:

type Query1Type = {
  name: string;
  countries: {
    name: string;
  } | null;  // <- This is debatably nullable given postgres constraints but reasonable
}[] | null;

type Query2Type = {
  name: string;
  countries: {
    name: string;
  };  // <- This should absolutely not be nullable
}[] | null;

type Query3Type = {
  name: string;
  countries: {
    name: string;
  };  // <- This should absolutely not be nullable
}[] | null;

type Query4Type = {
  name: string;
  countries: {
    name: string;
  };  // <- This should absolutely not be nullable
}[] | null;

In all cases, where I have labeled "This should absolutely not be nullable", supabase-js returns a nullable type

Screenshots

If applicable, add screenshots to help explain your problem.

System information

Additional context

Add any other context about the problem here.

masda70 commented 2 months ago

I believe this issue significantly impacts the usability of type inference for queries that involve nested resources, particularly in scenarios where the nested resource's presence is essential for the integrity of the returned data.

To elaborate, even when a database schema explicitly defines a foreign key (e.g., country_id) as NOT NULL, the type of Query1Type.countries could still be nullable. This situation arises not from the database schema itself but from the potential limitations of the client's permissions making the query. Therefore, unless the framework acknowledges the client's permissions level—specifically, whether it operates with sufficient privileges (such as a service role client)—or allows field-specific permissions overrides, marking countries as nullable remains technically correct.

However, the !inner and is not null constructs currently serve as mechanisms to enforce non-nullability constraints on a per-field basis, implicitly handling permission constraints by excluding parent objects lacking sufficient permissions. This approach, while functional, could lead to verbose query syntax due to the frequent need to specify !inner, especially in complex queries where non-nullability is a common requirement.

One potential improvement could be introducing a mechanism to set !inner as the default behavior for certain queries or within the context of specific clients or roles. This change would streamline query construction without sacrificing the flexibility or security model provided by the current permission system.

Considering that !inner aligns with the typical default behavior for Postgres queries, adopting it as a default in our context seems logical. However, this approach might diverge due to choices made in the PostgREST library, which might not set !inner as the default. While I am not fully aware of all the implications such a default behavior might entail, especially when exceptions are necessary for specific fields, I believe it is an idea worth exploring for its potential to simplify query syntax and enhance overall usability.