Federation v2 support added

arun-sureshkumar commented 1 year ago

Add Support to Federation v2

apollo-federation-subgraph-compatibility - Passed

arun-sureshkumar commented 1 year ago

@patrick91 - apollo-federation-subgraph-compatibility Passed

arun-sureshkumar commented 1 year ago

@erikwrede - Can you please review the PR?

erikwrede commented 1 year ago

@arun-strollby great job! I'll have a look in the weekend, but the final word should be on @tcleonard or @patrick91, since they are more experienced with federation.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3196356399

152 of 196 (77.55%) changed or added relevant lines in 11 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-7.8%) to 83.25%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
graphene_federation/override.py	10	14	71.43%
graphene_federation/tag.py	10	14	71.43%
graphene_federation/service.py	43	54	79.63%
graphene_federation/inaccessible.py	19	31	61.29%
graphene_federation/shareable.py	19	32	59.38%
<!--	Total:	152	196	77.55%	-->

Files with Coverage Reduction	New Missed Lines	%
graphene_federation/service.py	3	88.33%
<!--	Total:	3		-->

Totals
Change from base Build 3015879032:	-7.8%
Covered Lines:	333
Relevant Lines:	400

💛 - Coveralls

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3196356399

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

155 of 188 (82.45%) changed or added relevant lines in 13 files are covered.
28 unchanged lines in 4 files lost coverage.
Overall coverage decreased (-7.8%) to 83.25%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
graphene_federation/entity.py	12	13	92.31%
graphene_federation/override.py	6	8	75.0%
graphene_federation/tag.py	6	8	75.0%
graphene_federation/service.py	60	65	92.31%
graphene_federation/inaccessible.py	11	21	52.38%
graphene_federation/shareable.py	19	32	59.38%
<!--	Total:	155	188	82.45%	-->

Files with Coverage Reduction	New Missed Lines	%
graphene_federation/provides.py	1	96.67%
graphene_federation/types.py	3	75.0%
graphene_federation/service.py	9	88.33%
graphene_federation/entity.py	15	70.37%
<!--	Total:	28	-->

Totals
Change from base Build 3015879032:	-7.8%
Covered Lines:	333
Relevant Lines:	400

💛 - Coveralls

patrick91 commented 1 year ago

@arun-sureshkumar shall we also add support for @key(resolvable=True/False)? :)

arun-sureshkumar commented 1 year ago

@arun-sureshkumar shall we also add support for @key(resolvable=True/False)? :)

We can add this!! I will add this support soon and get back to you @patrick91 !!

patrick91 commented 1 year ago

@erikwrede do we have any strategy around docs for this repo?

erikwrede commented 1 year ago

@patrick91 Sphinx/mkdocs -Style docs and doc strings are fine for now. Still working on doc server access. I'll push an intermediate GitHub pages docs pipeline soon, so documentation work can already begin right now.

It would be great if we had some docs [at least doc strings] for the new features.

Looking forward to pushing a release once patricks review items are implemented. Finally got PyPi access!

patrick91 commented 1 year ago

@arun-sureshkumar shall we also add support for @key(resolvable=True/False)? :)

We can add this!! I will add this support soon and get back to you @patrick91 !!

@arun-sureshkumar let me know if you need any help with this 😊

arunsureshkumar commented 1 year ago

@arun-sureshkumar shall we also add support for @key(resolvable=True/False)? :)

We can add this!! I will add this support soon and get back to you @patrick91 !!

@arun-sureshkumar let me know if you need any help with this 😊

I will work on this later this week, will update you the progress.

adarshdigievo commented 1 year ago

@arun-sureshkumar shall we also add support for @key(resolvable=True/False)? :)

This was added by the last pull request ✌️

adarshdigievo commented 1 year ago

@patrick91

As per the suggestions, have incorporated the below changes in the implementation:

Made federation V2 support opt-in (enable_federation_2 argument in build_schema)
Added resolvable argument to @key directive
Added test case for compound keys
Renamed _from argument to from_
Added validation for key fields in entity and extends(single, multiple and compound keys)
Added test cases for compound keys
Corrected Docstrings
Minor edits to Readme regarding compound keys

Please review the changes

Meemaw commented 1 year ago

