-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix: remove legacy features preventing federating 2 PostGraphiles #25
Conversation
@benjie can you review it? |
It's a breaking change so I cannot merge it at this time, I'm also uncomfortable with renaming the nodeId field purely for Apollo Federation compatibility; however people can use the plugin that you have written here with PostGraphile without needing this to be merged into this plugin, so it's a great example for others - thanks! |
0ceb7c3
to
732265c
Compare
@benjie I've improved the implementation so that it will not affect Relay. Kindly review and let me know if it's possible to merge now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to simply delete the query
and node
fields from the Query
type. Since the Relay object identification specification isn't really supported by Apollo Federation anyway, I don't think keeping node
is necessary; and the query
field was always a hack/workaround for really old Relay anyway and is not needed for modern GraphQL clients.
732265c
to
bbe7633
Compare
@benjie Done. I've checked Apollo Federation which works fine regardless of query/node fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't actually need to rename the nodeIdFieldName
, we just need an alias to use for the Node
type itself - if this is the case, can you use a new separate option instead such as nodeTypeName
and use that directly: if (value === 'Node') { return nodeTypeName; }
and update the comments to match?
I actually wanted to reuse the value of So I think the current implementation is valid since users have less burden to set correct configurations for it to work. Let me know what you think? |
Where does the |
When two or more PostgraphQL services are consumed by Apollo Gateway, the |
Please delete the Query.nodeId field and test again 👍 |
The |
Just the nodeId on the Query type, not all of them 👍 |
Can not delete it because of |
Ah; remove the Node interface from Query too 👍 |
We should not remove Here is a default schema (without Federation) for my postgraphql database having two tables, securities and securityschedules for you to understand the depth of the issue.
|
Please remove the Node interface from Query and let me know the next error you get. Here's a plugin to achieve that: https://github.com/graphile/gatsby-source-pg/blob/master/lib/RemoveNodeInterfaceFromQueryPlugin.js The |
@benjie Removed the interface and then deleted
Let me know if you still want to proceed (I still feel that just renaming |
Why not? We only removed it from Query where it was causing conflicts, not any of the other types where it’s actually useful.
Again we’re only removing it from Query, not from User or Forum or Post or any other type. Honestly it’s not particularly useful having it on Query, most Relay schemas do not have a Node interface on the Query type, it’s fine. I’m trying to lead us towards the most minimally invasive way of addressing the conflicts you raise. I’m hoping Federation is smart enough to spot the Node interface is equivalent on both schemas and use it as-is regardless of naming conflict. |
…er PostgraphQL service
bbe7633
to
26928fb
Compare
=> My Bad, do not know what I was thinking (may be late night OSS 😄). You can query nodeId as before.
=> Makes sense.
=> Agree. Have pushed the updates, Please take a look. |
Gosh look at this, isn't it beautifully simple?! 🙌 Does it definitely work? |
@benjie Yeah, I'm also amazed at the final fix after spending so many hours to figure every possible way 😄 Indeed, it is working. |
@codef0rmerz I've merged in the changes I would like and have updated the issue description and title; if you can get CI to pass then we can merge 👍 You should be able to run Please do not rebase. We'll be squash merging at the end anyway; by not rebasing you make my code review easier. |
@benjie Fixed the broken tests. |
@benjie I can see that you have approved the PR but do not think I can merge it. Can you? |
Released in @graphile/[email protected] after adding #28 and #29 |
Thanks @benjie |
## DescriptionThis plugin suffixes theNode
interface withnodeIdFieldName
and deletesquery/node
fields from the rootQuery
type in order to fixGraphQLSchemaValidationError: There can be only one type named "Nodequery/node"
error. This helps Apollo Gateway to consume two or more PostgraphQL services.Only requirement for this to work is to set a uniquenodeIdFieldName
in the options parameter ofcreatePostGraphQLSchema/postgraphile
methods per PostgraphQL service.More Info: graphile/crystal#1505New description by Benjie
When you attempt to federate two PostGraphile instances you get various errors; these turn out to boil down to conflicts on fields on the Query type itself. By removing legacy features from the Query type, two or more PostGraphile instances may be merged via Apollo Federation.
Legacy features removed:
Query.query: Query
field - a hack for Relay 1, not required for modern clientsQuery
implementing theNode
interface - there's no need for Query to implement Node; we did it "for completeness" but I've never seen anyone using it in a valuable wayQuery.nodeId
field - see above - no need.Query.node(id: ID!): Node
field - this is required as part of the GraphQL Global Object Identification Specification spec, however we're not implementing that spec with Apollo Federation so we should be able to drop it.🚨 THIS IS A BREAKING CHANGE 🚨