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

#521: add federation to core: #1132

Merged
merged 27 commits into from
Sep 29, 2022
Merged

Conversation

t1
Copy link
Collaborator

@t1 t1 commented Oct 26, 2021

  • move federation api into smallrye-graphql-api
  • add config to enable federation
  • add dependency on com.apollographql.federation:federation-graphql-java-support
  • transform federation schema
  • remove old federation module
  • implement Bootstrap#fetchEntities and Bootstrap#fetchEntityType
  • add tests
  • properly resolve the class name
  • update README.

@johgoe
Copy link

johgoe commented Oct 29, 2021

@t1 In our project we got a working quarkus server with federation, with some technical tricks like working with a 2nd jandex index and manipulation of the schema in the federation gateway itself.
Because of that we find some cases that hat not been working before in Smallrye Federation. I pushed this changes to
#1135
Maybe it can support you for rewriting.

@t1 t1 force-pushed the 521-federation-into-core branch from 8e2acae to 9c62ec9 Compare October 29, 2021 12:30
@t1 t1 force-pushed the 521-federation-into-core branch 2 times, most recently from a606806 to 297f521 Compare December 5, 2021 09:10
@t1 t1 force-pushed the 521-federation-into-core branch from 297f521 to 8294643 Compare January 11, 2022 04:41
@t1 t1 force-pushed the 521-federation-into-core branch from 8294643 to ae68ffc Compare February 25, 2022 17:12
@t1 t1 force-pushed the 521-federation-into-core branch from ae68ffc to 7678343 Compare March 6, 2022 07:56
@t1 t1 force-pushed the 521-federation-into-core branch from 7678343 to fac3651 Compare March 18, 2022 04:32
@t1 t1 force-pushed the 521-federation-into-core branch from fac3651 to 0d90a1f Compare May 27, 2022 08:18
@t1 t1 force-pushed the 521-federation-into-core branch from b6b49fb to 4448db5 Compare September 2, 2022 06:52
@t1 t1 requested a review from phillip-kruger September 2, 2022 07:30
@t1
Copy link
Collaborator Author

t1 commented Sep 2, 2022

@phillip-kruger & @johgoe : I think the core federation functionality should be working. I have some trouble to get my tests with the ApolloGateway to work properly. It seem that it doesn't recognise the @key(fields : ["id"]) directive, but AFAIU this should be legal. IIRC I had to work around this in the former, hand-made solution. Could you help me double-check?

@t1
Copy link
Collaborator Author

t1 commented Sep 2, 2022

It works after updating Apollo to the latest version!!! So it was a bug.

I shared my demo project: https://github.com/t1/smallrye-graphql-federation-demo

I'll continue writing tests now.

Comments welcome!!!!!!!1111

@t1 t1 force-pushed the 521-federation-into-core branch 2 times, most recently from 76f53d9 to c6b3a6d Compare September 3, 2022 04:39
@phillip-kruger
Copy link
Member

Thanks @t1. I'll have a look a.s.a.p

@t1 t1 marked this pull request as ready for review September 4, 2022 16:44
@t1
Copy link
Collaborator Author

t1 commented Sep 4, 2022

Is the documentation sufficient?

@robp94
Copy link
Contributor

robp94 commented Sep 5, 2022

Looks nice. Does it support mutiny return types? Would this already work with quarkus? Then we could test it with our services and see if we find anything.

@t1
Copy link
Collaborator Author

t1 commented Sep 5, 2022

Does it support mutiny return types?

It probably should, but I haven't tried.

Would this already work with quarkus?

The quarkus tests fail with compile errors that I don't recognise immediately.

@t1
Copy link
Collaborator Author

t1 commented Sep 5, 2022

I think this is complete now and could be merged.

@phillip-kruger
Copy link
Member

Hi @t1 - Thanks for this. I'll have a look as soon as I have a gap

@t1 t1 requested a review from jmartisk September 15, 2022 16:10
@t1
Copy link
Collaborator Author

t1 commented Sep 15, 2022

@jmartisk: it looks like @phillip-kruger is extremely busy with other things right now. maybe you can give some feedback?

@t1 t1 force-pushed the 521-federation-into-core branch from 1cc3f34 to 55d8ff7 Compare September 15, 2022 16:11
@phillip-kruger
Copy link
Member

I'll look at this soon. This will need a thorough review and we need to see if this will work in Quarkus

@jmartisk
Copy link
Member

This could also use writing some Quarkus-side integration test coverage as part of doing the review (well, if possible - because running a real integration test seems to require a gateway and at least two Quarkus instances, which makes it very difficult)
If you prefer I can get to it, but I'm quite unfamiliar with Federation so it will take me quite some time

@t1 t1 force-pushed the 521-federation-into-core branch from dd3ea18 to 0a77d5b Compare September 23, 2022 11:46
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks @t1 - this is a big contribution ! If we can move that one part to the model, I think we are good to go !

@phillip-kruger
Copy link
Member

@jmartisk - I am merging here. Can you help with creating a new 1.9.x branch and backport (non-Jakarata) to there ? Then I cal look at the jandex in that branch and once we are happy we can get this into Quarkus. Le me know

@phillip-kruger phillip-kruger added this to the 2.0.0 milestone Sep 29, 2022
@phillip-kruger phillip-kruger added the back-port This needs to back-port to the 2.0.x branch label Sep 29, 2022
@phillip-kruger
Copy link
Member

@t1 Thanks for this massive contribution !

@phillip-kruger phillip-kruger merged commit 41ab806 into smallrye:main Sep 29, 2022
@jmartisk
Copy link
Member

I'll create a 1.9.x branch with this that will target Quarkus 2.15, which is due some time in December or January, so we will have enough time to polish it. At minimum, I believe we should make the new dependency (federation-graphql-java-support) optional so it doesn't have to be included with every application even if it doesn't use Federation.

@t1 t1 deleted the 521-federation-into-core branch September 30, 2022 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-port This needs to back-port to the 2.0.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants