improve merging of strings and numbers (#998)

Author: ahl

typify-impl/src/merge.rs

@@ -720,8 +720,52 @@ fn merge_so_number(
     match (a, b) {
         (None, other) | (other, None) => Ok(other.cloned().map(Box::new)),
         (Some(a), Some(b)) if a == b => Ok(Some(Box::new(a.clone()))),
-        (Some(_), Some(_)) => {
-            unimplemented!("this is fairly fussy and I don't want to do it")
+        (Some(a), Some(b)) => {
+            let maximum = choose_value(a.maximum, b.maximum, f64::min);
+            let exclusive_maximum =
+                choose_value(a.exclusive_maximum, b.exclusive_maximum, f64::min);
+            let minimum = choose_value(a.minimum, b.minimum, f64::max);
+            let exclusive_minimum =
+                choose_value(a.exclusive_minimum, b.exclusive_minimum, f64::max);
+
+            let multiple_of = choose_value(a.multiple_of, b.multiple_of, |a, b| {
+                let (mut x, mut y) = (a, b);
+                while y != 0.0 {
+                    (x, y) = (y, x % y);
+                }
+                (a / x) * b
+            });
+
+            // Normalize: when both inclusive and exclusive bounds are set,
+            // drop whichever is subsumed by the other.
+            let (minimum, exclusive_minimum) = match (minimum, exclusive_minimum) {
+                (Some(inc), Some(exc)) if exc >= inc => (None, Some(exc)),
+                (Some(inc), Some(_exc)) => (Some(inc), None),
+                pair => pair,
+            };
+            let (maximum, exclusive_maximum) = match (maximum, exclusive_maximum) {
+                (Some(inc), Some(exc)) if exc <= inc => (None, Some(exc)),
+                (Some(inc), Some(_exc)) => (Some(inc), None),
+                pair => pair,
+            };
+
+            // We'll return an error if the merged schema is unsatisfiable.
+            match (minimum, exclusive_minimum, maximum, exclusive_maximum) {
+                (Some(min), None, Some(max), None) if min > max => return Err(()),
+                (Some(min), None, None, Some(xmax)) if min >= xmax => return Err(()),
+                (None, Some(xmin), Some(max), None) if xmin >= max => return Err(()),
+                (None, Some(xmin), None, Some(xmax)) if xmin >= xmax => return Err(()),
+                (Some(_), Some(_), _, _) | (_, _, Some(_), Some(_)) => unreachable!(),
+                _ => {}
+            }
+
+            Ok(Some(Box::new(NumberValidation {
+                multiple_of,
+                maximum,
+                exclusive_maximum,
+                minimum,
+                exclusive_minimum,
+            })))
         }
     }
 }
@@ -733,8 +777,26 @@ fn merge_so_string(
     match (a, b) {
         (None, other) | (other, None) => Ok(other.cloned().map(Box::new)),
         (Some(a), Some(b)) if a == b => Ok(Some(Box::new(a.clone()))),
-        (Some(_), Some(_)) => {
-            unimplemented!("this is fairly fussy and I don't want to do it")
+        (Some(a), Some(b)) => {
+            let max_length = choose_value(a.max_length, b.max_length, Ord::min);
+            let min_length = choose_value(a.min_length, b.min_length, Ord::max);
+            let pattern = match (&a.pattern, &b.pattern) {
+                (None, v) | (v, None) => v.clone(),
+                (Some(x), Some(y)) if x == y => Some(x.clone()),
+                _ => unimplemented!("merging distinct patterns is impractical"),
+            };
+
+            if let (Some(min), Some(max)) = (min_length, max_length) {
+                if min > max {
+                    return Err(());
+                }
+            }
+
+            Ok(Some(Box::new(StringValidation {
+                max_length,
+                min_length,
+                pattern,
+            })))
         }
     }
 }
@@ -961,7 +1023,6 @@ fn merge_items_array<'a>(
     Ok((items, true))
 }
 
-/// Prefer Some over None and the result of `prefer` if both are Some.
 fn choose_value<T, F>(a: Option<T>, b: Option<T>, prefer: F) -> Option<T>
 where
     F: FnOnce(T, T) -> T,

typify/tests/schemas/merged-schemas.json

@@ -491,6 +491,36 @@
         }
       ]
     },
