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

Use primary key fields if available for the @key directive #2

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jgzuke
Copy link

@jgzuke jgzuke commented Jun 30, 2019

If there are a set of primaryKeyConstraints for a given resource, use those for the federation key.
If there is no primaryKeyConstraint, use the nodeId same as before.

See #1 for more explanation

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m happy merging something like this, but you also need to add support for this selection set to the _entities field which currently uses resolveNode (the node interface) to load the data. I think you’d need to use selectGraphQLResultsFromTable instead, figuring out the table from the __typename, and using the new build.scopeByType lookup (something like scopeByType(getTypeByName(__typename)).pgIntrospection).

@jgzuke
Copy link
Author

jgzuke commented Jul 6, 2019

@benjie I couldnt find selectGraphQLResultsFromTable , but using getNodeIdForTypeAndIdentifiers seemed to work well so I went with that

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent start! Sorry for the delay getting back to you, July was incredibly busy but I'm spending August catching up so responsiveness should be better soon :) Hopefully the comments in this review are useful.

? primaryKeyNames.join(" ")
: nodeIdFieldName;

astNode.directives.push(Directive("key", { fields: StringValue(keyName) }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely support the nodeId by default; using the primary keys directly is not a best practice. Adding support for primary keys is desired if the node plugin is disabled, and it could also be used as an alternate fetcher. Fortunately according to the Apollo federation spec you can provide @key multiple times; so this is what we should do - once for nodeId, and once for the PKs.

} = scopeByType.get(type);
const identifiers = attrs.map(
attr => representation[inflection.column(attr)]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good until here, but from here on we want to query in a way that will work even if we don't have the NodePlugin loaded. You should have access to resolveInfo.graphile.selectGraphQLResultsFromTable which you can read about here:

https://www.graphile.org/postgraphile/make-extend-schema-plugin/#the-selectgraphqlresultfromtable-helper

@phryneas
Copy link
Contributor

Hey @jgzuke - I've tried to re-implement this with benjies requested changes over in #3 - could you please give that a spin if it works out for you so we could maybe move this topic forward?

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

Successfully merging this pull request may close these issues.

3 participants