supabase / pg_graphql

GraphQL support for PostgreSQL
https://supabase.github.io/pg_graphql
Apache License 2.0
2.87k stars 99 forks source link

Naming reverse related fields #502

Open Aichnerc opened 6 months ago

Aichnerc commented 6 months ago

Summary

I have a table customers that contains two foreign keys (coach, partner), which reference its own table. Using GraphQL, I aim to design a query that would allow me to retrieve customers along with their associated coach and partner, both of which are also customers. However, the current GraphQL schema only provides a single customers field on customersEdge, which does not differentiate between coach and partner.

query getCustomers {
  customersCollection {
    edges {
      node {
        id
        name
        coach { <-- Should resolve the related customer
          name
        }
        partner { <-- Should resolve the related customer
          name
        }
      }
    }
  }
}

Instead, my query looks like this, combining both foreign keys under a single customers field without a clear way to distinguish between the two roles:

query getCustomers {
  customersCollection {
    edges {
      node {
        id
        name
        customers { <-- Resolves me just one foreign key. No way for me to seperate between coaches and partners.
          name
        }
      }
    }
  }
}

Is there a method or workaround to achieve the desired query structure?

Rationale

Creating distinct fields for coach and partner within the customers query is essential for clarity and to accurately represent the relationships within our data model. This would allow consumers of the GraphQL API to specifically request the data they need and understand the relationships between different customers.

Design

I do not have any known solutions.

Alternatives

Unresolved Questions

olirice commented 6 months ago

dupe of #345

the explanation of how to resolve is here: https://github.com/supabase/pg_graphql/issues/345#issuecomment-1485733489

Previously we decided that the current behavior is expected but this has come up enough times we might need to revisit.

Do you have any suggestions for what behavior you'd find more intuitive?

olirice commented 6 months ago

for example we could do

customers and customers2 to make both accessible and nudge people to manually rename one or both of the constraints with a comment directive

a challenge there would be that if the foreign key under customers gets changed, customers2 would get renamed to customers which could be confusing

Aichnerc commented 6 months ago

Thanks for the quick response!

I'm inclined towards the first option of making every relationship accessible and encouraging users to rename them. Is it feasible to display the actual column name in this process?

Additionally, I've scoured the documentation, searched online, reached out to the Supabase community, and even consulted GPT, but unfortunately, I couldn't find any information on this topic.

olirice commented 6 months ago

The solution is in docs here https://supabase.github.io/pg_graphql/configuration/#relationships-field but you're right that we should explicitly call out this issue as something it can address if we don't resolve it with the customers, customers2 solution in the next release

pawarren commented 2 months ago

I'm running into the same issue, but with unique partial indices, and I'm unable to resolve it using comment directives

(if I'm misunderstanding and this is better posted as a separate issue, let me know and I'm happy to move it!)

I have a table that looks something like this:

CREATE TABLE IF NOT EXISTS userspace.readthroughs (
    user_id UUID NOT NULL,
    work_id UUID NOT NULL,
    status userspace.READTHROUGH_STATUS NOT NULL,
    FOREIGN KEY (user_id) REFERENCES auth.users (id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (work_id) REFERENCES catalog.works (work_id) ON UPDATE CASCADE,
    FOREIGN KEY (user_id, work_id) REFERENCES userspace.works (user_id, work_id) ON DELETE CASCADE ON UPDATE CASCADE,
);

When I look at userspaceWorks in the GraphiQL docs, I see:

userspaceReadthroughsCollection(
first: Int
last: Int
before: Cursor
after: Cursor
offset: Int
filter: userspaceReadthroughsFilter
orderBy: [userspaceReadthroughsOrderBy!]
): userspaceReadthroughsConnection!

So far so good. But now I want to add a unique partial index to ensure data integrity:

CREATE UNIQUE INDEX idx_unique_active_readthrough ON userspace.readthroughs (user_id, work_id)
WHERE status IN ('in_progress', 'want_to_read');

When I do that, and go back to userspaceWorks in the GraphiQL docs, userspaceReadthroughsCollection is gone and replaced with userspaceReadthroughs: userspaceReadthroughs!

This is exactly the same behavior I see when I run into name conflict issues with foreign key references (table1.foo references FIELD, and table1.bar also references FIELD), so I think this is caused by having both the (user_id, work_id) compound foreign key and the (user_id, work_id) as part of a unique partial index

...but I think the unique partial indices don't actually create an fkey, and comments on the indices seem to be ignored, so I'm actually unable to resolve this

I'm currently using triggers to ensure data integrity, but would of course prefer to use a much cleaner unique partial index.

pawarren commented 2 months ago

Also: I would've preferred customer, customer1 instead of simply colliding the names

This is also a great fit for another linter rule, I think

When I ran into this in the wild, it was a headache to chase down

I had a table like this:

CREATE TABLE IF NOT EXISTS userspace.list_items (
    list_id UUID NOT NULL,
    user_id UUID NOT NULL,
    work_id UUID DEFAULT NULL,
    FOREIGN KEY (list_id) REFERENCES userspace.lists (list_id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (user_id, list_id) REFERENCES userspace.lists (user_id, list_id) ON DELETE CASCADE ON UPDATE CASCADE,
);

Then we decided to make it so lists could contain other lists, so we changed it to something like:

CREATE TABLE IF NOT EXISTS userspace.list_items (
    list_id UUID NOT NULL,
    user_id UUID NOT NULL,
    work_id UUID DEFAULT NULL,
    other_list_id UUID DEFAULT NULL,
    FOREIGN KEY (list_id) REFERENCES userspace.lists (list_id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (other_list_id) REFERENCES userspace.lists (list_id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (user_id, list_id) REFERENCES userspace.lists (user_id, list_id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (user_id, other_list_id) REFERENCES userspace.lists (
        user_id, list_id
    ) ON DELETE CASCADE ON UPDATE CASCADE,
);

All of a sudden, lists broke

Took a while to chase down, but it was the same issue as above:

    FOREIGN KEY (list_id) REFERENCES userspace.lists (list_id) ON DELETE CASCADE ON UPDATE CASCADE,
    FOREIGN KEY (other_list_id) REFERENCES userspace.lists (list_id) ON DELETE CASCADE ON UPDATE CASCADE,

because they reference the same field, they collided in the GraphQL docs, and the "list_id" reference was silently overwritten by "other_list_id" reference

I had to add this so the two relationships were considered separate by pg_graphql:

COMMENT ON CONSTRAINT list_items_other_list_id_fkey ON "userspace"."list_items" IS E'@graphql({"local_name": "otherListId", "foreign_name": "otherList"})';

Similarly, I had to do the same for the compound fkey: COMMENT ON CONSTRAINT list_items_user_id_other_list_id_fkey ON "userspace"."list_items" IS E'@graphql({"local_name": "userOtherList", "foreign_name": "useOtherList"})';

I definitely would've preferred that the two relationships were considered separate by in pg_graphql

And this also feels like something that would be a good warning in a linter, "hey you have two fkeys in the same table pointing to the same field, consider adding a comment directive to distinguish them" (or to rename them)

olirice commented 2 months ago

@pawarren this sounds like, at minimum, we may be handling partial unique indexes as fully unique indexes.

Could you please try to create a minimal reproducible example and open a separate issue for it. You covered a lot of ground in the explanation there. The more narrowly focused the issue is the easier it'll be for us to track down

pawarren commented 2 months ago

Done!