diff --git a/.changesets/chore_update_router_bridge.md b/.changesets/chore_update_router_bridge.md deleted file mode 100644 index bc23827874..0000000000 --- a/.changesets/chore_update_router_bridge.md +++ /dev/null @@ -1,8 +0,0 @@ -> [!IMPORTANT] -> If you have enabled [Distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), this release changes the hashing algorithm used for the cache keys. On account of this, you should anticipate additional cache regeneration cost when updating between these versions while the new hashing algorithm comes into service. - -### Update to Federation v2.9.1 ([PR #6029](https://github.com/apollographql/router/pull/6029)) - -This release updates to Federation v2.9.1, which fixes edge cases in subgraph extraction logic when using spec renaming or spec URLs (e.g., `specs.apollo.dev`) that could impact the planner's ability to plan a query. - -By [@lrlna](https://github.com/lrlna) in https://github.com/apollographql/router/pull/6027 diff --git a/.changesets/chore_update_router_bridge_292.md b/.changesets/chore_update_router_bridge_292.md deleted file mode 100644 index ba482bd648..0000000000 --- a/.changesets/chore_update_router_bridge_292.md +++ /dev/null @@ -1,5 +0,0 @@ -### Update to Federation v2.9.2 ([PR #6069](https://github.com/apollographql/router/pull/6069)) - -This release updates to Federation v2.9.2, with a small fix to the internal `__typename` optimization and a fix to prevent argument name collisions in the `@context`/`@fromContext` directives. - -By [@dariuszkuc](https://github.com/dariuszkuc) in https://github.com/apollographql/router/pull/6069 diff --git a/.changesets/config_renee_router_768_mode_metrics.md b/.changesets/config_renee_router_768_mode_metrics.md deleted file mode 100644 index 840f45ab75..0000000000 --- a/.changesets/config_renee_router_768_mode_metrics.md +++ /dev/null @@ -1,9 +0,0 @@ -### Add metrics for Rust vs. Deno configuration values ([PR #6056](https://github.com/apollographql/router/pull/6056)) - -We are working on migrating the implementation of several JavaScript components in the router to native Rust versions. - -To track this work, the router now reports the values of the following configuration options to Apollo: -- `apollo.router.config.experimental_query_planner_mode` -- `apollo.router.config.experimental_introspection_mode` - -By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6056 \ No newline at end of file diff --git a/.changesets/feat_feat_key_from_file.md b/.changesets/feat_feat_key_from_file.md deleted file mode 100644 index afa3090192..0000000000 --- a/.changesets/feat_feat_key_from_file.md +++ /dev/null @@ -1,9 +0,0 @@ -### feat: allow users to load apollo key from file ([PR #5917](https://github.com/apollographql/router/pull/5917)) - -Users sometimes would rather not pass sensitive keys to the router through environment variables out of an abundance of caution. To help address this, you can now pass an argument `--apollo-key-path` or env var `APOLLO_KEY_PATH`, that takes a file location as an argument which is read and then used as the Apollo key for use with Uplink and usage reporting. - -This addresses a portion of #3264, specifically the APOLLO_KEY. - -Note: This feature is not available on Windows. - -By [@lleadbet](https://github.com/lleadbet) in https://github.com/apollographql/router/pull/5917 diff --git a/.changesets/maint_feature_rhaitelemetry.md b/.changesets/maint_feature_rhaitelemetry.md new file mode 100644 index 0000000000..b8d780b491 --- /dev/null +++ b/.changesets/maint_feature_rhaitelemetry.md @@ -0,0 +1,5 @@ +### Added telemetry for Rhai usage ([PR #6027](https://github.com/apollographql/router/pull/6027)) + +Added telemetry for Rhai usage + +By [@andrewmcgivery](https://github.com/andrewmcgivery) in https://github.com/apollographql/router/pull/6027 diff --git a/.circleci/config.yml b/.circleci/config.yml index d2be4bcd00..eea532b73b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -356,9 +356,9 @@ commands: equal: [ *arm_linux_test_executor, << parameters.platform >> ] steps: - run: - name: Install nightly-2024-09-22 Rust to build the fuzzers + name: Install nightly Rust to build the fuzzers command: | - rustup install nightly-2024-09-22 + rustup install nightly install_extra_tools: steps: @@ -524,7 +524,7 @@ commands: path: ./target/nextest/ci/junit.xml fuzz_build: steps: - - run: cargo +nightly-2024-09-22 fuzz build + - run: cargo +nightly fuzz build jobs: lint: diff --git a/.gitleaks.toml b/.gitleaks.toml index ee8cdf4cf9..98ec4f9428 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -15,6 +15,7 @@ paths = [ '''^apollo-router\/src\/.+\/testdata\/.+''', '''^apollo-router\/tests\/snapshots\/apollo_otel_traces__.+\.snap$''', + "docs/source/configuration/entity-caching.mdx" ] [[ rules ]] diff --git a/CHANGELOG.md b/CHANGELOG.md index 419cee0eb9..be7d77c929 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,85 @@ All notable changes to Router will be documented in this file. This project adheres to [Semantic Versioning v2.0.0](https://semver.org/spec/v2.0.0.html). +# [1.56.0] - 2024-10-01 + +> [!IMPORTANT] +> If you have enabled [Distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), this release changes the hashing algorithm used for the cache keys. On account of this, you should anticipate additional cache regeneration cost when updating between these versions while the new hashing algorithm comes into service. + +## 🚀 Features + +## Native query planner is now in public preview + +The native query planner is now in public preview. You can configure the `experimental_query_planner_mode` option in the router configuration YAML to change the mode of the native query planner. The following modes are available: + +- `new`: Enable _only_ the new Rust-native query planner in the hot-path of query execution. +- `legacy`: Enable _only_ the legacy JavaScript query planner in the hot-path of query execution. +- `both_best_effort`: Enables _both_ the new and legacy query planners. They are configured in a comparison-based mode of operation with the legacy planner in the hot-path and the and the new planner in the cold-path. Comparisons are made between the two plans on a sampled basis and metrics are available to analyze the differences in aggregate. + +### Support loading Apollo key from file ([PR #5917](https://github.com/apollographql/router/pull/5917)) + +You can now specific the location to a file containing the Apollo key that's used by Apollo Uplink and usage reporting. The router now supports both the `--apollo-key-path` CLI argument and the `APOLLO_KEY_PATH` environment variable for passing the file containing your Apollo key. + +Previously, the router supported only the `APOLLO_KEY` environment variable to provide the key. The new CLI argument and environment variable help users who prefer not to pass sensitive keys through environment variables. + +Note: This feature is unavailable for Windows. + +By [@lleadbet](https://github.com/lleadbet) in https://github.com/apollographql/router/pull/5917 + + +## 🐛 Fixes + +### Prevent sending internal `apollo_private.*` attributes to Jaeger collector ([PR #6033](https://github.com/apollographql/router/pull/6033)) + +When using the router's Jaeger collector to send traces, you will no longer receive span attributes with the `apollo_private.` prefix. Those attributes were incorrectly sent, as that prefix is reserved for internal attributes. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/6033 + +### Fix displaying custom event attributes on subscription events ([PR #6033](https://github.com/apollographql/router/pull/6033)) + +The router now properly displays custom event attributes that are set with selectors at the supergraph level. + +An example configuration: + +```yaml title=router.yaml +telemetry: + instrumentation: + events: + supergraph: + supergraph.event: + message: supergraph event + on: event_response # on every supergraph event (like subscription event for example) + level: info + attributes: + test: + static: foo + response.data: + response_data: $ # Display all the response data payload + response.errors: + response_errors: $ # Display all the response errors payload +``` + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/6033 + +### Update to Federation v2.9.2 ([PR #6069](https://github.com/apollographql/router/pull/6069)) + +This release updates to Federation v2.9.2, with a small fix to the internal `__typename` optimization and a fix to prevent argument name collisions in the `@context`/`@fromContext` directives. + +By [@dariuszkuc](https://github.com/dariuszkuc) in https://github.com/apollographql/router/pull/6069 + +## 📃 Configuration + +### Add metrics for Rust vs. Deno configuration values ([PR #6056](https://github.com/apollographql/router/pull/6056)) + +To help track the migration from JavaScript (Deno) to native Rust implementations, the router now reports the values of the following configuration options to Apollo: + +- `apollo.router.config.experimental_query_planner_mode` +- `apollo.router.config.experimental_introspection_mode` + +By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6056 + + + # [1.55.0] - 2024-09-24 > [!IMPORTANT] @@ -11,6 +90,41 @@ This project adheres to [Semantic Versioning v2.0.0](https://semver.org/spec/v2. ## 🚀 Features +### Entity cache invalidation preview ([PR #5889](https://github.com/apollographql/router/pull/5889)) + +> ⚠️ This is a preview for an [Enterprise feature](https://www.apollographql.com/blog/platform/evaluating-apollo-router-understanding-free-and-open-vs-commercial-features/) of the Apollo Router. It requires an organization with a [GraphOS Enterprise plan](https://www.apollographql.com/pricing/). If your organization doesn't currently have an Enterprise plan, you can test out this functionality with a [free Enterprise trial](https://studio.apollographql.com/signup?type=enterprise-trial). +> +> As a preview feature, it's subject to our [Preview launch stage](https://www.apollographql.com/docs/resources/product-launch-stages/#preview) expectations and configuration and performance may change in future releases. + +As a follow up to the Entity cache preview that was published in the router 1.46.0 release, we're introducing a new feature that allows you to invalidate cached entries. + +This introduces two ways to invalidate cached entries: +- through an HTTP endpoint exposed by the router +- via GraphQL response `extensions` returned from subgraph requests + +The invalidation endpoint can be defined in the router's configuration, as follows: + +```yaml +preview_entity_cache: + enabled: true + + # global invalidation configuration + invalidation: + # address of the invalidation endpoint + # this should only be exposed to internal networks + listen: "127.0.0.1:3000" + path: "/invalidation" +``` + +Invalidation requests can target cached entries by: +- subgraph name +- subgraph name and type name +- subgraph name, type name and entity key + +You can learn more about invalidation in the [documentation](https://www.apollographql.com/docs/router/configuration/entity-caching#entity-cache-invalidation). + +By [@bnjjj](https://github.com/bnjjj),[@bryncooke](https://github.com/bryncooke), [@garypen](https://github.com/garypen), [@Geal](https://github.com/Geal), [@IvanGoncharov](https://github.com/IvanGoncharov) in https://github.com/apollographql/router/pull/5889 + ### Support aliasing standard attributes for telemetry ([Issue #5930](https://github.com/apollographql/router/issues/5930)) The router now supports creating aliases for standard attributes for telemetry. diff --git a/Cargo.lock b/Cargo.lock index e0ae6cb85f..7128f24194 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -178,7 +178,7 @@ dependencies = [ [[package]] name = "apollo-federation" -version = "1.55.0" +version = "1.56.0" dependencies = [ "apollo-compiler", "derive_more", @@ -192,7 +192,6 @@ dependencies = [ "petgraph", "ron", "serde", - "serde_json", "serde_json_bytes", "sha1", "strum 0.26.3", @@ -229,7 +228,7 @@ dependencies = [ [[package]] name = "apollo-router" -version = "1.55.0" +version = "1.56.0" dependencies = [ "access-json", "ahash", @@ -237,7 +236,6 @@ dependencies = [ "apollo-compiler", "apollo-federation", "arc-swap", - "askama", "async-channel 1.9.0", "async-compression", "async-trait", @@ -260,7 +258,6 @@ dependencies = [ "ci_info", "clap", "console", - "console-subscriber", "cookie", "crossbeam-channel", "dashmap", @@ -399,7 +396,7 @@ dependencies = [ [[package]] name = "apollo-router-benchmarks" -version = "1.55.0" +version = "1.56.0" dependencies = [ "apollo-parser", "apollo-router", @@ -415,7 +412,7 @@ dependencies = [ [[package]] name = "apollo-router-scaffold" -version = "1.55.0" +version = "1.56.0" dependencies = [ "anyhow", "cargo-scaffold", @@ -436,7 +433,6 @@ dependencies = [ "anyhow", "apollo-router", "async-trait", - "futures", "schemars", "serde", "serde_json", @@ -497,50 +493,6 @@ version = "0.9.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "71938f30533e4d95a6d17aa530939da3842c2ab6f4f84b9dae68447e4129f74a" -[[package]] -name = "askama" -version = "0.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b79091df18a97caea757e28cd2d5fda49c6cd4bd01ddffd7ff01ace0c0ad2c28" -dependencies = [ - "askama_derive", - "askama_escape", - "humansize", - "num-traits", - "percent-encoding", -] - -[[package]] -name = "askama_derive" -version = "0.12.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "19fe8d6cb13c4714962c072ea496f3392015f0989b1a2847bb4b2d9effd71d83" -dependencies = [ - "askama_parser", - "basic-toml", - "mime", - "mime_guess", - "proc-macro2", - "quote", - "serde", - "syn 2.0.76", -] - -[[package]] -name = "askama_escape" -version = "0.10.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "619743e34b5ba4e9703bba34deac3427c72507c7159f5fd030aea8cac0cfe341" - -[[package]] -name = "askama_parser" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acb1161c6b64d1c3d83108213c2a2533a342ac225aabd0bda218278c2ddb00c0" -dependencies = [ - "nom", -] - [[package]] name = "assert-json-diff" version = "2.0.2" @@ -1710,43 +1662,6 @@ dependencies = [ "windows-sys 0.52.0", ] -[[package]] -name = "console-api" -version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fd326812b3fd01da5bb1af7d340d0d555fd3d4b641e7f1dfcf5962a902952787" -dependencies = [ - "futures-core", - "prost 0.12.6", - "prost-types 0.12.6", - "tonic 0.10.2", - "tracing-core", -] - -[[package]] -name = "console-subscriber" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7481d4c57092cd1c19dd541b92bdce883de840df30aa5d03fd48a3935c01842e" -dependencies = [ - "console-api", - "crossbeam-channel", - "crossbeam-utils", - "futures-task", - "hdrhistogram", - "humantime", - "prost-types 0.12.6", - "serde", - "serde_json", - "thread_local", - "tokio", - "tokio-stream", - "tonic 0.10.2", - "tracing", - "tracing-core", - "tracing-subscriber", -] - [[package]] name = "const-oid" version = "0.9.6" @@ -2550,11 +2465,6 @@ dependencies = [ "async-graphql-axum", "axum", "env_logger", - "futures", - "lazy_static", - "log", - "rand 0.8.5", - "serde_json", "tokio", "tower", ] @@ -3180,10 +3090,7 @@ version = "7.5.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "765c9198f173dd59ce26ff9f95ef0aafd0a0fe01fb9d72841bc5066a4c06511d" dependencies = [ - "base64 0.21.7", "byteorder", - "flate2", - "nom", "num-traits", ] @@ -3393,15 +3300,6 @@ version = "1.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" -[[package]] -name = "humansize" -version = "2.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6cb51c9a029ddc91b07a787f1d86b53ccfa49b0e86688c946ebe8d3555685dd7" -dependencies = [ - "libm", -] - [[package]] name = "humantime" version = "2.1.0" @@ -3889,12 +3787,6 @@ dependencies = [ "pkg-config", ] -[[package]] -name = "libm" -version = "0.2.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4ec2a862134d2a7d32d7983ddcdd1c4923530833c9f2ea1a44fc5fa473989058" - [[package]] name = "libredox" version = "0.1.3" @@ -5626,7 +5518,6 @@ name = "router-fuzz" version = "0.0.0" dependencies = [ "anyhow", - "apollo-compiler", "apollo-parser", "apollo-router", "apollo-smith", @@ -5636,12 +5527,10 @@ dependencies = [ "libfuzzer-sys", "log", "reqwest", - "router-bridge", "schemars", "serde", "serde_json", "serde_json_bytes", - "tokio", "tower", ] @@ -6688,7 +6577,6 @@ dependencies = [ "signal-hook-registry", "socket2 0.5.7", "tokio-macros", - "tracing", "windows-sys 0.48.0", ] @@ -6856,33 +6744,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "tonic" -version = "0.10.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d560933a0de61cf715926b9cac824d4c883c2c43142f787595e48280c40a1d0e" -dependencies = [ - "async-stream", - "async-trait", - "axum", - "base64 0.21.7", - "bytes", - "h2", - "http 0.2.12", - "http-body 0.4.6", - "hyper", - "hyper-timeout", - "percent-encoding", - "pin-project", - "prost 0.12.6", - "tokio", - "tokio-stream", - "tower", - "tower-layer", - "tower-service", - "tracing", -] - [[package]] name = "tonic" version = "0.11.0" diff --git a/apollo-federation/Cargo.toml b/apollo-federation/Cargo.toml index 5de40a5380..9cece4b44b 100644 --- a/apollo-federation/Cargo.toml +++ b/apollo-federation/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-federation" -version = "1.55.0" +version = "1.56.0" authors = ["The Apollo GraphQL Contributors"] edition = "2021" description = "Apollo Federation" @@ -29,7 +29,6 @@ multimap = "0.10.0" nom = "7.1.3" petgraph = { version = "0.6.4", features = ["serde-1"] } serde.workspace = true -serde_json.workspace = true serde_json_bytes.workspace = true strum = "0.26.0" strum_macros = "0.26.0" diff --git a/apollo-federation/src/api_schema.rs b/apollo-federation/src/api_schema.rs index bf1b1316d7..d515009084 100644 --- a/apollo-federation/src/api_schema.rs +++ b/apollo-federation/src/api_schema.rs @@ -110,7 +110,7 @@ pub struct ApiSchemaOptions { pub include_stream: bool, } -pub fn to_api_schema( +pub(crate) fn to_api_schema( schema: ValidFederationSchema, options: ApiSchemaOptions, ) -> Result { diff --git a/apollo-federation/src/compat.rs b/apollo-federation/src/compat.rs index d4faa04880..bd571f3f5c 100644 --- a/apollo-federation/src/compat.rs +++ b/apollo-federation/src/compat.rs @@ -76,7 +76,7 @@ fn retain_semantic_directives_ast(directives: &mut apollo_compiler::ast::Directi /// Remove non-semantic directive applications from the schema representation. /// This only keeps directive applications that are observable in introspection. -pub fn remove_non_semantic_directives(schema: &mut Schema) { +pub(crate) fn remove_non_semantic_directives(schema: &mut Schema) { let root_definitions = schema.schema_definition.make_mut(); retain_semantic_directives(&mut root_definitions.directives); @@ -241,7 +241,7 @@ fn coerce_arguments_default_values( /// This is not what we would want to do for coercion in a real execution scenario, but it matches /// a behaviour in graphql-js so we can compare API schema results between federation-next and JS /// federation. We can consider removing this when we no longer rely on JS federation. -pub fn coerce_schema_default_values(schema: &mut Schema) { +pub(crate) fn coerce_schema_default_values(schema: &mut Schema) { // Keep a copy of the types in the schema so we can mutate the schema while walking it. let types = schema.types.clone(); @@ -357,7 +357,7 @@ fn coerce_operation_values(schema: &Valid, operation: &mut Node, document: &mut ExecutableDocument) { +pub(crate) fn coerce_executable_values(schema: &Valid, document: &mut ExecutableDocument) { if let Some(operation) = &mut document.operations.anonymous { coerce_operation_values(schema, operation); } @@ -369,7 +369,7 @@ pub fn coerce_executable_values(schema: &Valid, document: &mut Executabl /// Applies default value coercion and removes non-semantic directives so that /// the apollo-rs serialized output of the schema matches the result of /// `printSchema(buildSchema()` in graphql-js. -pub fn make_print_schema_compatible(schema: &mut Schema) { +pub(crate) fn make_print_schema_compatible(schema: &mut Schema) { remove_non_semantic_directives(schema); coerce_schema_default_values(schema); } diff --git a/apollo-federation/src/display_helpers.rs b/apollo-federation/src/display_helpers.rs index f89693123a..330ec64003 100644 --- a/apollo-federation/src/display_helpers.rs +++ b/apollo-federation/src/display_helpers.rs @@ -1,7 +1,6 @@ use std::fmt; use std::fmt::Debug; use std::fmt::Display; -use std::ops::Deref; use serde::Serializer; @@ -22,8 +21,8 @@ impl<'a, 'b> State<'a, 'b> { self.indent_level } - pub(crate) fn write(&mut self, value: T) -> fmt::Result { - write!(self.output, "{}", value) + pub(crate) fn write(&mut self, value: T) -> fmt::Result { + write!(self.output, "{value}") } pub(crate) fn write_fmt(&mut self, args: fmt::Arguments<'_>) -> fmt::Result { @@ -116,23 +115,6 @@ where ser.serialize_str(&data.to_string()) } -pub(crate) fn serialize_option_as_string(data: &Option, ser: S) -> Result -where - T: Display, - S: Serializer, -{ - serialize_as_string(&DisplayOption(data.as_ref()), ser) -} - -pub(crate) fn serialize_vec_as_string(data: &P, ser: S) -> Result -where - P: Deref>, - T: Display, - S: Serializer, -{ - serialize_as_string(&DisplaySlice(data), ser) -} - pub(crate) fn serialize_optional_vec_as_string( data: &Option>, ser: S, diff --git a/apollo-federation/src/lib.rs b/apollo-federation/src/lib.rs index ed02b15ada..99d340e881 100644 --- a/apollo-federation/src/lib.rs +++ b/apollo-federation/src/lib.rs @@ -13,10 +13,17 @@ //! See [Router documentation](https://www.apollographql.com/docs/router/federation-version-support/) //! for which Federation versions are supported by which Router versions. -// TODO: This is fine while we're iterating, but should be removed later. -#![allow(dead_code)] -// TODO: silence false positives (apollo_compiler::Name) and investigate the rest -#![allow(clippy::mutable_key_type)] +#![warn( + rustdoc::broken_intra_doc_links, + unreachable_pub, + unreachable_patterns, + unused, + unused_qualifications, + dead_code, + while_true, + unconditional_panic, + clippy::all +)] mod api_schema; mod compat; diff --git a/apollo-federation/src/link/cost_spec_definition.rs b/apollo-federation/src/link/cost_spec_definition.rs index db49185b04..9e2aed07cc 100644 --- a/apollo-federation/src/link/cost_spec_definition.rs +++ b/apollo-federation/src/link/cost_spec_definition.rs @@ -147,11 +147,6 @@ impl CostSpecDefinition { apollo_compiler::ast::DirectiveList, Node::new ); - propagate_demand_control_directives!( - propagate_demand_control_schema_directives, - apollo_compiler::schema::DirectiveList, - Component::from - ); propagate_demand_control_directives_to_position!( propagate_demand_control_directives_for_enum, diff --git a/apollo-federation/src/link/graphql_definition.rs b/apollo-federation/src/link/graphql_definition.rs index 31226d6a7d..bb2b54f2d1 100644 --- a/apollo-federation/src/link/graphql_definition.rs +++ b/apollo-federation/src/link/graphql_definition.rs @@ -80,15 +80,6 @@ impl Display for BooleanOrVariable { } } -impl BooleanOrVariable { - pub(crate) fn to_ast_value(&self) -> Value { - match self { - BooleanOrVariable::Boolean(b) => Value::Boolean(*b), - BooleanOrVariable::Variable(name) => Value::Variable(name.clone()), - } - } -} - impl From for Value { fn from(b: BooleanOrVariable) -> Self { match b { diff --git a/apollo-federation/src/link/inaccessible_spec_definition.rs b/apollo-federation/src/link/inaccessible_spec_definition.rs index c6e0fbf149..fe72ba8c57 100644 --- a/apollo-federation/src/link/inaccessible_spec_definition.rs +++ b/apollo-federation/src/link/inaccessible_spec_definition.rs @@ -5,7 +5,6 @@ use apollo_compiler::collections::IndexSet; use apollo_compiler::name; use apollo_compiler::schema::Component; use apollo_compiler::schema::ComponentName; -use apollo_compiler::schema::Directive; use apollo_compiler::schema::DirectiveDefinition; use apollo_compiler::schema::DirectiveLocation; use apollo_compiler::schema::ExtendedType; @@ -54,27 +53,12 @@ impl InaccessibleSpecDefinition { } } - pub(crate) fn inaccessible_directive( - &self, - schema: &FederationSchema, - ) -> Result { - let name_in_schema = self - .directive_name_in_schema(schema, &INACCESSIBLE_DIRECTIVE_NAME_IN_SPEC)? - .ok_or_else(|| SingleFederationError::Internal { - message: "Unexpectedly could not find inaccessible spec in schema".to_owned(), - })?; - Ok(Directive { - name: name_in_schema, - arguments: Vec::new(), - }) - } - /// Returns the `@inaccessible` spec used in the given schema, if any. /// /// # Errors /// Returns an error if the schema specifies an `@inaccessible` spec version that is not /// supported by this version of the apollo-federation crate. - pub fn get_from_schema( + pub(crate) fn get_from_schema( schema: &FederationSchema, ) -> Result, FederationError> { let inaccessible_link = match schema @@ -94,11 +78,14 @@ impl InaccessibleSpecDefinition { )) } - pub fn validate_inaccessible(&self, schema: &FederationSchema) -> Result<(), FederationError> { + pub(crate) fn validate_inaccessible( + &self, + schema: &FederationSchema, + ) -> Result<(), FederationError> { validate_inaccessible(schema, self) } - pub fn remove_inaccessible_elements( + pub(crate) fn remove_inaccessible_elements( &self, schema: &mut FederationSchema, ) -> Result<(), FederationError> { @@ -535,7 +522,7 @@ impl IsInaccessibleExt for position::ObjectTypeDefinitionPosition { Ok(object.directives.has(inaccessible_directive)) } } -impl IsInaccessibleExt for position::ObjectFieldDefinitionPosition { +impl IsInaccessibleExt for ObjectFieldDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, @@ -549,7 +536,7 @@ impl IsInaccessibleExt for position::ObjectFieldDefinitionPosition { .is_inaccessible(schema, inaccessible_directive)?) } } -impl IsInaccessibleExt for position::ObjectFieldArgumentDefinitionPosition { +impl IsInaccessibleExt for ObjectFieldArgumentDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, @@ -573,7 +560,7 @@ impl IsInaccessibleExt for position::InterfaceTypeDefinitionPosition { Ok(interface.directives.has(inaccessible_directive)) } } -impl IsInaccessibleExt for position::InterfaceFieldDefinitionPosition { +impl IsInaccessibleExt for InterfaceFieldDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, @@ -587,7 +574,7 @@ impl IsInaccessibleExt for position::InterfaceFieldDefinitionPosition { .is_inaccessible(schema, inaccessible_directive)?) } } -impl IsInaccessibleExt for position::InterfaceFieldArgumentDefinitionPosition { +impl IsInaccessibleExt for InterfaceFieldArgumentDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, @@ -611,7 +598,7 @@ impl IsInaccessibleExt for position::InputObjectTypeDefinitionPosition { Ok(input_object.directives.has(inaccessible_directive)) } } -impl IsInaccessibleExt for position::InputObjectFieldDefinitionPosition { +impl IsInaccessibleExt for InputObjectFieldDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, @@ -655,7 +642,7 @@ impl IsInaccessibleExt for position::EnumTypeDefinitionPosition { Ok(enum_.directives.has(inaccessible_directive)) } } -impl IsInaccessibleExt for position::EnumValueDefinitionPosition { +impl IsInaccessibleExt for EnumValueDefinitionPosition { fn is_inaccessible( &self, schema: &FederationSchema, diff --git a/apollo-federation/src/merge.rs b/apollo-federation/src/merge.rs index 5c5531c5ec..231fb973b0 100644 --- a/apollo-federation/src/merge.rs +++ b/apollo-federation/src/merge.rs @@ -1578,20 +1578,6 @@ fn add_core_feature_inaccessible(supergraph: &mut Schema) { ); } -// TODO use apollo_compiler::executable::FieldSet -fn parse_keys<'a>( - directives: impl Iterator> + Sized, -) -> IndexSet<&'a str> { - IndexSet::from_iter( - directives - .flat_map(|k| { - let field_set = directive_string_arg_value(k, &name!("fields")).unwrap(); - field_set.split_whitespace() - }) - .collect::>(), - ) -} - fn merge_directive( supergraph_directives: &mut IndexMap>, directive: &Node, diff --git a/apollo-federation/src/operation/contains.rs b/apollo-federation/src/operation/contains.rs index e69f978b3f..6cc257e7d6 100644 --- a/apollo-federation/src/operation/contains.rs +++ b/apollo-federation/src/operation/contains.rs @@ -13,10 +13,10 @@ pub(super) fn is_deferred_selection(directives: &executable::DirectiveList) -> b /// Options for the `.containment()` family of selection functions. #[derive(Debug, Clone, Copy)] -pub struct ContainmentOptions { +pub(crate) struct ContainmentOptions { /// If the right-hand side has a __typename selection but the left-hand side does not, /// still consider the left-hand side to contain the right-hand side. - pub ignore_missing_typename: bool, + pub(crate) ignore_missing_typename: bool, } // Currently Default *can* be derived, but if we add a new option @@ -31,7 +31,7 @@ impl Default for ContainmentOptions { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Containment { +pub(crate) enum Containment { /// The left-hand selection does not fully contain right-hand selection. NotContained, /// The left-hand selection fully contains the right-hand selection, and more. @@ -41,17 +41,21 @@ pub enum Containment { } impl Containment { /// Returns true if the right-hand selection set is strictly contained or equal. - pub fn is_contained(self) -> bool { + pub(crate) fn is_contained(self) -> bool { matches!(self, Containment::StrictlyContained | Containment::Equal) } - pub fn is_equal(self) -> bool { + pub(crate) fn is_equal(self) -> bool { matches!(self, Containment::Equal) } } impl Selection { - pub fn containment(&self, other: &Selection, options: ContainmentOptions) -> Containment { + pub(crate) fn containment( + &self, + other: &Selection, + options: ContainmentOptions, + ) -> Containment { match (self, other) { (Selection::Field(self_field), Selection::Field(other_field)) => { self_field.containment(other_field, options) @@ -69,13 +73,17 @@ impl Selection { } /// Returns true if this selection is a superset of the other selection. - pub fn contains(&self, other: &Selection) -> bool { + pub(crate) fn contains(&self, other: &Selection) -> bool { self.containment(other, Default::default()).is_contained() } } impl FieldSelection { - pub fn containment(&self, other: &FieldSelection, options: ContainmentOptions) -> Containment { + pub(crate) fn containment( + &self, + other: &FieldSelection, + options: ContainmentOptions, + ) -> Containment { if self.field.name() != other.field.name() || self.field.alias != other.field.alias || self.field.arguments != other.field.arguments @@ -95,15 +103,14 @@ impl FieldSelection { } } } - - /// Returns true if this selection is a superset of the other selection. - pub fn contains(&self, other: &FieldSelection) -> bool { - self.containment(other, Default::default()).is_contained() - } } impl FragmentSpreadSelection { - pub fn containment(&self, other: &Selection, options: ContainmentOptions) -> Containment { + pub(crate) fn containment( + &self, + other: &Selection, + options: ContainmentOptions, + ) -> Containment { match other { // Using keys here means that @defer fragments never compare equal. // This is a bit odd but it is consistent: the selection set data structure would not @@ -114,15 +121,14 @@ impl FragmentSpreadSelection { _ => Containment::NotContained, } } - - /// Returns true if this selection is a superset of the other selection. - pub fn contains(&self, other: &Selection) -> bool { - self.containment(other, Default::default()).is_contained() - } } impl InlineFragmentSelection { - pub fn containment(&self, other: &Selection, options: ContainmentOptions) -> Containment { + pub(crate) fn containment( + &self, + other: &Selection, + options: ContainmentOptions, + ) -> Containment { match other { // Using keys here means that @defer fragments never compare equal. // This is a bit odd but it is consistent: the selection set data structure would not @@ -136,15 +142,10 @@ impl InlineFragmentSelection { _ => Containment::NotContained, } } - - /// Returns true if this selection is a superset of the other selection. - pub fn contains(&self, other: &Selection) -> bool { - self.containment(other, Default::default()).is_contained() - } } impl SelectionSet { - pub fn containment(&self, other: &Self, options: ContainmentOptions) -> Containment { + pub(crate) fn containment(&self, other: &Self, options: ContainmentOptions) -> Containment { if other.selections.len() > self.selections.len() { // If `other` has more selections but we're ignoring missing __typename, then in the case where // `other` has a __typename but `self` does not, then we need the length of `other` to be at @@ -194,7 +195,7 @@ impl SelectionSet { } /// Returns true if this selection is a superset of the other selection. - pub fn contains(&self, other: &Self) -> bool { + pub(crate) fn contains(&self, other: &Self) -> bool { self.containment(other, Default::default()).is_contained() } } diff --git a/apollo-federation/src/operation/directive_list.rs b/apollo-federation/src/operation/directive_list.rs index 913a1184e6..df03995b53 100644 --- a/apollo-federation/src/operation/directive_list.rs +++ b/apollo-federation/src/operation/directive_list.rs @@ -181,7 +181,7 @@ impl Deref for DirectiveList { } impl Hash for DirectiveList { - fn hash(&self, state: &mut H) { + fn hash(&self, state: &mut H) { state.write_u64(self.inner.as_ref().map_or(0, |inner| inner.hash)) } } diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 029294e9ac..f9ae87afa4 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -35,7 +35,6 @@ use serde::Serialize; use crate::compat::coerce_executable_values; use crate::error::FederationError; use crate::error::SingleFederationError; -use crate::error::SingleFederationError::Internal; use crate::link::graphql_definition::BooleanOrVariable; use crate::link::graphql_definition::DeferDirectiveArguments; use crate::query_graph::graph_path::OpPathElement; @@ -274,7 +273,6 @@ mod selection_map { use crate::operation::field_selection::FieldSelection; use crate::operation::fragment_spread_selection::FragmentSpreadSelection; use crate::operation::inline_fragment_selection::InlineFragmentSelection; - use crate::operation::DirectiveList; use crate::operation::HasSelectionKey; use crate::operation::Selection; use crate::operation::SelectionKey; @@ -307,6 +305,7 @@ mod selection_map { SelectionMap(IndexMap::default()) } + #[cfg(test)] pub(crate) fn clear(&mut self) { self.0.clear(); } @@ -315,11 +314,6 @@ mod selection_map { self.0.insert(value.key(), value) } - /// Insert a selection at a specific index. - pub(crate) fn insert_at(&mut self, index: usize, value: Selection) -> Option { - self.0.shift_insert(index, value.key(), value) - } - /// Remove a selection from the map. Returns the selection and its numeric index. pub(crate) fn remove(&mut self, key: &SelectionKey) -> Option<(usize, Selection)> { // We specifically use shift_remove() instead of swap_remove() to maintain order. @@ -486,14 +480,7 @@ mod selection_map { } } - pub(super) fn directives(&self) -> &'_ DirectiveList { - match self { - Self::Field(field) => &field.get().field.directives, - Self::FragmentSpread(frag) => &frag.get().spread.directives, - Self::InlineFragment(frag) => &frag.get().inline_fragment.directives, - } - } - + #[cfg(test)] pub(super) fn get_selection_set_mut(&mut self) -> Option<&mut SelectionSet> { match self { Self::Field(field) => field.get_selection_set_mut().as_mut(), @@ -532,6 +519,7 @@ mod selection_map { Self(fragment_spread_selection) } + #[cfg(test)] pub(crate) fn get_selection_set_mut(&mut self) -> &mut SelectionSet { &mut Arc::make_mut(self.0).selection_set } @@ -564,7 +552,7 @@ mod selection_map { } impl<'a> Entry<'a> { - pub fn or_insert( + pub(crate) fn or_insert( self, produce: impl FnOnce() -> Result, ) -> Result, FederationError> { @@ -582,22 +570,9 @@ mod selection_map { self.0.get() } - pub(crate) fn get_mut(&mut self) -> SelectionValue { - SelectionValue::new(self.0.get_mut()) - } - pub(crate) fn into_mut(self) -> SelectionValue<'a> { SelectionValue::new(self.0.into_mut()) } - - pub(crate) fn key(&self) -> &SelectionKey { - self.0.key() - } - - pub(crate) fn remove(self) -> Selection { - // We specifically use shift_remove() instead of swap_remove() to maintain order. - self.0.shift_remove() - } } pub(crate) struct VacantEntry<'a>(indexmap::map::VacantEntry<'a, SelectionKey, Selection>); @@ -812,7 +787,7 @@ impl Selection { Selection::Field(field_selection) => { Ok(OpPathElement::Field(field_selection.field.clone())) } - Selection::FragmentSpread(_) => Err(Internal { + Selection::FragmentSpread(_) => Err(SingleFederationError::Internal { message: "Fragment spread does not have element".to_owned(), } .into()), @@ -907,23 +882,6 @@ impl Selection { self.with_updated_selection_set(Some(new_sub_selection)) } - pub(crate) fn with_updated_directives( - &self, - directives: impl Into, - ) -> Result { - match self { - Selection::Field(field) => Ok(Selection::Field(Arc::new( - field.with_updated_directives(directives), - ))), - Selection::InlineFragment(inline_fragment) => Ok(Selection::InlineFragment(Arc::new( - inline_fragment.with_updated_directives(directives), - ))), - Selection::FragmentSpread(_) => { - Err(FederationError::internal("unexpected fragment spread")) - } - } - } - /// Apply the `mapper` to self.selection_set, if it exists, and return a new `Selection`. /// - Note: The returned selection may have no subselection set or an empty one if the mapper /// returns so, which may make the returned selection invalid. It's caller's responsibility @@ -1031,7 +989,6 @@ mod field_selection { use std::hash::Hasher; use std::ops::Deref; - use apollo_compiler::ast; use apollo_compiler::Name; use serde::Serialize; @@ -1041,7 +998,6 @@ mod field_selection { use crate::operation::HasSelectionKey; use crate::operation::SelectionKey; use crate::operation::SelectionSet; - use crate::query_graph::graph_path::OpPathElement; use crate::query_plan::FetchDataPathElement; use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::FieldDefinitionPosition; @@ -1080,17 +1036,6 @@ mod field_selection { } } - pub(crate) fn with_updated_directives(&self, directives: impl Into) -> Self { - Self { - field: self.field.with_updated_directives(directives), - selection_set: self.selection_set.clone(), - } - } - - pub(crate) fn element(&self) -> OpPathElement { - OpPathElement::Field(self.field.clone()) - } - pub(crate) fn with_updated_alias(&self, alias: Name) -> Field { let mut data = self.field.data().clone(); data.alias = Some(alias); @@ -1146,14 +1091,6 @@ mod field_selection { } } - /// Create a trivial field selection without any arguments or directives. - pub(crate) fn from_position( - schema: &ValidFederationSchema, - field_position: FieldDefinitionPosition, - ) -> Self { - Self::new(FieldData::from_position(schema, field_position)) - } - // Note: The `schema` argument must be a subgraph schema, so the __typename field won't // need to be rebased, which would fail (since __typename fields are undefined). pub(crate) fn new_introspection_typename( @@ -1221,10 +1158,6 @@ mod field_selection { &self.data } - pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { - &mut self.data.directives - } - pub(crate) fn sibling_typename(&self) -> Option<&SiblingTypename> { self.data.sibling_typename.as_ref() } @@ -1285,8 +1218,9 @@ mod field_selection { } impl FieldData { + #[cfg(test)] /// Create a trivial field selection without any arguments or directives. - pub fn from_position( + pub(crate) fn from_position( schema: &ValidFederationSchema, field_position: FieldDefinitionPosition, ) -> Self { @@ -1308,10 +1242,6 @@ mod field_selection { self.alias.clone().unwrap_or_else(|| self.name().clone()) } - fn output_ast_type(&self) -> Result<&ast::Type, FederationError> { - Ok(&self.field_position.get(self.schema.schema())?.ty) - } - pub(crate) fn output_base_type(&self) -> Result { let definition = self.field_position.get(self.schema.schema())?; self.schema @@ -1411,10 +1341,6 @@ mod fragment_spread_selection { pub(crate) fn data(&self) -> &FragmentSpreadData { &self.data } - - pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { - &mut self.data.directives - } } impl HasSelectionKey for FragmentSpread { @@ -1464,16 +1390,6 @@ pub(crate) use fragment_spread_selection::FragmentSpreadData; pub(crate) use fragment_spread_selection::FragmentSpreadSelection; impl FragmentSpreadSelection { - /// Copies fragment spread selection and assigns it a new unique selection ID. - pub(crate) fn with_unique_id(&self) -> Self { - let mut data = self.spread.data().clone(); - data.selection_id = SelectionId::new(); - Self { - spread: FragmentSpread::new(data), - selection_set: self.selection_set.clone(), - } - } - /// Normalize this fragment spread into a "normalized" spread representation with following /// modifications /// - Stores the schema (may be useful for directives). @@ -1672,10 +1588,6 @@ mod inline_fragment_selection { &self.data } - pub(super) fn directives_mut(&mut self) -> &mut DirectiveList { - &mut self.data.directives - } - pub(crate) fn with_updated_type_condition( &self, new: Option, @@ -1901,7 +1813,7 @@ impl SelectionSet { } #[cfg(any(doc, test))] - pub fn parse( + pub(crate) fn parse( schema: ValidFederationSchema, type_position: CompositeTypeDefinitionPosition, source_text: &str, @@ -1922,7 +1834,7 @@ impl SelectionSet { pub(crate) fn contains_top_level_field(&self, field: &Field) -> Result { if let Some(selection) = self.selections.get(&field.key()) { let Selection::Field(field_selection) = selection else { - return Err(Internal { + return Err(SingleFederationError::Internal { message: format!( "Field selection key for field \"{}\" references non-field selection", field.field_position, @@ -1996,7 +1908,7 @@ impl SelectionSet { executable::Selection::FragmentSpread(fragment_spread_selection) => { let Some(fragment) = fragments.get(&fragment_spread_selection.fragment_name) else { - return Err(Internal { + return Err(SingleFederationError::Internal { message: format!( "Fragment spread referenced non-existent fragment \"{}\"", fragment_spread_selection.fragment_name, @@ -2950,12 +2862,6 @@ pub(crate) struct CollectedFieldInSet { field: Arc, } -impl CollectedFieldInSet { - pub(crate) fn field(&self) -> &Arc { - &self.field - } -} - struct FieldInPath { path: Vec, field: Arc, @@ -3237,16 +3143,6 @@ impl InlineFragmentSelection { } } - /// Copies inline fragment selection and assigns it a new unique selection ID. - pub(crate) fn with_unique_id(&self) -> Self { - let mut data = self.inline_fragment.data().clone(); - data.selection_id = SelectionId::new(); - Self { - inline_fragment: InlineFragment::new(data), - selection_set: self.selection_set.clone(), - } - } - /// Normalize this inline fragment selection (merging selections with the same keys), with the /// following additional transformations: /// - Expand fragment spreads into inline fragments. @@ -3433,18 +3329,6 @@ impl NamedFragments { Arc::make_mut(&mut self.fragments).insert(fragment.name.clone(), Node::new(fragment)); } - fn try_insert(&mut self, fragment: Fragment) -> Result<(), FederationError> { - match Arc::make_mut(&mut self.fragments).entry(fragment.name.clone()) { - indexmap::map::Entry::Occupied(_) => { - Err(FederationError::internal("Duplicate fragment name")) - } - indexmap::map::Entry::Vacant(entry) => { - let _ = entry.insert(Node::new(fragment)); - Ok(()) - } - } - } - pub(crate) fn get(&self, name: &str) -> Option<&Node> { self.fragments.get(name) } @@ -3571,13 +3455,13 @@ const DEFER_IF_ARGUMENT_NAME: Name = name!("if"); pub(crate) struct NormalizedDefer { /// The operation modified to normalize @defer applications. - pub operation: Operation, + pub(crate) operation: Operation, /// True if the operation contains any @defer applications. - pub has_defers: bool, + pub(crate) has_defers: bool, /// `@defer(label:)` values assigned by normalization. - pub assigned_defer_labels: IndexSet, + pub(crate) assigned_defer_labels: IndexSet, /// Map of variable conditions to the @defer labels depending on those conditions. - pub defer_conditions: IndexMap>, + pub(crate) defer_conditions: IndexMap>, } struct DeferNormalizer { @@ -3687,11 +3571,6 @@ impl Fragment { } impl NamedFragments { - /// Returns true if any fragment uses the @defer directive. - fn has_defer(&self) -> bool { - self.iter().any(|fragment| fragment.has_defer()) - } - /// Creates new fragment definitions with the @defer directive removed. fn without_defer(&self, filter: DeferFilter<'_>) -> Result { let mut new_fragments = NamedFragments { @@ -4056,17 +3935,6 @@ impl Fragment { } } -impl NamedFragments { - /// Collect the usages of fragments that are used within the selection of other fragments. - pub(crate) fn collect_used_fragment_names(&self, aggregator: &mut IndexMap) { - for fragment in self.fragments.values() { - fragment - .selection_set - .collect_used_fragment_names(aggregator); - } - } -} - // Collect used variables from operation types. pub(crate) struct VariableCollector<'s> { @@ -4099,7 +3967,7 @@ impl<'s> VariableCollector<'s> { } } - fn visit_directive(&mut self, directive: &'s executable::Directive) { + fn visit_directive(&mut self, directive: &'s Directive) { for arg in directive.arguments.iter() { self.visit_value(&arg.value); } @@ -4177,6 +4045,7 @@ impl Fragment { impl SelectionSet { /// Returns the variable names that are used by this selection set, including through fragment /// spreads. + #[cfg(test)] pub(crate) fn used_variables(&self) -> IndexSet<&'_ Name> { let mut collector = VariableCollector::new(); collector.visit_selection_set(self); diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index 79fe71c8ef..e9e897304f 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -247,7 +247,9 @@ impl Selection { (self.selection_set(), other.selection_set()) { let common = self_sub_selection.intersection(other_sub_selection)?; - if !common.is_empty() { + if common.is_empty() { + return Ok(None); + } else { return self .with_updated_selections( self_sub_selection.type_position.clone(), @@ -1600,32 +1602,6 @@ impl Operation { } } -/// Returns a consistent GraphQL name for the given index. -fn fragment_name(mut index: usize) -> Name { - /// https://spec.graphql.org/draft/#NameContinue - const NAME_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"; - /// https://spec.graphql.org/draft/#NameStart - const NAME_START_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_"; - - if index < NAME_START_CHARS.len() { - Name::new_static_unchecked(&NAME_START_CHARS[index..index + 1]) - } else { - let mut s = String::new(); - - let i = index % NAME_START_CHARS.len(); - s.push(NAME_START_CHARS.as_bytes()[i].into()); - index /= NAME_START_CHARS.len(); - - while index > 0 { - let i = index % NAME_CHARS.len(); - s.push(NAME_CHARS.as_bytes()[i].into()); - index /= NAME_CHARS.len(); - } - - Name::new_unchecked(&s) - } -} - #[derive(Debug, Default)] struct FragmentGenerator { fragments: NamedFragments, @@ -1634,10 +1610,6 @@ struct FragmentGenerator { } impl FragmentGenerator { - fn next_name(&self) -> Name { - fragment_name(self.fragments.len()) - } - // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! // In the future, we will just use `.next_name()`. fn generate_name(&mut self, frag: &InlineFragmentSelection) -> Name { @@ -1818,6 +1790,32 @@ mod tests { }}; } + /// Returns a consistent GraphQL name for the given index. + fn fragment_name(mut index: usize) -> Name { + /// https://spec.graphql.org/draft/#NameContinue + const NAME_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"; + /// https://spec.graphql.org/draft/#NameStart + const NAME_START_CHARS: &str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_"; + + if index < NAME_START_CHARS.len() { + Name::new_static_unchecked(&NAME_START_CHARS[index..index + 1]) + } else { + let mut s = String::new(); + + let i = index % NAME_START_CHARS.len(); + s.push(NAME_START_CHARS.as_bytes()[i].into()); + index /= NAME_START_CHARS.len(); + + while index > 0 { + let i = index % NAME_CHARS.len(); + s.push(NAME_CHARS.as_bytes()[i].into()); + index /= NAME_CHARS.len(); + } + + Name::new_unchecked(&s) + } + } + #[test] fn generated_fragment_names() { assert_eq!(fragment_name(0), "a"); @@ -2281,12 +2279,12 @@ mod tests { type Query { t: T } - + type T { a: A b: Int } - + type A { x: String y: String @@ -2312,14 +2310,14 @@ mod tests { x y } - + fragment FT on T { a { __typename ...FA } } - + query { t { ...FT @@ -2346,19 +2344,19 @@ mod tests { type Query { t: T } - + type T { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2376,7 +2374,7 @@ mod tests { m } } - + { t { ...FragT @@ -2415,23 +2413,23 @@ mod tests { type Query { i: I } - + interface I { a: String } - + type T implements I { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2449,7 +2447,7 @@ mod tests { m } } - + { i { ... on T { @@ -2489,19 +2487,19 @@ mod tests { type Query { t: T } - + type T { a: String b: B c: Int d: D } - + type B { x: String y: String } - + type D { m: String n: String @@ -2527,7 +2525,7 @@ mod tests { m } } - + fragment Frag2 on T { a b { @@ -2539,7 +2537,7 @@ mod tests { n } } - + { t { ...Frag1 @@ -2573,11 +2571,11 @@ mod tests { type Query { t: T } - + interface I { x: String } - + type T implements I { x: String a: String @@ -2591,7 +2589,7 @@ mod tests { a } } - + { t { ...FragI @@ -2615,12 +2613,12 @@ mod tests { type Query { t: T } - + type T { a: String u: U } - + type U { x: String y: String @@ -2631,7 +2629,7 @@ mod tests { fragment Frag1 on T { a } - + fragment Frag2 on T { u { x @@ -2639,13 +2637,13 @@ mod tests { } ...Frag1 } - + fragment Frag3 on Query { t { ...Frag2 } } - + { ...Frag3 } @@ -2670,16 +2668,16 @@ mod tests { type Query { t1: T1 } - + interface I { x: Int } - + type T1 implements I { x: Int y: Int } - + type T2 implements I { x: Int z: Int @@ -2695,7 +2693,7 @@ mod tests { z } } - + { t1 { ...FragOnI @@ -2718,24 +2716,24 @@ mod tests { type Query { i2: I2 } - + interface I1 { x: Int } - + interface I2 { y: Int } - + interface I3 { z: Int } - + type T1 implements I1 & I2 { x: Int y: Int } - + type T2 implements I1 & I3 { x: Int z: Int @@ -2751,7 +2749,7 @@ mod tests { z } } - + { i2 { ...FragOnI1 @@ -2781,13 +2779,13 @@ mod tests { type Query { t1: T1 } - + union U = T1 | T2 - + type T1 { x: Int } - + type T2 { y: Int } @@ -2802,7 +2800,7 @@ mod tests { y } } - + { t1 { ...OnU @@ -2912,18 +2910,18 @@ mod tests { type Query { t1: T1 } - + union U1 = T1 | T2 | T3 union U2 = T2 | T3 - + type T1 { x: Int } - + type T2 { y: Int } - + type T3 { z: Int } @@ -2935,7 +2933,7 @@ mod tests { ...Outer } } - + fragment Outer on U1 { ... on T1 { x @@ -2947,7 +2945,7 @@ mod tests { ... Inner } } - + fragment Inner on U2 { ... on T2 { y @@ -3006,23 +3004,23 @@ mod tests { type Query { t1: T1 } - + union U1 = T1 | T2 | T3 union U2 = T2 | T3 - + type T1 { x: Int } - + type T2 { y1: Y y2: Y } - + type T3 { z: Int } - + type Y { v: Int } @@ -3034,7 +3032,7 @@ mod tests { ...Outer } } - + fragment Outer on U1 { ... on T1 { x @@ -3046,7 +3044,7 @@ mod tests { ... Inner } } - + fragment Inner on U2 { ... on T2 { y1 { @@ -3057,7 +3055,7 @@ mod tests { } } } - + fragment WillBeUnused on Y { v } @@ -3092,14 +3090,14 @@ mod tests { t1: T t2: T } - + type T { a1: Int a2: Int b1: B b2: B } - + type B { x: Int y: Int @@ -3115,7 +3113,7 @@ mod tests { ...TFields } } - + fragment TFields on T { ...DirectFieldsOfT b1 { @@ -3125,12 +3123,12 @@ mod tests { ...BFields } } - + fragment DirectFieldsOfT on T { a1 a2 } - + fragment BFields on B { x y @@ -3213,7 +3211,7 @@ mod tests { t2: T t3: T } - + type T { a: Int b: Int @@ -3226,7 +3224,7 @@ mod tests { fragment DirectiveInDef on T { a @include(if: $cond1) } - + query myQuery($cond1: Boolean!, $cond2: Boolean!) { t1 { a @@ -3263,7 +3261,7 @@ mod tests { t2: T t3: T } - + type T { a: Int b: Int @@ -3276,7 +3274,7 @@ mod tests { fragment NoDirectiveDef on T { a } - + query myQuery($cond1: Boolean!) { t1 { ...NoDirectiveDef diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 9ba4d08e58..25ac19ec5a 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -337,20 +337,6 @@ impl FieldSelection { } } - /// Returns a field selection "equivalent" to the one represented by this object, but such that its parent type - /// is the one provided as argument. - /// - /// Obviously, this operation will only succeed if this selection (both the field itself and its subselections) - /// make sense from the provided parent type. If this is not the case, this method will throw. - pub(crate) fn rebase_on( - &self, - parent_type: &CompositeTypeDefinitionPosition, - named_fragments: &NamedFragments, - schema: &ValidFederationSchema, - ) -> Result { - self.rebase_inner(parent_type, named_fragments, schema, Default::default()) - } - fn can_add_to( &self, parent_type: &CompositeTypeDefinitionPosition, @@ -660,15 +646,6 @@ impl InlineFragmentSelection { } } - pub(crate) fn rebase_on( - &self, - parent_type: &CompositeTypeDefinitionPosition, - named_fragments: &NamedFragments, - schema: &ValidFederationSchema, - ) -> Result { - self.rebase_inner(parent_type, named_fragments, schema, Default::default()) - } - fn can_add_to( &self, parent_type: &CompositeTypeDefinitionPosition, @@ -691,16 +668,6 @@ impl InlineFragmentSelection { Ok(true) } } - - fn can_rebase_on( - &self, - parent_type: &CompositeTypeDefinitionPosition, - parent_schema: &ValidFederationSchema, - ) -> bool { - self.inline_fragment - .can_rebase_on(parent_type, parent_schema) - .0 - } } impl OperationElement { @@ -780,7 +747,7 @@ impl SelectionSet { /// Returns true if the selection set would select cleanly from the given type in the given /// schema. - pub fn can_rebase_on( + pub(crate) fn can_rebase_on( &self, parent_type: &CompositeTypeDefinitionPosition, schema: &ValidFederationSchema, diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index 8555b241a2..6ada508a6e 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -149,7 +149,7 @@ impl FragmentSpreadSelection { impl InlineFragmentSelection { fn flatten_unnecessary_fragments( - &self, + self: &Arc, parent_type: &CompositeTypeDefinitionPosition, named_fragments: &NamedFragments, schema: &ValidFederationSchema, @@ -176,10 +176,9 @@ impl InlineFragmentSelection { // 2. if it's the same type as the current type: it's not restricting types further. // 3. if the current type is an object more generally: because in that case the condition // cannot be restricting things further (it's typically a less precise interface/union). - let useless_fragment = match this_condition { - None => true, - Some(c) => self.inline_fragment.schema == *schema && c == parent_type, - }; + let useless_fragment = this_condition.map_or(true, |type_condition| { + self.inline_fragment.schema == *schema && type_condition == parent_type + }); if useless_fragment || parent_type.is_object_type() { // Try to skip this fragment and flatten_unnecessary_fragments self.selection_set with `parent_type`, // instead of its original type. @@ -264,27 +263,22 @@ impl InlineFragmentSelection { for (_, selection) in selection_set.selections.iter() { match selection { Selection::FragmentSpread(spread_selection) => { - let type_condition = - spread_selection.spread.type_condition_position.clone(); + let type_condition = &spread_selection.spread.type_condition_position; if type_condition.is_object_type() - && runtime_types_intersect(parent_type, &type_condition, schema) + && runtime_types_intersect(parent_type, type_condition, schema) { - liftable_selections - .insert(Selection::FragmentSpread(spread_selection.clone())); + liftable_selections.insert(selection.clone()); } } Selection::InlineFragment(inline_fragment_selection) => { - if let Some(type_condition) = inline_fragment_selection + if let Some(type_condition) = &inline_fragment_selection .inline_fragment .type_condition_position - .clone() { if type_condition.is_object_type() - && runtime_types_intersect(parent_type, &type_condition, schema) + && runtime_types_intersect(parent_type, type_condition, schema) { - liftable_selections.insert(Selection::InlineFragment( - inline_fragment_selection.clone(), - )); + liftable_selections.insert(selection.clone()); } }; } @@ -311,16 +305,18 @@ impl InlineFragmentSelection { // applied recursively. This could be worth investigating. let rebased_inline_fragment = self.inline_fragment.rebase_on(parent_type, schema)?; - let mut mutable_selections = self.selection_set.selections.clone(); - let final_fragment_selections = Arc::make_mut(&mut mutable_selections); - final_fragment_selections.retain(|k, _| !liftable_selections.contains_key(k)); + + let mut nonliftable_selections = selection_set.selections.clone(); + Arc::make_mut(&mut nonliftable_selections) + .retain(|k, _| !liftable_selections.contains_key(k)); + let rebased_casted_type = rebased_inline_fragment.casted_type(); let final_inline_fragment: Selection = InlineFragmentSelection::new( rebased_inline_fragment, SelectionSet { schema: schema.clone(), type_position: rebased_casted_type, - selections: Arc::new(final_fragment_selections.clone()), + selections: nonliftable_selections, }, ) .into(); @@ -332,8 +328,8 @@ impl InlineFragmentSelection { .collect::>()?; let mut final_selection_map = SelectionMap::new(); - final_selection_map.insert(final_inline_fragment); final_selection_map.extend(liftable_selections); + final_selection_map.insert(final_inline_fragment); let final_selections = SelectionSet { schema: schema.clone(), type_position: parent_type.clone(), @@ -348,21 +344,19 @@ impl InlineFragmentSelection { && self.selection_set == selection_set { // flattening did not change the fragment - // TODO(@goto-bus-stop): no change, but we still create a non-trivial clone here - Ok(Some(SelectionOrSet::Selection(Selection::InlineFragment( - Arc::new(self.clone()), - )))) + Ok(Some(Selection::InlineFragment(Arc::clone(self)).into())) } else { let rebased_inline_fragment = self.inline_fragment.rebase_on(parent_type, schema)?; let rebased_casted_type = rebased_inline_fragment.casted_type(); let rebased_selection_set = selection_set.rebase_on(&rebased_casted_type, named_fragments, schema)?; - Ok(Some(SelectionOrSet::Selection(Selection::InlineFragment( - Arc::new(InlineFragmentSelection::new( + Ok(Some( + Selection::InlineFragment(Arc::new(InlineFragmentSelection::new( rebased_inline_fragment, rebased_selection_set, - )), - )))) + ))) + .into(), + )) } } } @@ -474,3 +468,93 @@ impl SelectionSet { Ok(normalized_selections) } } + +#[cfg(test)] +mod tests { + use apollo_compiler::Schema; + + use super::*; + use crate::operation::Operation; + + #[test] + fn does_not_duplicate_fragments_regression_router782() { + let schema = Schema::parse_and_validate( + r#" +interface Node { + id: ID! +} +interface HasNodes { + nodes: [Node!]! +} +type MySpecialNode implements Node & HasNodes { + id: ID! + value: String + nodes: [Node] +} +type Query { + nodes: [Node] +} + "#, + "apischema.graphql", + ) + .unwrap(); + let schema = ValidFederationSchema::new(schema).unwrap(); + + // Below, the second `... on MySpecialNode` is redundant, + // the same selection is already guaranteed by the first. + let operation = Operation::parse( + schema.clone(), + r#" +{ + nodes { + __typename + ... on MySpecialNode { + value + } + ... on HasNodes { + ... on Node { + __typename + ... on MySpecialNode { + value + } + } + } + } +} + "#, + "query.graphql", + None, + ) + .unwrap(); + + let expanded_and_flattened = operation + .selection_set + .flatten_unnecessary_fragments( + &operation.selection_set.type_position, + &NamedFragments::default(), + &schema, + ) + .unwrap(); + + // Use apollo-compiler's selection set printer directly instead of the minimized + // apollo-federation printer + let compiler_set = + apollo_compiler::executable::SelectionSet::try_from(&expanded_and_flattened).unwrap(); + + insta::assert_snapshot!(compiler_set, @r#" + { + nodes { + __typename + ... on MySpecialNode { + value + } + ... on HasNodes { + ... on Node { + __typename + } + } + } + } + "#); + } +} diff --git a/apollo-federation/src/operation/tests/mod.rs b/apollo-federation/src/operation/tests/mod.rs index b5e4d8e591..52eb810f7d 100644 --- a/apollo-federation/src/operation/tests/mod.rs +++ b/apollo-federation/src/operation/tests/mod.rs @@ -1366,11 +1366,7 @@ mod lazy_map_tests { } } -fn field_element( - schema: &ValidFederationSchema, - object: apollo_compiler::Name, - field: apollo_compiler::Name, -) -> OpPathElement { +fn field_element(schema: &ValidFederationSchema, object: Name, field: Name) -> OpPathElement { OpPathElement::Field(super::Field::new(super::FieldData { schema: schema.clone(), field_position: ObjectTypeDefinitionPosition::new(object) @@ -1506,7 +1502,10 @@ fn add_at_path_collapses_unnecessary_fragments() { Some( &SelectionSet::parse( schema.clone(), - InterfaceTypeDefinitionPosition::new(name!("X")).into(), + InterfaceTypeDefinitionPosition { + type_name: name!("X"), + } + .into(), "... on C { d }", ) .unwrap() diff --git a/apollo-federation/src/query_graph/condition_resolver.rs b/apollo-federation/src/query_graph/condition_resolver.rs index 02f709c618..e547f2a235 100644 --- a/apollo-federation/src/query_graph/condition_resolver.rs +++ b/apollo-federation/src/query_graph/condition_resolver.rs @@ -31,6 +31,8 @@ pub(crate) enum ConditionResolution { path_tree: Option>, }, Unsatisfied { + // NOTE: This seems to be a false positive... + #[allow(dead_code)] reason: Option, }, } @@ -53,7 +55,7 @@ impl ConditionResolution { } } -#[derive(Debug)] +#[derive(Debug, derive_more::IsVariant)] pub(crate) enum ConditionResolutionCacheResult { /// Cache hit. Hit(ConditionResolution), @@ -63,20 +65,6 @@ pub(crate) enum ConditionResolutionCacheResult { NotApplicable, } -impl ConditionResolutionCacheResult { - pub(crate) fn is_hit(&self) -> bool { - matches!(self, Self::Hit(_)) - } - - pub(crate) fn is_miss(&self) -> bool { - matches!(self, Self::Miss) - } - - pub(crate) fn is_not_applicable(&self) -> bool { - matches!(self, Self::NotApplicable) - } -} - pub(crate) struct ConditionResolverCache { // For every edge having a condition, we cache the resolution its conditions when possible. // We save resolution with the set of excluded edges that were used to compute it: the reason we do this is @@ -179,7 +167,7 @@ mod tests { &empty_destinations, &empty_conditions ) - .is_hit()); + .is_hit(),); let edge2 = EdgeIndex::new(2); diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index af24a60d75..6d18e07e4a 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -279,7 +279,7 @@ impl Deref for OpPath { } } -impl std::fmt::Display for OpPath { +impl Display for OpPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { for (i, element) in self.0.iter().enumerate() { if i > 0 { @@ -568,7 +568,7 @@ impl std::fmt::Debug for SimultaneousPaths { } } -impl std::fmt::Display for SimultaneousPaths { +impl Display for SimultaneousPaths { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { self.fmt_indented(&mut IndentedFormatter::new(f)) } @@ -769,14 +769,8 @@ impl Display for Unadvanceable { } #[derive(Debug, Clone, strum_macros::Display, serde::Serialize)] -enum UnadvanceableReason { - UnsatisfiableKeyCondition, - UnsatisfiableRequiresCondition, - UnresolvableInterfaceObject, - NoMatchingTransition, - UnreachableType, - IgnoredIndirectPath, -} +// PORT_NOTE: This is only used by composition, which is not ported to Rust yet. +enum UnadvanceableReason {} /// One of the options for a `ClosedBranch` (see the documentation of that struct for details). Note /// there is an optimization here, in that if some ending section of the path within the GraphQL @@ -801,7 +795,7 @@ impl ClosedPath { } } -impl std::fmt::Display for ClosedPath { +impl Display for ClosedPath { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { if let Some(ref selection_set) = self.selection_set { write!(f, "{} -> {}", self.paths, selection_set) @@ -1985,12 +1979,6 @@ impl OpGraphPath { (new_self, new_others) } - pub(crate) fn is_overridden_by(&self, other: &Self) -> bool { - self.overriding_path_ids - .iter() - .any(|overriding_id| other.own_path_ids.contains(overriding_id)) - } - pub(crate) fn subgraph_jumps(&self) -> Result { self.subgraph_jumps_at_idx(0) } @@ -3602,7 +3590,7 @@ impl SimultaneousPathsWithLazyIndirectPaths { // PORT_NOTE: JS passes a ConditionResolver here, we do not: see port note for // `SimultaneousPathsWithLazyIndirectPaths` -pub fn create_initial_options( +pub(crate) fn create_initial_options( initial_path: GraphPath>, initial_type: &QueryGraphNodeType, initial_context: OpGraphPathContext, @@ -3707,12 +3695,12 @@ impl ClosedBranch { } impl OpPath { - pub fn len(&self) -> usize { + pub(crate) fn len(&self) -> usize { self.0.len() } pub(crate) fn is_empty(&self) -> bool { - self.0.is_empty() + self.len() == 0 } pub(crate) fn strip_prefix(&self, maybe_prefix: &Self) -> Option { diff --git a/apollo-federation/src/query_graph/mod.rs b/apollo-federation/src/query_graph/mod.rs index a69ba3cabf..344d9dc01d 100644 --- a/apollo-federation/src/query_graph/mod.rs +++ b/apollo-federation/src/query_graph/mod.rs @@ -69,7 +69,7 @@ pub(crate) struct QueryGraphNode { } impl QueryGraphNode { - pub fn is_root_node(&self) -> bool { + pub(crate) fn is_root_node(&self) -> bool { matches!(self.type_, QueryGraphNodeType::FederatedRootType(_)) } } @@ -528,10 +528,6 @@ impl QueryGraph { }) } - pub(crate) fn non_trivial_followup_edges(&self) -> &IndexMap> { - &self.non_trivial_followup_edges - } - /// All outward edges from the given node (including self-key and self-root-type-resolution /// edges). Primarily used by `@defer`, when needing to re-enter a subgraph for a deferred /// section. diff --git a/apollo-federation/src/query_graph/output.rs b/apollo-federation/src/query_graph/output.rs index b17390dd9d..4b68c2d047 100644 --- a/apollo-federation/src/query_graph/output.rs +++ b/apollo-federation/src/query_graph/output.rs @@ -6,7 +6,6 @@ use std::sync::Arc; use petgraph::dot::Config; use petgraph::dot::Dot; -use petgraph::graph::DiGraph; use petgraph::graph::EdgeIndex; use petgraph::stable_graph::StableGraph; @@ -14,7 +13,6 @@ use crate::query_graph::QueryGraph; use crate::query_graph::QueryGraphEdge; use crate::query_graph::QueryGraphNode; -type InnerGraph = DiGraph; type StableInnerGraph = StableGraph; ////////////////////////////////////////////////////////////////////////////// diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index 07388b7366..7017c53a8a 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -26,12 +26,6 @@ pub(crate) enum Conditions { Boolean(bool), } -#[derive(Debug, Clone, PartialEq)] -pub(crate) enum Condition { - Variable(VariableCondition), - Boolean(bool), -} - /// A list of variable conditions, represented as a map from variable names to whether that variable /// is negated in the condition. We maintain the invariant that there's at least one condition (i.e. /// the map is non-empty), and that there's at most one condition per variable name. @@ -47,29 +41,12 @@ impl VariableConditions { Self(Arc::new(map)) } - pub fn insert(&mut self, name: Name, negated: bool) { - Arc::make_mut(&mut self.0).insert(name, negated); - } - - /// Returns true if there are no conditions. - pub fn is_empty(&self) -> bool { - self.0.is_empty() - } - - /// Returns a variable condition by name. - pub fn get(&self, name: &str) -> Option { - self.0.get_key_value(name).map(|(variable, &negated)| { - let variable = variable.clone(); - VariableCondition { variable, negated } - }) - } - /// Returns whether a variable condition is negated, or None if there is no condition for the variable name. - pub fn is_negated(&self, name: &str) -> Option { + pub(crate) fn is_negated(&self, name: &str) -> Option { self.0.get(name).copied() } - pub fn iter(&self) -> impl Iterator { + pub(crate) fn iter(&self) -> impl Iterator { self.0.iter().map(|(name, &negated)| (name, negated)) } } @@ -185,13 +162,6 @@ impl Conditions { } } -fn is_constant_condition(condition: &Conditions) -> bool { - match condition { - Conditions::Variables(_) => false, - Conditions::Boolean(_) => true, - } -} - pub(crate) fn remove_conditions_from_selection_set( selection_set: &SelectionSet, conditions: &Conditions, diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index e3ab57f441..f31bd0d2ff 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -203,7 +203,7 @@ impl FetchIdGenerator { } /// Generate a new ID for a fetch dependency node. - pub fn next_id(&self) -> u64 { + pub(crate) fn next_id(&self) -> u64 { self.next.fetch_add(1, std::sync::atomic::Ordering::Relaxed) } } @@ -369,10 +369,10 @@ struct ProcessingState { /// Nodes that can be handled (because all their parents/dependencies have been processed before). // TODO(@goto-bus-stop): Seems like this should be an IndexSet, since every `.push()` first // checks if the element is unique. - pub next: Vec, + pub(crate) next: Vec, /// Nodes that needs some parents/dependencies to be processed first before they can be themselves. /// Note that we make sure that this never hold node with no "edges". - pub unhandled: Vec, + pub(crate) unhandled: Vec, } impl DeferContext { @@ -399,14 +399,14 @@ impl Default for DeferContext { } impl ProcessingState { - pub fn empty() -> Self { + pub(crate) fn empty() -> Self { Self { next: vec![], unhandled: vec![], } } - pub fn of_ready_nodes(nodes: Vec) -> Self { + pub(crate) fn of_ready_nodes(nodes: Vec) -> Self { Self { next: nodes, unhandled: vec![], @@ -417,7 +417,7 @@ impl ProcessingState { // structure as `create_state_for_children_of_processed_node`, because it needs access to the // graph. - pub fn merge_with(self, other: ProcessingState) -> ProcessingState { + pub(crate) fn merge_with(self, other: ProcessingState) -> ProcessingState { let mut next = self.next; for g in other.next { if !next.contains(&g) { @@ -471,7 +471,7 @@ impl ProcessingState { ProcessingState { next, unhandled } } - pub fn update_for_processed_nodes(self, processed: &[NodeIndex]) -> ProcessingState { + pub(crate) fn update_for_processed_nodes(self, processed: &[NodeIndex]) -> ProcessingState { let mut next = self.next; let mut unhandled = vec![]; for UnhandledNode { @@ -711,10 +711,6 @@ impl FetchDependencyGraph { } } - pub(crate) fn next_fetch_id(&self) -> u64 { - self.fetch_id_generation.next_id() - } - pub(crate) fn root_node_by_subgraph_iter( &self, ) -> impl Iterator, &NodeIndex)> { @@ -820,25 +816,6 @@ impl FetchDependencyGraph { )?)) } - pub(crate) fn edge_weight( - &self, - edge: EdgeIndex, - ) -> Result<&Arc, FederationError> { - self.graph - .edge_weight(edge) - .ok_or_else(|| FederationError::internal("Edge unexpectedly missing".to_owned())) - } - - /// Does not take `&mut self` so that other fields can be mutated while this borrow lasts - fn edge_weight_mut( - graph: &mut FetchDependencyGraphPetgraph, - edge: EdgeIndex, - ) -> Result<&mut FetchDependencyGraphEdge, FederationError> { - Ok(Arc::make_mut(graph.edge_weight_mut(edge).ok_or_else( - || FederationError::internal("Edge unexpectedly missing"), - )?)) - } - fn get_or_create_key_node( &mut self, subgraph_name: &Arc, @@ -987,10 +964,6 @@ impl FetchDependencyGraph { self.parents_of(maybe_child_id).any(|id| id == node_id) } - fn is_child_of(&self, node_id: NodeIndex, maybe_parent_id: NodeIndex) -> bool { - self.parent_relation(node_id, maybe_parent_id).is_some() - } - fn is_descendant_of(&self, node_id: NodeIndex, maybe_ancestor_id: NodeIndex) -> bool { petgraph::algo::has_path_connecting(&self.graph, maybe_ancestor_id, node_id, None) } @@ -2493,8 +2466,10 @@ impl std::fmt::Display for FetchDependencyGraphEdge { } impl FetchDependencyGraph { - // GraphViz output for FetchDependencyGraph - pub fn to_dot(&self) -> String { + // NOTE: This method is not used during query planning. Rather, it is used during debugging. + #[allow(dead_code)] + /// GraphViz output for FetchDependencyGraph + pub(crate) fn to_dot(&self) -> String { fn label_node(node_id: NodeIndex, node: &FetchDependencyGraphNode) -> String { let label_str = node.multiline_display(node_id).to_string(); format!("label=\"{}\"", label_str.replace('"', "\\\"")) @@ -2845,7 +2820,7 @@ impl FetchDependencyGraphNode { // A variation of `fn display` with multiline output, which is more suitable for // GraphViz output. - pub fn multiline_display(&self, index: NodeIndex) -> impl std::fmt::Display + '_ { + pub(crate) fn multiline_display(&self, index: NodeIndex) -> impl std::fmt::Display + '_ { use std::fmt; use std::fmt::Display; use std::fmt::Formatter; @@ -4665,7 +4640,6 @@ fn path_for_parent( #[cfg(test)] mod tests { use super::*; - use crate::schema::position::InterfaceTypeDefinitionPosition; #[test] fn type_condition_fetching_disabled() { @@ -4793,10 +4767,10 @@ mod tests { fn object_field_element( schema: &ValidFederationSchema, - object: apollo_compiler::Name, - field: apollo_compiler::Name, + object: Name, + field: Name, ) -> OpPathElement { - OpPathElement::Field(super::Field::new(super::FieldData { + OpPathElement::Field(super::Field::new(FieldData { schema: schema.clone(), field_position: ObjectTypeDefinitionPosition::new(object) .field(field) @@ -4808,27 +4782,10 @@ mod tests { })) } - fn interface_field_element( - schema: &ValidFederationSchema, - interface: apollo_compiler::Name, - field: apollo_compiler::Name, - ) -> OpPathElement { - OpPathElement::Field(super::Field::new(super::FieldData { - schema: schema.clone(), - field_position: InterfaceTypeDefinitionPosition::new(interface) - .field(field) - .into(), - alias: None, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - })) - } - fn inline_fragment_element( schema: &ValidFederationSchema, - parent_type_name: apollo_compiler::Name, - type_condition_name: Option, + parent_type_name: Name, + type_condition_name: Option, ) -> OpPathElement { let parent_type = schema .get_type(parent_type_name) diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index 6d75453422..500076ce4c 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -16,7 +16,6 @@ use super::fetch_dependency_graph::FetchIdGenerator; use super::ConditionNode; use crate::error::FederationError; use crate::error::SingleFederationError; -use crate::link::federation_spec_definition::FederationSpecDefinition; use crate::operation::normalize_operation; use crate::operation::NamedFragments; use crate::operation::NormalizedDefer; @@ -53,7 +52,6 @@ use crate::ApiSchemaOptions; use crate::Supergraph; pub(crate) const CONTEXT_DIRECTIVE: &str = "context"; -pub(crate) const JOIN_FIELD: &str = "join__field"; #[derive(Debug, Clone, Hash)] pub struct QueryPlannerConfig { @@ -226,8 +224,6 @@ pub struct QueryPlanner { federated_query_graph: Arc, supergraph_schema: ValidFederationSchema, api_schema: ValidFederationSchema, - subgraph_federation_spec_definitions: - Arc, &'static FederationSpecDefinition>>, /// A set of the names of interface types for which at least one subgraph use an /// @interfaceObject to abstract that interface. interface_types_with_interface_objects: IndexSet, @@ -338,9 +334,6 @@ impl QueryPlanner { federated_query_graph: Arc::new(query_graph), supergraph_schema, api_schema, - // TODO(@goto-bus-stop): not sure how this is going to be used, - // keeping empty for the moment - subgraph_federation_spec_definitions: Default::default(), interface_types_with_interface_objects, abstract_types_with_inconsistent_runtime_types, }) diff --git a/apollo-federation/src/query_plan/query_planning_traversal.rs b/apollo-federation/src/query_plan/query_planning_traversal.rs index 0fba2e801a..787013471a 100644 --- a/apollo-federation/src/query_plan/query_planning_traversal.rs +++ b/apollo-federation/src/query_plan/query_planning_traversal.rs @@ -134,16 +134,16 @@ impl std::fmt::Debug for PlanInfo { #[derive(Serialize)] pub(crate) struct BestQueryPlanInfo { /// The fetch dependency graph for this query plan. - pub fetch_dependency_graph: FetchDependencyGraph, + pub(crate) fetch_dependency_graph: FetchDependencyGraph, /// The path tree for the closed branch options chosen for this query plan. - pub path_tree: Arc, + pub(crate) path_tree: Arc, /// The cost of this query plan. - pub cost: QueryPlanCost, + pub(crate) cost: QueryPlanCost, } impl BestQueryPlanInfo { // PORT_NOTE: The equivalent of `createEmptyPlan` in the JS codebase. - pub fn empty(parameters: &QueryPlanningParameters) -> Self { + pub(crate) fn empty(parameters: &QueryPlanningParameters) -> Self { Self { fetch_dependency_graph: FetchDependencyGraph::new( parameters.supergraph_schema.clone(), @@ -163,7 +163,7 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { feature = "snapshot_tracing", tracing::instrument(level = "trace", skip_all, name = "QueryPlanningTraversal::new") )] - pub fn new( + pub(crate) fn new( // TODO(@goto-bus-stop): This probably needs a mutable reference for some of the // yet-unimplemented methods, and storing a mutable ref in `Self` here smells bad. // The ownership of `QueryPlanningParameters` is awkward and should probably be @@ -270,7 +270,7 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { name = "QueryPlanningTraversal::find_best_plan" ) )] - pub fn find_best_plan(mut self) -> Result, FederationError> { + pub(crate) fn find_best_plan(mut self) -> Result, FederationError> { self.find_best_plan_inner()?; Ok(self.best_plan) } diff --git a/apollo-federation/src/schema/argument_composition_strategies.rs b/apollo-federation/src/schema/argument_composition_strategies.rs index 93499fcc6c..e231d98222 100644 --- a/apollo-federation/src/schema/argument_composition_strategies.rs +++ b/apollo-federation/src/schema/argument_composition_strategies.rs @@ -5,6 +5,7 @@ use lazy_static::lazy_static; use crate::schema::FederationSchema; +#[allow(dead_code)] #[derive(Clone, Copy)] pub(crate) enum ArgumentCompositionStrategy { Max, @@ -28,7 +29,7 @@ lazy_static! { } impl ArgumentCompositionStrategy { - pub fn name(&self) -> &str { + pub(crate) fn name(&self) -> &str { match self { Self::Max => MAX_STRATEGY.name(), Self::Min => MIN_STRATEGY.name(), @@ -38,7 +39,11 @@ impl ArgumentCompositionStrategy { } } - pub fn is_type_supported(&self, schema: &FederationSchema, ty: &Type) -> Result<(), String> { + pub(crate) fn is_type_supported( + &self, + schema: &FederationSchema, + ty: &Type, + ) -> Result<(), String> { match self { Self::Max => MAX_STRATEGY.is_type_supported(schema, ty), Self::Min => MIN_STRATEGY.is_type_supported(schema, ty), @@ -48,7 +53,7 @@ impl ArgumentCompositionStrategy { } } - pub fn merge_values(&self, values: &[Value]) -> Value { + pub(crate) fn merge_values(&self, values: &[Value]) -> Value { match self { Self::Max => MAX_STRATEGY.merge_values(values), Self::Min => MIN_STRATEGY.merge_values(values), @@ -57,17 +62,6 @@ impl ArgumentCompositionStrategy { Self::Union => UNION_STRATEGY.merge_values(values), } } - - pub fn from_str(s: &str) -> Option { - match s { - "MAX" => Some(Self::Max), - "MIN" => Some(Self::Min), - "SUM" => Some(Self::Sum), - "INTERSECTION" => Some(Self::Intersection), - "UNION" => Some(Self::Union), - _ => None, - } - } } ////////////////////////////////////////////////////////////////////////////// @@ -88,7 +82,7 @@ pub(crate) trait ArgumentComposition { #[derive(Clone)] pub(crate) struct FixedTypeSupportValidator { - pub supported_types: Vec, + pub(crate) supported_types: Vec, } impl FixedTypeSupportValidator { diff --git a/apollo-federation/src/schema/field_set.rs b/apollo-federation/src/schema/field_set.rs index 442162efd9..a0a9375405 100644 --- a/apollo-federation/src/schema/field_set.rs +++ b/apollo-federation/src/schema/field_set.rs @@ -17,7 +17,6 @@ use crate::schema::position::FieldDefinitionPosition; use crate::schema::position::InterfaceTypeDefinitionPosition; use crate::schema::position::ObjectTypeDefinitionPosition; use crate::schema::position::UnionTypeDefinitionPosition; -use crate::schema::FederationSchema; use crate::schema::ValidFederationSchema; // Federation spec does not allow the alias syntax in field set strings. @@ -167,37 +166,6 @@ pub(crate) fn collect_target_fields_from_field_set( Ok(fields) } -// PORT_NOTE: This is meant as a companion function for collect_target_fields_from_field_set(), as -// some callers will also want to include interface field implementations. -pub(crate) fn add_interface_field_implementations( - fields: Vec, - schema: &FederationSchema, -) -> Result, FederationError> { - let mut new_fields = vec![]; - for field in fields { - let interface_field = if let FieldDefinitionPosition::Interface(field) = &field { - Some(field.clone()) - } else { - None - }; - new_fields.push(field); - if let Some(interface_field) = interface_field { - for implementing_type in &schema - .referencers - .get_interface_type(&interface_field.type_name)? - .object_types - { - new_fields.push( - implementing_type - .field(interface_field.field_name.clone()) - .into(), - ); - } - } - } - Ok(new_fields) -} - #[cfg(test)] mod tests { use apollo_compiler::Name; diff --git a/apollo-federation/src/schema/mod.rs b/apollo-federation/src/schema/mod.rs index 14ae55e56b..3def48068b 100644 --- a/apollo-federation/src/schema/mod.rs +++ b/apollo-federation/src/schema/mod.rs @@ -163,10 +163,6 @@ impl FederationSchema { }) } - pub(crate) fn validate(self) -> Result { - self.validate_or_return_self().map_err(|e| e.1) - } - /// Similar to `Self::validate` but returns `self` as part of the error should it be needed by /// the caller pub(crate) fn validate_or_return_self( @@ -232,10 +228,6 @@ impl ValidFederationSchema { Self::new_assume_valid(schema).map_err(|(_schema, error)| error) } - pub(crate) fn ptr_eq(&self, other: &Self) -> bool { - Arc::ptr_eq(&self.schema, &other.schema) - } - /// Construct a ValidFederationSchema by assuming the given FederationSchema is valid. fn new_assume_valid( mut schema: FederationSchema, @@ -283,13 +275,12 @@ impl ValidFederationSchema { return Ok(name); } - // TODO: this otherwise needs to check for a type name in schema based + // TODO for composition: this otherwise needs to check for a type name in schema based // on the latest federation version. - // FED-311 - Err(SingleFederationError::Internal { - message: String::from("typename should have been looked in a federation feature"), - } - .into()) + // This code path is not hit during planning. + Err(FederationError::internal( + "typename should have been looked in a federation feature", + )) } pub(crate) fn is_interface_object_type( diff --git a/apollo-federation/src/schema/position.rs b/apollo-federation/src/schema/position.rs index ad0a81f944..ca4f16a1a7 100644 --- a/apollo-federation/src/schema/position.rs +++ b/apollo-federation/src/schema/position.rs @@ -232,13 +232,6 @@ impl TypeDefinitionPosition { )), } } - - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema ExtendedType> { - self.get(schema).ok() - } } fallible_conversions!(TypeDefinitionPosition::Scalar -> ScalarTypeDefinitionPosition); @@ -286,45 +279,6 @@ impl OutputTypeDefinitionPosition { OutputTypeDefinitionPosition::Enum(type_) => &type_.type_name, } } - - fn describe(&self) -> &'static str { - match self { - OutputTypeDefinitionPosition::Scalar(_) => ScalarTypeDefinitionPosition::EXPECTED, - OutputTypeDefinitionPosition::Object(_) => ObjectTypeDefinitionPosition::EXPECTED, - OutputTypeDefinitionPosition::Interface(_) => InterfaceTypeDefinitionPosition::EXPECTED, - OutputTypeDefinitionPosition::Union(_) => UnionTypeDefinitionPosition::EXPECTED, - OutputTypeDefinitionPosition::Enum(_) => EnumTypeDefinitionPosition::EXPECTED, - } - } - - pub(crate) fn get<'schema>( - &self, - schema: &'schema Schema, - ) -> Result<&'schema ExtendedType, PositionLookupError> { - let name = self.type_name(); - let ty = schema - .types - .get(name) - .ok_or_else(|| PositionLookupError::TypeMissing(name.clone()))?; - match (ty, self) { - (ExtendedType::Scalar(_), OutputTypeDefinitionPosition::Scalar(_)) - | (ExtendedType::Object(_), OutputTypeDefinitionPosition::Object(_)) - | (ExtendedType::Interface(_), OutputTypeDefinitionPosition::Interface(_)) - | (ExtendedType::Union(_), OutputTypeDefinitionPosition::Union(_)) - | (ExtendedType::Enum(_), OutputTypeDefinitionPosition::Enum(_)) => Ok(ty), - _ => Err(PositionLookupError::TypeWrongKind( - name.clone(), - self.describe(), - )), - } - } - - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema ExtendedType> { - self.get(schema).ok() - } } fallible_conversions!(OutputTypeDefinitionPosition::Scalar -> ScalarTypeDefinitionPosition); @@ -447,13 +401,6 @@ impl CompositeTypeDefinitionPosition { )), } } - - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema ExtendedType> { - self.get(schema).ok() - } } fallible_conversions!(CompositeTypeDefinitionPosition::Object -> ObjectTypeDefinitionPosition); @@ -489,73 +436,6 @@ impl AbstractTypeDefinitionPosition { AbstractTypeDefinitionPosition::Union(type_) => &type_.type_name, } } - - fn describe(&self) -> &'static str { - match self { - AbstractTypeDefinitionPosition::Interface(_) => { - InterfaceTypeDefinitionPosition::EXPECTED - } - AbstractTypeDefinitionPosition::Union(_) => UnionTypeDefinitionPosition::EXPECTED, - } - } - - pub(crate) fn field( - &self, - field_name: Name, - ) -> Result { - match self { - AbstractTypeDefinitionPosition::Interface(type_) => Ok(type_.field(field_name).into()), - AbstractTypeDefinitionPosition::Union(type_) => { - let field = type_.introspection_typename_field(); - if *field.field_name() == field_name { - Ok(field.into()) - } else { - Err(FederationError::internal(format!( - r#"Union types don't have field "{}", only "{}""#, - field_name, - field.field_name(), - ))) - } - } - } - } - - pub(crate) fn introspection_typename_field(&self) -> FieldDefinitionPosition { - match self { - AbstractTypeDefinitionPosition::Interface(type_) => { - type_.introspection_typename_field().into() - } - AbstractTypeDefinitionPosition::Union(type_) => { - type_.introspection_typename_field().into() - } - } - } - - pub(crate) fn get<'schema>( - &self, - schema: &'schema Schema, - ) -> Result<&'schema ExtendedType, PositionLookupError> { - let name = self.type_name(); - let ty = schema - .types - .get(name) - .ok_or_else(|| PositionLookupError::TypeMissing(name.clone()))?; - match (ty, self) { - (ExtendedType::Interface(_), AbstractTypeDefinitionPosition::Interface(_)) - | (ExtendedType::Union(_), AbstractTypeDefinitionPosition::Union(_)) => Ok(ty), - _ => Err(PositionLookupError::TypeWrongKind( - name.clone(), - self.describe(), - )), - } - } - - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema ExtendedType> { - self.get(schema).ok() - } } fallible_conversions!(AbstractTypeDefinitionPosition::Interface -> InterfaceTypeDefinitionPosition); @@ -590,17 +470,6 @@ impl ObjectOrInterfaceTypeDefinitionPosition { } } - fn describe(&self) -> &'static str { - match self { - ObjectOrInterfaceTypeDefinitionPosition::Object(_) => { - ObjectTypeDefinitionPosition::EXPECTED - } - ObjectOrInterfaceTypeDefinitionPosition::Interface(_) => { - InterfaceTypeDefinitionPosition::EXPECTED - } - } - } - pub(crate) fn field(&self, field_name: Name) -> ObjectOrInterfaceFieldDefinitionPosition { match self { ObjectOrInterfaceTypeDefinitionPosition::Object(type_) => { @@ -612,17 +481,6 @@ impl ObjectOrInterfaceTypeDefinitionPosition { } } - pub(crate) fn introspection_typename_field(&self) -> FieldDefinitionPosition { - match self { - ObjectOrInterfaceTypeDefinitionPosition::Object(type_) => { - type_.introspection_typename_field().into() - } - ObjectOrInterfaceTypeDefinitionPosition::Interface(type_) => { - type_.introspection_typename_field().into() - } - } - } - pub(crate) fn fields<'a>( &'a self, schema: &'a Schema, @@ -639,34 +497,6 @@ impl ObjectOrInterfaceTypeDefinitionPosition { } } } - - pub(crate) fn get<'schema>( - &self, - schema: &'schema Schema, - ) -> Result<&'schema ExtendedType, PositionLookupError> { - let name = self.type_name(); - let ty = schema - .types - .get(name) - .ok_or_else(|| PositionLookupError::TypeMissing(name.clone()))?; - match (ty, self) { - (ExtendedType::Object(_), ObjectOrInterfaceTypeDefinitionPosition::Object(_)) - | (ExtendedType::Interface(_), ObjectOrInterfaceTypeDefinitionPosition::Interface(_)) => { - Ok(ty) - } - _ => Err(PositionLookupError::TypeWrongKind( - name.clone(), - self.describe(), - )), - } - } - - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema ExtendedType> { - self.get(schema).ok() - } } fallible_conversions!(ObjectOrInterfaceTypeDefinitionPosition::Object -> ObjectTypeDefinitionPosition); @@ -761,13 +591,6 @@ impl Debug for ObjectOrInterfaceFieldDefinitionPosition { impl ObjectOrInterfaceFieldDefinitionPosition { const EXPECTED: &'static str = "an object/interface field"; - pub(crate) fn type_name(&self) -> &Name { - match self { - ObjectOrInterfaceFieldDefinitionPosition::Object(field) => &field.type_name, - ObjectOrInterfaceFieldDefinitionPosition::Interface(field) => &field.type_name, - } - } - pub(crate) fn field_name(&self) -> &Name { match self { ObjectOrInterfaceFieldDefinitionPosition::Object(field) => &field.field_name, @@ -775,10 +598,6 @@ impl ObjectOrInterfaceFieldDefinitionPosition { } } - pub(crate) fn is_introspection_typename_field(&self) -> bool { - *self.field_name() == *INTROSPECTION_TYPENAME_FIELD_NAME - } - pub(crate) fn parent(&self) -> ObjectOrInterfaceTypeDefinitionPosition { match self { ObjectOrInterfaceFieldDefinitionPosition::Object(field) => field.parent().into(), @@ -896,29 +715,6 @@ impl SchemaDefinitionPosition { Ok(()) } - /// Remove a directive application from the schema definition. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) -> Result<(), FederationError> { - let is_link = Self::is_link(schema, &directive.name)?; - let schema_definition = self.make_mut(&mut schema.schema); - if !schema_definition.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - schema_definition - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - if is_link { - schema.links_metadata = links_metadata(&schema.schema)?.map(Box::new); - } - Ok(()) - } - fn insert_references( &self, schema_definition: &Node, @@ -1081,49 +877,6 @@ impl SchemaRootDefinitionPosition { self.get(schema).ok() } - fn make_mut<'schema>( - &self, - schema: &'schema mut Schema, - ) -> Result<&'schema mut ComponentName, FederationError> { - let schema_definition = self.parent().make_mut(schema).make_mut(); - - match self.root_kind { - SchemaRootDefinitionKind::Query => schema_definition.query.as_mut().ok_or_else(|| { - SingleFederationError::Internal { - message: format!("Schema definition has no root {} type", self), - } - .into() - }), - SchemaRootDefinitionKind::Mutation => { - schema_definition.mutation.as_mut().ok_or_else(|| { - SingleFederationError::Internal { - message: format!("Schema definition has no root {} type", self), - } - .into() - }) - } - SchemaRootDefinitionKind::Subscription => { - schema_definition.subscription.as_mut().ok_or_else(|| { - SingleFederationError::Internal { - message: format!("Schema definition has no root {} type", self), - } - .into() - }) - } - } - } - - fn try_make_mut<'schema>( - &self, - schema: &'schema mut Schema, - ) -> Option<&'schema mut ComponentName> { - if self.try_get(schema).is_some() { - self.make_mut(schema).ok() - } else { - None - } - } - pub(crate) fn insert( &self, schema: &mut FederationSchema, @@ -1386,35 +1139,6 @@ impl ScalarTypeDefinitionPosition { Ok(Some(referencers)) } - /// Remove this scalar type from the schema - pub(crate) fn remove_recursive( - &self, - schema: &mut FederationSchema, - ) -> Result<(), FederationError> { - let Some(referencers) = self.remove_internal(schema)? else { - return Ok(()); - }; - for field in referencers.object_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.object_field_arguments { - argument.remove(schema)?; - } - for field in referencers.interface_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.interface_field_arguments { - argument.remove(schema)?; - } - for field in referencers.input_object_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.directive_arguments { - argument.remove(schema)?; - } - Ok(()) - } - fn remove_internal( &self, schema: &mut FederationSchema, @@ -1471,26 +1195,6 @@ impl ScalarTypeDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - type_ - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, type_: &Node, @@ -1816,26 +1520,6 @@ impl ObjectTypeDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - type_ - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - pub(crate) fn insert_implements_interface( &self, schema: &mut FederationSchema, @@ -2051,10 +1735,6 @@ pub(crate) struct ObjectFieldDefinitionPosition { } impl ObjectFieldDefinitionPosition { - pub(crate) fn is_introspection_typename_field(&self) -> bool { - self.field_name == *INTROSPECTION_TYPENAME_FIELD_NAME - } - pub(crate) fn parent(&self) -> ObjectTypeDefinitionPosition { ObjectTypeDefinitionPosition { type_name: self.type_name.clone(), @@ -2475,38 +2155,6 @@ impl ObjectFieldArgumentDefinitionPosition { } } - pub(crate) fn insert( - &self, - schema: &mut FederationSchema, - argument: Node, - ) -> Result<(), FederationError> { - if self.argument_name != argument.name { - return Err(SingleFederationError::Internal { - message: format!( - "Object field argument \"{}\" given argument named \"{}\"", - self, argument.name, - ), - } - .into()); - } - if self.try_get(&schema.schema).is_some() { - // TODO: Handle old spec edge case of arguments with non-unique names - return Err(SingleFederationError::Internal { - message: format!( - "Object field argument \"{}\" already exists in schema", - self, - ), - } - .into()); - } - self.parent() - .make_mut(&mut schema.schema)? - .make_mut() - .arguments - .push(argument); - self.insert_references(self.get(&schema.schema)?, &mut schema.referencers) - } - /// Remove this argument from the field. /// /// This can make the schema invalid if this is an implementing field of an interface. @@ -2523,30 +2171,6 @@ impl ObjectFieldArgumentDefinitionPosition { Ok(()) } - pub(crate) fn insert_directive( - &self, - schema: &mut FederationSchema, - directive: Node, - ) -> Result<(), FederationError> { - let argument = self.make_mut(&mut schema.schema)?; - if argument - .directives - .iter() - .any(|other_directive| other_directive.ptr_eq(&directive)) - { - return Err(SingleFederationError::Internal { - message: format!( - "Directive application \"@{}\" already exists on object field argument \"{}\"", - directive.name, self, - ), - } - .into()); - } - let name = directive.name.clone(); - argument.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - /// Remove a directive application from this position by name. pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(argument) = self.try_make_mut(&mut schema.schema) else { @@ -2559,26 +2183,6 @@ impl ObjectFieldArgumentDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Node, - ) { - let Some(argument) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !argument.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - argument - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, argument: &Node, @@ -2730,10 +2334,6 @@ pub(crate) struct InterfaceTypeDefinitionPosition { impl InterfaceTypeDefinitionPosition { const EXPECTED: &'static str = "an interface type"; - pub(crate) fn new(type_name: Name) -> Self { - Self { type_name } - } - pub(crate) fn field(&self, field_name: Name) -> InterfaceFieldDefinitionPosition { InterfaceFieldDefinitionPosition { type_name: self.type_name.clone(), @@ -2988,26 +2588,6 @@ impl InterfaceTypeDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - type_ - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - pub(crate) fn insert_implements_interface( &self, schema: &mut FederationSchema, @@ -3154,10 +2734,6 @@ pub(crate) struct InterfaceFieldDefinitionPosition { } impl InterfaceFieldDefinitionPosition { - pub(crate) fn is_introspection_typename_field(&self) -> bool { - self.field_name == *INTROSPECTION_TYPENAME_FIELD_NAME - } - pub(crate) fn parent(&self) -> InterfaceTypeDefinitionPosition { InterfaceTypeDefinitionPosition { type_name: self.type_name.clone(), @@ -3576,38 +3152,6 @@ impl InterfaceFieldArgumentDefinitionPosition { } } - pub(crate) fn insert( - &self, - schema: &mut FederationSchema, - argument: Node, - ) -> Result<(), FederationError> { - if self.argument_name != argument.name { - return Err(SingleFederationError::Internal { - message: format!( - "Interface field argument \"{}\" given argument named \"{}\"", - self, argument.name, - ), - } - .into()); - } - if self.try_get(&schema.schema).is_some() { - // TODO: Handle old spec edge case of arguments with non-unique names - return Err(SingleFederationError::Internal { - message: format!( - "Interface field argument \"{}\" already exists in schema", - self, - ), - } - .into()); - } - self.parent() - .make_mut(&mut schema.schema)? - .make_mut() - .arguments - .push(argument); - self.insert_references(self.get(&schema.schema)?, &mut schema.referencers) - } - /// Remove this argument from its field definition. /// /// This can make the schema invalid if this argument is required and also declared in @@ -3625,32 +3169,6 @@ impl InterfaceFieldArgumentDefinitionPosition { Ok(()) } - pub(crate) fn insert_directive( - &self, - schema: &mut FederationSchema, - directive: Node, - ) -> Result<(), FederationError> { - let argument = self.make_mut(&mut schema.schema)?; - if argument - .directives - .iter() - .any(|other_directive| other_directive.ptr_eq(&directive)) - { - return Err( - SingleFederationError::Internal { - message: format!( - "Directive application \"@{}\" already exists on interface field argument \"{}\"", - directive.name, - self, - ) - }.into() - ); - } - let name = directive.name.clone(); - argument.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - /// Remove a directive application from this position by name. pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(argument) = self.try_make_mut(&mut schema.schema) else { @@ -3663,26 +3181,6 @@ impl InterfaceFieldArgumentDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Node, - ) { - let Some(argument) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !argument.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - argument - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, argument: &Node, @@ -3836,10 +3334,6 @@ pub(crate) struct UnionTypeDefinitionPosition { impl UnionTypeDefinitionPosition { const EXPECTED: &'static str = "a union type"; - pub(crate) fn new(type_name: Name) -> Self { - Self { type_name } - } - pub(crate) fn introspection_typename_field(&self) -> UnionTypenameFieldDefinitionPosition { UnionTypenameFieldDefinitionPosition { type_name: self.type_name.clone(), @@ -4036,40 +3530,20 @@ impl UnionTypeDefinitionPosition { .into()); } let name = directive.name.clone(); - type_.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - - /// Remove a directive application from this position by name. - pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - self.remove_directive_name_references(&mut schema.referencers, name); - type_ - .make_mut() - .directives - .retain(|other_directive| other_directive.name != name); - } - - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { + type_.make_mut().directives.push(directive); + self.insert_directive_name_references(&mut schema.referencers, &name) + } + + /// Remove a directive application from this position by name. + pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(type_) = self.try_make_mut(&mut schema.schema) else { return; }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } + self.remove_directive_name_references(&mut schema.referencers, name); type_ .make_mut() .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); + .retain(|other_directive| other_directive.name != name); } pub(crate) fn insert_member( @@ -4225,13 +3699,6 @@ impl UnionTypenameFieldDefinitionPosition { }) } - pub(crate) fn try_get<'schema>( - &self, - schema: &'schema Schema, - ) -> Option<&'schema Component> { - self.get(schema).ok() - } - fn insert_references(&self, referencers: &mut Referencers) -> Result<(), FederationError> { self.insert_type_references(referencers)?; Ok(()) @@ -4429,35 +3896,6 @@ impl EnumTypeDefinitionPosition { Ok(Some(referencers)) } - /// Remove this enum from the schema, and recursively remove its references. - pub(crate) fn remove_recursive( - &self, - schema: &mut FederationSchema, - ) -> Result<(), FederationError> { - let Some(referencers) = self.remove_internal(schema)? else { - return Ok(()); - }; - for field in referencers.object_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.object_field_arguments { - argument.remove(schema)?; - } - for field in referencers.interface_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.interface_field_arguments { - argument.remove(schema)?; - } - for field in referencers.input_object_fields { - field.remove_recursive(schema)?; - } - for argument in referencers.directive_arguments { - argument.remove(schema)?; - } - Ok(()) - } - fn remove_internal( &self, schema: &mut FederationSchema, @@ -4514,26 +3952,6 @@ impl EnumTypeDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - type_ - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, type_: &Node, @@ -4699,47 +4117,6 @@ impl EnumValueDefinitionPosition { Ok(()) } - /// Remove this value from the enum definition. If it is the only value in the enum, - /// recursively remove the enum from the schema as well. - pub(crate) fn remove_recursive( - &self, - schema: &mut FederationSchema, - ) -> Result<(), FederationError> { - self.remove(schema)?; - let parent = self.parent(); - let Some(type_) = parent.try_get(&schema.schema) else { - return Ok(()); - }; - if type_.values.is_empty() { - parent.remove_recursive(schema)?; - } - Ok(()) - } - - pub(crate) fn insert_directive( - &self, - schema: &mut FederationSchema, - directive: Node, - ) -> Result<(), FederationError> { - let value = self.make_mut(&mut schema.schema)?; - if value - .directives - .iter() - .any(|other_directive| other_directive.ptr_eq(&directive)) - { - return Err(SingleFederationError::Internal { - message: format!( - "Directive application \"@{}\" already exists on enum value \"{}\"", - directive.name, self, - ), - } - .into()); - } - let name = directive.name.clone(); - value.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - /// Remove a directive application from this position by name. pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(value) = self.try_make_mut(&mut schema.schema) else { @@ -4752,36 +4129,15 @@ impl EnumValueDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Node, - ) { - let Some(value) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !value.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - value - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, value: &Component, referencers: &mut Referencers, ) -> Result<(), FederationError> { if is_graphql_reserved_name(&self.value_name) { - return Err(SingleFederationError::Internal { - message: format!("Cannot insert reserved enum value \"{}\"", self), - } - .into()); + return Err(FederationError::internal(format!( + "Cannot insert reserved enum value \"{self}\"" + ))); } validate_node_directives(value.directives.deref())?; for directive_reference in value.directives.iter() { @@ -5078,25 +4434,6 @@ impl InputObjectTypeDefinitionPosition { } /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Component, - ) { - let Some(type_) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !type_.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - type_ - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, type_: &Node, @@ -5287,30 +4624,6 @@ impl InputObjectFieldDefinitionPosition { Ok(()) } - pub(crate) fn insert_directive( - &self, - schema: &mut FederationSchema, - directive: Node, - ) -> Result<(), FederationError> { - let field = self.make_mut(&mut schema.schema)?; - if field - .directives - .iter() - .any(|other_directive| other_directive.ptr_eq(&directive)) - { - return Err(SingleFederationError::Internal { - message: format!( - "Directive application \"@{}\" already exists on input object field \"{}\"", - directive.name, self, - ), - } - .into()); - } - let name = directive.name.clone(); - field.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - /// Remove a directive application from this position by name. pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(field) = self.try_make_mut(&mut schema.schema) else { @@ -5323,26 +4636,6 @@ impl InputObjectFieldDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Node, - ) { - let Some(field) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !field.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - field - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, field: &Component, @@ -5514,17 +4807,6 @@ impl DirectiveDefinitionPosition { .ok_or_else(|| PositionLookupError::DirectiveMissing(self.clone())) } - fn try_make_mut<'schema>( - &self, - schema: &'schema mut Schema, - ) -> Option<&'schema mut Node> { - if self.try_get(schema).is_some() { - self.make_mut(schema).ok() - } else { - None - } - } - pub(crate) fn pre_insert(&self, schema: &mut FederationSchema) -> Result<(), FederationError> { if schema .referencers @@ -5753,35 +5035,6 @@ impl DirectiveArgumentDefinitionPosition { } } - pub(crate) fn insert( - &self, - schema: &mut FederationSchema, - argument: Node, - ) -> Result<(), FederationError> { - if self.argument_name != argument.name { - return Err(SingleFederationError::Internal { - message: format!( - "Directive argument \"{}\" given argument named \"{}\"", - self, argument.name, - ), - } - .into()); - } - if self.try_get(&schema.schema).is_some() { - // TODO: Handle old spec edge case of arguments with non-unique names - return Err(SingleFederationError::Internal { - message: format!("Directive argument \"{}\" already exists in schema", self,), - } - .into()); - } - self.parent() - .make_mut(&mut schema.schema)? - .make_mut() - .arguments - .push(argument); - self.insert_references(self.get(&schema.schema)?, &mut schema.referencers) - } - /// Remove this argument definition from its directive. Any applications of the directive that /// use this argument will become invalid. pub(crate) fn remove(&self, schema: &mut FederationSchema) -> Result<(), FederationError> { @@ -5797,30 +5050,6 @@ impl DirectiveArgumentDefinitionPosition { Ok(()) } - pub(crate) fn insert_directive( - &self, - schema: &mut FederationSchema, - directive: Node, - ) -> Result<(), FederationError> { - let argument = self.make_mut(&mut schema.schema)?; - if argument - .directives - .iter() - .any(|other_directive| other_directive.ptr_eq(&directive)) - { - return Err(SingleFederationError::Internal { - message: format!( - "Directive application \"@{}\" already exists on directive argument \"{}\"", - directive.name, self, - ), - } - .into()); - } - let name = directive.name.clone(); - argument.make_mut().directives.push(directive); - self.insert_directive_name_references(&mut schema.referencers, &name) - } - /// Remove a directive application from this position by name. pub(crate) fn remove_directive_name(&self, schema: &mut FederationSchema, name: &str) { let Some(argument) = self.try_make_mut(&mut schema.schema) else { @@ -5833,26 +5062,6 @@ impl DirectiveArgumentDefinitionPosition { .retain(|other_directive| other_directive.name != name); } - /// Remove a directive application. - pub(crate) fn remove_directive( - &self, - schema: &mut FederationSchema, - directive: &Node, - ) { - let Some(argument) = self.try_make_mut(&mut schema.schema) else { - return; - }; - if !argument.directives.iter().any(|other_directive| { - (other_directive.name == directive.name) && !other_directive.ptr_eq(directive) - }) { - self.remove_directive_name_references(&mut schema.referencers, &directive.name); - } - argument - .make_mut() - .directives - .retain(|other_directive| !other_directive.ptr_eq(directive)); - } - fn insert_references( &self, argument: &Node, @@ -6253,7 +5462,9 @@ mod tests { ) .unwrap(); - let position = InterfaceTypeDefinitionPosition::new(name!("UserProfile")); + let position = InterfaceTypeDefinitionPosition { + type_name: name!("UserProfile"), + }; position.remove_recursive(&mut schema).unwrap(); insta::assert_snapshot!(schema.schema(), @r#" diff --git a/apollo-federation/src/schema/referencer.rs b/apollo-federation/src/schema/referencer.rs index e4cf9973c3..63022c435b 100644 --- a/apollo-federation/src/schema/referencer.rs +++ b/apollo-federation/src/schema/referencer.rs @@ -42,30 +42,6 @@ impl Referencers { || self.input_object_types.contains_key(name) } - pub(crate) fn get_scalar_type( - &self, - name: &str, - ) -> Result<&ScalarTypeReferencers, FederationError> { - self.scalar_types.get(name).ok_or_else(|| { - SingleFederationError::Internal { - message: "Scalar type referencers unexpectedly missing type".to_owned(), - } - .into() - }) - } - - pub(crate) fn get_object_type( - &self, - name: &str, - ) -> Result<&ObjectTypeReferencers, FederationError> { - self.object_types.get(name).ok_or_else(|| { - SingleFederationError::Internal { - message: "Object type referencers unexpectedly missing type".to_owned(), - } - .into() - }) - } - pub(crate) fn get_interface_type( &self, name: &str, @@ -78,42 +54,6 @@ impl Referencers { }) } - pub(crate) fn get_union_type( - &self, - name: &str, - ) -> Result<&UnionTypeReferencers, FederationError> { - self.union_types.get(name).ok_or_else(|| { - SingleFederationError::Internal { - message: "Union type referencers unexpectedly missing type".to_owned(), - } - .into() - }) - } - - pub(crate) fn get_enum_type( - &self, - name: &str, - ) -> Result<&EnumTypeReferencers, FederationError> { - self.enum_types.get(name).ok_or_else(|| { - SingleFederationError::Internal { - message: "Enum type referencers unexpectedly missing type".to_owned(), - } - .into() - }) - } - - pub(crate) fn get_input_object_type( - &self, - name: &str, - ) -> Result<&InputObjectTypeReferencers, FederationError> { - self.input_object_types.get(name).ok_or_else(|| { - SingleFederationError::Internal { - message: "Input object type referencers unexpectedly missing type".to_owned(), - } - .into() - }) - } - pub(crate) fn get_directive( &self, name: &str, diff --git a/apollo-federation/src/schema/subgraph_metadata.rs b/apollo-federation/src/schema/subgraph_metadata.rs index 936b0b6e9c..7655f8395b 100644 --- a/apollo-federation/src/schema/subgraph_metadata.rs +++ b/apollo-federation/src/schema/subgraph_metadata.rs @@ -8,11 +8,8 @@ use crate::link::spec::Version; use crate::link::spec_definition::SpecDefinition; use crate::operation::Selection; use crate::operation::SelectionSet; -use crate::schema::field_set::add_interface_field_implementations; use crate::schema::field_set::collect_target_fields_from_field_set; -use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::FieldDefinitionPosition; -use crate::schema::position::ObjectOrInterfaceFieldDefinitionPosition; use crate::schema::position::ObjectOrInterfaceTypeDefinitionPosition; use crate::schema::FederationSchema; @@ -26,7 +23,6 @@ fn unwrap_schema(fed_schema: &Valid) -> &Valid { #[derive(Debug, Clone)] pub(crate) struct SubgraphMetadata { federation_spec_definition: &'static FederationSpecDefinition, - is_fed2: bool, external_metadata: ExternalMetadata, } @@ -35,13 +31,9 @@ impl SubgraphMetadata { schema: &Valid, federation_spec_definition: &'static FederationSpecDefinition, ) -> Result { - let is_fed2 = federation_spec_definition - .version() - .satisfies(&Version { major: 2, minor: 0 }); let external_metadata = ExternalMetadata::new(schema, federation_spec_definition)?; Ok(Self { federation_spec_definition, - is_fed2, external_metadata, }) } @@ -50,10 +42,6 @@ impl SubgraphMetadata { self.federation_spec_definition } - pub(crate) fn is_fed2(&self) -> bool { - self.is_fed2 - } - pub(crate) fn external_metadata(&self) -> &ExternalMetadata { &self.external_metadata } @@ -70,9 +58,6 @@ pub(crate) struct ExternalMetadata { /// Fields with an `@external` directive that can't actually be external due to also being /// referenced in a `@key` directive. fake_external_fields: IndexSet, - /// Fields that are only sometimes external, and sometimes reachable due to being included - /// in a `@provides` directive. - provided_fields: IndexSet, /// Fields that are external because their parent type has an `@external` directive. fields_on_external_types: IndexSet, } @@ -85,7 +70,6 @@ impl ExternalMetadata { let external_fields = Self::collect_external_fields(federation_spec_definition, schema)?; let fake_external_fields = Self::collect_fake_externals(federation_spec_definition, schema)?; - let provided_fields = Self::collect_provided_fields(federation_spec_definition, schema)?; // We do not collect @external on types for Fed 1 schemas since those will be discarded by // the schema upgrader. The schema upgrader, through calls to `is_external()`, relies on the // populated `fields_on_external_types` set to inform when @shareable should be @@ -103,7 +87,6 @@ impl ExternalMetadata { Ok(Self { external_fields, fake_external_fields, - provided_fields, fields_on_external_types, }) } @@ -188,47 +171,6 @@ impl ExternalMetadata { Ok(fake_external_fields) } - fn collect_provided_fields( - federation_spec_definition: &'static FederationSpecDefinition, - schema: &Valid, - ) -> Result, FederationError> { - let mut provided_fields = IndexSet::default(); - let provides_directive_definition = - federation_spec_definition.provides_directive_definition(schema)?; - let provides_directive_referencers = schema - .referencers - .get_directive(&provides_directive_definition.name)?; - let mut provides_field_positions: Vec = vec![]; - for object_field_position in &provides_directive_referencers.object_fields { - provides_field_positions.push(object_field_position.clone().into()); - } - for interface_field_position in &provides_directive_referencers.interface_fields { - provides_field_positions.push(interface_field_position.clone().into()); - } - for field_position in provides_field_positions { - let field = field_position.get(schema.schema())?; - let field_type_position: CompositeTypeDefinitionPosition = schema - .get_type(field.ty.inner_named_type().clone())? - .try_into()?; - for provides_directive_application in field - .directives - .get_all(&provides_directive_definition.name) - { - let provides_directive_arguments = federation_spec_definition - .provides_directive_arguments(provides_directive_application)?; - provided_fields.extend(add_interface_field_implementations( - collect_target_fields_from_field_set( - unwrap_schema(schema), - field_type_position.type_name().clone(), - provides_directive_arguments.fields, - )?, - schema, - )?); - } - } - Ok(provided_fields) - } - fn collect_fields_on_external_types( federation_spec_definition: &'static FederationSpecDefinition, schema: &Valid, @@ -287,20 +229,4 @@ impl ExternalMetadata { } false } - - pub(crate) fn is_partially_external( - &self, - field_definition_position: &FieldDefinitionPosition, - ) -> bool { - self.is_external(field_definition_position) - && self.provided_fields.contains(field_definition_position) - } - - pub(crate) fn is_fully_external( - &self, - field_definition_position: &FieldDefinitionPosition, - ) -> bool { - self.is_external(field_definition_position) - && !self.provided_fields.contains(field_definition_position) - } } diff --git a/apollo-federation/src/schema/type_and_directive_specification.rs b/apollo-federation/src/schema/type_and_directive_specification.rs index e1078b0415..3396c8e0d9 100644 --- a/apollo-federation/src/schema/type_and_directive_specification.rs +++ b/apollo-federation/src/schema/type_and_directive_specification.rs @@ -1,3 +1,9 @@ +#![allow(dead_code)] +// NOTE: There are several (technically) unused fields, type aliases, and methods in this module. +// Unfortunely, there is not a good way to clean this up because of how `` it is used for testing. +// Rather than littering this module with `#[allow(dead_code)]`s or adding a config_atr to the +// crate wide directive, allowing dead code here seems like the best options + use apollo_compiler::ast::DirectiveLocation; use apollo_compiler::ast::FieldDefinition; use apollo_compiler::ast::Value; @@ -20,8 +26,6 @@ use apollo_compiler::Node; use crate::error::FederationError; use crate::error::MultipleFederationErrors; use crate::error::SingleFederationError; -use crate::link::spec::Version; -use crate::link::spec_definition::SpecDefinition; use crate::schema::argument_composition_strategies::ArgumentCompositionStrategy; use crate::schema::position::DirectiveDefinitionPosition; use crate::schema::position::EnumTypeDefinitionPosition; @@ -37,16 +41,16 @@ use crate::schema::FederationSchema; /// Schema-dependent argument specification #[derive(Clone)] pub(crate) struct ArgumentSpecification { - pub name: Name, + pub(crate) name: Name, // PORT_NOTE: In TS, get_type returns `InputType`. - pub get_type: fn(schema: &FederationSchema) -> Result, - pub default_value: Option, + pub(crate) get_type: fn(schema: &FederationSchema) -> Result, + pub(crate) default_value: Option, } /// The resolved version of `ArgumentSpecification` pub(crate) struct ResolvedArgumentSpecification { - pub name: Name, - pub ty: Type, + pub(crate) name: Name, + pub(crate) ty: Type, default_value: Option, } @@ -66,9 +70,9 @@ impl From<&ResolvedArgumentSpecification> for InputValueDefinition { } pub(crate) struct FieldSpecification { - pub name: Name, - pub ty: Type, - pub arguments: Vec, + pub(crate) name: Name, + pub(crate) ty: Type, + pub(crate) arguments: Vec, } impl From<&FieldSpecification> for FieldDefinition { @@ -96,7 +100,7 @@ pub(crate) trait TypeAndDirectiveSpecification { } pub(crate) struct ScalarTypeSpecification { - pub name: Name, // Type's name + pub(crate) name: Name, // Type's name } impl TypeAndDirectiveSpecification for ScalarTypeSpecification { @@ -123,8 +127,8 @@ impl TypeAndDirectiveSpecification for ScalarTypeSpecification { } pub(crate) struct ObjectTypeSpecification { - pub name: Name, - pub fields: fn(&FederationSchema) -> Vec, + pub(crate) name: Name, + pub(crate) fields: fn(&FederationSchema) -> Vec, } impl TypeAndDirectiveSpecification for ObjectTypeSpecification { @@ -174,8 +178,8 @@ pub(crate) struct UnionTypeSpecification where F: Fn(&FederationSchema) -> IndexSet, { - pub name: Name, - pub members: F, + pub(crate) name: Name, + pub(crate) members: F, } impl TypeAndDirectiveSpecification for UnionTypeSpecification @@ -241,13 +245,13 @@ where } pub(crate) struct EnumValueSpecification { - pub name: Name, - pub description: Option, + pub(crate) name: Name, + pub(crate) description: Option, } pub(crate) struct EnumTypeSpecification { - pub name: Name, - pub values: Vec, + pub(crate) name: Name, + pub(crate) values: Vec, } impl TypeAndDirectiveSpecification for EnumTypeSpecification { @@ -322,29 +326,32 @@ impl TypeAndDirectiveSpecification for EnumTypeSpecification { #[derive(Clone)] pub(crate) struct DirectiveArgumentSpecification { - pub base_spec: ArgumentSpecification, - pub composition_strategy: Option, + pub(crate) base_spec: ArgumentSpecification, + pub(crate) composition_strategy: Option, } type ArgumentMergerFn = dyn Fn(&str, &[Value]) -> Value; pub(crate) struct ArgumentMerger { - pub merge: Box, - pub to_string: Box String>, + pub(crate) merge: Box, + pub(crate) to_string: Box String>, } type ArgumentMergerFactory = dyn Fn(&FederationSchema) -> Result; pub(crate) struct DirectiveCompositionSpecification { - pub supergraph_specification: fn(federation_version: Version) -> Box, + pub(crate) supergraph_specification: + fn( + federation_version: crate::link::spec::Version, + ) -> Box, /// Factory function returning an actual argument merger for given federation schema. - pub argument_merger: Option>, + pub(crate) argument_merger: Option>, } pub(crate) struct DirectiveSpecification { - pub name: Name, - pub composition: Option, + pub(crate) name: Name, + pub(crate) composition: Option, args: Vec, repeatable: bool, locations: Vec, @@ -354,14 +361,16 @@ pub(crate) struct DirectiveSpecification { // composition. // https://apollographql.atlassian.net/browse/FED-172 impl DirectiveSpecification { - pub fn new( + pub(crate) fn new( name: Name, args: &[DirectiveArgumentSpecification], repeatable: bool, locations: &[DirectiveLocation], composes: bool, supergraph_specification: Option< - fn(federation_version: Version) -> Box, + fn( + federation_version: crate::link::spec::Version, + ) -> Box, >, ) -> Self { let mut composition: Option = None; diff --git a/apollo-federation/src/subgraph/database.rs b/apollo-federation/src/subgraph/database.rs index e4f8776fd6..141e57d5c8 100644 --- a/apollo-federation/src/subgraph/database.rs +++ b/apollo-federation/src/subgraph/database.rs @@ -13,29 +13,21 @@ use std::sync::Arc; -use apollo_compiler::executable::Directive; use apollo_compiler::executable::SelectionSet; -use apollo_compiler::name; -use apollo_compiler::Name; -use apollo_compiler::Schema; - -use crate::link::database::links_metadata; -use crate::link::spec::Identity; -use crate::link::spec::APOLLO_SPEC_DOMAIN; -use crate::link::Link; // TODO: we should define this as part as some more generic "FederationSpec" definition, but need // to define the ground work for that in `apollo-at-link` first. -pub fn federation_link_identity() -> Identity { - Identity { - domain: APOLLO_SPEC_DOMAIN.to_string(), - name: name!("federation"), +#[cfg(test)] +pub(crate) fn federation_link_identity() -> crate::link::spec::Identity { + crate::link::spec::Identity { + domain: crate::link::spec::APOLLO_SPEC_DOMAIN.to_string(), + name: apollo_compiler::name!("federation"), } } #[derive(Eq, PartialEq, Debug, Clone)] -pub struct Key { - pub type_name: Name, +pub(crate) struct Key { + pub(crate) type_name: apollo_compiler::Name, // TODO: this should _not_ be an Option below; but we don't know how to build the SelectionSet, // so until we have a solution, we use None to have code that compiles. selections: Option>, @@ -45,13 +37,14 @@ impl Key { // TODO: same remark as above: not meant to be `Option` // TODO remove suppression OR use method in final version #[allow(dead_code)] - pub fn selections(&self) -> Option> { + pub(crate) fn selections(&self) -> Option> { self.selections.clone() } + #[cfg(test)] pub(crate) fn from_directive_application( - type_name: &Name, - directive: &Directive, + type_name: &apollo_compiler::Name, + directive: &apollo_compiler::executable::Directive, ) -> Option { directive .arguments @@ -66,8 +59,9 @@ impl Key { } } -pub fn federation_link(schema: &Schema) -> Arc { - links_metadata(schema) +#[cfg(test)] +pub(crate) fn federation_link(schema: &apollo_compiler::Schema) -> Arc { + crate::link::database::links_metadata(schema) // TODO: error handling? .unwrap_or_default() .unwrap_or_default() @@ -78,11 +72,16 @@ pub fn federation_link(schema: &Schema) -> Arc { /// The name of the @key directive in this subgraph. /// This will either return 'federation__key' if the `@key` directive is not imported, /// or whatever never it is imported under otherwise. Commonly, this would just be `key`. -pub fn key_directive_name(schema: &Schema) -> Name { - federation_link(schema).directive_name_in_schema(&name!("key")) +#[cfg(test)] +pub(crate) fn key_directive_name(schema: &apollo_compiler::Schema) -> apollo_compiler::Name { + federation_link(schema).directive_name_in_schema(&apollo_compiler::name!("key")) } -pub fn keys(schema: &Schema, type_name: &Name) -> Vec { +#[cfg(test)] +pub(crate) fn keys( + schema: &apollo_compiler::Schema, + type_name: &apollo_compiler::Name, +) -> Vec { let key_name = key_directive_name(schema); if let Some(type_def) = schema.types.get(type_name) { type_def diff --git a/apollo-federation/src/subgraph/spec.rs b/apollo-federation/src/subgraph/spec.rs index e5fd352a8c..44ba278309 100644 --- a/apollo-federation/src/subgraph/spec.rs +++ b/apollo-federation/src/subgraph/spec.rs @@ -722,7 +722,17 @@ impl Default for LinkSpecDefinitions { #[cfg(test)] mod tests { use super::*; - use crate::subgraph::database::federation_link_identity; + use crate::link::spec::Identity; + use crate::link::spec::APOLLO_SPEC_DOMAIN; + + // TODO: we should define this as part as some more generic "FederationSpec" definition, but need + // to define the ground work for that in `apollo-at-link` first. + fn federation_link_identity() -> Identity { + Identity { + domain: APOLLO_SPEC_DOMAIN.to_string(), + name: name!("federation"), + } + } #[test] fn handle_unsupported_federation_version() { diff --git a/apollo-federation/src/supergraph/subgraph.rs b/apollo-federation/src/supergraph/subgraph.rs index 7697d3b569..ebdad7def2 100644 --- a/apollo-federation/src/supergraph/subgraph.rs +++ b/apollo-federation/src/supergraph/subgraph.rs @@ -35,10 +35,6 @@ impl FederationSubgraphs { Ok(()) } - fn get(&self, name: &str) -> Option<&FederationSubgraph> { - self.subgraphs.get(name) - } - pub(super) fn get_mut(&mut self, name: &str) -> Option<&mut FederationSubgraph> { self.subgraphs.get_mut(name) } diff --git a/apollo-federation/src/utils/fallible_iterator.rs b/apollo-federation/src/utils/fallible_iterator.rs index 36dc23b845..0a26b9c7fd 100644 --- a/apollo-federation/src/utils/fallible_iterator.rs +++ b/apollo-federation/src/utils/fallible_iterator.rs @@ -410,7 +410,7 @@ pub(crate) trait FallibleIterator: Sized + Itertools { impl FallibleIterator for I {} /// The struct returned by [fallible_filter](FallibleIterator::fallible_filter). -pub struct FallibleFilter { +pub(crate) struct FallibleFilter { iter: I, predicate: F, } @@ -435,7 +435,7 @@ where } /// The struct returned by [and_then_filter](FallibleIterator::and_then_filter). -pub struct AndThenFilter { +pub(crate) struct AndThenFilter { iter: I, predicate: F, } @@ -462,7 +462,7 @@ where } } -pub struct AndThen { +pub(crate) struct AndThen { iter: I, map: F, } @@ -479,7 +479,7 @@ where } } -pub struct OrElse { +pub(crate) struct OrElse { iter: I, map: F, } @@ -515,11 +515,13 @@ mod tests { i % 2 == 0 } + #[test] fn test_fallible_filter() { let vals = (1..6).fallible_filter(|i| is_prime(*i)); itertools::assert_equal(vals, vec![Err(()), Ok(2), Ok(3)]); } + #[test] fn test_and_then_filter() { let vals = vec![Ok(0), Err(()), Err(()), Ok(3), Ok(4)] .into_iter() @@ -527,6 +529,7 @@ mod tests { itertools::assert_equal(vals, vec![Err(()), Err(()), Err(()), Ok(3)]); } + #[test] fn test_fallible_all() { assert_eq!(Ok(true), [].into_iter().fallible_all(is_prime)); assert_eq!(Ok(true), (2..4).fallible_all(is_prime)); @@ -535,6 +538,7 @@ mod tests { assert_eq!(Err(()), (1..5).fallible_all(is_prime)); } + #[test] fn test_ok_and_all() { let first_values: Vec = vec![]; let second_values: Vec = vec![Ok(1), Err(())]; @@ -547,6 +551,7 @@ mod tests { assert_eq!(Err(()), fourth_values.into_iter().ok_and_all(is_even)); } + #[test] fn test_and_then_all() { let first_values: Vec = vec![]; let second_values: Vec = vec![Ok(0), Err(())]; @@ -563,6 +568,7 @@ mod tests { assert_eq!(Ok(false), sixth_values.into_iter().and_then_all(is_prime)); } + #[test] fn test_fallible_any() { assert_eq!(Ok(false), [].into_iter().fallible_any(is_prime)); assert_eq!(Ok(true), (2..5).fallible_any(is_prime)); @@ -571,6 +577,7 @@ mod tests { assert_eq!(Err(()), (1..5).fallible_any(is_prime)); } + #[test] fn test_ok_and_any() { let first_values: Vec = vec![]; let second_values: Vec = vec![Ok(0), Err(())]; @@ -583,6 +590,7 @@ mod tests { assert_eq!(Err(()), fourth_values.into_iter().ok_and_any(is_even)); } + #[test] fn test_and_then_any() { let first_values: Vec = vec![]; let second_values: Vec = vec![Ok(0), Err(())]; diff --git a/apollo-federation/src/utils/mod.rs b/apollo-federation/src/utils/mod.rs index fe4b815704..8d62d08a04 100644 --- a/apollo-federation/src/utils/mod.rs +++ b/apollo-federation/src/utils/mod.rs @@ -1,6 +1,6 @@ //! This module contains various tools that help the ergonomics of this crate. mod fallible_iterator; -pub mod logging; +pub(crate) mod logging; pub(crate) use fallible_iterator::*; diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/handles_fragments_with_directive_conditions.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/handles_fragments_with_directive_conditions.rs index 07a1633f84..a310ffcffa 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/handles_fragments_with_directive_conditions.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/handles_fragments_with_directive_conditions.rs @@ -72,7 +72,7 @@ fn fragment_with_intersecting_parent_type_and_directive_condition() { } #[test] -fn nested_fragment_with_interseting_parent_type_and_directive_condition() { +fn nested_fragment_with_intersecting_parent_type_and_directive_condition() { let planner = planner!( A: r#" directive @test on INLINE_FRAGMENT @@ -124,18 +124,6 @@ fn nested_fragment_with_interseting_parent_type_and_directive_condition() { } "#; - // expect(operation.expandAllFragments().toString()).toMatchInlineSnapshot(` - // "{ - // i { - // _id - // ... on I2 { - // ... on I2 @test { - // id - // } - // } - // } - // }" - // `); assert_plan!( &planner, operation, diff --git a/apollo-federation/tests/query_plan/supergraphs/nested_fragment_with_interseting_parent_type_and_directive_condition.graphql b/apollo-federation/tests/query_plan/supergraphs/nested_fragment_with_intersecting_parent_type_and_directive_condition.graphql similarity index 100% rename from apollo-federation/tests/query_plan/supergraphs/nested_fragment_with_interseting_parent_type_and_directive_condition.graphql rename to apollo-federation/tests/query_plan/supergraphs/nested_fragment_with_intersecting_parent_type_and_directive_condition.graphql diff --git a/apollo-router-benchmarks/Cargo.toml b/apollo-router-benchmarks/Cargo.toml index 38759d2feb..65114fead0 100644 --- a/apollo-router-benchmarks/Cargo.toml +++ b/apollo-router-benchmarks/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-router-benchmarks" -version = "1.55.0" +version = "1.56.0" authors = ["Apollo Graph, Inc. "] edition = "2021" license = "Elastic-2.0" diff --git a/apollo-router-scaffold/Cargo.toml b/apollo-router-scaffold/Cargo.toml index 4801a6c616..1ab31e1840 100644 --- a/apollo-router-scaffold/Cargo.toml +++ b/apollo-router-scaffold/Cargo.toml @@ -1,11 +1,17 @@ [package] name = "apollo-router-scaffold" -version = "1.55.0" +version = "1.56.0" authors = ["Apollo Graph, Inc. "] edition = "2021" license = "Elastic-2.0" publish = false +[package.metadata.cargo-machete] +ignored = [ + # usage not found because the crate name is `inflector` without `str_` + "str_inflector", +] + [dependencies] anyhow = "1.0.80" clap = { version = "4.5.1", features = ["derive"] } diff --git a/apollo-router-scaffold/scaffold-test/Cargo.toml b/apollo-router-scaffold/scaffold-test/Cargo.toml index b2b03bbccd..692dc3d08c 100644 --- a/apollo-router-scaffold/scaffold-test/Cargo.toml +++ b/apollo-router-scaffold/scaffold-test/Cargo.toml @@ -10,9 +10,8 @@ path = "src/main.rs" [dependencies] anyhow = "1.0.58" -apollo-router = { path ="../../apollo-router" } +apollo-router = { path = "../../apollo-router" } async-trait = "0.1.52" -futures = "0.3.21" schemars = "0.8.10" serde = "1.0.149" serde_json = "1.0.79" diff --git a/apollo-router-scaffold/templates/base/Cargo.template.toml b/apollo-router-scaffold/templates/base/Cargo.template.toml index a1c3d2a42d..d132a5d999 100644 --- a/apollo-router-scaffold/templates/base/Cargo.template.toml +++ b/apollo-router-scaffold/templates/base/Cargo.template.toml @@ -22,11 +22,10 @@ apollo-router = { path ="{{integration_test}}apollo-router" } apollo-router = { git="https://github.com/apollographql/router.git", branch="{{branch}}" } {{else}} # Note if you update these dependencies then also update xtask/Cargo.toml -apollo-router = "1.55.0" +apollo-router = "1.56.0" {{/if}} {{/if}} async-trait = "0.1.52" -futures = "0.3.21" schemars = "0.8.10" serde = "1.0.149" serde_json = "1.0.79" diff --git a/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml b/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml index c566a3a0ca..b6d72bde79 100644 --- a/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml +++ b/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml @@ -13,7 +13,7 @@ apollo-router-scaffold = { path ="{{integration_test}}apollo-router-scaffold" } {{#if branch}} apollo-router-scaffold = { git="https://github.com/apollographql/router.git", branch="{{branch}}" } {{else}} -apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.55.0" } +apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.56.0" } {{/if}} {{/if}} anyhow = "1.0.58" diff --git a/apollo-router/Cargo.toml b/apollo-router/Cargo.toml index 14bb6bbf19..ff09306e2a 100644 --- a/apollo-router/Cargo.toml +++ b/apollo-router/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-router" -version = "1.55.0" +version = "1.56.0" authors = ["Apollo Graph, Inc. "] repository = "https://github.com/apollographql/router/" documentation = "https://docs.rs/apollo-router" @@ -44,11 +44,6 @@ dhat-ad-hoc = ["dhat"] # alerted early when something is wrong instead of receiving an invalid result. failfast = [] -# Enables usage of tokio-console with the router -# tokio-console also requires at build time the environment variable -# RUSTFLAGS="--cfg tokio_unstable" -console = ["tokio/tracing", "console-subscriber"] - # "fake" feature to disable V8 usage when building on docs.rs # See https://github.com/apollographql/federation-rs/pull/185 docs_rs = ["router-bridge/docs_rs"] @@ -64,11 +59,10 @@ ci = [] features = ["docs_rs"] [dependencies] -askama = "0.12.1" access-json = "0.1.0" anyhow = "1.0.86" apollo-compiler.workspace = true -apollo-federation = { path = "../apollo-federation", version = "=1.55.0" } +apollo-federation = { path = "../apollo-federation", version = "=1.56.0" } arc-swap = "1.6.0" async-channel = "1.9.0" async-compression = { version = "0.4.6", features = [ @@ -89,7 +83,6 @@ clap = { version = "4.5.8", default-features = false, features = [ "std", "help", ] } -console-subscriber = { version = "0.2.0", optional = true } cookie = { version = "0.18.0", default-features = false } crossbeam-channel = "0.5" ci_info = { version = "0.14.14", features = ["serde-1"] } @@ -364,6 +357,15 @@ tonic-build = "0.9.2" basic-toml = "0.1.9" serde_json.workspace = true +[package.metadata.cargo-machete] +ignored = [ + # Pinned to versions pre-MSRV bump. Remove when we update our rust-toolchain. + "rowan", + "aws-sdk-sso", + "aws-sdk-ssooidc", + "aws-sdk-sts", +] + [[test]] name = "integration_tests" path = "tests/integration_tests.rs" diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index b8bd132912..5b497b94d8 100644 --- a/apollo-router/src/configuration/metrics.rs +++ b/apollo-router/src/configuration/metrics.rs @@ -211,6 +211,14 @@ impl InstrumentData { opt.subgraph.response, "$.subgraph..response" ); + populate_config_instrument!( + apollo.router.config.rhai, + "$.rhai", + opt.scripts, + "$[?(@.scripts)]", + opt.main, + "$[?(@.main)]" + ); populate_config_instrument!( apollo.router.config.persisted_queries, "$.persisted_queries[?(@.enabled == true)]", diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@rhai.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@rhai.router.yaml.snap new file mode 100644 index 0000000000..eaa300b173 --- /dev/null +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@rhai.router.yaml.snap @@ -0,0 +1,11 @@ +--- +source: apollo-router/src/configuration/metrics.rs +expression: "&metrics.non_zero()" +--- +- name: apollo.router.config.rhai + data: + datapoints: + - value: 1 + attributes: + opt.main: true + opt.scripts: true diff --git a/apollo-router/src/configuration/testdata/metrics/rhai.router.yaml b/apollo-router/src/configuration/testdata/metrics/rhai.router.yaml new file mode 100644 index 0000000000..a8dae18af6 --- /dev/null +++ b/apollo-router/src/configuration/testdata/metrics/rhai.router.yaml @@ -0,0 +1,3 @@ +rhai: + scripts: rhai + main: main.rhai \ No newline at end of file diff --git a/apollo-router/src/metrics/filter.rs b/apollo-router/src/metrics/filter.rs index 0fcb52bdff..fa6dad38df 100644 --- a/apollo-router/src/metrics/filter.rs +++ b/apollo-router/src/metrics/filter.rs @@ -94,7 +94,7 @@ impl FilterMeterProvider { .delegate(delegate) .allow( Regex::new( - r"apollo\.(graphos\.cloud|router\.(operations?|lifecycle|config|schema|query|query_planning|telemetry))(\..*|$)", + r"apollo\.(graphos\.cloud|router\.(operations?|lifecycle|config|schema|query|query_planning|telemetry))(\..*|$)|apollo_router_uplink_fetch_count_total|apollo_router_uplink_fetch_duration_seconds", ) .expect("regex should have been valid"), ) diff --git a/apollo-router/src/plugins/telemetry/config_new/events.rs b/apollo-router/src/plugins/telemetry/config_new/events.rs index e3bbd668f1..a58264bfb7 100644 --- a/apollo-router/src/plugins/telemetry/config_new/events.rs +++ b/apollo-router/src/plugins/telemetry/config_new/events.rs @@ -9,6 +9,7 @@ use parking_lot::Mutex; use schemars::JsonSchema; use serde::Deserialize; use tower::BoxError; +use tracing::info_span; use tracing::Span; use super::instruments::Instrumented; @@ -738,7 +739,10 @@ where let mut new_attributes = selectors.on_response_event(response, ctx); attributes.append(&mut new_attributes); } - + // Stub span to make sure the custom attributes are saved in current span extensions + // It won't be extracted or sampled at all + let span = info_span!("supergraph_event_send_event"); + let _entered = span.enter(); inner.send_event(attributes); } diff --git a/apollo-router/src/plugins/telemetry/dynamic_attribute.rs b/apollo-router/src/plugins/telemetry/dynamic_attribute.rs index 0b4af0964d..d9cde3ca21 100644 --- a/apollo-router/src/plugins/telemetry/dynamic_attribute.rs +++ b/apollo-router/src/plugins/telemetry/dynamic_attribute.rs @@ -240,7 +240,9 @@ impl EventDynAttribute for ::tracing::Span { self.with_subscriber(move |(id, dispatch)| { if let Some(reg) = dispatch.downcast_ref::() { match reg.span(id) { - None => eprintln!("no spanref, this is a bug"), + None => { + eprintln!("no spanref, this is a bug"); + } Some(s) => { if s.is_sampled() { let mut extensions = s.extensions_mut(); diff --git a/apollo-router/src/plugins/telemetry/tracing/jaeger.rs b/apollo-router/src/plugins/telemetry/tracing/jaeger.rs index 13f61da5c4..f50aebefc2 100644 --- a/apollo-router/src/plugins/telemetry/tracing/jaeger.rs +++ b/apollo-router/src/plugins/telemetry/tracing/jaeger.rs @@ -139,7 +139,8 @@ impl TracingConfigurator for Config { Ok(builder.with_span_processor( BatchSpanProcessor::builder(exporter, runtime::Tokio) .with_batch_config(batch_processor.clone().into()) - .build(), + .build() + .filtered(), )) } _ => Ok(builder), diff --git a/apollo-router/src/query_planner/dual_introspection.rs b/apollo-router/src/query_planner/dual_introspection.rs index 400de5dbb6..f07ffe2036 100644 --- a/apollo-router/src/query_planner/dual_introspection.rs +++ b/apollo-router/src/query_planner/dual_introspection.rs @@ -109,9 +109,19 @@ fn normalize_default_value(key: &str, value: &Value) -> Option { .arguments .first()? .value; - parsed_default_value.as_object()?; - let normalized = normalize_parsed_default_value(parsed_default_value); - Some(normalized.serialize().no_indent().to_string().into()) + match parsed_default_value.as_ref() { + ast::Value::List(_) | ast::Value::Object(_) => { + let normalized = normalize_parsed_default_value(parsed_default_value); + Some(normalized.serialize().no_indent().to_string().into()) + } + ast::Value::Null + | ast::Value::Enum(_) + | ast::Value::Variable(_) + | ast::Value::String(_) + | ast::Value::Float(_) + | ast::Value::Int(_) + | ast::Value::Boolean(_) => None, + } } fn normalize_parsed_default_value(value: &ast::Value) -> ast::Value { diff --git a/apollo-router/src/query_planner/dual_query_planner.rs b/apollo-router/src/query_planner/dual_query_planner.rs index 91d273fb26..c62fb621b2 100644 --- a/apollo-router/src/query_planner/dual_query_planner.rs +++ b/apollo-router/src/query_planner/dual_query_planner.rs @@ -386,7 +386,6 @@ fn vec_matches_result( item_matches(this, other) .map_err(|err| err.add_description(&format!("under item[{}]", index))) })?; - assert!(vec_matches(this, other, |a, b| item_matches(a, b).is_ok())); // Note: looks redundant Ok(()) } @@ -398,7 +397,7 @@ fn vec_matches_sorted(this: &[T], other: &[T]) -> bool { vec_matches(&this_sorted, &other_sorted, T::eq) } -fn vec_matches_sorted_by( +fn vec_matches_sorted_by( this: &[T], other: &[T], compare: impl Fn(&T, &T) -> std::cmp::Ordering, @@ -414,19 +413,20 @@ fn vec_matches_sorted_by( Ok(()) } +// `this` vector includes `other` vector as a set +fn vec_includes_as_set(this: &[T], other: &[T], item_matches: impl Fn(&T, &T) -> bool) -> bool { + other.iter().all(|other_node| { + this.iter() + .any(|this_node| item_matches(this_node, other_node)) + }) +} + // performs a set comparison, ignoring order fn vec_matches_as_set(this: &[T], other: &[T], item_matches: impl Fn(&T, &T) -> bool) -> bool { // Set-inclusion test in both directions this.len() == other.len() - && this.iter().all(|this_node| { - other - .iter() - .any(|other_node| item_matches(this_node, other_node)) - }) - && other.iter().all(|other_node| { - this.iter() - .any(|this_node| item_matches(this_node, other_node)) - }) + && vec_includes_as_set(this, other, &item_matches) + && vec_includes_as_set(other, this, &item_matches) } fn vec_matches_result_as_set( @@ -457,7 +457,6 @@ fn vec_matches_result_as_set( )); } } - assert!(vec_matches_as_set(this, other, item_matches)); Ok(()) } @@ -720,6 +719,49 @@ fn same_ast_operation_definition( Ok(()) } +// `x` may be coerced to `y`. +// - `x` should be a value from JS QP. +// - `y` should be a value from Rust QP. +// - Assume: x and y are already checked not equal. +// Due to coercion differences, we need to compare AST values with special cases. +fn ast_value_maybe_coerced_to(x: &ast::Value, y: &ast::Value) -> bool { + match (x, y) { + // Special case 1: JS QP may convert an enum value into string. + // - In this case, compare them as strings. + (ast::Value::String(ref x), ast::Value::Enum(ref y)) => { + if x == y.as_str() { + return true; + } + } + + // Special case 2: Rust QP expands a object value by filling in its + // default field values. + // - If the Rust QP object value subsumes the JS QP object value, consider it a match. + // - Assuming the Rust QP object value has only default field values. + // - Warning: This is an unsound heuristic. + (ast::Value::Object(ref x), ast::Value::Object(ref y)) => { + if vec_includes_as_set(y, x, |(yy_name, yy_val), (xx_name, xx_val)| { + xx_name == yy_name + && (xx_val == yy_val || ast_value_maybe_coerced_to(xx_val, yy_val)) + }) { + return true; + } + } + + // Recurse into list items. + (ast::Value::List(ref x), ast::Value::List(ref y)) => { + if vec_matches(x, y, |xx, yy| { + xx == yy || ast_value_maybe_coerced_to(xx, yy) + }) { + return true; + } + } + + _ => {} // otherwise, fall through + } + false +} + // Use this function, instead of `VariableDefinition`'s `PartialEq` implementation, // due to known differences. fn same_variable_definition( @@ -730,27 +772,8 @@ fn same_variable_definition( check_match_eq!(x.ty, y.ty); if x.default_value != y.default_value { if let (Some(x), Some(y)) = (&x.default_value, &y.default_value) { - match (x.as_ref(), y.as_ref()) { - // Special case 1: JS QP may convert an enum value into string. - // - In this case, compare them as strings. - (ast::Value::String(ref x), ast::Value::Enum(ref y)) => { - if x == y.as_str() { - return Ok(()); - } - } - - // Special case 2: Rust QP expands an empty object value by filling in its - // default field values. - // - If the JS QP value is an empty object, consider any object is a match. - // - Assuming the Rust QP object value has only default field values. - // - Warning: This is an unsound heuristic. - (ast::Value::Object(ref x), ast::Value::Object(_)) => { - if x.is_empty() { - return Ok(()); - } - } - - _ => {} // otherwise, fall through + if ast_value_maybe_coerced_to(x, y) { + return Ok(()); } } @@ -868,7 +891,7 @@ mod ast_comparison_tests { } #[test] - fn test_query_variable_decl_object_value_coercion() { + fn test_query_variable_decl_object_value_coercion_empty_case() { // Note: Rust QP expands empty object default values by filling in its default field // values. let op_x = r#"query($qv1: T! = {}) { x(arg1: $qv1) }"#; @@ -879,6 +902,28 @@ mod ast_comparison_tests { assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); } + #[test] + fn test_query_variable_decl_object_value_coercion_non_empty_case() { + // Note: Rust QP expands an object default values by filling in its default field values. + let op_x = r#"query($qv1: T! = {field1: true}) { x(arg1: $qv1) }"#; + let op_y = + r#"query($qv1: T! = { field1: true, field2: "default_value" }) { x(arg1: $qv1) }"#; + let ast_x = ast::Document::parse(op_x, "op_x").unwrap(); + let ast_y = ast::Document::parse(op_y, "op_y").unwrap(); + assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); + } + + #[test] + fn test_query_variable_decl_list_of_object_value_coercion() { + // Testing a combination of list and object value coercion. + let op_x = r#"query($qv1: [T!]! = [{}]) { x(arg1: $qv1) }"#; + let op_y = + r#"query($qv1: [T!]! = [{field1: true, field2: "default_value"}]) { x(arg1: $qv1) }"#; + let ast_x = ast::Document::parse(op_x, "op_x").unwrap(); + let ast_y = ast::Document::parse(op_y, "op_y").unwrap(); + assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); + } + #[test] fn test_entities_selection_order() { let op_x = r#" diff --git a/apollo-router/src/services/layers/static_page.rs b/apollo-router/src/services/layers/static_page.rs index ffc0b29049..75ecfb2538 100644 --- a/apollo-router/src/services/layers/static_page.rs +++ b/apollo-router/src/services/layers/static_page.rs @@ -5,7 +5,6 @@ use std::ops::ControlFlow; -use askama::Template; use bytes::Bytes; use http::header::CONTENT_TYPE; use http::HeaderMap; @@ -100,36 +99,15 @@ fn prefers_html(headers: &HeaderMap) -> bool { }) } -#[derive(Template)] -#[template(path = "sandbox_index.html")] -struct SandboxTemplate { - apollo_router_version: &'static str, -} - pub(crate) fn sandbox_page_content() -> Vec { - let template = SandboxTemplate { - apollo_router_version: std::env!("CARGO_PKG_VERSION"), - }; - let mut buffer = Vec::new(); - template.write_into(&mut buffer).expect("cannot fail"); - buffer -} - -#[derive(Template)] -#[template(path = "homepage_index.html")] -struct HomepageTemplate { - graph_ref: String, + const TEMPLATE: &str = include_str!("../../../templates/sandbox_index.html"); + TEMPLATE + .replace("{{APOLLO_ROUTER_VERSION}}", std::env!("CARGO_PKG_VERSION")) + .into_bytes() } pub(crate) fn home_page_content(homepage_config: &Homepage) -> Vec { - let template = HomepageTemplate { - graph_ref: homepage_config - .graph_ref - .as_ref() - .cloned() - .unwrap_or_default(), - }; - let mut buffer = Vec::new(); - template.write_into(&mut buffer).expect("cannot fail"); - buffer + const TEMPLATE: &str = include_str!("../../../templates/homepage_index.html"); + let graph_ref = serde_json::to_string(&homepage_config.graph_ref).expect("cannot fail"); + TEMPLATE.replace("{{GRAPH_REF}}", &graph_ref).into_bytes() } diff --git a/apollo-router/templates/homepage_index.html b/apollo-router/templates/homepage_index.html index d4d117fcfd..4412555976 100644 --- a/apollo-router/templates/homepage_index.html +++ b/apollo-router/templates/homepage_index.html @@ -41,7 +41,7 @@

