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

Types for join involving Computed Relationships do not work #602

Open
2 tasks done
hmnd opened this issue Feb 14, 2025 · 5 comments
Open
2 tasks done

Types for join involving Computed Relationships do not work #602

hmnd opened this issue Feb 14, 2025 · 5 comments
Labels
bug Something isn't working

Comments

@hmnd
Copy link

hmnd commented Feb 14, 2025

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

Resolving joins on Computed Relationships in types broke somewhere between v1.16.3 and v1.17.7, resulting in an error like: SelectQueryError<"could not find the relation between placement and hotel">.

To Reproduce

See src/supabase.test-d.ts in the working and broken branches of my repro repo: https://github.com/hmnd/postgrest-computed-rels-repro

Expected behavior

Computed Relationship is resolved successfully.

@hmnd hmnd added the bug Something isn't working label Feb 14, 2025
@hmnd
Copy link
Author

hmnd commented Feb 14, 2025

Possibly related to #474, but this broke in a later version of supabase-js: v2.47.7

@hmnd
Copy link
Author

hmnd commented Feb 14, 2025

I've added an additional branch to the same repo that demonstrates a separate, but related issue: https://github.com/hmnd/postgrest-computed-rels-repro/tree/test-diff-name

When a computed relationship is defined with a name that does not match the table involved (eg. function is placements(hotel), whereas table is placement), the types only work with the table's name, rather than the function's name, which is the opposite of what the postgrest api expects.

@jdgamble555
Copy link

Yes, this is definitely needed. I believe this broke when the other typing problems were fixed.

Perfect example:

CREATE OR REPLACE FUNCTION public.course_options_single(courses)
RETURNS course_options
LANGUAGE sql
SET search_path = ''
AS $$
    SELECT *
    FROM public.course_options co
    WHERE co.user_id = auth.uid() AND co.course_id = $1.course_id LIMIT 1;
$$;

Returning a type should return the type in a single fashion (or null), while returning a setof type should return an array. Neither of these work.

J

@avallete
Copy link
Member

avallete commented Feb 24, 2025

Hey there ! Thank's for the ping @jdgamble555 and thank's for the reproduction case @hmnd

However I think we're confusing two things togethers. For what I can see in the repro, the issue is that when a function name is exactly the same as a table name, then the types attempts to do an "embedded relationship", and when it fail to find a relationship between the current table embedded table relation, does not fallback onto the functions call. I think what happened before was just that we didn't checked if the relations existed, if your looking closely at your reproduction, before the new version it also had a bug, as the infered result wan't your function returns, but instead, it was the table embeding. You can see that if you do the following:

test("computed relationships", async () => {
  const { data, error } = await supabase
    .from("placement")
    // This should not infer, since `placements` is available on the table row, but is not part of your function return
    // but it does work in older version, because the result of the inference isn't actually your function return result but rather
    // the `Database['public']['hotel']['Row']` embedded ressource
    .select(`id, hotel(id,name, placements)`);
  if (error) {
    // ignore
  }
  expectTypeOf(data).toEqualTypeOf<Array<{
    id: number;
    hotel: Array<Pick<Tables<"hotel">, "id" | "name" | "placements">>;
  }> | null>();
});

What you're speaking about if I'm following @jdgamble555 is about having an inference for when the return type is "another table" as mentioned here this will require changes in the way we introspect the database to get this information and provide it at type level as mentioned here: #474 (comment)

We have plan to address this eventually (as it also give us trouble in our own codebase 😅) but it'll probably be addressed into a completely separate issue.

PS: For now, one of the mitigation we'll provide is a way to just "override" what is failing to be automatically infered in your query (eg: function based embedding) (see: #606). This should mitigate the pain of having to completely redeclare the returns type of a query and allow you to only "hint what it fails to infer properly" but keep most of the result infered from the query string.

@hmnd
Copy link
Author

hmnd commented Feb 24, 2025

@avallete thanks for looking into this; and interesting background regarding the previous buggy behavior.

I like the mitigation of overrideType! Could @supabase/supabase-js maybe get an update soon, so I can use it in our supabase projects?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants