From 10fdfb7455d3941b32c67c45d914029771751229 Mon Sep 17 00:00:00 2001
From: Samir Talwar <samir.talwar@hasura.io>
Date: Thu, 8 Aug 2024 10:41:45 +0200
Subject: [PATCH] Replace the health check with a readiness check.

The NDC specification states that the health check should respond with
`200 OK` when the connector is ready to start accepting requests (i.e.
"readiness").

Whether the underlying data source is up and running is not important
here.

This adds a default implementation to the method to make it trivial for
connector authors to handle this particular request. The method name has
been changed to clarify matters and also to force connector authors to
understand the change.

We will consider adding further endpoints to check "liveness" and
"connectedness" in the future.
---
 crates/sdk/src/connector.rs         | 15 +++++++++++----
 crates/sdk/src/connector/example.rs |  7 -------
 crates/sdk/src/default_main.rs      | 17 ++++++++++-------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/crates/sdk/src/connector.rs b/crates/sdk/src/connector.rs
index 67107f3..f8cbe08 100644
--- a/crates/sdk/src/connector.rs
+++ b/crates/sdk/src/connector.rs
@@ -38,7 +38,7 @@ pub use error::*;
 /// a connection string would be configuration, but a connection pool object
 /// created from that connection string would be state.
 #[async_trait]
-pub trait Connector {
+pub trait Connector: Send {
     /// The type of validated configuration
     type Configuration: Sync + Send;
     /// The type of unserializable state
@@ -55,9 +55,16 @@ pub trait Connector {
 
     /// Check the health of the connector.
     ///
-    /// For example, this function should check that the connector
-    /// is able to reach its data source over the network.
-    async fn health_check(configuration: &Self::Configuration, state: &Self::State) -> Result<()>;
+    /// This should simply verify that the connector is ready to start accepting
+    /// requests. It should not verify that external data sources are available.
+    ///
+    /// For most use cases, the default implementation should be fine.
+    async fn get_health_readiness(
+        _configuration: &Self::Configuration,
+        _state: &Self::State,
+    ) -> Result<()> {
+        Ok(())
+    }
 
     /// Get the connector's capabilities.
     ///
diff --git a/crates/sdk/src/connector/example.rs b/crates/sdk/src/connector/example.rs
index 1ee2a21..fae3a71 100644
--- a/crates/sdk/src/connector/example.rs
+++ b/crates/sdk/src/connector/example.rs
@@ -38,13 +38,6 @@ impl Connector for Example {
         Ok(())
     }
 
-    async fn health_check(
-        _configuration: &Self::Configuration,
-        _state: &Self::State,
-    ) -> Result<()> {
-        Ok(())
-    }
-
     async fn get_capabilities() -> models::Capabilities {
         models::Capabilities {
             relationships: None,
diff --git a/crates/sdk/src/default_main.rs b/crates/sdk/src/default_main.rs
index 9fef731..d2e04c4 100644
--- a/crates/sdk/src/default_main.rs
+++ b/crates/sdk/src/default_main.rs
@@ -7,7 +7,7 @@ use axum::{
     http::{HeaderValue, Request, StatusCode},
     response::IntoResponse as _,
     routing::{get, post},
-    Json, Router,
+    Json,
 };
 use axum_extra::extract::WithRejection;
 use clap::{Parser, Subcommand};
@@ -293,25 +293,28 @@ pub async fn init_server_state<Setup: ConnectorSetup>(
     Ok(ServerState::new(configuration, state, metrics))
 }
 
-pub fn create_router<C>(state: ServerState<C>, service_token_secret: Option<String>) -> Router
+pub fn create_router<C>(
+    state: ServerState<C>,
+    service_token_secret: Option<String>,
+) -> axum::Router<()>
 where
     C: Connector + 'static,
     C::Configuration: Clone,
     C::State: Clone,
 {
-    Router::new()
+    axum::Router::new()
         .route("/capabilities", get(get_capabilities::<C>))
-        .route("/health", get(get_health::<C>))
         .route("/metrics", get(get_metrics::<C>))
         .route("/schema", get(get_schema::<C>))
         .route("/query", post(post_query::<C>))
         .route("/query/explain", post(post_query_explain::<C>))
         .route("/mutation", post(post_mutation::<C>))
         .route("/mutation/explain", post(post_mutation_explain::<C>))
-        .with_state(state)
         .layer(ValidateRequestHeaderLayer::custom(auth_handler(
             service_token_secret,
         )))
+        .route("/health", get(get_health_readiness::<C>)) // health checks are not authenticated
+        .with_state(state)
         .layer(
             TraceLayer::new_for_http()
                 .make_span_with(make_span)
@@ -382,8 +385,8 @@ async fn get_capabilities<C: Connector>() -> JsonResponse<CapabilitiesResponse>
     .into()
 }
 
-async fn get_health<C: Connector>(State(state): State<ServerState<C>>) -> Result<()> {
-    C::health_check(&state.configuration, &state.state).await
+async fn get_health_readiness<C: Connector>(State(state): State<ServerState<C>>) -> Result<()> {
+    C::get_health_readiness(&state.configuration, &state.state).await
 }
 
 async fn get_schema<C: Connector>(