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

v11.0.0 bug/breaking change: null filtering on embedded resource and disambiguation #2862

Closed
steve-chavez opened this issue Jul 13, 2023 · 4 comments · Fixed by #2880
Closed
Labels
breaking change A bug fix or enhancement that would cause a breaking change bug embedding resource embedding

Comments

@steve-chavez
Copy link
Member

Having:

create table profiles (
  id uuid not null,
  username text null,
  constraint profiles_pkey primary key (id)
);

create table user_friend (
  id bigint generated by default as identity not null,
  user1 uuid null,
  user2 uuid null,
  constraint user_friend_pkey primary key (id),
  constraint user_friend_user1_fkey foreign key (user1) references profiles (id) on delete cascade,
  constraint user_friend_user2_fkey foreign key (user2) references profiles (id) on delete cascade
);

The following now fails.

curl 'localhost:3000/user_friend?select=*,user1(*),user2(*)&user1=eq.a02fb934-3a4d-469b-a6b6-4fcd88b973cf'

{"code":"PGRST120","details":"Only is null or not is null filters are allowed on embedded resources","hint":null,"message":"Bad operator on the 'user1' embedded resource"}stevechavez@pop-os:~/

Because the filter is interpreted as a null filter on an embed.

A workaround is:

curl 'localhost:3000/user_friend?select=*,profile1:profiles!user1(*),profile2:profiles!user2(*)&user1=eq.a02fb934-3a4d-469b-a6b6-4fcd88b973cf'
@steve-chavez steve-chavez added embedding resource embedding breaking change A bug fix or enhancement that would cause a breaking change labels Jul 13, 2023
@steve-chavez
Copy link
Member Author

steve-chavez commented Jul 13, 2023

The issue is bc it uses target disambiguation using a column name for disambiguating multiple relationships between two tables. The above workaround is basically moving it to use hint disambiguation with the table name + column.

Considering #2863, I'll label it as won't fix and close it once we announce the deprecation.

@steve-chavez steve-chavez changed the title v11 breaking change: null filtering on embedded resource and disambiguation v11.0.0 breaking change: null filtering on embedded resource and disambiguation Jul 13, 2023
@steve-chavez
Copy link
Member Author

Seems this is unfixable when having a foreign key column that has the same name as the table. Like:

create table foo(
status integer references status(id)
)

These cases are unrelated to disambiguation.

This is definitely worth fixing but I'm not sure how to do it atm. We might need to rework the null filtering feature.

@steve-chavez steve-chavez added bug and removed wont-fix labels Jul 25, 2023
@steve-chavez steve-chavez changed the title v11.0.0 breaking change: null filtering on embedded resource and disambiguation v11.0.0 bug/breaking change: null filtering on embedded resource and disambiguation Jul 25, 2023
@steve-chavez
Copy link
Member Author

We might need to rework the null filtering feature.

One option could be to ditch the is.null validation on embeds and interpret eq.3 and others are done on columns. But this would also lead to ambiguity related errors.

Another safer option might be doing:

  • embed=exists instead of embed=not.is.null
  • embed=not.exists instead of embed=is.null

Exists would only operate on embedded resources. This would also cause a breaking change regardless.

@steve-chavez
Copy link
Member Author

One option could be to ditch the is.null validation on embeds and interpret eq.3 and others are done on columns

This might not be a bad fix in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A bug fix or enhancement that would cause a breaking change bug embedding resource embedding
Development

Successfully merging a pull request may close this issue.

1 participant