Fix issue where EnvOpList was not properly deserialized and bypassed the multiple priority config check.

Signed-off-by: Nichol Yip <nyip@imageworks.com>

Author: Nichol Yip

crates/spk-schema/src/embedded_packages_list.rs

@@ -8,7 +8,7 @@ use spk_schema_foundation::spec_ops::Named;
 use spk_schema_foundation::IsDefault;
 use spk_schema_ident::AnyIdent;
 
-use super::{BuildSpec, InstallSpec, Spec};
+use super::{BuildSpec, InstallSpec, Lint, LintedItem, Lints, Spec};
 use crate::component_embedded_packages::ComponentEmbeddedPackage;
 use crate::foundation::ident_build::Build;
 use crate::Package;
@@ -61,53 +61,142 @@ impl std::ops::DerefMut for EmbeddedPackagesList {
     }
 }
 
+#[derive(Default)]
+struct EmbeddedPackagesListVisitor {
+    embedded: Vec<Spec>,
+    lints: Vec<Lint>,
+}
+
+impl Lints for EmbeddedPackagesListVisitor {
+    fn lints(&mut self) -> Vec<Lint> {
+        std::mem::take(&mut self.lints)
+    }
+}
+
+impl From<EmbeddedPackagesListVisitor> for EmbeddedPackagesList {
+    fn from(value: EmbeddedPackagesListVisitor) -> Self {
+        Self(value.embedded)
+    }
+}
+
 impl<'de> Deserialize<'de> for EmbeddedPackagesList {
     fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
     where
-        D: serde::Deserializer<'de>,
+        D: serde::de::Deserializer<'de>,
+    {
+        Ok(deserializer
+            .deserialize_seq(EmbeddedPackagesListVisitor::default())?
+            .into())
+    }
+}
+
+impl<'de> Deserialize<'de> for LintedItem<EmbeddedPackagesList> {
+    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
+    where
+        D: serde::de::Deserializer<'de>,
     {
-        struct EmbeddedPackagesListVisitor;
+        Ok(deserializer
+            .deserialize_seq(EmbeddedPackagesListVisitor::default())?
+            .into())
+    }
+}
 
-        impl<'de> serde::de::Visitor<'de> for EmbeddedPackagesListVisitor {
-            type Value = EmbeddedPackagesList;
+impl<'de> serde::de::Visitor<'de> for EmbeddedPackagesListVisitor {
+    type Value = EmbeddedPackagesListVisitor;
 
-            fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-                f.write_str("a list of embedded packages")
-            }
+    fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        f.write_str("a list of embedded packages")
+    }
+
+    fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
+    where
+        A: serde::de::SeqAccess<'de>,
+    {
+        let size_hint = seq.size_hint().unwrap_or(0);
+        let mut lints = Vec::new();
+        let mut embedded_stubs = Vec::with_capacity(size_hint);
+        let mut default_build_spec = BuildSpec::default();
+        let mut default_install_spec = InstallSpec::default();
+        while let Some(mut embedded) =
+            seq.next_element::<LintedItem<super::v0::Spec<AnyIdent>>>()?
+        {
+            lints.append(&mut embedded.lints);
 
-            fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
-            where
-                A: serde::de::SeqAccess<'de>,
-            {
-                let size_hint = seq.size_hint().unwrap_or(0);
-                let mut embedded_stubs = Vec::with_capacity(size_hint);
-                let mut default_build_spec = BuildSpec::default();
-                let mut default_install_spec = InstallSpec::default();
-                while let Some(embedded) = seq.next_element::<super::v0::Spec<AnyIdent>>()? {
-                    default_build_spec
-                        .options
-                        .clone_from(&embedded.build.options);
-                    if default_build_spec != embedded.build {
-                        return Err(serde::de::Error::custom(
-                            "embedded packages can only specify build.options",
-                        ));
-                    }
-                    default_install_spec.components = embedded.install.components.clone();
-                    if default_install_spec != embedded.install {
-                        return Err(serde::de::Error::custom(
-                            "embedded packages can only specify install.components",
-                        ));
-                    }
-                    let embedded = embedded.map_ident(|i| {
-                        i.into_base()
-                            .into_build_ident(Build::Embedded(EmbeddedSource::Unknown))
-                    });
-                    embedded_stubs.push(Spec::V0Package(embedded));
-                }
-                Ok(EmbeddedPackagesList(embedded_stubs))
+            default_build_spec
+                .options
+                .clone_from(&embedded.item.build.options);
+            if default_build_spec != embedded.item.build {
+                return Err(serde::de::Error::custom(
+                    "embedded packages can only specify build.options",
+                ));
             }
+            default_install_spec.components = embedded.item.install.components.clone();
+            if default_install_spec != embedded.item.install {
+                return Err(serde::de::Error::custom(
+                    "embedded packages can only specify install.components",
+                ));
+            }
+            let embedded = embedded.item.map_ident(|i| {
+                i.into_base()
+                    .into_build_ident(Build::Embedded(EmbeddedSource::Unknown))
+            });
+            embedded_stubs.push(Spec::V0Package(embedded));
         }
 
-        deserializer.deserialize_seq(EmbeddedPackagesListVisitor)
+        Ok(Self {
+            embedded: embedded_stubs,
+            lints,
+        })
     }
 }
+
+// impl<'de> Deserialize<'de> for EmbeddedPackagesList {
+//     fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
+//     where
+//         D: serde::Deserializer<'de>,
+//     {
+//         struct EmbeddedPackagesListVisitor;
+
+//         impl<'de> serde::de::Visitor<'de> for EmbeddedPackagesListVisitor {
+//             type Value = EmbeddedPackagesList;
+
+//             fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+//                 f.write_str("a list of embedded packages")
+//             }
+
+//             fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
+//             where
+//                 A: serde::de::SeqAccess<'de>,
+//             {
+//                 let size_hint = seq.size_hint().unwrap_or(0);
+//                 let mut embedded_stubs = Vec::with_capacity(size_hint);
+//                 let mut default_build_spec = BuildSpec::default();
+//                 let mut default_install_spec = InstallSpec::default();
+//                 while let Some(embedded) = seq.next_element::<LintedItem<super::v0::Spec<AnyIdent>>>()? {
+//                     default_build_spec
+//                         .options
+//                         .clone_from(&embedded.item.build.options);
+//                     if default_build_spec != embedded.build {
+//                         return Err(serde::de::Error::custom(
+//                             "embedded packages can only specify build.options",
+//                         ));
+//                     }
+//                     default_install_spec.components = embedded.item.install.components.clone();
+//                     if default_install_spec != embedded.item.install {
+//                         return Err(serde::de::Error::custom(
+//                             "embedded packages can only specify install.components",
+//                         ));
+//                     }
+//                     let embedded = embedded.map_ident(|i| {
+//                         i.into_base()
+//                             .into_build_ident(Build::Embedded(EmbeddedSource::Unknown))
+//                     });
+//                     embedded_stubs.push(Spec::V0Package(embedded));
+//                 }
+//                 Ok(EmbeddedPackagesList(embedded_stubs))
+//             }
+//         }
+
+//         deserializer.deserialize_seq(EmbeddedPackagesListVisitor)
+//     }
+// }

crates/spk-schema/src/environ.rs

@@ -188,9 +188,46 @@ impl EnvOp {
     }
 }
 
-#[derive(Clone, Debug, Default, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
+#[derive(Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
 pub struct EnvOpList(Vec<EnvOp>);
 
