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

[WIP] Fix (#4283 #3582): Union with interfaces issue #4298

Closed

Conversation

mpospelov
Copy link

@mpospelov mpospelov commented Jun 23, 2020

Purpose

This is a bugfix for #4283 issue

Root cause

The problem comes from the following optimization #3582

The idea of optimization is to prevent expanding interfaces if it's possible, the problem that this case also appears where it should not - when types are defined in separate services.
As a result, this optimization leads to ignoring fetching from external services

Kudos to @id-ilych for finding a solution!
Thanks for your hard work! 🚀 And hope you will find this bug fix useful 😉

@apollo-cla
Copy link

@mpospelov: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@id-ilych id-ilych force-pushed the union-with-interfaces-issue branch from 8eb655b to 05875b4 Compare June 23, 2020 19:11
@id-ilych id-ilych force-pushed the union-with-interfaces-issue branch from 05875b4 to 4e72fb3 Compare June 24, 2020 09:00
@mpospelov mpospelov changed the title Union with interfaces issue Fix (#4283 #3582): Union with interfaces issue Jun 24, 2020
@abernix abernix closed this Jun 24, 2020
@id-ilych
Copy link

@abernix Hello 👋 Could you elaborate on the reason this PR is closed?

@abernix
Copy link
Member

abernix commented Jun 24, 2020

Unintentional closing! Please stand-by: #4304

@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:00
@havenchyk
Copy link

@abernix could you please take a look? we need this soon 😄

@LucienLee
Copy link

I got the same issue after I upgrade over v0.11.x version. Hope that it can be merged asap!

@mpospelov
Copy link
Author

mpospelov commented Jul 20, 2020

This optimization is broken even more than we expected, further fixes are required.
We found 2 more issue with value type interfaces and another case. We have a fix for it but the code needs polishing first, so I would like to mark this PR as WIP for now

@mpospelov mpospelov changed the title Fix (#4283 #3582): Union with interfaces issue [WIP] Fix (#4283 #3582): Union with interfaces issue Jul 20, 2020
@havenchyk
Copy link

@trevor-scheer maybe you can also take a look?

@trevor-scheer
Copy link
Member

Hey @mpospelov, thank you for hard work on this (and anyone else involved!). I can't guarantee how soon I can give this the attention it deserves, but it's high on my list.

For context, (GraphQL Summit)[https://summit.graphql.com/] is right around the corner and we're all quite busy for the next couple weeks. Hope to see you there!

@id-ilych
Copy link

id-ilych commented Jul 27, 2020

Hey @trevor-scheer 👋
I've created another branch with slightly modified version of the test to demonstrate the difficulties I currently have with implementing a fix - it seems that behavior of the splitFields when dealing with interface as a parentType should be different when the interface is the return type of the parent field and when the parent field is of union type with ... on Interface fragment. That is demonstrated by the following test-cases correspondingly:

  • handles interface-typed field when interface is used on multiple services
  • handles interface fragments for union-typed field from multiple services

in packages/apollo-gateway/src/__tests__/integration/value-type-interfaces.test.ts file of my branch.

The issue is that I can't see how those cases can be distinguished based on existing arguments. The gist of the problem is that possibleTypes for the interface should be narrowed down to those implementations that are known to the current service when the interface is the return type, and they should not be when the enclosing scope is e.g. a union of a types from different services.

UPD Added another failing test case when query have a fragment on interface that is only defined on other services and is unknown to the service owning the root field of the query:

  • handles interface fragments on interfaces from other services

@id-ilych
Copy link

Hi @trevor-scheer 👋
Any news? Can you confirm the bug?

@id-ilych
Copy link

Hi @trevor-scheer @abernix 👋
I see that the query planner was replaced (#4534). Does it solve the issue? If not then are there plans to fix it?

@glasser
Copy link
Member

glasser commented Oct 7, 2022

The gateway code moved to the https://github.com/apollographql/federation repository a few years ago. Sorry for the delay!

@glasser glasser closed this Oct 7, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants