GraphQL API Versioning #3155

kamilkisiela · 2022-01-13T13:56:59Z

This is the initial implementation of versioning.

To do:

resolve version of WebSocket connections
switch from header-based approach to url-based
make sure we use default (latest) version correctly
remove the example implementation of a feature flag

I left few comments on the interesting parts of the code to make it easier to review.

store/postgres/src/deployment_store.rs

kamilkisiela · 2022-01-14T11:57:55Z

server/websocket/src/server.rs

            }
+            &["subgraphs", "name", _] | &["subgraphs", "name", _, _] => Ok(state(
+                store,
+                target_from_name(path_segments[2..].join("/"), Default::default()), // TODO: version


resolve version of WebSocket connections

kamilkisiela · 2022-01-14T11:58:11Z

store/postgres/src/deployment_store.rs

@@ -275,7 +276,9 @@ impl DeploymentStore {
        // if that's Fred the Dog, Fred the Cat or both.
        //
        // This assumes that there are no concurrent writes to a subgraph.
-        let schema = self.subgraph_info_with_conn(&conn, &layout.site)?.api;
+        let schema = self
+            .subgraph_info_with_conn(&conn, &layout.site, &Default::default())? // TODO: ask if we need a non-default version here


Do we need a version here? Is default (latest) enough?

kamilkisiela · 2022-01-14T11:58:32Z

store/postgres/src/deployment_store.rs

@@ -923,7 +931,7 @@ impl DeploymentStore {
    ) -> Result<StoreEvent, StoreError> {
        let event = conn.transaction(|| -> Result<_, StoreError> {
            // Don't revert past a graft point
-            let info = self.subgraph_info_with_conn(&conn, site.as_ref())?;
+            let info = self.subgraph_info_with_conn(&conn, site.as_ref(), &Default::default())?; // TODO: ask if we need a non-default version here


Do we need a version here? Is default (latest) enough?

kamilkisiela · 2022-01-14T11:59:27Z

store/postgres/src/subgraph_store.rs

        };

-        let (store, site) = self.store(&id)?;
+        let (store, site) = self.store(&id)?; // TODO: maybe use a tuple (id, version) here?


not sure if we want to get the store based on id or (id, version).

kamilkisiela · 2022-01-14T11:59:35Z

store/postgres/src/subgraph_store.rs

@@ -858,7 +858,7 @@ impl SubgraphStoreInner {
                .ok_or_else(|| constraint_violation!("no chain info for {}", deployment_id))?;
            let latest_ethereum_block_number =
                chain.latest_block.as_ref().map(|ref block| block.number());
-            let subgraph_info = store.subgraph_info(site.as_ref())?;
+            let subgraph_info = store.subgraph_info(site.as_ref(), &Default::default())?; // TODO: ask if we need to use non-default version here


Do we need a version here? Is default (latest) enough?

kamilkisiela · 2022-01-14T11:59:46Z

store/postgres/src/subgraph_store.rs

@@ -1022,13 +1022,18 @@ impl SubgraphStoreTrait for SubgraphStore {

    fn input_schema(&self, id: &DeploymentHash) -> Result<Arc<Schema>, StoreError> {
        let (store, site) = self.store(&id)?;
-        let info = store.subgraph_info(site.as_ref())?;
+        // TODO: ask if we need to use non-default version here, I think we don't because it's schema provided by user
+        let info = store.subgraph_info(site.as_ref(), &Default::default())?;


Do we need a version here? Is default (latest) enough?

I think we don't because it's schema provided by user

kamilkisiela · 2022-01-14T12:00:21Z

graph/src/components/versions/features.rs

+#[derive(Clone, PartialEq)]
+pub enum FeatureFlag {
+    // A description of the feature. Give it a little context.
+    BasicOrdering,


An example flag, just to demo the versioning.

kamilkisiela · 2022-01-14T12:01:15Z

graph/src/components/versions/registry.rs

+        let supported_versions: Vec<(&str, Vec<FeatureFlag>)> = vec![
+            // baseline version
+            ("1.0.0", vec![]),
+            // Versions with feature flags
+            ("1.1.0", vec![FeatureFlag::BasicOrdering])
+        ];


This is something we need to maintain. It's the single source of truth.

kamilkisiela · 2022-01-14T12:02:04Z

graph/src/components/versions/registry.rs

+    static ref LATEST_VERSION: String = {
+        let keys: Vec<VersionNumber> = VERSIONS.clone().into_keys().collect();
+
+        let last_version = keys.last().unwrap();
+
+        last_version.0.clone()
+    };
+}


Latest version is the last key of VERSIONS.

kamilkisiela · 2022-01-14T12:02:51Z

graph/src/components/versions/registry.rs

+    pub fn validate(version: String) -> Result<(), String> {
+        let chunks: Vec<&str> = version.split(".").collect();
+
+        if chunks.len() != 3 {
+            return Err(format!("Invalid version number: {}", version));
+        }
+
+        let major = chunks[0].parse::<u32>();
+        let minor = chunks[1].parse::<u32>();
+        let patch = chunks[2].parse::<u32>();
+
+        if major.is_err() || minor.is_err() || patch.is_err() {
+            return Err(format!("Invalid version number: {}", version));
+        }
+
+        if !VERSIONS.contains_key(&VersionNumber::from(version)) {
+            return Err("No versions found".to_string());
+        }
+
+        Ok(())
+    }
+}


Validation happens in http and websocket part of the server.

kamilkisiela · 2022-01-14T12:04:03Z

graph/src/data/query/query.rs

+impl QueryTarget {
+    pub fn get_version_number(&self) -> VersionNumber {
+        match self {
+            Self::Name(_, version) => version.clone(),
+            Self::Deployment(_, version) => version.clone(),
+        }
    }
-}


I tried to look for the best place to put the requested version and QueryTarget won the contest.

graphql/src/runner.rs

kamilkisiela · 2022-01-14T12:05:02Z

graphql/src/schema/api.rs

+    if version.supports(FeatureFlag::BasicOrdering) {
+        add_order_direction_enum(&mut schema);
+    }


An example implementation of a feature flag.

kamilkisiela · 2022-01-14T12:07:01Z

server/http/src/service.rs

+    fn resolve_version_number(
+        &self,
+        request: &Request<Body>,
+    ) -> Result<VersionNumber, GraphQLServerError> {
+        let version_header = request.headers().get("api-version");
+
+        let mut version = VersionNumber::default();
+
+        if let Some(header_value) = version_header {
+            let version_value = header_value.to_str().map_err(|_| {
+                GraphQLServerError::ClientError(format!(
+                    "Invalid api-version header value {:?}",
+                    header_value
+                ))
+            })?;
+            VersionNumber::validate(version_value.to_string())
+                .map_err(|error| GraphQLServerError::ClientError(error))?;
+
+            version = version_value.into();
+        }
+
+        Ok(version)
+    }


I used api-version header because it was the easiest to implement quickly but before merging the Pull Request we should switch to url-based approach (this way we could use GraphiQL - it does not support headers).

kamilkisiela · 2022-01-14T12:07:29Z

server/index-node/src/schema.graphql

@@ -19,6 +19,7 @@ type Query {
    indexer: Bytes
  ): Bytes
  subgraphFeatures(subgraphId: String!): SubgraphFeatures!
+  subgraphVersions(subgraphId: String!): [SubgraphVersion!]!


This is for the Gateway.

kamilkisiela · 2022-01-14T12:08:22Z

server/index-node/src/schema.graphql

+type SubgraphVersion {
+  """
+  Version number in SemVer format
+  """
+  version: String!
+}


Why an object instead of a list? It's extendable and harder to break backwards-compatibility.

kamilkisiela · 2022-01-14T12:08:44Z

server/websocket/src/connection.rs

@@ -313,7 +313,7 @@ where
                    };

                    // Construct a subscription
-                    let target = QueryTarget::Deployment(deployment.clone());
+                    let target = QueryTarget::Deployment(deployment.clone(), Default::default()); // TODO: resolve and validate a version number from WebSocket connection


resolve version of WebSocket connections

Query preprocessing eliminated all variable references

That makes it possible to use the same schema for data nad for introspection queries

The old name wasn't matching what we use it for any more

No cows were harmed in making this change

That is demanded by the GraphQL spec. We need to have a predictable order to ensure attestations remain stable, though this change breaks attestations since it changes the order in which fields appear in the output from alphabetical (however a `BTreeMap` orders string keys) to the order in which fields appear in a query. It also allows us to replace `BTreeMaps`, which are fairly memory intensive, with cheaper `Vec`. The test changes all reflect the changed output behavior; they only reorder fields in the expected output but do not otherwise alter the tests. Fixes #2943

Fixes #2720

Rather than use a string name, use the actual object type to identify types. It's not possible to do this with plain references, for example, because we pass a reference to a SelectionSet to graph::spawn_blocking, so we do the next best thing and use an Arc. Unfortunately, because `graphql_parser` doesn't wrap its object types in an Arc, that means we need to keep a copy of all of them in ApiSchema.

This just shuffles some code around, but doesn't change anything else, in preparation for representing the schema in a way that's more useful to us.

Set ENABLE_GRAPHQL_VALIDATIONS to any value in the environment to enable validations, rather than enabling them by default and disabling them on demand

Make sure that we handle queries that have a selection from a scalar field by ignoring the selection or that have no selection for a non-scalar field by ignoring that field. The latter differs from the previous behavior where the result would contain an entry for such a field, but the data for the field would be an empty object or a list of empty objects.

Instead of immediately reporting an error, treat missing variables as nulls and let the rest of the execution logic deal with nulls

kamilkisiela force-pushed the kamil-versioning branch from c08d3e5 to 6273c09 Compare January 13, 2022 14:14

kamilkisiela commented Jan 14, 2022

View reviewed changes

store/postgres/src/deployment_store.rs Outdated Show resolved Hide resolved

kamilkisiela commented Jan 14, 2022

View reviewed changes

store/postgres/src/deployment_store.rs Outdated Show resolved Hide resolved

kamilkisiela force-pushed the kamil-versioning branch from f2b6739 to 11971fa Compare January 14, 2022 11:56

kamilkisiela commented Jan 14, 2022

View reviewed changes

graphql/src/runner.rs Outdated Show resolved Hide resolved

kamilkisiela commented Jan 14, 2022

View reviewed changes

dotansimha requested review from lutter and That3Percent January 18, 2022 09:17

dotansimha changed the title ~~Versioning~~ GraphQL API Versioning Jan 18, 2022

lutter force-pushed the lutter/gql branch 2 times, most recently from c98485d to 3727335 Compare January 20, 2022 00:58

lutter added 4 commits January 20, 2022 12:15

graph, graphql, server: Encapsulate maps in Value in a new type

4943e71

graphql: Move complexity check/field validation into intermediate struct

4b5ca34

graph, graphql: Resolve variables before executing query

f4185c1

graphql: Do not pass variables around during execution

baf6ce4

Query preprocessing eliminated all variable references

lutter added 22 commits January 20, 2022 12:15

graphql: Mix the introspection schema into the API schema

21388fd

That makes it possible to use the same schema for data nad for introspection queries

graphql: Simplify constructing an ExecutionContext for introspection

fa9e165

graphql: Transform the query before executing it

ea4e887

graphql: Clean up imports in schema::ast

a9ece31

graphql: Rename ObjectCondition to ObjectType

24a6555

The old name wasn't matching what we use it for any more

graphql: Simplify CollectedAttributeNames and rename it

ab23241

graphql: Remove unused CollectedResponseKey

a909d10

graphql: Simplify Join type in prefetch a little

b0aa676

graphql: Pass a schema, not the query, to coerce_argument_values

66a6aaa

graphql: Coerce arguments when constructing the query

b0026b7

graphql: Keep original field order when grouping by block constraint

1777fb1

core: Do not rely on debug output in interface tests

b8b6a68

No cows were harmed in making this change

graphql: Verify that root fragments are expanded

3bf9aab

Fixes #2720

graph, graphql: Encapsulate access to the underlying schema in ApiSchema

ab280d2

This just shuffles some code around, but doesn't change anything else, in preparation for representing the schema in a way that's more useful to us.

graphql: Remove unused functions

1bb7a36

graphql: Clarify/fix some comments

95ee91a

graphql: Make sure that undefined arguments are ignored

081480a

graphql: Change the default for running validations

528d6ae

Set ENABLE_GRAPHQL_VALIDATIONS to any value in the environment to enable validations, rather than enabling them by default and disabling them on demand

graphql: Treat missing variables as null values

4c4a261

Instead of immediately reporting an error, treat missing variables as nulls and let the rest of the execution logic deal with nulls

lutter force-pushed the lutter/gql branch from 3727335 to 4c4a261 Compare January 20, 2022 20:15

kamilkisiela force-pushed the kamil-versioning branch from 558ad33 to 291a675 Compare January 24, 2022 11:37

Versioning

306a886

kamilkisiela force-pushed the kamil-versioning branch from 291a675 to 306a886 Compare January 24, 2022 11:52

lutter force-pushed the lutter/gql branch from 4c4a261 to 9e6435b Compare January 24, 2022 23:42

lutter deleted the branch lutter/gql January 24, 2022 23:43

lutter closed this Jan 24, 2022

kamilkisiela mentioned this pull request Jan 25, 2022

all: GraphQL API Versioning #3185

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL API Versioning #3155

GraphQL API Versioning #3155

kamilkisiela commented Jan 13, 2022 •

edited

Loading

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022 •

edited

Loading

kamilkisiela Jan 14, 2022 •

edited

Loading

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

kamilkisiela Jan 14, 2022

GraphQL API Versioning #3155

GraphQL API Versioning #3155

Conversation

kamilkisiela commented Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

kamilkisiela Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

kamilkisiela Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilkisiela commented Jan 13, 2022 •

edited

Loading

kamilkisiela Jan 14, 2022 •

edited

Loading

kamilkisiela Jan 14, 2022 •

edited

Loading