Skip to content

Commit

Permalink
Move allow partial supergraph flag to OpenDD flags instead of an env …
Browse files Browse the repository at this point in the history
…var (#1205)

### What
Currently if you want v3-engine to accept metadata that is only a
partial supergraph (ie missing some subgraphs), you must set the
PARTIAL_SUPERGRAPH env var. Builds sent to MBS get sent to a special
endpoints `/validate/partial` and `/build/partial` that runs the engine
build process with that configuration option set.

This results in a terrible user experience for local builds, because MBS
will accept a partial supergraph and yield build artifacts, but
v3-engine will refuse to run them unless you set that env var.

This PR removes that env var and creates a new OpenDD flag called
`allow_partial_supergraph`. When MBS's `/validate/partial` endpoint is
used, that flag is set in the build artifacts. v3-engine then looks at
that flag to enable partial supergraph mode. This means a
`/build/partial` build via MBS just works when you run it locally via
v3-engine.

### How

`metadata_resolve`'s `Configuration.allow_unknown_subgraphs` has been
removed, and `metadata_resolve` now looks at OpenDD's new
`Flags.allow_partial_supergraph` instead.

In MBS, usage of `ValidationMode` (the enum that enables partial builds)
previously used to set `Configuration.allow_unknown_subgraphs`; now it
is used in `compute_open_dds_flags` in order to set
`Flags.allow_partial_supergraph`.

The existing metadata_resolve test that tested partial supergraphs has
been modified to use the flags rather than the removed configuration
option
(`crates/metadata-resolve/tests/passing/missing_subgraph_when_ignoring_unknown_subgraphs`).

A new `metadata_resolve` test has been added that checks that comparable
relationships in `BooleanExpressionType` properly respects the
`allow_partial_subgraphs` flag (this functionality was added in #1182).

V3_GIT_ORIGIN_REV_ID: 2c984eb791263a1fb0606c6c44a2a1ae4a5e7370
  • Loading branch information
daniel-chambers authored and hasura-bot committed Oct 7, 2024
1 parent 62d9cba commit 7c4d34d
Show file tree
Hide file tree
Showing 16 changed files with 3,840 additions and 42 deletions.
9 changes: 9 additions & 0 deletions v3/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,20 @@
builds the engine and serves it along with a sample schema using
`ndc-postgres` and a `postgres` database.

- Subgraph builds that have relationships to other external subgraphs can now be
run locally and no longer fail with missing subgraph errors. Subgraph builds
are marked with a new OpenDD flag and when these builds are run by the engine
relationships to unknown subgraphs are automatically pruned.

### Changed

- metadata-build-service POST endpoints now accept zstd (preferred) or gzip
-encoded request bodies

- The `--partial-supergraph` command-line argument and `PARTIAL_SUPERGRAPH`
environment variable have been removed. Builds now contain an OpenDD flag that
indicates if they are subgraph builds and should be run as such.

## [v2024.10.02]

### Added
Expand Down
5 changes: 0 additions & 5 deletions v3/crates/engine/bin/engine/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,6 @@ struct ServerOptions {
value_delimiter = ','
)]
cors_allow_origin: Vec<String>,
/// Allow unknown subgraphs, pruning relationships that refer to them.
/// Useful when working with part of a supergraph.
#[arg(long, env = "PARTIAL_SUPERGRAPH")]
partial_supergraph: bool,
/// List of internal unstable features to enable, separated by commas
#[arg(
long = "unstable-feature",
Expand Down Expand Up @@ -342,7 +338,6 @@ impl EngineRouter {
#[allow(clippy::print_stdout)]
async fn start_engine(server: &ServerOptions) -> Result<(), StartupError> {
let metadata_resolve_configuration = metadata_resolve::configuration::Configuration {
allow_unknown_subgraphs: server.partial_supergraph,
unstable_features: resolve_unstable_features(&server.unstable_features),
};

Expand Down
2 changes: 0 additions & 2 deletions v3/crates/engine/tests/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,6 @@ pub fn test_execute_explain(
enable_subscriptions: false,
enable_jsonapi: false,
},
..Default::default()
};
let gds = GDS::new(
open_dds::traits::OpenDd::deserialize(metadata, jsonpath::JSONPath::new())?,
Expand Down Expand Up @@ -508,7 +507,6 @@ pub fn test_execute_explain(
pub(crate) fn test_metadata_resolve_configuration() -> metadata_resolve::configuration::Configuration
{
metadata_resolve::configuration::Configuration {
allow_unknown_subgraphs: false,
unstable_features: metadata_resolve::configuration::UnstableFeatures {
enable_order_by_expressions: false,
enable_ndc_v02_support: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
}
},
{
"version": "v1",
"kind": "ObjectType",
"version": "v1",
"definition": {
"name": "Track",
"fields": [
Expand Down Expand Up @@ -310,8 +310,8 @@
}
},
{
"version": "v1",
"kind": "Model",
"version": "v1",
"definition": {
"name": "Tracks",
"objectType": "Track",
Expand Down
1 change: 0 additions & 1 deletion v3/crates/graphql-ws/tests/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ pub(crate) async fn start_websocket_server() -> TestServer {
let raw_metadata = std::fs::read_to_string(metadata_path).unwrap();
let metadata = open_dds::Metadata::from_json_str(&raw_metadata).unwrap();
let metadata_resolve_configuration = metadata_resolve::configuration::Configuration {
allow_unknown_subgraphs: false,
unstable_features: metadata_resolve::configuration::UnstableFeatures {
enable_subscriptions: true,
..Default::default()
Expand Down
5 changes: 1 addition & 4 deletions v3/crates/jsonapi/tests/jsonapi_golden_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,5 @@ fn get_metadata_resolve_configuration() -> metadata_resolve::configuration::Conf
enable_jsonapi: true,
};

metadata_resolve::configuration::Configuration {
allow_unknown_subgraphs: false,
unstable_features,
}
metadata_resolve::configuration::Configuration { unstable_features }
}
8 changes: 2 additions & 6 deletions v3/crates/metadata-resolve/src/stages/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,8 @@ pub fn resolve(
type_permissions::resolve(&metadata_accessor, object_types).map_err(Error::from)?;

// collect raw relationships information
let relationships = relationships::resolve(
configuration,
&metadata_accessor,
&object_types_with_permissions,
)
.map_err(Error::from)?;
let relationships = relationships::resolve(&metadata_accessor, &object_types_with_permissions)
.map_err(Error::from)?;

// Resolve fancy new boolean expression types
let boolean_expressions::BooleanExpressionsOutput {
Expand Down
19 changes: 13 additions & 6 deletions v3/crates/metadata-resolve/src/stages/relationships/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
mod error;
mod types;
use super::type_permissions;
use crate::{configuration::Configuration, helpers, types::subgraph::Qualified};
use crate::{helpers, types::subgraph::Qualified};
pub use error::RelationshipError;
use open_dds::{
identifier::SubgraphName,
Expand All @@ -14,7 +14,6 @@ pub use types::{Relationship, Relationships};
// This stage only collects relationships and collects them by subgraph,
// the `object_relationships` stage resolves them meaningfully in the context of their objects
pub fn resolve<'s>(
configuration: &Configuration,
metadata_accessor: &'s open_dds::accessor::MetadataAccessor,
object_types_with_permissions: &type_permissions::ObjectTypesWithPermissions,
) -> Result<Relationships<'s>, RelationshipError> {
Expand Down Expand Up @@ -43,7 +42,11 @@ pub fn resolve<'s>(
if let Some(existing_relationship) = relationships.get_mut(&qualified_type_name) {
let result = existing_relationship.insert(
relationship.name.clone(),
mk_relationship(configuration, &metadata_accessor.subgraphs, relationship),
mk_relationship(
&metadata_accessor.flags,
&metadata_accessor.subgraphs,
relationship,
),
);

// explode if we find duplicates for a name
Expand All @@ -57,7 +60,11 @@ pub fn resolve<'s>(
let mut inner_map = BTreeMap::new();
inner_map.insert(
relationship.name.clone(),
mk_relationship(configuration, &metadata_accessor.subgraphs, relationship),
mk_relationship(
&metadata_accessor.flags,
&metadata_accessor.subgraphs,
relationship,
),
);
relationships.insert(qualified_type_name, inner_map);
};
Expand All @@ -67,12 +74,12 @@ pub fn resolve<'s>(
}

fn mk_relationship<'s>(
configuration: &Configuration,
flags: &open_dds::flags::Flags,
known_subgraphs: &HashSet<SubgraphName>,
relationship: &'s RelationshipV1,
) -> Relationship<'s> {
// If we have allowed the usage of unknown subgraphs...
if configuration.allow_unknown_subgraphs {
if flags.allow_partial_supergraph {
let subgraph = helpers::relationship::get_target_subgraph(relationship);

// ...and the subgraph is unknown
Expand Down
1 change: 0 additions & 1 deletion v3/crates/metadata-resolve/src/types/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
#[derive(Debug, Clone, Default, serde::Deserialize)]
#[serde(default, deny_unknown_fields, rename_all = "camelCase")]
pub struct Configuration {
pub allow_unknown_subgraphs: bool,
pub unstable_features: UnstableFeatures,
}

Expand Down
10 changes: 2 additions & 8 deletions v3/crates/metadata-resolve/tests/metadata_golden_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,8 @@ fn read_test_configuration(
if configuration_path.exists() {
let reader = fs::File::open(configuration_path)?;
let configuration = serde_json::from_reader(reader)?;
Ok(configuration::Configuration {
unstable_features,
..configuration
})
Ok(configuration)
} else {
Ok(configuration::Configuration {
allow_unknown_subgraphs: false,
unstable_features,
})
Ok(configuration::Configuration { unstable_features })
}
}
Loading

0 comments on commit 7c4d34d

Please sign in to comment.