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/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,6 @@ where
     D: Default,
 {
     fn lints(&mut self) -> Vec<Lint> {
-        // self.lints.extend(std::mem::take(&mut self.environment.lints));
         std::mem::take(&mut self.lints)
     }
 }
@@ -81,7 +79,7 @@ where
     _phantom: PhantomData<D>,
 }
 
-impl<D> From<InstallSpecVisitor<D>> for InstallSpec
+impl<D> From<InstallSpecVisitor<D>> for RawInstallSpec
 where
     D: Default,
 {
@@ -206,69 +204,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>

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};

crates/spk-schema/src/v0/spec.rs

@@ -66,6 +66,7 @@ use crate::{
     Opt,
     Package,
     PackageMut,
+    RawInstallSpec,
     Recipe,
     RequirementsList,
     Result,
@@ -990,7 +991,7 @@ struct SpecVisitor<B, T> {
     sources: Option<Vec<LintedItem<SourceSpec>>>,
     build: Option<LintedItem<BuildSpec>>,
     tests: Option<Vec<LintedItem<TestSpec>>>,
-    install: Option<LintedItem<InstallSpec>>,
+    install: Option<LintedItem<RawInstallSpec>>,
     #[field_names_as_array(skip)]
     check_build_spec: bool,
     #[field_names_as_array(skip)]
@@ -1047,7 +1048,7 @@ where
                 .iter()
                 .map(|l| l.item.clone())
                 .collect_vec(),
-            install: value.install.take().unwrap_or_default().item,
+            install: value.install.take().unwrap_or_default().item.into(),
         }
     }
 }
@@ -1143,7 +1144,7 @@ where
                     }
                 }
                 "tests" => self.tests = Some(map.next_value::<Vec<LintedItem<TestSpec>>>()?),
-                "install" => self.install = Some(map.next_value::<LintedItem<InstallSpec>>()?),
+                "install" => self.install = Some(map.next_value::<LintedItem<RawInstallSpec>>()?),
                 "api" => {
                     map.next_value::<serde::de::IgnoredAny>()?;
                 }