We are currently in process of adding graphene-federation to our stack, and would like to start with v2 directly. Is there a rough date when this could be released? We can go with v1 for the time being if there is no estimate.

erikwrede commented 1 year ago

@Meemaw best case would be to have some feedback from how your integration goes based on this PR 🙂 For now, the plan is to do another round of reviews and go on from there. The main functionality should already be working (no guarantees though).

patrick91 commented 1 year ago

@erikwrede I'll add reviewing this PR to my todos for this week :)

do we have an easy way to publish pre-releases on PyPI? It might make it easier to test this :)

erikwrede commented 1 year ago

Awesome! I'll try to find some time to add a new review in the next days, too. @patrick91 @Meemaw we can push a new 3.1 alpha-1 tag to PyPi using the pipeline. That would require us to move this branch to the main repo and open another merge request into master or just have an intermediate PR to a 3.1 branch.

patrick91 commented 1 year ago

I'm also going to make a port of https://github.com/strawberry-graphql/federation-demo to Graphene and see if all works well :)

arunsureshkumar commented 1 year ago

Added some more comments. @arun-sureshkumar do you think we can keep tests for v1, I think we should make sure we don't break existing code 😊

Sure, lets keep the tests for v1.

adarshdigievo commented 1 year ago

Added some more comments. @arun-sureshkumar do you think we can keep tests for v1, I think we should make sure we don't break existing code 😊

Done 👍. Added v1 tests alongside current test cases.

erikwrede commented 1 year ago

Currently playing around with the PR for my final review and it's working great so far!

I have some minor remarks. We should discuss the behavior of the @shareable directive on interfaces:

@shareable
class ReviewInterface(Interface):
    interfaced_body = graphene.String()

class Review(ObjectType):
    class Meta:
        interfaces = (ReviewInterface,)

    id = graphene.ID()
    body = graphene.String()
    interfaced_body = graphene.String()

    def resolve_interfaced_body(self, info):
        return "".join([self.body, "interfaced"])

Currently, this ignores the directive on the interfaces (i.e. it is not included in the SDL). According to the Federation Docs, starting from federation v2.2 this should not be allowed instead of ignored:

The @shareable directive is about indicating when an object field can be resolved by multiple subgraphs. As interface fields are not directly resolved (their implementation is), @shareable is not meaningful on an interface field and is not allowed (at least since federation 2.2; earlier versions of federation 2 mistakenly ignored @shareable on interface fields).

Since we are just introducing Federation v2 here, maybe we should adopt this behavior from the start to avoid making breaking changes later on. What do you guys think?

erikwrede commented 1 year ago

Correcting my above post, it seems like Interfaces in general do not work with the directives yet. For example: inaccessible on graphene.Interface doesn't seem to work either, using strawberry federation as a working python reference:

@strawberry.interface(directives=[Inaccessible()])
class ReviewInterface:
    interfaced_body: str

@strawberry.type
class Review(ReviewInterface):
    id: int
    body: str

Resulting SDL:

type Review implements ReviewInterface {
  interfacedBody: String!
  id: Int!
  body: String!
}

interface ReviewInterface @inaccessible {
  interfacedBody: String!
}

Versus the SDL from my example above using @inaccessible instead of @shareable:

type Review implements ReviewInterface {
  interfacedBody: String
  id: ID
  body: String
}

interface ReviewInterface {
  interfacedBody: String
}

As a consequence, ReviewInterface is included in the stitched schema.

Seems like the Apollo Subgraph Compatibility test doesn't incorporate this case, which is probably why it was missed. Maybe it makes sense to add some additional tests specifically for Interfaces here. On the other hand, I don't want to hold the release of this back if it's too much work to add. What do you guys think? @patrick91 @arun-sureshkumar

erikwrede commented 1 year ago

gentle push, let's get this over the finish line 🙂 @patrick91 @arun-sureshkumar @adarshdigievo

patrick91 commented 1 year ago

@arun-sureshkumar @arunsureshkumar @adarshdigievo are you planning to update this PR or shall I take over? 😊

arunsureshkumar commented 1 year ago

Patrick,

I will close this by end of this week.

Thanks, Arun

On Tue, 21 Feb 2023 at 01:45, Patrick Arminio @.***> wrote:

