apollo-federation-php icon indicating copy to clipboard operation
apollo-federation-php copied to clipboard

Published 20 hours ago • verified publisher icon Skillshare

Implement Apollo Federation v2

Open Aeliot-Tm opened this issue 2 years ago • 8 comments

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:

  1. 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!
  2. 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

  • [x] This PR has unit tests
  • [ ] This PR does not have unit tests: why?

Ref #24

Aeliot-Tm avatar Aug 04 '22 14:08 Aeliot-Tm

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

Aeliot-Tm avatar Sep 07 '22 18:09 Aeliot-Tm

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

Klaas-MY avatar Oct 06 '22 13:10 Klaas-MY

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!

maccath avatar Oct 07 '22 09:10 maccath

Any updates on this PR?

Makapashev avatar Dec 19 '22 08:12 Makapashev

Hi @maccath ,

Why does it take so long to check this PR?

Aeliot-Tm avatar Feb 09 '23 18:02 Aeliot-Tm

@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.

xiian avatar Feb 23 '23 15:02 xiian

@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.

Aeliot-Tm avatar Feb 26 '23 05:02 Aeliot-Tm

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?

rvanlaak avatar May 30 '23 11:05 rvanlaak