Welcome to the Apollo Router

It appears that you might be offline. POST to this endpoint to query your graph:

- + diff --git a/apollo-router/templates/sandbox_index.html b/apollo-router/templates/sandbox_index.html index 006fe13f51..ec9db97705 100644 --- a/apollo-router/templates/sandbox_index.html +++ b/apollo-router/templates/sandbox_index.html @@ -51,7 +51,7 @@

Welcome to the Apollo Router

style="width: 100vw; height: 100vh; position: absolute; top: 0;" id="embeddableSandbox" > - + diff --git a/apollo-router/tests/fixtures/schema_to_introspect.graphql b/apollo-router/tests/fixtures/schema_to_introspect.graphql index f45dfa4335..617f41c9c9 100644 --- a/apollo-router/tests/fixtures/schema_to_introspect.graphql +++ b/apollo-router/tests/fixtures/schema_to_introspect.graphql @@ -62,7 +62,7 @@ Root query type type TheQuery implements I @join__type(graph: SUBGRAPH1) { id: ID! ints: [[Int!]]! @deprecated(reason: "…") - url(arg: In = { b: 4, a: 2 }): Url + url(arg: [In] = [{ b: 4, a: 2 }]): Url union: U @deprecated(reason: null) } diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__both_mode_integration.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__both_mode_integration.snap index 1bad697545..0e4e353322 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__both_mode_integration.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__both_mode_integration.snap @@ -70,11 +70,15 @@ expression: "response.json::().await.unwrap()" "name": "arg", "description": null, "type": { - "kind": "INPUT_OBJECT", - "name": "In", - "ofType": null + "kind": "LIST", + "name": null, + "ofType": { + "kind": "INPUT_OBJECT", + "name": "In", + "ofType": null + } }, - "defaultValue": "{a: 2, b: 4}", + "defaultValue": "[{a: 2, b: 4}]", "isDeprecated": false, "deprecationReason": null } diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__integration.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__integration.snap index 40e9d17440..c47a5895c1 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__integration.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__introspection__integration.snap @@ -1310,11 +1310,15 @@ expression: "response.json::().await.unwrap()" "name": "arg", "description": null, "type": { - "kind": "INPUT_OBJECT", - "name": "In", - "ofType": null + "kind": "LIST", + "name": null, + "ofType": { + "kind": "INPUT_OBJECT", + "name": "In", + "ofType": null + } }, - "defaultValue": "{b: 4, a: 2}", + "defaultValue": "[{b: 4, a: 2}]", "isDeprecated": false, "deprecationReason": null } diff --git a/apollo-router/tests/integration/telemetry/fixtures/json.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/json.router.yaml index 2e8b047638..fa8fba775e 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/json.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/json.router.yaml @@ -68,6 +68,12 @@ telemetry: eq: - "log" - response_header: "x-log-request" + my.response_event.on_event: + message: "my response event message" + level: warn + on: event_response + attributes: + on_supergraph_response_event: on_supergraph_event subgraph: # Standard events request: info diff --git a/apollo-router/tests/integration/telemetry/fixtures/json.sampler_off.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/json.sampler_off.router.yaml index 1bbd2ac994..3190c14d34 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/json.sampler_off.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/json.sampler_off.router.yaml @@ -51,6 +51,12 @@ telemetry: eq: - "log" - request_header: "x-log-request" + my.response_event.on_event: + message: "my response event message" + level: warn + on: event_response + attributes: + on_supergraph_response_event: on_supergraph_event my.request.event: message: "my event message" level: info diff --git a/apollo-router/tests/integration/telemetry/fixtures/text.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/text.router.yaml index 66c7508443..ef009e55bd 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/text.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/text.router.yaml @@ -52,6 +52,12 @@ telemetry: eq: - "log" - request_header: "x-log-request" + my.response_event.on_event: + message: "my response event message" + level: warn + on: event_response + attributes: + on_supergraph_response_event: on_supergraph_event my.request.event: message: "my event message" level: info diff --git a/apollo-router/tests/integration/telemetry/logging.rs b/apollo-router/tests/integration/telemetry/logging.rs index a50f0fa20e..64b43fe032 100644 --- a/apollo-router/tests/integration/telemetry/logging.rs +++ b/apollo-router/tests/integration/telemetry/logging.rs @@ -30,6 +30,10 @@ async fn test_json() -> Result<(), BoxError> { router.execute_query(&query).await; router.assert_log_contains(r#""static_one":"test""#).await; router.execute_query(&query).await; + router + .assert_log_contains(r#""on_supergraph_response_event":"on_supergraph_event""#) + .await; + router.execute_query(&query).await; router.assert_log_contains(r#""response_status":200"#).await; router.graceful_shutdown().await; @@ -160,6 +164,10 @@ async fn test_json_sampler_off() -> Result<(), BoxError> { router.execute_query(&query).await; router.assert_log_contains(r#""static_one":"test""#).await; router.execute_query(&query).await; + router + .assert_log_contains(r#""on_supergraph_response_event":"on_supergraph_event""#) + .await; + router.execute_query(&query).await; router.assert_log_contains(r#""response_status":200"#).await; router.graceful_shutdown().await; @@ -188,6 +196,10 @@ async fn test_text() -> Result<(), BoxError> { router.assert_log_contains("trace_id").await; router.execute_query(&query).await; router.assert_log_contains("span_id").await; + router + .assert_log_contains(r#"on_supergraph_response_event=on_supergraph_event"#) + .await; + router.execute_query(&query).await; router.execute_query(&query).await; router.assert_log_contains("response_status=200").await; router.graceful_shutdown().await; diff --git a/deny.toml b/deny.toml index 378e5ed28c..915ca9e58c 100644 --- a/deny.toml +++ b/deny.toml @@ -27,7 +27,10 @@ git-fetch-with-cli = true # output a note when they are encountered. # rustsec advisory exemptions -ignore = ["RUSTSEC-2023-0071"] +ignore = [ + "RUSTSEC-2023-0071", + "RUSTSEC-2024-0376", # we do not use tonic::transport::Server +] # This section is considered when running `cargo deny check licenses` # More documentation for the licenses section can be found here: diff --git a/dockerfiles/tracing/docker-compose.datadog.yml b/dockerfiles/tracing/docker-compose.datadog.yml index 3b58c1d702..4b3558d572 100644 --- a/dockerfiles/tracing/docker-compose.datadog.yml +++ b/dockerfiles/tracing/docker-compose.datadog.yml @@ -3,7 +3,7 @@ services: apollo-router: container_name: apollo-router - image: ghcr.io/apollographql/router:v1.55.0 + image: ghcr.io/apollographql/router:v1.56.0 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/datadog.router.yaml:/etc/config/configuration.yaml diff --git a/dockerfiles/tracing/docker-compose.jaeger.yml b/dockerfiles/tracing/docker-compose.jaeger.yml index fcb75930d2..ca7b4ab265 100644 --- a/dockerfiles/tracing/docker-compose.jaeger.yml +++ b/dockerfiles/tracing/docker-compose.jaeger.yml @@ -4,7 +4,7 @@ services: apollo-router: container_name: apollo-router #build: ./router - image: ghcr.io/apollographql/router:v1.55.0 + image: ghcr.io/apollographql/router:v1.56.0 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/jaeger.router.yaml:/etc/config/configuration.yaml diff --git a/dockerfiles/tracing/docker-compose.zipkin.yml b/dockerfiles/tracing/docker-compose.zipkin.yml index 0cb933a7ac..564933ab0c 100644 --- a/dockerfiles/tracing/docker-compose.zipkin.yml +++ b/dockerfiles/tracing/docker-compose.zipkin.yml @@ -4,7 +4,7 @@ services: apollo-router: container_name: apollo-router build: ./router - image: ghcr.io/apollographql/router:v1.55.0 + image: ghcr.io/apollographql/router:v1.56.0 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/zipkin.router.yaml:/etc/config/configuration.yaml diff --git a/docs/source/configuration/entity-caching.mdx b/docs/source/configuration/entity-caching.mdx index 7cff65e71a..a47967c253 100644 --- a/docs/source/configuration/entity-caching.mdx +++ b/docs/source/configuration/entity-caching.mdx @@ -111,9 +111,8 @@ preview_entity_cache: ### Configure time to live (TTL) -To decide whether to cache an entity, the router honors the [`Cache-Control` header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control) returned with the subgraph response. Because `Cache-Control` might not contain a `max-age` or `s-max-age` option, a default TTL must either be defined per subgraph configuration or inherited from the global configuration. - -The router also generates a `Cache-Control` header for the client response by aggregating the TTL information from all response parts. If a subgraph doesn't return the header, its response is assumed to be `no-store`. +Besides configuring a global TTL for all the entries in Redis, the GraphOS Router also honors the [`Cache-Control` header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control) returned with the subgraph response. It generates a `Cache-Control` header for the client response by aggregating the TTL information from all response parts. +A TTL has to be configured for all subgraphs using entity caching, either defined in the per subgraph configuration or inherited from the global configuration, in case the subgraph returns a `Cache-Control` header without a `max-age`. ### Customize Redis cache key @@ -129,9 +128,192 @@ This entry contains an object with the `all` field to affect all subgraph reques "data": "key2" } } +``` + +### Entity cache invalidation + +When existing cache entries need to be replaced, the router supports a couple of ways for you to invalidate entity cache entries: +- [**Invalidation endpoint**](#invalidation-http-endpoint) - the router exposes an invalidation endpoint that can receive invalidation requests from any authorized service. This is primarily intended as an alternative to the extensions mechanism described below. For example a subgraph could use it to trigger invalidation events "out of band" from any requests received by the router or a platform operator could use it to invalidate cache entries in response to events which aren't directly related to a router. +- **Subgraph response extensions** - you can send invalidation requests via subgraph response extensions, allowing a subgraph to invalidate cached data right after a mutation. + +One invalidation request can invalidate multiple cached entries at once. It can invalidate: +- All cached entries for a specific subgraph +- All cached entries for a specific type in a specific subgraph +- All cached entries for a specific entity in a specific subgraph + +To process an invalidation request, the router first sends a `SCAN` command to Redis to find all the keys that match the invalidation request. After iterating over the scan cursor, the router sends a `DEL` command to Redis to remove the matching keys. + +#### Configuration + +You can configure entity cache invalidation globally with `preview_entity_cache.invalidation`. You can also override the global setting for a subgraph with `preview_entity_cache.subgraph.subgraphs.invalidation`. The example below shows both: + +```yaml title="router.yaml" +preview_entity_cache: + enabled: true + + # global invalidation configuration + invalidation: + # address of the invalidation endpoint + # this should only be exposed to internal networks + listen: "127.0.0.1:3000" + path: "/invalidation" + scan_count: 1000 + + subgraph: + all: + enabled: true + redis: + urls: ["redis://..."] + invalidation: + # base64 string that will be provided in the `Authorization: Basic` header value + shared_key: "agm3ipv7egb78dmxzv0gr5q0t5l6qs37" + subgraphs: + products: + # per subgraph invalidation configuration overrides global configuration + invalidation: + # whether invalidation is enabled for this subgraph + enabled: true + # override the shared key for this particular subgraph. If another key is provided, the invalidation requests for this subgraph's entities will not be executed + shared_key: "czn5qvjylm231m90hu00hgsuayhyhgjv" +``` + +##### `listen` + +The address and port to listen on for invalidation requests. + +##### `path` + +The path to listen on for invalidation requests. + +##### `shared_key` + +A string that will be used to authenticate invalidation requests. + +##### `scan_count` + +The number of keys to scan in a single `SCAN` command. This can be used to reduce the number of requests to Redis. + +#### Invalidation request format + +Invalidation requests are defined as JSON objects with the following format: + +- Subgraph invalidation request: + +```json +{ + "kind": "subgraph", + "subgraph": "accounts" +} +``` + +- Subgraph type invalidation request: +```json +{ + "kind": "subgraph", + "subgraph": "accounts", + "type": "User" +} ``` +- Subgraph entity invalidation request: + +```json +{ + "kind": "subgraph", + "subgraph": "accounts", + "type": "User", + "key": { + "id": "1" + } +} +``` + + + +The key field is the same argument as defined in the subgraph's `@key` directive. If a subgraph has multiple keys defined and the entity is being invalidated, it is likely you'll need to send a request for each key definition. + + + + +#### Invalidation HTTP endpoint + +The invalidation endpoint exposed by the router expects to receive an array of invalidation requests and will process them in sequence. For authorization, you must provide a shared key in the request header. For example, with the previous configuration you should send the following request: + +``` +POST http://127.0.0.1:3000/invalidation +Authorization: agm3ipv7egb78dmxzv0gr5q0t5l6qs37 +Content-Length:96 +Content-Type:application/json +Accept: application/json + +[{ + "kind": "type", + "subgraph": "invalidation-subgraph-type-accounts", + "type": "Query" +}] +``` + +The router would send the following response: + +``` +HTTP/1.1 200 OK +Content-Type: application/json + +{ + "count": 300 +} +``` + +The `count` field indicates the number of keys that were removed from Redis. + +#### Invalidation through subgraph response extensions + +A subgraph can return an `invalidation` array with invalidation requests in its response's `extensions` field. This can be used to invalidate entries in response to a mutation. + +```json +{ + "data": { "invalidateProductReview": 1 }, + "extensions": { + "invalidation": [{ + "kind": "entity", + "subgraph": "invalidation-entity-key-reviews", + "type": "Product", + "key": { + "upc": "1" + } + }] + } +} +``` + +#### Observability + +Invalidation requests are instrumented with the following metrics: +- `apollo.router.operations.entity.invalidation.event` - counter triggered when a batch of invalidation requests is received. It has a label `origin` that can be either `endpoint` or `extensions`. +- `apollo.router.operations.entity.invalidation.entry` - counter measuring how many entries are removed per `DEL` call. It has a label `origin` that can be either `endpoint` or `extensions`, and a label `subgraph.name` with the name of the receiving subgraph. +- `apollo.router.cache.invalidation.keys` - histogram measuring the number of keys that were removed from Redis per invalidation request. +- `apollo.router.cache.invalidation.duration` - histogram measuring the time spent handling one invalidation request. + +Invalidation requests are also reported under the following spans: +- `cache.invalidation.batch` - span covering the processing of a list of invalidation requests. It has a label `origin` that can be either `endpoint` or `extensions`. +- `cache.invalidation.request` - span covering the processing of a single invalidation request. + +#### Failure cases + +Entity caching will greatly reduce traffic to subgraphs. Should there be an availability issue with a Redis cache, this could cause traffic to subgraphs to increase to a level where infrastructure becomes overwhelmed. To avoid such issues, the router should be configured with [rate limiting for subgraph requests](/router/configuration/traffic-shaping/#rate-limiting-1) to avoid overwhelming the subgraphs. It could also be paired with [subgraph query deduplication](/router/configuration/traffic-shaping/#query-deduplication) to further reduce traffic. + +#### Scalability and performance + +The scalability and performance of entity cache invalidation is based on its implementation with the Redis [`SCAN` command](https://redis.io/docs/latest/commands/scan/). The `SCAN` command provides a cursor for iterating over the entire key space and returns a list of keys matching a pattern. When executing an invalidation request, the router first runs a series of `SCAN` calls and then it runs [`DEL`](https://redis.io/docs/latest/commands/del/) calls for any matching keys. + +The time complexity of a single invalidation request grows linearly with the number of entries, as each entry requires `SCAN` to iterate over. The router can also execute multiple invalidation requests simultaneously. This lowers latency but might increase the load on Redis instances. + +To help tune invalidation performance and scalability, you should benchmark the ratio of the invalidation rate against the number of entries that will be recorded. If it's too low, you can tune it with the following: +- Increase the number of pooled Redis connections. +- Increasing the `SCAN` count option. This shouldn't be too large, with 1000 as a generally reasonable value, because larger values will reduce the operation throughput of the Redis instance. +- Use separate Redis instances for some subgraphs. + ### Private information caching A subgraph can return a response with the header `Cache-Control: private`, indicating that it contains user-personalized data. Although this usually forbids intermediate servers from storing data, the router may be able to recognize different users and store their data in different parts of the cache. @@ -265,7 +447,3 @@ When used alongside the router's [authorization directives](./authorization), ca ### Schema updates and entity caching On schema updates, the router ensures that queries unaffected by the changes keep their cache entries. Queries with affected fields need to be cached again to ensure the router doesn't serve invalid data from before the update. - -### Entity cache invalidation not supported - -Cache invalidation is not yet supported and is planned for a future release. diff --git a/docs/source/configuration/telemetry/instrumentation/standard-instruments.mdx b/docs/source/configuration/telemetry/instrumentation/standard-instruments.mdx index 828a764f24..7f10f228c1 100644 --- a/docs/source/configuration/telemetry/instrumentation/standard-instruments.mdx +++ b/docs/source/configuration/telemetry/instrumentation/standard-instruments.mdx @@ -78,7 +78,7 @@ The coprocessor operations metric has the following attributes: -- `apollo_router_uplink_fetch_duration_seconds_bucket` - Uplink request duration, attributes: +- `apollo_router_uplink_fetch_duration_seconds` - Uplink request duration, attributes: - `url`: The Uplink URL that was polled - `query`: The query that the router sent to Uplink (`SupergraphSdl` or `License`) - `kind`: (`new`, `unchanged`, `http_error`, `uplink_error`) diff --git a/docs/source/federation-version-support.mdx b/docs/source/federation-version-support.mdx index 80ff912745..defc58f1c4 100644 --- a/docs/source/federation-version-support.mdx +++ b/docs/source/federation-version-support.mdx @@ -37,7 +37,15 @@ The table below shows which version of federation each router release is compile - v1.55.0 and later (see latest releases) + v1.56.0 and later (see latest releases) + + + 2.9.2 + + + + + v1.55.0 2.9.1 diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 797f895ed3..3b73906d4a 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -11,16 +11,12 @@ cargo-fuzz = true [dependencies] libfuzzer-sys = "0.4" -apollo-compiler.workspace = true apollo-parser.workspace = true apollo-smith.workspace = true env_logger = "0.11.0" log = "0.4" reqwest = { workspace = true, features = ["json", "blocking"] } serde_json.workspace = true -tokio.workspace = true -# note: this dependency should _always_ be pinned, prefix the version with an `=` -router-bridge = "=0.6.3+v2.9.2" [dev-dependencies] anyhow = "1" diff --git a/fuzz/subgraph/Cargo.toml b/fuzz/subgraph/Cargo.toml index 99c60ef6bf..130edb8e8f 100644 --- a/fuzz/subgraph/Cargo.toml +++ b/fuzz/subgraph/Cargo.toml @@ -8,10 +8,5 @@ axum = "0.6.20" async-graphql = "6" async-graphql-axum = "6" env_logger = "0.11" -futures = "0.3.17" -lazy_static = "1.4.0" -log = "0.4.16" -rand = { version = "0.8.5", features = ["std_rng"] } -serde_json = "1.0.79" tokio = { version = "1.22.0", features = ["time", "full"] } tower = "0.4.0" diff --git a/helm/chart/router/Chart.yaml b/helm/chart/router/Chart.yaml index 7a1c6d615a..9cbe1d5e12 100644 --- a/helm/chart/router/Chart.yaml +++ b/helm/chart/router/Chart.yaml @@ -20,10 +20,10 @@ type: application # so it matches the shape of our release process and release automation. # By proxy of that decision, this version uses SemVer 2.0.0, though the prefix # of "v" is not included. -version: 1.55.0 +version: 1.56.0 # This is the version number of the application being deployed. This version number should be # incremented each time you make changes to the application. Versions are not expected to # follow Semantic Versioning. They should reflect the version the application is using. # It is recommended to use it with quotes. -appVersion: "v1.55.0" +appVersion: "v1.56.0" diff --git a/helm/chart/router/README.md b/helm/chart/router/README.md index fb09765b10..181bb9602c 100644 --- a/helm/chart/router/README.md +++ b/helm/chart/router/README.md @@ -2,7 +2,7 @@ [router](https://github.com/apollographql/router) Rust Graph Routing runtime for Apollo Federation -![Version: 1.55.0](https://img.shields.io/badge/Version-1.55.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.55.0](https://img.shields.io/badge/AppVersion-v1.55.0-informational?style=flat-square) +![Version: 1.56.0](https://img.shields.io/badge/Version-1.56.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.56.0](https://img.shields.io/badge/AppVersion-v1.56.0-informational?style=flat-square) ## Prerequisites @@ -11,7 +11,7 @@ ## Get Repo Info ```console -helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.55.0 +helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.56.0 ``` ## Install Chart @@ -19,7 +19,7 @@ helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.55.0 **Important:** only helm3 is supported ```console -helm upgrade --install [RELEASE_NAME] oci://ghcr.io/apollographql/helm-charts/router --version 1.55.0 --values my-values.yaml +helm upgrade --install [RELEASE_NAME] oci://ghcr.io/apollographql/helm-charts/router --version 1.56.0 --values my-values.yaml ``` _See [configuration](#configuration) below._ diff --git a/licenses.html b/licenses.html index e415d536c4..764c755e37 100644 --- a/licenses.html +++ b/licenses.html @@ -44,7 +44,7 @@

