Keep the telemetry opt-out when config.json has a bad or unknown key

Author: zmofei

CHANGELOG.md

@@ -264,7 +264,10 @@ that may never merge. They are not releases and are not listed here.
   shell, the way `MAPBOX_CLI_NO_TELEMETRY=1` does for one: the run's event,
   the `User-Agent` markers and the update notice. The run that turns it off
   records nothing either. `mapbox config list` now also reports `telemetry`,
-  and `mapbox doctor` reports it as `telemetry_persisted`.
+  and `mapbox doctor` reports it as `telemetry_persisted`; its
+  `telemetry_allowed` still means the environment variable alone. Version
+  0.3.0 doesn't know this key, and its `mapbox config set` drops it from the
+  file, so with two installs, set it again after using the older one.
 
 - Each run sends one `cli.command` telemetry event to Mapbox, from a
   background process the command doesn't wait for, with your own token

docs/commands.md

@@ -4591,6 +4591,10 @@ Telemetry:     on
 </td></tr>
 </table>
 
+In `switches`, `telemetry_allowed` is `MAPBOX_CLI_NO_TELEMETRY` alone and
+`telemetry_persisted` is `mapbox config set telemetry`; telemetry is on only
+when both are `true`.
+
 With `--verify`, a `connectivity` object joins the JSON and a `Reachable:`
 line joins the text — `{ "reachable": true, "status": 200 }`, or `{
 "reachable": false }` (plus an `error` field under `--debug`) when the

src/config.rs

@@ -19,8 +19,8 @@ use std::path::PathBuf;
 use anyhow::{Context, Result};
 use clap::builder::PossibleValuesParser;
 use clap::{Arg, ArgMatches, Command};
-use serde::{Deserialize, Serialize};
-use serde_json::{json, Value};
+use serde::Serialize;
+use serde_json::{json, Map, Value};
 
 use crate::auth;
 use crate::output::{self, CliError, Mode};
@@ -43,7 +43,7 @@ const OFF: &str = "off";
 /// default is `on` — distinct from `Some(true)`, which is someone turning it
 /// back on after having turned it off, but read identically by
 /// [`update_check_setting`].