+    "merge-number-bounds": {
+      "$comment": "merging number constraints takes the most restrictive bounds; mixed inclusive/exclusive bounds are normalized to drop the subsumed one",
+      "allOf": [
+        {
+          "type": "number",
+          "minimum": 1.0,
+          "maximum": 100.0
+        },
+        {
+          "type": "number",
+          "exclusiveMinimum": 5.0,
+          "exclusiveMaximum": 50.0
+        }
+      ]
+    },
+    "merge-string-bounds": {
+      "$comment": "merging string constraints takes the most restrictive bounds: max_length takes the min, min_length takes the max",
+      "allOf": [
+        {
+          "type": "string",
+          "minLength": 5,
+          "maxLength": 20
+        },
+        {
+          "type": "string",
+          "minLength": 2,
+          "maxLength": 10
+        }
+      ]
+    },
     "unchanged-by-merge": {
       "allOf": [
         {

typify/tests/schemas/merged-schemas.rs

@@ -481,6 +481,152 @@ impl MergeEmpty {
         Default::default()
     }
 }
+#[doc = "`MergeNumberBounds`"]
+#[doc = r""]
+#[doc = r" <details><summary>JSON schema</summary>"]
+#[doc = r""]
+#[doc = r" ```json"]
+#[doc = "{"]
+#[doc = "  \"allOf\": ["]
+#[doc = "    {"]
+#[doc = "      \"type\": \"number\","]
+#[doc = "      \"maximum\": 100.0,"]
+#[doc = "      \"minimum\": 1.0"]
+#[doc = "    },"]
+#[doc = "    {"]
+#[doc = "      \"type\": \"number\","]
+#[doc = "      \"exclusiveMaximum\": 50.0,"]
+#[doc = "      \"exclusiveMinimum\": 5.0"]
+#[doc = "    }"]
+#[doc = "  ],"]
+#[doc = "  \"$comment\": \"merging number constraints takes the most restrictive bounds; mixed inclusive/exclusive bounds are normalized to drop the subsumed one\""]
+#[doc = "}"]
+#[doc = r" ```"]
+#[doc = r" </details>"]
+#[derive(:: serde :: Deserialize, :: serde :: Serialize, Clone, Debug)]
+#[serde(transparent)]
+pub struct MergeNumberBounds(pub f64);
+impl ::std::ops::Deref for MergeNumberBounds {
+    type Target = f64;
+    fn deref(&self) -> &f64 {
+        &self.0
+    }
+}
+impl ::std::convert::From<MergeNumberBounds> for f64 {
+    fn from(value: MergeNumberBounds) -> Self {
+        value.0
+    }
+}
+impl ::std::convert::From<f64> for MergeNumberBounds {
+    fn from(value: f64) -> Self {
+        Self(value)
+    }
+}
+impl ::std::str::FromStr for MergeNumberBounds {
+    type Err = <f64 as ::std::str::FromStr>::Err;
+    fn from_str(value: &str) -> ::std::result::Result<Self, Self::Err> {
+        Ok(Self(value.parse()?))
+    }
+}
+impl ::std::convert::TryFrom<&str> for MergeNumberBounds {
+    type Error = <f64 as ::std::str::FromStr>::Err;
+    fn try_from(value: &str) -> ::std::result::Result<Self, Self::Error> {
+        value.parse()
+    }
+}
+impl ::std::convert::TryFrom<String> for MergeNumberBounds {
+    type Error = <f64 as ::std::str::FromStr>::Err;
+    fn try_from(value: String) -> ::std::result::Result<Self, Self::Error> {
+        value.parse()
+    }
+}
+impl ::std::fmt::Display for MergeNumberBounds {
+    fn fmt(&self, f: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
+        self.0.fmt(f)
+    }
+}
+#[doc = "`MergeStringBounds`"]
+#[doc = r""]
+#[doc = r" <details><summary>JSON schema</summary>"]
+#[doc = r""]
+#[doc = r" ```json"]
+#[doc = "{"]
+#[doc = "  \"allOf\": ["]
+#[doc = "    {"]
+#[doc = "      \"type\": \"string\","]
+#[doc = "      \"maxLength\": 20,"]
+#[doc = "      \"minLength\": 5"]
+#[doc = "    },"]
+#[doc = "    {"]
+#[doc = "      \"type\": \"string\","]
+#[doc = "      \"maxLength\": 10,"]
+#[doc = "      \"minLength\": 2"]
+#[doc = "    }"]
+#[doc = "  ],"]
+#[doc = "  \"$comment\": \"merging string constraints takes the most restrictive bounds: max_length takes the min, min_length takes the max\""]
+#[doc = "}"]
+#[doc = r" ```"]
+#[doc = r" </details>"]
+#[derive(:: serde :: Serialize, Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[serde(transparent)]
+pub struct MergeStringBounds(::std::string::String);
+impl ::std::ops::Deref for MergeStringBounds {
+    type Target = ::std::string::String;
+    fn deref(&self) -> &::std::string::String {
+        &self.0
+    }
+}
+impl ::std::convert::From<MergeStringBounds> for ::std::string::String {
+    fn from(value: MergeStringBounds) -> Self {
+        value.0
+    }
+}
+impl ::std::str::FromStr for MergeStringBounds {
+    type Err = self::error::ConversionError;
+    fn from_str(value: &str) -> ::std::result::Result<Self, self::error::ConversionError> {
+        if value.chars().count() > 10usize {
+            return Err("longer than 10 characters".into());
+        }
+        if value.chars().count() < 5usize {
+            return Err("shorter than 5 characters".into());
+        }
+        Ok(Self(value.to_string()))
+    }
+}
+impl ::std::convert::TryFrom<&str> for MergeStringBounds {
+    type Error = self::error::ConversionError;
+    fn try_from(value: &str) -> ::std::result::Result<Self, self::error::ConversionError> {
+        value.parse()
+    }
+}
+impl ::std::convert::TryFrom<&::std::string::String> for MergeStringBounds {
+    type Error = self::error::ConversionError;
+    fn try_from(
+        value: &::std::string::String,
+    ) -> ::std::result::Result<Self, self::error::ConversionError> {
+        value.parse()
+    }
+}
+impl ::std::convert::TryFrom<::std::string::String> for MergeStringBounds {
+    type Error = self::error::ConversionError;
+    fn try_from(
+        value: ::std::string::String,
+    ) -> ::std::result::Result<Self, self::error::ConversionError> {
+        value.parse()
+    }
+}
+impl<'de> ::serde::Deserialize<'de> for MergeStringBounds {
+    fn deserialize<D>(deserializer: D) -> ::std::result::Result<Self, D::Error>
+    where
+        D: ::serde::Deserializer<'de>,
+    {
+        ::std::string::String::deserialize(deserializer)?
+            .parse()
+            .map_err(|e: self::error::ConversionError| {
+                <D::Error as ::serde::de::Error>::custom(e.to_string())
+            })
+    }
+}
 #[doc = "`NarrowNumber`"]
 #[doc = r""]
 #[doc = r" <details><summary>JSON schema</summary>"]