Third Party Licenses

Overview of licenses:

    -
  • Apache License 2.0 (448)
    • +
  • Apache License 2.0 (447)
  • MIT License (155)
  • BSD 3-Clause "New" or "Revised" License (11)
  • ISC License (8)
    • @@ -5057,7 +5057,6 @@

    Used by:

    Apache License 2.0

    Used by:

    diff --git a/scripts/install.sh b/scripts/install.sh index f03b383d8d..1346bb9b5e 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -11,7 +11,7 @@ BINARY_DOWNLOAD_PREFIX="https://github.com/apollographql/router/releases/downloa # Router version defined in apollo-router's Cargo.toml # Note: Change this line manually during the release steps. -PACKAGE_VERSION="v1.55.0" +PACKAGE_VERSION="v1.56.0" download_binary() { downloader --check diff --git a/xtask/src/commands/release/process.rs b/xtask/src/commands/release/process.rs index 64ad9f36c2..ce76ada44b 100644 --- a/xtask/src/commands/release/process.rs +++ b/xtask/src/commands/release/process.rs @@ -488,7 +488,7 @@ impl Process { .status()?; //Step 15 - //FIXME: replace this step with an askama template + //FIXME: replace this step with a template let perl = which::which("perl")?; let output = std::process::Command::new(&perl) .args([ @@ -754,7 +754,7 @@ impl Process { println!("{}", style("Updating release notes").bold().bright()); // step 15 - //FIXME: replace this step with an askama template + //FIXME: replace this step with a template let perl = which::which("perl")?; let output = std::process::Command::new(&perl) .args([