Avoid roundtripping into JsonValue when (de)serializing schemas (#36694)

This links up the various `*Json` types (eg. ValidatorJson, TableDefinitionJson, ..) to include
each other directly instead of indirecting via `JsonValue` - which requires a recursive copy every
time that we call `serde_json::from_value`. Now we do JSON parsing into a typed data structure
in one step, then convert the entire thing into the "nice" data structure in a second step.

I've also added a new trait `JsonSerializable` to abstract over the `*Json` types to make the pattern clearer.

I believe the old code had quadratic time complexity wrt. nesting depth and was a source of perf
problems with schema enforcement.

GitOrigin-RevId: 52ec985991a68a5a84fb85d403d096a91b3b6e61

Author: goffrie

crates/common/src/bootstrap_model/components/definition.rs

@@ -23,6 +23,7 @@ use crate::{
         ComponentName,
         Reference,
     },
+    json::JsonSerializable as _,
     schemas::validator::Validator,
 };
 
@@ -338,7 +339,7 @@ impl TryFrom<ComponentArgumentValidator> for SerializedComponentArgumentValidato
     fn try_from(r: ComponentArgumentValidator) -> anyhow::Result<Self> {
         Ok(match r {
             ComponentArgumentValidator::Value(v) => SerializedComponentArgumentValidator::Value {
-                value: serde_json::to_string(&JsonValue::try_from(v)?)?,
+                value: v.json_serialize()?,
             },
         })
     }
@@ -350,9 +351,7 @@ impl TryFrom<SerializedComponentArgumentValidator> for ComponentArgumentValidato
     fn try_from(r: SerializedComponentArgumentValidator) -> anyhow::Result<Self> {
         Ok(match r {
             SerializedComponentArgumentValidator::Value { value: v } => {
-                ComponentArgumentValidator::Value(Validator::try_from(serde_json::from_str::<
-                    JsonValue,
-                >(&v)?)?)
+                ComponentArgumentValidator::Value(Validator::json_deserialize(&v)?)
             },
         })
     }

crates/common/src/bootstrap_model/schema_metadata.rs

@@ -2,14 +2,16 @@ use serde::{
     Deserialize,
     Serialize,
 };
-use serde_json::Value as JsonValue;
 use value::codegen_convex_serialization;
 
 use super::schema_state::{
     SchemaState,
     SerializedSchemaState,
 };
-use crate::schemas::DatabaseSchema;
+use crate::{
+    json::JsonSerializable,
+    schemas::DatabaseSchema,
+};
 
 #[derive(Debug, Clone, PartialEq)]
 #[cfg_attr(any(test, feature = "testing"), derive(proptest_derive::Arbitrary))]
@@ -20,13 +22,11 @@ pub struct SchemaMetadata {
 
 impl SchemaMetadata {
     pub fn database_schema(&self) -> anyhow::Result<DatabaseSchema> {
-        let deserialized_value: JsonValue = serde_json::from_str(&self.raw_schema)?;
-        DatabaseSchema::try_from(deserialized_value)
+        DatabaseSchema::json_deserialize(&self.raw_schema)
     }
 
     pub fn new(state: SchemaState, schema: DatabaseSchema) -> anyhow::Result<Self> {
-        let json_schema: JsonValue = schema.try_into()?;
-        let raw_schema = serde_json::to_string(&json_schema)?;
+        let raw_schema = schema.json_serialize()?;
         Ok(Self { state, raw_schema })
     }
 }

crates/common/src/json/mod.rs

@@ -1,12 +1,62 @@
+use std::fmt::Debug;
+
+use anyhow::Context as _;
 use errors::ErrorMetadata;
 
 mod expression;
 mod query;
 pub use expression::JsonExpression;
+use serde::{
+    de::DeserializeOwned,
+    Serialize,
+};
 
 #[cfg(test)]
 mod tests;
 
 pub fn invalid_json() -> ErrorMetadata {
     ErrorMetadata::bad_request("InvalidJson", "Invalid JSON")
 }
+
+/// The standard serde idiom is to directly implement `Serialize` and
+/// `Deserialize` on types to make them JSON-serializable.
+/// However, this has some downsides, e.g. making it hard to let the JSON & Rust
+/// structures differ from each other, and it also means that any custom
+/// validation logic has to fit into Serde's error model - which collapses our
+/// typed `ErrorMetadata` errors into strings.
+///
+/// Instead we have a pattern of defining our Rust types, and then defining
+/// parallel "*Json" types that implement Serialize/Deserialize (usually
+/// derived); this does the first layer of serialization; and then we have
+/// TryFrom impls in both directions to do any final validation steps.
+///
+/// This trait makes it possible to name those "*Json" types uniformly.
+pub trait JsonSerializable
+where
+    Self: TryFrom<<Self as JsonSerializable>::Json>,
+    anyhow::Error: From<<Self as TryFrom<<Self as JsonSerializable>::Json>>::Error>
+        + From<<<Self as JsonSerializable>::Json as TryFrom<Self>>::Error>,
+{
+    type Json: Serialize + DeserializeOwned + TryFrom<Self> + Debug;
+
+    fn json_serialize(self) -> anyhow::Result<String> {
+        Ok(serde_json::to_string(&<Self::Json>::try_from(self)?)?)
+    }
+
+    fn json_deserialize(s: &str) -> anyhow::Result<Self> {
+        Ok(serde_json::from_str::<Self::Json>(s)
+            .with_context(invalid_json)?
+            .try_into()?)
+    }
+
+    /// Deserialize from a `serde_json::Value`.
+    ///
+    /// This should generally not be preferred because `serde_json::Value` is a
+    /// very dynamic and expensive type to construct. Prefer using `Self::Json`
+    /// instead.
+    fn json_deserialize_value(s: serde_json::Value) -> anyhow::Result<Self> {
+        Ok(serde_json::from_value::<Self::Json>(s)
+            .with_context(invalid_json)?
+            .try_into()?)
+    }
+}