Implement Apollo Federation v2 #26

Aeliot-Tm · 2022-08-04T14:44:45Z

Proposed changes

Implementation of Apollo Federation v2
Added support of "compound" key fields

Implemented directives:

@link
@inaccessible
@override
@shareable
@key (added support of argument resolvable)

⚠️ IMPORTANT: There are some backward compatibility breaks:

deprecated optionkeyFields for entity configs! Use option keys instead of it. Pay attention that value of the new option is not compatible with the previous one. But there was done some work around for supporting of old configurations. Old configs will work "ok" for entities, but they are broken for entity references!

there was added some constraints on @key directive of entity reference for better matching of specification. There is not possible multiple directive @key and new argument resolvable: false is required.

How to test

All tests run on CI.

Unit Tests

This PR has unit tests
This PR does not have unit tests: why?

Ref #24

…testing configuration files pattern and permit safe overriding configs in local environment.

ldiego08

Amazing work, @Aeliot-Tm! Changes look good to me but will defer to @xiian or anyone else
at SK willing to give this a deeper review and a test run.

maccath

Hi @Aeliot-Tm, and thanks for the PR.

I've not reviewed all of the code yet, but I've added some comments and questions on what I've looked through so far. Hopefully you can clarify for me!

src/Types/EntityObjectType.php

src/Types/EntityRefObjectType.php

src/Types/SchemaExtensionType.php

# Conflicts: # .gitignore # composer.lock # src/FederatedSchema.php

Aeliot-Tm · 2022-09-07T18:45:07Z

@maccath I made updates. Could you recheck it, please?

Klaas-MY · 2022-10-06T13:33:38Z

Any update on this PR? We could really use V2 :)

maccath · 2022-10-07T09:43:15Z

Any update on this PR? We could really use V2 :)

Hi @Klaas-MY - we are still working through this on our end. To speed things up, you could help us test this by specifying this branch in your composer.json (docs: Loading a package from a VCS repository) and let us know if you spot any issues!

Thanks!

Makapashev · 2022-12-19T08:25:01Z

Any updates on this PR?

tcarrio · 2023-02-08T23:19:21Z

composer.json

