all: GraphQL API Versioning #3185

kamilkisiela · 2022-01-25T09:09:57Z

This is the initial implementation of versioning.

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.

Continues #3155

server/websocket/src/server.rs

store/postgres/src/deployment_store.rs

store/postgres/src/subgraph_store.rs

kamilkisiela · 2022-01-25T09:12:40Z

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.

graph/src/components/versions/registry.rs

graph/src/data/query/query.rs

kamilkisiela · 2022-01-25T09:13:51Z

graphql/src/schema/api.rs

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


An example implementation of a feature flag.

server/http/src/service.rs

server/index-node/src/schema.graphql

server/websocket/src/connection.rs

graphql/src/schema/api.rs

evaporei · 2022-05-11T15:41:06Z

@kamilkisiela is this ready to review? I see that the CI is failing.

kamilkisiela · 2022-05-11T16:00:46Z

@evaporei yes, the code won't change much even after a rebase. I've got some questions both in code and gh comments, because I'm not sure if it's the right implementation.

graph/src/data/query/query.rs

lutter · 2022-05-16T15:25:42Z

graphql/src/runner.rs

@@ -154,10 +154,11 @@ where
        // while the query is running. `self.store` can not be used after this
        // point, and everything needs to go through the `store` we are
        // setting up here
-        let store = self.store.query_store(target, false).await?;
+
+        let store = self.store.query_store(target.clone(), false).await?;


I think the query store should 'bind' to the version in the same way it binds to the deployment hash, so that api_schema() would always return the schema for that version. That makes it less likely that versions get switched during query execution. And the QueryStore is only used for the duration of executing one query.

server/index-node/src/schema.graphql

server/websocket/src/connection.rs

store/postgres/src/subgraph_store.rs

lutter · 2022-05-17T10:35:08Z

store/postgres/src/deployment_store.rs

@@ -87,7 +87,7 @@ pub struct StoreInner {
    conn_round_robin_counter: AtomicUsize,

    /// A cache of commonly needed data about a subgraph.
-    subgraph_cache: Mutex<LruCache<DeploymentHash, SubgraphInfo>>,
+    subgraph_cache: Mutex<LruCache<(DeploymentHash, VersionNumber), SubgraphInfo>>,


How about keeping DeploymentHash as the cache key and changing SubgraphInfo.api to be a HashMap<Version, Arc<ApiSchema>>? As long as there's only a few API versions, we could even populate that HashMap eagerly; once there are more versions, we might want to populate entries there on demand.

That would make it possible to not pass in a version to subgraph_info and only use it when we actually need an ApiSchema in the QueryStore

azf20 · 2022-06-14T15:41:29Z

hey @kamilkisiela have you got further updates here? just checking that you are not blocked

kamilkisiela · 2022-06-14T15:45:18Z

@azf20 no updates, I'm trying to implement support for interfaces in #3184 and once it's ready, I'll apply David's comments here.

kamilkisiela · 2022-06-20T11:54:10Z

@lutter ready for a review again, I implemented everything you asked for.

Those two failing tests (just the order of fields) are related to the fact I moved the ordering logic behind a feature flag to show what the versioning will look like. Once I remove the example feature flag, it will be fine.

kamilkisiela · 2022-06-20T11:55:57Z

@azf20 I read the GIP and applied some changes to the PR. When we get a version range, the semver package resolves to the most recent applicable version.

lutter

The main thing we need to clear up is what the initial version(s) should be

lutter · 2022-07-26T13:18:06Z

graph/src/components/versions/registry.rs

+        let mut map = HashMap::new();
+
+        // Sort version by major, minor, patch, from higher to lower.
+        supported_versions.sort_by(|a, b| b.0.partial_cmp(&a.0).unwrap());


Is this sort necessary? They all go into a HashMap afterwards, so doesn't seem like it's needed.

I did a refactor, now only the vector is sorted

lutter · 2022-07-26T13:21:35Z

graph/src/components/versions/registry.rs

+        let keys: Vec<Version> = VERSIONS.clone().into_keys().collect();
+
+        // Versions are sorted by major, minor, patch, from higher to lower.
+        keys.first().unwrap().to_string()


This is wrong HashMap.into_keys will return keys in arbitrary order. BTreeMap would work, but I think first would return the smallest version, so either reverse the iterator or put the keys into a Vec and sort that

I did a refactor and it's now using VERSION_COLLECTION: HashMap<Version, Vec<FeatureFlag>> to get the feature flags and VERSIONS: Vec<&'static Version> (sorted from latest to oldest version, higher -> lower).

The VERSIONS vector is sorted because of the logic in resolve() function.

lutter · 2022-07-26T13:23:29Z

graph/src/components/versions/registry.rs

+
+impl ApiVersion {
+    pub fn new(version_requirement: &VersionReq) -> Result<Self, String> {
+        let version = Self::resolve(&version_requirement);


It would be cleaner if resolve returned Result<Self, String> instead of an Option

It does now, I did a refactor

lutter · 2022-07-26T13:26:23Z

graphql/src/schema/api.rs

+        ),
+    ];
+
+    if version.supports(FeatureFlag::BasicOrdering) {


I am losing track of the plot a little here - should we really have a version without ordering?

No no, it's just an example of how to use the feature flags
#3185 (comment)

kamilkisiela · 2022-07-27T09:17:38Z

@lutter I did a refactor, it includes your suggestions.

lutter

Looks good now!

@azf20 Are you ok with the base version being 1.0.0? All fine by me, just wanted to check that that's where we should start.

azf20 · 2022-07-27T15:29:12Z

Thanks @kamilkisiela and @lutter! I think starting with 1.0.0 is ok, but also interested in @That3Percent's expectations of a 1.x release

That3Percent · 2022-07-28T16:39:38Z

I think starting with 1.0.0 is ok, but also interested in @That3Percent's expectations of a 1.x release

I also think a 1.0.0 release is a good choice. Even if there are changes we know we would like to make, that's always the case so it's better to have a 1.0 and 2.0 than 0.x in perpetuity.

That3Percent

Currently the protocol expects version "0" to exist so that we can be backward-compatible with the existing attestation format. As implemented, graph-node is only supporting 1.0.0 here.

I think this is probably fine, as long as we default to 1.0 upstream from here (indexer agent, gateway) and accept that version "0" should never be queried directly (it would be a historical relic). Thoughts?

azf20 · 2022-08-12T13:24:33Z

Agree that this approach seems fine, and I think the GIP already allows for that kind of minimum version specification

azf20 · 2022-08-22T17:50:42Z

Hey @kamilkisiela I think this is good to merge, though looks like there are a couple of conflicts first! cc @lutter