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

Deprecate target disambiguation #2863

Closed
steve-chavez opened this issue Jul 13, 2023 · 4 comments · Fixed by #2898
Closed

Deprecate target disambiguation #2863

steve-chavez opened this issue Jul 13, 2023 · 4 comments · Fixed by #2898
Labels
embedding resource embedding

Comments

@steve-chavez
Copy link
Member

steve-chavez commented Jul 13, 2023

Problem

Edit: Decided below to only deprecate target disambiguation.

Embedding disambiguation is complex and it can fail in surprising ways (#2862).

Adding more syntax (#2448) would further complicate matters.

Solution

  1. Deprecate Embedding Disambiguation and document all edge cases here using computed relationships. Can be done on the next minor release.

    • Instead of "Resource Disambiguation", we can have a section for each complex relationship type. Like recursive o2o, recursive m2o, recursive m2m, multiple relationships between two tables.
  2. (WIP, needs more thinking) Remove the 300 Multiple Choices error message/hints. Disambiguating should be a server side concern only. Maybe we can log to stderr each time embedding is possible but needs a computed rel.

  3. Remove the disambiguation feature. Should be after 2 major releases.

This way we keep resource embedding simple (works for simple relationships OOTB) and avoid many errors. Computed relationships are much more predictable/understandable.

@steve-chavez
Copy link
Member Author

To completely solve the disambiguation problem, I think we need some analysis at schema cache build time. So when starting up or on a NOTIFY we could say:

WARNING: projects and clients have more than one relationship, create a computed rel to disambiguate.
WARNING: users and subscriptions ...

Otherwise these errors will only be discovered at runtime.

@steve-chavez
Copy link
Member Author

On second thought, the runtime embedded disambiguation has its uses. But it definitely cannot handle recursive m2m without complicating the syntax.

So as a first step we can deprecate target disamb and leave hint disamb documented .

@steve-chavez steve-chavez changed the title Deprecate/remove Embedding Disambiguation in favor of computed relationships Deprecate target disambiguation Aug 10, 2023
@steve-chavez
Copy link
Member Author

I believe we're in a much better state now that the docs have been revamped to mention Foreign Key Joins explicitly: https://postgrest--666.org.readthedocs.build/en/666/references/api/resource_embedding.html#foreign-key-joins

The need to specify the FK constraint is much more understandable now: https://postgrest--666.org.readthedocs.build/en/666/references/api/resource_embedding.html#foreign-key-joins-on-multiple-foreign-key-relationships

Computed relationships are still needed for recursive relationships, but pretty much every other corner case can be addressed with the !fk syntax.

@steve-chavez
Copy link
Member Author

steve-chavez commented Aug 10, 2023

Also want to mention that thanks to the nice hint we give in the error message (documented now), I no longer get support burden related to disambiguation (literally 0 issues since the hint introduction).

Users are able to solve their issues by just following the hint and they specify the FK as seen on this supabase issue

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

Successfully merging a pull request may close this issue.

1 participant