@@ -4,7 +4,7 @@
    "type": "library",
    "license": "MIT",
    "require": {
-        "php": "^7.1||^8.0",
+        "php": "^7.4||^8.0",


Should we consider dropping support for PHP 7.4 now?

Downgraded to PHP 7.1

But it cannot be tested on this version of PHP. It was locked implicitly on php >=7.3 when required PHPunit ^9.5

apollo-federation-php/composer.json

Line 30 in c1bc216

"phpunit/phpunit": "^9.5",

It would be great if the version bump could happen in #31 , and this PR could become mergeable without this PHP bump first. A minor release can happen first, and the version bumps of #31 can happen in a major breaking release.

When considering the version bump the Stitcher blogposts are helpful: https://stitcher.io/blog/php-version-stats-january-2023
The one thing I am always curious about is how much % of these stats are caused by CI pipelines (and hence the actual PHP <= 7.4 usage is lower).

The PHP supported versions is a better guidance, especially when considering a major release as a library.
The old library versions will still remain available through Packagist, and the need for new functionality will stimulate engineers to get their PHP versions updated.

Aeliot-Tm · 2023-02-09T18:17:49Z

Hi @maccath ,

Why does it take so long to check this PR?

xiian · 2023-02-23T15:06:29Z

@Aeliot-Tm Thank you for contributing this PR and apologies for delays on reviews. In order to test this, we've been trying to integrate this on our end and that can take a bit of time.

In addition to testing internally, we also run our code against the apollo-federation-subgraph-compatibility repo (which powers this page on apollographql.com). As you can see on the website, Apollo Federation PHP does not currently support Federation v2, but does support all features of Federation v1 (except federated tracing).

We've run the test suite against this PR locally and had mixed success. We've put up a version of that test in a PR in our fork of apollo-federation-subgraph-compatibiity. It seems that the branch does not have the intended effect.

The code for the test was updated according to the description of the PR and the updates to the README.md, but I think there are more modifications needed.

`SchemaBuilder` documentation

I see the addition of a SchemaBuilder class in the PR which does not seem referenced anywhere else (outside tests). If this is now required for usage we need to have the README.md updated with proper usage for this as well.

`FederatedSchemaTrait` justification

There's also a FederatedSchemaTrait that was created that seems to be pulled out functionality from FederatedSchema, but there's no information about why it was pulled out aside from removing those methods (and properties) from the FederatedSchema class. Not to say that there isn't potential use cases for this trait, but we should provide reasons for the change.

`@link` implementation documentation

As this is the core required functionality of Federation v2, we must have proper documentation on how to use this. As it is, I've attempted to figure out the correct usage locally and been unable to do so. I'm not sure if I'm doing it wrong, or if the functionality is broken.

Other New Directives implementation

While I see the addition of the other directives for Federation v2 support, I don't see any instructions for using them. The unit tests are good for covering the configuration of those directives, and we do have a test for writing them out in the SDL, but there is nothing to cover applying those directives on fields/objects/etc.

`@key` directive backwards-compatibility break

While I understand that BC is sometimes necessary, we are not looking to break this library's compatibility with Federation v1. It's possible that our updates to the code were incorrect, but the @key (single) test is now failing. If there is a different way of achieving this, please document it.

I would suggest breaking this PR up into several smaller PRs, maybe one per directive. This will make it much easier to review, but also much easier to ensure that the functionality is working correctly. We can merge them in as we go,

Also, checking out a local copy of https://github.com/apollographql/apollo-federation-subgraph-compatibility and updating the code inside of implementations/php will allow you to perform the same tests locally.

Aeliot-Tm · 2023-02-26T05:23:32Z

@xiian Thank you for feedback and the link to the testing repo.
I'll have a look what is the reason of mixed success. And I'll update documentation of course.

rvanlaak · 2023-05-30T11:52:01Z

Is there anything that can get some help to get this functionality to a mergeable state?

The deprecation functionality that was added in webonyx/graphql-php v15.4.0 might seem to be a great option to get the BC break on this PR resolved in a different way? But, using that deprecation feature would require a package and a PHP version bump, and that might not be something you preferably want to do through this PR?

a-melnikov added 30 commits August 2, 2022 12:01

Use Enum class as single source of directive names

2a39bee

Use constants as single source of entity config options names

c0ff092

Fix code style

68ec4c5

Create TypeEnum as single source of type names

90231df

Fix code style

9af5719

Fix code style

877810b

Print simple and compound directives' argument "fields"

268eee6

Declare reserved names

0769802

Fix code style

6849706

Fixes during investigation

df46520

Update phpDocs

79e14dc

Make printer better extendable

245f9ad

Extend base schema printer and reduce copy-paste

cd86cde

Reduce copy-paste & fix code style

5077032

Refactor code to make more readable

d23332d

Fix code style

5724b2c

Extract FederatedSchemaTrait

3e217de

Fix code style

4a21ab5

Fix tests code style

8ad0047

Fix method call params order

e0c45d7

Fix code style

10a2a4a

Sort schema printer methods

b5d5e48

Add directives of Apollo Federation v2

17bf125

Implement directive @link

c0f3941

Add handling of argument "resolvable" of directive @key

1bac625

Extend validation of type reference

970ca0b

Add validation if Referenced entity has only one @key directive.

75f8b3a

Rename file "phpunit.xml" to "phpunit.xml.dist" to stick to standard …

86ae658

…testing configuration files pattern and permit safe overriding configs in local environment.

Extract getting of required federated directives to the schema builder

ae58a6e

Fix extendability of schema

78ba78c

ldiego08 previously approved these changes Aug 8, 2022

View reviewed changes

ldiego08 mentioned this pull request Aug 8, 2022

Federation v2 support #24

Open

maccath suggested changes Aug 9, 2022

View reviewed changes

a-melnikov added 4 commits August 29, 2022 17:59

Update phpdoc

fe7456c

Merge branch 'main' into implement_v2

258c46f

# Conflicts: # .gitignore # composer.lock # src/FederatedSchema.php

Remove deprecated excess classes imports

408c3e1

Fix merge conflict: make updates equivalent to the commit 4f958dd

07a0f9e

Aeliot-Tm dismissed ldiego08’s stale review via 07a0f9e August 29, 2022 15:18

a-melnikov added 3 commits August 30, 2022 09:06

Fix documentation

a34d336

Implement tests

edce728

Implement tests

3562fe3

Fix test failed assertion message.

7002d92

a-melnikov added 2 commits October 28, 2022 00:14

Update entity reference resolver validation

1b663b5

Remove excess class import

60feec3

ramsey mentioned this pull request Dec 15, 2022

chore: improve static analysis, bump min PHP to 7.4, support PHP 8.2 #31

Open

2 tasks

tcarrio reviewed Feb 8, 2023

View reviewed changes

Downgrade to PHP 7.1

b818787

rvanlaak mentioned this pull request May 30, 2023

Test this library against apollo-federation-subgraph-compatibility axtiva/flexible-graphql-php#22

Open

rvanlaak mentioned this pull request Nov 7, 2024

Support for GraphQL Federation? api-platform/api-platform#1932

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Apollo Federation v2 #26

Implement Apollo Federation v2 #26

Aeliot-Tm commented Aug 4, 2022 •

edited

Loading

ldiego08 left a comment

maccath left a comment

Aeliot-Tm commented Sep 7, 2022

Klaas-MY commented Oct 6, 2022

maccath commented Oct 7, 2022

Makapashev commented Dec 19, 2022

tcarrio Feb 8, 2023

Aeliot-Tm Feb 9, 2023

rvanlaak May 30, 2023

Aeliot-Tm commented Feb 9, 2023 •

edited

Loading

xiian commented Feb 23, 2023

Aeliot-Tm commented Feb 26, 2023

rvanlaak commented May 30, 2023 •

edited

Loading

Implement Apollo Federation v2 #26

Are you sure you want to change the base?

Implement Apollo Federation v2 #26

Conversation

Aeliot-Tm commented Aug 4, 2022 • edited Loading

Proposed changes

Implemented directives:

How to test

Unit Tests

ldiego08 left a comment

Choose a reason for hiding this comment

maccath left a comment

Choose a reason for hiding this comment

Aeliot-Tm commented Sep 7, 2022

Klaas-MY commented Oct 6, 2022

maccath commented Oct 7, 2022

Makapashev commented Dec 19, 2022

tcarrio Feb 8, 2023

Choose a reason for hiding this comment

Aeliot-Tm Feb 9, 2023

Choose a reason for hiding this comment

rvanlaak May 30, 2023

Choose a reason for hiding this comment

Aeliot-Tm commented Feb 9, 2023 • edited Loading

xiian commented Feb 23, 2023

SchemaBuilder documentation

FederatedSchemaTrait justification

@link implementation documentation

Other New Directives implementation

@key directive backwards-compatibility break

Aeliot-Tm commented Feb 26, 2023

rvanlaak commented May 30, 2023 • edited Loading

Aeliot-Tm commented Aug 4, 2022 •

edited

Loading

Aeliot-Tm commented Feb 9, 2023 •

edited

Loading

`SchemaBuilder` documentation

`FederatedSchemaTrait` justification

`@link` implementation documentation

`@key` directive backwards-compatibility break

rvanlaak commented May 30, 2023 •

edited

Loading