-#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize)]
 struct Config {
     #[serde(default, skip_serializing_if = "Option::is_none")]
     update_check: Option<bool>,
@@ -53,6 +53,33 @@ struct Config {
     log: Option<bool>,
     #[serde(default, skip_serializing_if = "Option::is_none")]
     telemetry: Option<bool>,
+    /// Keys this version doesn't know, kept so that writing the file doesn't
+    /// erase a setting a newer version saved.
+    #[serde(flatten)]
+    rest: Map<String, Value>,
+}
+
+impl Config {
+    /// Reads each key on its own, so one malformed value resets only that
+    /// key. Parsed as a whole, a single bad value would reset every key,
+    /// `telemetry` included, and an opt-out would quietly turn back on.
+    fn parse(text: &str) -> Config {
+        let Ok(Value::Object(mut rest)) = serde_json::from_str(text) else {
+            return Config::default();
+        };
+        let mut flag = |key: &str| rest.remove(key).and_then(|value| value.as_bool());
+        let update_check = flag("update_check");
+        let history = flag("history");
+        let log = flag("log");
+        let telemetry = flag("telemetry");
+        Config {
+            update_check,
+            history,
+            log,
+            telemetry,
+            rest,
+        }
+    }
 }
 
 fn config_path() -> Option<PathBuf> {
@@ -67,7 +94,7 @@ fn config_path() -> Option<PathBuf> {
 fn read_config() -> Config {
     config_path()
         .and_then(|path| std::fs::read_to_string(path).ok())
-        .and_then(|text| serde_json::from_str(&text).ok())
+        .map(|text| Config::parse(&text))
         .unwrap_or_default()
 }
 
@@ -105,8 +132,8 @@ pub fn log_enabled() -> bool {
     read_config().log.unwrap_or(false)
 }
 
-/// Whether the run's telemetry event may be recorded, per the persisted
-/// setting. [`crate::telemetry_event`] checks it alongside `MAPBOX_CLI_NO_TELEMETRY`.
+/// Whether telemetry is on, per the persisted setting. Read through
+/// [`crate::telemetry::telemetry_allowed`], alongside `MAPBOX_CLI_NO_TELEMETRY`.
 pub fn telemetry_enabled() -> bool {
     read_config().telemetry.unwrap_or(true)
 }
@@ -285,15 +312,32 @@ mod tests {
         };
         let text = serde_json::to_string(&off).expect("serialize");
         assert_eq!(text, r#"{"update_check":false}"#);
-        let read: Config = serde_json::from_str(&text).expect("deserialize");
-        assert_eq!(read, off);
+        assert_eq!(Config::parse(&text), off);
 
         // A file from before this key existed, or one with nothing set yet.
-        let empty: Config = serde_json::from_str("{}").expect("an empty object");
+        let empty = Config::parse("{}");
         assert_eq!(empty.update_check, None);
         assert!(update_check_setting(&empty));
     }
 
+    #[test]
+    fn a_malformed_value_resets_only_its_own_key() {
+        let config = Config::parse(r#"{"telemetry":false,"history":"off"}"#);
+        assert_eq!(config.telemetry, Some(false));
+        assert_eq!(config.history, None);
+        assert_eq!(Config::parse("not json"), Config::default());
+    }
+
+    #[test]
+    fn keys_from_a_newer_version_survive_a_write() {
+        let mut config = Config::parse(r#"{"history":true,"future_key":[1]}"#);
+        config.history = Some(false);
+        assert_eq!(
+            serde_json::to_string(&config).expect("serialize"),
+            r#"{"history":false,"future_key":[1]}"#
+        );
+    }
+
     #[test]
     fn on_and_off_round_trip_through_on_off() {
         assert_eq!(on_off(true), ON);

src/telemetry.rs

@@ -110,7 +110,13 @@ fn terminal_marker() -> String {
 
 /// The full `User-Agent`: [`PRODUCT_TOKEN`], then markers if allowed.
 pub fn user_agent(command_group: Option<&str>) -> String {
-    if telemetry_allowed() {
+    user_agent_if(telemetry_allowed(), command_group)
+}
+
+/// [`user_agent`] with the decision passed in, so a test isn't at the mercy
+/// of the persisted setting on the machine running it.
+fn user_agent_if(allowed: bool, command_group: Option<&str>) -> String {
+    if allowed {
         assemble(&telemetry_markers(command_group))
     } else {
         assemble(&[])
@@ -148,11 +154,11 @@ mod tests {
         let previous = std::env::var_os(MAPBOX_CLI_NO_TELEMETRY_ENV);
         let read = |value: &&str| {
             std::env::set_var(MAPBOX_CLI_NO_TELEMETRY_ENV, value);
-            user_agent(None)
+            user_agent_if(env_allows_telemetry(), None)
         };
 
         std::env::remove_var(MAPBOX_CLI_NO_TELEMETRY_ENV);
-        let unset = user_agent(None);
+        let unset = user_agent_if(env_allows_telemetry(), None);
         let opted_out: Vec<String> = opt_outs.iter().map(read).collect();
         let allowed: Vec<String> = left_alone.iter().map(read).collect();
 
@@ -162,6 +168,7 @@ mod tests {
         }
 
         assert!(unset.starts_with(PRODUCT_TOKEN), "{unset}");
+        assert_ne!(unset, PRODUCT_TOKEN, "no markers with the switch unset");
         for (value, agent) in opt_outs.iter().zip(&opted_out) {
             assert_eq!(agent, PRODUCT_TOKEN, "MAPBOX_CLI_NO_TELEMETRY={value:?}");
         }

tests/source_guards.rs

@@ -546,3 +546,24 @@ fn every_test_that_runs_the_binary_decides_about_telemetry() {
          token sends real telemetry: {undecided:?}. Set it on the command they build."
     );
 }
+
+/// `env_allows_telemetry` is `MAPBOX_CLI_NO_TELEMETRY` alone, so a caller
+/// that gates telemetry on it ignores `mapbox config set telemetry off`.
+/// Only `telemetry.rs`, which combines the two, and `doctor.rs`, which
+/// reports them apart, may call it.
+#[test]
+fn only_telemetry_and_doctor_read_the_env_opt_out_alone() {
+    let unexpected: Vec<String> = sources()
+        .into_iter()
+        .filter(|(name, source)| {
+            !["telemetry.rs", "doctor.rs"].contains(&name.as_str())
+                && source.contains("env_allows_telemetry")
+        })
+        .map(|(name, _)| format!("src/{name}"))
+        .collect();
+    assert!(
+        unexpected.is_empty(),
+        "these modules call `telemetry::env_allows_telemetry`, which ignores the \
+         persisted setting: {unexpected:?}. Call `telemetry::telemetry_allowed` instead."
+    );
+}

tests/telemetry_events.rs

@@ -240,6 +240,11 @@ fn either_opt_out_records_nothing() {
         received.recv_timeout(Duration::from_secs(3)).is_err(),
         "`telemetry off` still sent an event"
     );
+    // Not left to the send child's own check: the run itself records nothing.
+    assert!(
+        !config_dir(&home).join(".telemetry").exists(),
+        "`telemetry off` still wrote telemetry"
+    );
 }
 
 #[test]