From 6bfa1cc59a2d1b06b408ddeacb4abae3cef1200e Mon Sep 17 00:00:00 2001
From: J Robert Ray <jrray@jrray.org>
Date: Mon, 17 Jun 2024 18:18:46 -0700
Subject: [PATCH] Fix install.environment silently ignoring configuration

This came up today when someone tried using:

```
install:
  environment:
    - append: VAR_NAME
      value: value
      priority: 1
```

The existing code first parses the `append` op but then replaces it with
a `priority` op, discarding the `append`.

The proper recipe content is:

```
install:
  environment:
    - append: VAR_NAME
      value: value
    - priority: 1
```

Now this is detected as an error instead of behaving with unexpected
results.

Signed-off-by: J Robert Ray <jrray@jrray.org>
---
 crates/spk-schema/src/environ.rs      | 28 ++++++++++++++++++++++++++-
 crates/spk-schema/src/environ_test.rs |  9 +++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/crates/spk-schema/src/environ.rs b/crates/spk-schema/src/environ.rs
index 0dc9aefcc0..483494f678 100644
--- a/crates/spk-schema/src/environ.rs
+++ b/crates/spk-schema/src/environ.rs
@@ -24,7 +24,8 @@ const OP_SET: &str = "set";
 const OP_NAMES: &[&str] = &[OP_APPEND, OP_COMMENT, OP_PREPEND, OP_SET];
 
 /// The set of operation types for use in deserialization
-#[derive(Copy, Clone, Debug, PartialEq)]
+#[derive(Copy, Clone, Debug, PartialEq, strum::Display)]
+#[strum(serialize_all = "lowercase")]
 pub enum OpKind {
     Append,
     Comment,
@@ -197,30 +198,55 @@ impl<'de> Deserialize<'de> for EnvOp {
                 while let Some(key) = map.next_key::<String>()? {
                     match key.as_str() {
                         OP_PREPEND => {
+                            if let Some((existing_op, _)) = &self.op_and_var {
+                                return Err(serde::de::Error::custom(format!(
+                                    "encountered {key} but operation already defined as {existing_op}",
+                                )));
+                            }
                             self.op_and_var = Some((
                                 OpKind::Prepend,
                                 ConfKind::Operation(map.next_value::<Stringified>()?.0),
                             ));
                         }
                         OP_PRIORITY => {
+                            if let Some((existing_op, _)) = &self.op_and_var {
+                                return Err(serde::de::Error::custom(format!(
+                                    "encountered {key} but operation already defined as {existing_op}",
+                                )));
+                            }
                             self.op_and_var = Some((
                                 OpKind::Priority,
                                 ConfKind::Priority(map.next_value::<u8>()?),
                             ));
                         }
                         OP_COMMENT => {
+                            if let Some((existing_op, _)) = &self.op_and_var {
+                                return Err(serde::de::Error::custom(format!(
+                                    "encountered {key} but operation already defined as {existing_op}",
+                                )));
+                            }
                             self.op_and_var = Some((
                                 OpKind::Comment,
                                 ConfKind::Operation(map.next_value::<Stringified>()?.0),
                             ));
                         }
                         OP_APPEND => {
+                            if let Some((existing_op, _)) = &self.op_and_var {
+                                return Err(serde::de::Error::custom(format!(
+                                    "encountered {key} but operation already defined as {existing_op}",
+                                )));
+                            }
                             self.op_and_var = Some((
                                 OpKind::Append,
                                 ConfKind::Operation(map.next_value::<Stringified>()?.0),
                             ));
                         }
                         OP_SET => {
+                            if let Some((existing_op, _)) = &self.op_and_var {
+                                return Err(serde::de::Error::custom(format!(
+                                    "encountered {key} but operation already defined as {existing_op}",
+                                )));
+                            }
                             self.op_and_var = Some((
                                 OpKind::Set,
                                 ConfKind::Operation(map.next_value::<Stringified>()?.0),
diff --git a/crates/spk-schema/src/environ_test.rs b/crates/spk-schema/src/environ_test.rs
index 02660d5532..cf47f6d175 100644
--- a/crates/spk-schema/src/environ_test.rs
+++ b/crates/spk-schema/src/environ_test.rs
@@ -68,3 +68,12 @@ fn test_yaml_round_trip(#[case] op: &str) {
     let op2: EnvOp = serde_yaml::from_str(&yaml).unwrap();
     assert_eq!(op2, op, "should be the same after sending through yaml");
 }
+
+/// Test that ambiguous/overlapping op definitions cause a parse error instead
+/// of silently ignoring some of the configuration.
+#[rstest]
+#[case("{set: SPK_TEST_VAR, append: SPK_TEST_VAR, value: simple")]
+fn test_multiple_ops_cause_error(#[case] op: &str) {
+    let result: Result<EnvOp, _> = serde_yaml::from_str(op);
+    assert!(result.is_err(), "should fail to parse multiple ops");
+}