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.

Sign up for GitHub

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

Jump to bottom

Ensure directives are preserved when serializing subgraph schemas in planner #568

Merged
merged 6 commits into from
Aug 26, 2024
Merged

Ensure directives are preserved when serializing subgraph schemas in planner #568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56fd74c
Ensure directives are preserved when serializing subgraph schemas in …
tninesling Aug 23, 2024
bec6cc0
Fix existing test which now has directives preserved
tninesling Aug 23, 2024
ac6c6d2
Update remaining snapshots for tests that now have directives preserved
tninesling Aug 23, 2024
4ff041b
Use fed internals schema printer
tninesling Aug 26, 2024
4cf29eb
Fix package version
tninesling Aug 26, 2024
837f731
Use separate snapshots for each subgraph in extract test
tninesling Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion router-bridge/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "router-bridge"
version = "0.6.0-beta.0+v2.9.0-beta.0"
version = "0.6.0-beta.1+v2.9.0-beta.0"
authors = ["Apollo <packages@apollographql.com>"]
edition = "2018"
description = "JavaScript bridge for the Apollo Router"
Expand Down
3 changes: 2 additions & 1 deletion router-bridge/js-src/plan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
import {
Operation,
operationFromDocument,
printSchema as printSchemaWithDirectives,
Supergraph,
} from "@apollo/federation-internals";
import {
Expand Down Expand Up @@ -281,7 +282,7 @@ export class BridgeQueryPlanner {
let result = new Map<string, string>();

subgraphs.names().forEach((name) => {
let sdl = printSchema(subgraphs.get(name).schema.toGraphQLJSSchema({}));
let sdl = printSchemaWithDirectives(subgraphs.get(name).schema);
Copy link
Contributor

@sachindshinde sachindshinde Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(This comment is more of an FYI, nothing to fix.) Looking at the PR that introduced subgraphs(), it seems this method is used to generate subgraph schemas to validate subgraph responses against. Two things here:

  1. I'm guessing here that the presence of directive applications in the subgraph schema shouldn't impede this subgraph response validation.
  2. We're also presumably now asserting downstream that the subgraph schemas with directive applications are valid GraphQL (which they ought to be), but I checked and that's not a new assertion in router (we use the default of shouldValidate when calling Supergraph.build() in router), so it should be fine (i.e., if there were bugs in how we generated/printed subgraph schemas, they wouldn't suddenly be triggered after this PR).

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'm guessing here that the presence of directive applications in the subgraph schema shouldn't impede this subgraph response validation.

It shouldn't affect that part of the code. One thing I'm noticing when running the router tests against this change is that it changes the query hash in a few tests because the hash will include directives if they exist. So, this will bust the cache, but that happens anyway when we bump the federation version. I think this will still be correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tninesling

One thing I'm noticing when running the router tests against this change is that it changes the query hash in a few tests because the hash will include directives if they exist.

This presumably also would cause cache misses when just a subgraph schema's directive applications change, though if such caches are also keyed by the supergraph schema (which presumably hasn't had directive applications stripped away), then this cache miss was going to happen anyway, so it's fine.

It might be something for @Geal to chime in on once they're available, but it probably shouldn't block a new RC release.

result.set(name, sdl);
});

Expand Down
2 changes: 2 additions & 0 deletions router-bridge/js-src/supported_features.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
REQUIRES_SCOPES_VERSIONS,
SOURCE_VERSIONS,
CONTEXT_VERSIONS,
COST_VERSIONS,
} from "@apollo/federation-internals";

export const ROUTER_SUPPORTED_SUPERGRAPH_FEATURES: Set<string> = new Set(
Expand All @@ -32,3 +33,4 @@ addToRouterFeatures(REQUIRES_SCOPES_VERSIONS);
addToRouterFeatures(POLICY_VERSIONS);
addToRouterFeatures(SOURCE_VERSIONS);
addToRouterFeatures(CONTEXT_VERSIONS);
addToRouterFeatures(COST_VERSIONS);
4 changes: 2 additions & 2 deletions router-bridge/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion router-bridge/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@apollo/router-bridge",
"private": true,
"version": "2.9.0-beta.0",
"version": "2.9.0-beta.1",
"description": "Apollo Router JS Bridge Entrypoint",
"scripts": {
"build": "make-dir bundled js-dist && rm -f tsconfig.tsbuildinfo && tsc --build --verbose && node esbuild/bundler.js && cp js-dist/runtime.js js-dist/do_api_schema.js js-dist/do_introspect.js js-dist/plan_worker.js js-dist/test_logger_worker.js js-dist/test_get_random_values.js js-dist/test_url.js bundled/",
Expand Down
13 changes: 13 additions & 0 deletions router-bridge/src/planner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2466,4 +2466,17 @@ feature https://specs.apollo.dev/unsupported-feature/v0.1 is for: SECURITY but i
)
.unwrap());
}

#[tokio::test]
async fn extracts_cost_directives() {
let schema = include_str!("testdata/custom_cost_schema.graphql");
let planner = Planner::<serde_json::Value>::new(schema.to_string(), Default::default())
.await
.expect("can create planner");
let subgraphs = planner.subgraphs().await.expect("can extract subgraphs");

for (name, schema) in subgraphs {
insta::assert_snapshot!(name, schema);
}
}
}
112 changes: 112 additions & 0 deletions router-bridge/src/snapshots/router_bridge__planner__error_display__subgraphWithCost.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
---
source: router-bridge/src/planner.rs
expression: schema
---
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])
{
query: Query
}

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE

directive @requires(fields: federation__FieldSet!) on FIELD_DEFINITION

directive @provides(fields: federation__FieldSet!) on FIELD_DEFINITION

directive @external(reason: String) on OBJECT | FIELD_DEFINITION