+impl<'de> Deserialize<'de> for EnvOpList {
+    fn deserialize<D>(deserializer: D) -> std::result::Result<EnvOpList, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        struct EnvConfVisitor;
+
+        impl<'de> serde::de::Visitor<'de> for EnvConfVisitor {
+            type Value = EnvOpList;
+
+            fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+                f.write_str("an environment configuration")
+            }
+
+            fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
+            where
+                A: serde::de::SeqAccess<'de>,
+            {
+                let mut vec = EnvOpList::default();
+
+                while let Some(elem) = seq.next_element::<EnvOp>()? {
+                    if vec.iter().any(|x: &EnvOp| x.kind() == OpKind::Priority)
+                        && elem.kind() == OpKind::Priority
+                    {
+                        return Err(serde::de::Error::custom(
+                            "Multiple priority config cannot be set.",
+                        ));
+                    };
+                    vec.push(elem);
+                }
+                Ok(vec)
+            }
+        }
+        deserializer.deserialize_seq(EnvConfVisitor)
+    }
+}
+
 impl IsDefault for EnvOpList {
     fn is_default(&self) -> bool {
         self.0.is_empty()

crates/spk-schema/src/install_spec.rs

@@ -17,12 +17,10 @@ use crate::foundation::option_map::OptionMap;
 use crate::{
     ComponentSpecList,
     EmbeddedPackagesList,
-    EnvOp,
     EnvOpList,
     Lint,
     LintedItem,
     Lints,
-    OpKind,
     Package,
     RequirementsList,
     Result,
@@ -38,6 +36,7 @@ mod install_spec_test;
     Clone,
     Debug,
     Default,
+    Deserialize,
     Eq,
     Hash,
     is_default_derive_macro::IsDefault,
@@ -63,7 +62,7 @@ where
     D: Default,
 {
     fn lints(&mut self) -> Vec<Lint> {
-        // self.lints.extend(std::mem::take(&mut self.environment.lints));
+        self.lints.extend(std::mem::take(&mut self.embedded.lints));
         std::mem::take(&mut self.lints)
     }
 }
@@ -74,21 +73,21 @@ where
     D: Default,
 {
     requirements: RequirementsList,
-    embedded: EmbeddedPackagesList,
+    embedded: LintedItem<EmbeddedPackagesList>,
     components: ComponentSpecList,
     environment: EnvOpList,
     lints: Vec<Lint>,
     _phantom: PhantomData<D>,
 }
 
-impl<D> From<InstallSpecVisitor<D>> for InstallSpec
+impl<D> From<InstallSpecVisitor<D>> for RawInstallSpec
 where
     D: Default,
 {
     fn from(value: InstallSpecVisitor<D>) -> Self {
         Self {
             requirements: value.requirements,
-            embedded: value.embedded,
+            embedded: value.embedded.item,
             components: value.components,
             environment: value.environment,
         }
@@ -206,69 +205,35 @@ impl From<RawInstallSpec> for InstallSpec {
     }
 }
 
-#[derive(Deserialize)]
-struct RawInstallSpec {
+/// A raw, unvalidated install spec.
+#[derive(Serialize, Default)]
+pub struct RawInstallSpec {
     #[serde(default)]
     requirements: RequirementsList,
     #[serde(default)]
     embedded: EmbeddedPackagesList,
     #[serde(default)]
     components: ComponentSpecList,
-    #[serde(default, deserialize_with = "deserialize_env_conf")]
+    #[serde(default)]
     environment: EnvOpList,
 }
 
-impl<'de> Deserialize<'de> for InstallSpec {
+impl<'de> Deserialize<'de> for RawInstallSpec {
     fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
     where
         D: serde::de::Deserializer<'de>,
     {
-        deserializer.deserialize_map(InstallSpecVisitor::<InstallSpec>::default())
+        deserializer.deserialize_map(InstallSpecVisitor::<RawInstallSpec>::default())
     }
 }
 
-impl<'de> Deserialize<'de> for LintedItem<InstallSpec> {
+impl<'de> Deserialize<'de> for LintedItem<RawInstallSpec> {
     fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
     where
         D: serde::de::Deserializer<'de>,
     {
-        deserializer.deserialize_map(InstallSpecVisitor::<LintedItem<InstallSpec>>::default())
-    }
-}
-
-fn deserialize_env_conf<'de, D>(deserializer: D) -> std::result::Result<EnvOpList, D::Error>
-where
-    D: serde::Deserializer<'de>,
-{
-    struct EnvConfVisitor;
-
-    impl<'de> serde::de::Visitor<'de> for EnvConfVisitor {
-        type Value = EnvOpList;
-
-        fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
-            f.write_str("an environment configuration")
-        }
-
-        fn visit_seq<A>(self, mut seq: A) -> std::result::Result<Self::Value, A::Error>
-        where
-            A: serde::de::SeqAccess<'de>,
-        {
-            let mut vec = EnvOpList::default();
-
-            while let Some(elem) = seq.next_element::<EnvOp>()? {
-                if vec.iter().any(|x: &EnvOp| x.kind() == OpKind::Priority)
-                    && elem.kind() == OpKind::Priority
-                {
-                    return Err(serde::de::Error::custom(
-                        "Multiple priority config cannot be set.",
-                    ));
-                };
-                vec.push(elem);
-            }
-            Ok(vec)
-        }
+        deserializer.deserialize_map(InstallSpecVisitor::<LintedItem<RawInstallSpec>>::default())
     }
-    deserializer.deserialize_seq(EnvConfVisitor)
 }
 
 impl<'de, D> serde::de::Visitor<'de> for InstallSpecVisitor<D>
@@ -288,7 +253,9 @@ where
         while let Some(key) = map.next_key::<Stringified>()? {
             match key.as_str() {
                 "requirements" => self.requirements = map.next_value::<RequirementsList>()?,
-                "embedded" => self.embedded = map.next_value::<EmbeddedPackagesList>()?,
+                "embedded" => {
+                    self.embedded = map.next_value::<LintedItem<EmbeddedPackagesList>>()?
+                }
                 "components" => self.components = map.next_value::<ComponentSpecList>()?,
                 "environment" => self.environment = map.next_value::<EnvOpList>()?,
                 unknown_key => {

crates/spk-schema/src/lib.rs

@@ -46,7 +46,7 @@ pub use environ::{
 };
 pub use error::{Error, Result};
 pub use input_variant::InputVariant;
-pub use install_spec::InstallSpec;
+pub use install_spec::{InstallSpec, RawInstallSpec};
 pub use lints::{Lint, LintedItem, Lints, UnknownKey};
 pub use option::{Inheritance, Opt};
 pub use package::{Package, PackageMut};