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

Add Federation Support #98

Merged
merged 35 commits into from
Feb 14, 2023
Merged

Add Federation Support #98

merged 35 commits into from
Feb 14, 2023

Conversation

samisuteria
Copy link
Contributor

@samisuteria samisuteria commented Feb 5, 2023

A basic implementation of federation. Some issues I have and could use guidance:

  • Using AnyCodable as a dependency Used the Map type from GraphQL
  • Where to store JSONEncoder/Decoder for reusability Use schema coders
  • Users having to implement FederationContext/FederationResolver and No custom protocol for federation resolver. Users still need to copy the SDL in directly
  • How to apply directives to fields/types (ie: @key(fields: "id"), @inaccessible, @shareable ) Key has been implemented on Type and maps to a function in the resolver.
  • Supporting older versions of Swift (any FederationContext requires 5.7) Updated FederationResolver protocol to offer a method for older versions of Swift No longer needed

@d-exclaimation
Copy link
Member

d-exclaimation commented Feb 8, 2023

How to apply directives to fields/types (ie: @key(fields: "id"), @inaccessible, @Shareable )

@NeedleInAJayStack is directive support already in Graphiti?

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Feb 8, 2023

How to apply directives to fields/types (ie: @key(fields: "id"), @inaccessible, @Shareable )

@NeedleInAJayStack is directive support already in Graphiti?

Not at the moment, but it's deinitely a highly sought-after feature

I should say, just for clarity, that the GraphQL standard directives 'skip' and 'include' are supported, but custom directives like Apollo Federation relies on are not supported today.

@samisuteria
Copy link
Contributor Author

Not at the moment, but it's deinitely a highly sought-after feature

I can work on that feature - would you prefer directive support before merging in federation?

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Feb 9, 2023

Awesome, thank you so much @samisuteria! Yeah, from my understanding federation support typically uses custom directives, so solving custom directives in a general way first makes sense to me. Thanks again, and great work so far!

@samisuteria
Copy link
Contributor Author

samisuteria commented Feb 10, 2023

Federation can work without directives in the application code. If you are writing schema first (I'm not sure the split between schema-first and code-first in the GraphQL ecosystem) you can enable federation without directives. As long as the service sdl query returns the schema federation will work correctly. Even Apollo's reference server avoids using directives in the code and just loads the schema from the file system here.

query {
   _service { sdl }
}

If you check out the samisuteria/graphiti branch on https://github.com/samisuteria/apollo-federation-subgraph-compatibility/ and run make test subgraph=swift-graphiti you'll see it passes all tests except federated tracing (which seems like a much larger project based on: https://www.apollographql.com/docs/graphos/metrics/sending-operation-metrics#tracing-format) and it appears about half the subgraph libraries have warning levels of support for it https://www.apollographql.com/docs/federation/building-supergraphs/supported-subgraphs .

*************
Federation v1 compatibility
*************
_service PASS
@key (single) PASS
@key (multi) PASS
@key (composite) PASS
repeatable @key PASS
@requires PASS
@provides PASS
federated tracing WARNING

*************
Federation v2 compatibility
*************
@link PASS
@shareable PASS
@tag PASS
@override PASS
@inaccessible PASS

I still plan on working on adding directive/sdl support for Graphiti but I think this PR is ready for review and can be used by people who are schema-first.

@d-exclaimation
Copy link
Member

I still plan on working on adding directive/sdl support for Graphiti but I think this PR is ready for review and can be used by people who are schema-first.

Sounds good for me. Nice work 👍 . While It's probably a bit awkward atm given you can't really fully do schema-first or use directive with Graphiti, those feature can be separated and worked on later with future PR / releases.

@NeedleInAJayStack
Copy link
Member

Great info @samisuteria! Thanks for walking me through that, and in that case I'm totally ok with this going in before directive support.

Sorry about the delay - I'll get a review in this weekend.

Copy link
Member

@NeedleInAJayStack NeedleInAJayStack left a comment

Choose a reason for hiding this comment

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

Overall, I like the direction a lot. However, I think there may be some opportunities to align this more with the existing patterns (similar to how Connection was added). Things like integrating Key definition directly into the schema DSL instead of using a resolver protocol, and creating relevant GraphQL types during the SchemaBuilder.build phase.

If you don't mind, I've done some scratch work on the tip of your branch to make sure some of these ideas work out, and I'd like to see if the changes are acceptable to you. I'll post a link in a bit.

}