directive @tag(name: String!) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA

directive @extends on OBJECT | INTERFACE

directive @shareable repeatable on OBJECT | FIELD_DEFINITION

directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION

directive @override(from: String!, label: String) on FIELD_DEFINITION

directive @composeDirective(name: String) repeatable on SCHEMA

directive @interfaceObject on OBJECT

directive @federation__authenticated on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__requiresScopes(scopes: [[federation__Scope!]!]!) on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__policy(policies: [[federation__Policy!]!]!) on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__sourceAPI repeatable on SCHEMA

directive @federation__sourceType repeatable on OBJECT | INTERFACE

directive @federation__sourceField repeatable on FIELD_DEFINITION

directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION

directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION

directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_DEFINITION | INPUT_FIELD_DEFINITION | OBJECT | SCALAR

directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION

enum link__Purpose {
"""
`SECURITY` features provide metadata necessary to securely resolve fields.
"""
SECURITY

"""
`EXECUTION` features provide metadata necessary for operation execution.
"""
EXECUTION
}

scalar link__Import

scalar federation__FieldSet

scalar federation__Scope

scalar federation__Policy

scalar federation__ContextFieldValue

enum AorB
@federation__cost(weight: 15)
{
A
B
}

scalar ExpensiveInt
@federation__cost(weight: 30)

type ExpensiveObject
@federation__cost(weight: 40)
{
id: ID
}

input InputTypeWithCost {
somethingWithCost: Int @federation__cost(weight: 20)
}

type Query {
fieldWithCost: Int @federation__cost(weight: 5)
argWithCost(arg: Int @federation__cost(weight: 10)): Int
enumWithCost: AorB
inputWithCost(someInput: InputTypeWithCost): Int
scalarWithCost: ExpensiveInt
objectWithCost: ExpensiveObject
_service: _Service!
}

scalar _Any

type _Service {
sdl: String
}
96 changes: 96 additions & 0 deletions router-bridge/src/snapshots/router_bridge__planner__error_display__subgraphWithListSize.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
---
source: router-bridge/src/planner.rs
expression: schema
---
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])
{
query: Query
}

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE

directive @requires(fields: federation__FieldSet!) on FIELD_DEFINITION

directive @provides(fields: federation__FieldSet!) on FIELD_DEFINITION

directive @external(reason: String) on OBJECT | FIELD_DEFINITION

directive @tag(name: String!) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA

directive @extends on OBJECT | INTERFACE

directive @shareable repeatable on OBJECT | FIELD_DEFINITION

directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION

directive @override(from: String!, label: String) on FIELD_DEFINITION

directive @composeDirective(name: String) repeatable on SCHEMA

directive @interfaceObject on OBJECT

directive @federation__authenticated on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__requiresScopes(scopes: [[federation__Scope!]!]!) on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__policy(policies: [[federation__Policy!]!]!) on FIELD_DEFINITION | OBJECT | INTERFACE | SCALAR | ENUM

directive @federation__sourceAPI repeatable on SCHEMA

directive @federation__sourceType repeatable on OBJECT | INTERFACE

directive @federation__sourceField repeatable on FIELD_DEFINITION

directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION

directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION

directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_DEFINITION | INPUT_FIELD_DEFINITION | OBJECT | SCALAR

directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION

enum link__Purpose {
"""
`SECURITY` features provide metadata necessary to securely resolve fields.
"""
SECURITY

"""
`EXECUTION` features provide metadata necessary for operation execution.
"""
EXECUTION
}

scalar link__Import

scalar federation__FieldSet

scalar federation__Scope

scalar federation__Policy

scalar federation__ContextFieldValue

type A {
id: ID
}

type Query {
fieldWithListSize: [String!] @federation__listSize(assumedSize: 2000, requireOneSlicingArgument: false)
fieldWithDynamicListSize(first: Int = 10): SizedField @federation__listSize(slicingArguments: ["first"], sizedFields: ["items"], requireOneSlicingArgument: true)
_service: _Service!
}

type SizedField {
items: [A]
}

scalar _Any

type _Service {
sdl: String
}
43 changes: 27 additions & 16 deletions router-bridge/src/snapshots/router_bridge__planner__tests__subgraphs-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
source: router-bridge/src/planner.rs
expression: schema
---
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])
{
query: Query
}

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE
Expand Down Expand Up @@ -72,31 +79,35 @@ scalar federation__Policy

scalar federation__ContextFieldValue

type Book implements Product {
type Book implements Product
@key(fields: "isbn")
{
isbn: String!
title: String
year: Int
similarBooks: [Book]!
metadata: [MetadataOrError]
upc: String!
sku: String!
name: String
price: String
details: ProductDetails
inStock: Boolean
upc: String! @external
sku: String! @external
name: String @external
price: String @external
details: ProductDetails @external
inStock: Boolean @external
}

type Error {
code: Int
message: String
code: Int @shareable
message: String @shareable
}

type KeyValue {
key: String!
value: String!
key: String! @shareable
value: String! @shareable
}

type Library {
type Library
@key(fields: "id")
{
id: ID!
name: String
}
Expand All @@ -117,13 +128,13 @@ interface ProductDetails {
}

type ProductDetailsBook implements ProductDetails {
country: String
pages: Int
country: String @shareable
pages: Int @shareable
}

type ProductDetailsFurniture implements ProductDetails {
country: String
color: String
country: String @shareable
color: String @shareable
}

type Query {
Expand Down
7 changes: 7 additions & 0 deletions router-bridge/src/snapshots/router_bridge__planner__tests__subgraphs-3.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@
source: router-bridge/src/planner.rs
expression: schema
---
schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])
{
query: Query
}

directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA

directive @key(fields: federation__FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE
Expand Down
Loading
Loading