@arun-sureshkumar https://github.com/arun-sureshkumar @arunsureshkumar https://github.com/arunsureshkumar @adarshdigievo https://github.com/adarshdigievo are you planning to update this PR or shall I take over? 😊

— Reply to this email directly, view it on GitHub https://github.com/graphql-python/graphene-federation/pull/4#issuecomment-1437506243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEIAZMGF4ITLPMLB7676PC3WYPGFRANCNFSM6AAAAAAQ4DCYVE . You are receiving this because you were mentioned.Message ID: @.***>

Meemaw commented 1 year ago

@arunsureshkumar any update on getting this in?

patrick91 commented 1 year ago

@arun-sureshkumar let me know if you want us to update the PR 😊 happy to take over it :D

arun-sureshkumar commented 1 year ago

@arun-sureshkumar let me know if you want us to update the PR 😊 happy to take over it :D

@patrick91 Please check now, let me know your comments!

arun-sureshkumar commented 1 year ago

Currently playing around with the PR for my final review and it's working great so far!

I have some minor remarks. We should discuss the behavior of the @shareable directive on interfaces:
@shareable
class ReviewInterface(Interface):
    interfaced_body = graphene.String()

class Review(ObjectType):
    class Meta:
        interfaces = (ReviewInterface,)

    id = graphene.ID()
    body = graphene.String()
    interfaced_body = graphene.String()

    def resolve_interfaced_body(self, info):
        return "".join([self.body, "interfaced"])
Currently, this ignores the directive on the interfaces (i.e. it is not included in the SDL). According to the Federation Docs, starting from federation v2.2 this should not be allowed instead of ignored:

The @Shareable directive is about indicating when an object field can be resolved by multiple subgraphs. As interface fields are not directly resolved (their implementation is), @Shareable is not meaningful on an interface field and is not allowed (at least since federation 2.2; earlier versions of federation 2 mistakenly ignored @Shareable on interface fields).

Since we are just introducing Federation v2 here, maybe we should adopt this behavior from the start to avoid making breaking changes later on. What do you guys think?

This will not be allowed instead of ignored, updating the code.

arun-sureshkumar commented 1 year ago

Correcting my above post, it seems like Interfaces in general do not work with the directives yet. For example: inaccessible on graphene.Interface doesn't seem to work either, using strawberry federation as a working python reference:
@strawberry.interface(directives=[Inaccessible()])
class ReviewInterface:
    interfaced_body: str

@strawberry.type
class Review(ReviewInterface):
    id: int
    body: str
Resulting SDL:
type Review implements ReviewInterface {
  interfacedBody: String!
  id: Int!
  body: String!
}

interface ReviewInterface @inaccessible {
  interfacedBody: String!
}
Versus the SDL from my example above using @inaccessible instead of @shareable:
type Review implements ReviewInterface {
  interfacedBody: String
  id: ID
  body: String
}

interface ReviewInterface {
  interfacedBody: String
}
As a consequence, ReviewInterface is included in the stitched schema.

Seems like the Apollo Subgraph Compatibility test doesn't incorporate this case, which is probably why it was missed. Maybe it makes sense to add some additional tests specifically for Interfaces here. On the other hand, I don't want to hold the release of this back if it's too much work to add. What do you guys think? @patrick91 @arun-sureshkumar

This issue is fixed.

erikwrede commented 1 year ago

Awesome, thanks for the changes! Can we add some tests for the added behavior? Apart from that LGTM!

arun-sureshkumar commented 1 year ago

Awesome, thanks for the changes! Can we add some tests for the added behavior? Apart from that LGTM!

Sure thing!

arun-sureshkumar commented 1 year ago

Awesome, thanks for the changes! Can we add some tests for the added behavior? Apart from that LGTM!

Sure thing!

Please check and let me know if anything to be added.

patrick91 commented 1 year ago

@arun-sureshkumar @erikwrede I'll do a review in the couple of days 😊

patrick91 commented 1 year ago

Just tested this locally and it seems to work well :D @erikwrede what are the next steps for getting this released?

erikwrede commented 1 year ago

Thanks everyone for the amazing work on this! 🎉

graphql-python / graphene-federation