public extension FederationEntity {
static var typename: String { Reflection.name(for: Self.self) }
Copy link
Member

Choose a reason for hiding this comment

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

Is this 'self' name reflection compatible with types that we have aliased in our GraphQL schema? For example:

Type(LocationObject.self, as: "Location") {
    Field("id", at: \.id)
    Field("name", at: \.name)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's compatible with the one in the schema. I just provided a default implementation for simplicity but the user can override it if needed. I did that here since there was a User type already defined in another test. https://github.com/GraphQLSwift/Graphiti/pull/98/files#diff-7086f136baa260fe52dc5fa6c66acf11b265a563f76708d829293b38aa10937cR84

Comment on lines 10 to 14
associatedtype Context
static var encoder: JSONEncoder { get }
static var decoder: JSONDecoder { get }
var sdl: String { get }
func entity(context: Context, key: FederationEntityKey, group: EventLoopGroup) -> EventLoopFuture<FederationEntity?>
Copy link
Member

Choose a reason for hiding this comment

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

This resolver protocol duplicates quite a bit of structure from the Schema type itself (things like coders and generic Context). Also, I'm a little wary to require protocol conformance on our resolver types, since that's not something we've done in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree and was not a fan of it.

Comment on lines 4 to 8
extension SchemaBuilder where Resolver: FederationResolver, Resolver.Context == Context {
@discardableResult
/// Enable federation to add additional capabilities for federation subgraph support
public func enableFederation() -> Self {
let federationSchema = PartialSchema<Resolver, Context>(
Copy link
Member

Choose a reason for hiding this comment

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

Could we do this in a similar way to the Apollo Connection support where we handle it during the schema creation itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see the Connection code but looking at it now - it really does seem the way to go.

associatedtype Context
static var encoder: JSONEncoder { get }
static var decoder: JSONDecoder { get }
var sdl: String { get }
Copy link
Member

Choose a reason for hiding this comment

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

For this parameter, we need to define our SDL string separately and ensure it corresponds to the schema definition we coded, right? It would be awesome if we could generate the SDL automatically from the defined schema, but I know that's probably a bigger task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I would really prefer to not require the SDL since that requires users to put federation specific code into their schema. Maybe have a way of verifying the the supplied SDL to the built schema to make sure all types/fields/keys are included?

Comment on lines 29 to 34
static let entityKeys: [(entity: FederationEntity.Type, keys: [FederationEntityKey.Type])] = [
(Product.self, [Product.EntityKey1.self, Product.EntityKey2.self, Product.EntityKey3.self]),
(DeprecatedProduct.self, [DeprecatedProduct.EntityKey.self]),
(ProductResearch.self, [ProductResearch.EntityKey.self]),
(ProductUser.self, [ProductUser.EntityKey.self]),
]
Copy link
Member

Choose a reason for hiding this comment

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

We map up our Entities to Keys through the resolver already - is there any way we could reuse that definition instead of requiring it twice?

@NeedleInAJayStack
Copy link
Member

@samisuteria See what you think of this twist on your implementation: https://github.com/NeedleInAJayStack/Graphiti/tree/federation_jay

If you're okay with it, we can pull those commits into this MR.

@samisuteria
Copy link
Contributor Author

samisuteria commented Feb 13, 2023

@NeedleInAJayStack I really like the changes on your branch and the Key implementation / mapping to the Resolver argument types is a much better way to implement federation.

The syntax for defining keys in the schema seems a bit complicated (anytime you have multiple builder closures in a function).

What do you think of something like this? It would just require keys: [KeyComponent<ObjectType, Resolver, Context>] to be mutable.

Type(Product.self) {
    Field("id", at: \.id)
    Field("sku", at: \.sku)
    Field("package", at: \.package)
    Field("variation", at: \.variation)
    Field("dimensions", at: \.dimensions)
    Field("createdBy", at: \.createdBy)
    Field("notes", at: \.notes)
    Field("research", at: \.research)
}
.key(at: ProductResolver.getProduct1) {
    Argument("id", at: \.id)
}
.key(at: ProductResolver.getProduct2) {
    Argument("sku", at: \.sku)
    Argument("package", at: \.package)
}
.key(at: ProductResolver.getProduct3) {
    Argument("sku", at: \.sku)
    Argument("variation", at: \.variation)
}

Eventually when the library can generate the SDL itself, I imagine other directives would work the same way and the syntax would be similar to SwiftUI's ViewModifier

@NeedleInAJayStack
Copy link
Member

NeedleInAJayStack commented Feb 13, 2023

@samisuteria Ooo, I love that idea. You're right, it looks cleaner and better preserves the public API

@NeedleInAJayStack
Copy link
Member

@samisuteria I just implemented your suggestion on my branch. If you like it, feel free to pull those commits into this MR. Thanks again!

@samisuteria
Copy link
Contributor Author

Just pulled in the changes - thank you so much for the help on this :)

@samisuteria
Copy link
Contributor Author

This is a useful dockerfile if you're on Apple Silicon and want to test older versions of Swift:

FROM swift:5.6-focal as swift5.6

WORKDIR /build

# Cache dependencies
COPY ./Package.* ./
RUN swift package resolve

COPY . .
RUN swift test --parallel

FROM swiftarm/swift:5.5.3-debian-buster as swift5.5

WORKDIR /build

# Cache dependencies
COPY ./Package.* ./
RUN swift package resolve

COPY . .
RUN swift test --parallel

FROM swiftarm/swift:5.4.3-focal-multi-arch as swift5.4

WORKDIR /build

# Cache dependencies
COPY ./Package.* ./
RUN swift package resolve

COPY . .
RUN swift test --parallel

FROM swift:latest

WORKDIR /build

# Make sure all stages actually run
COPY --from=swift5.6 ./build ./build-5.6
COPY --from=swift5.5 ./build ./build-5.5
COPY --from=swift5.4 ./build ./build-5.4

Copy link
Member

@NeedleInAJayStack NeedleInAJayStack left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the CI @samisuteria!

@NeedleInAJayStack NeedleInAJayStack merged commit 14f378c into GraphQLSwift:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants