mk-oracle: Refactorings

- Fail on malformed instance info rows instead of dropping them silently.
No change in behavior - No supported release produces such a row.
- Drop unused string values from the `Tenant::new` parsing. Rename the
  method, so its purpose is clear.

TESTS: Manual against 12.1, 19.3, 23.26,
and 11.2 (failing with ORA-00904) on Docker.

Change-Id: I5867f5bb767252cba7330cdfa27d85c88011ccb5

Author: andiumbreit

packages/mk-oracle/src/ora_sql/system.rs

@@ -118,9 +118,7 @@ fn _detect_version(spot: &OpenedSpot) -> Result<DetectedVersion> {
 fn _get_instances(spot: &OpenedSpot, custom_query: Option<&str>) -> Result<_InstanceEntries> {
     if let Some(query) = custom_query {
         // Replaces the probe entirely; the caller owns the column set.
-        return Ok(_to_instance_entries(
-            spot.query_table(&SqlQuery::new(query, &Vec::new())).0?,
-        ));
+        return _to_instance_entries(spot.query_table(&SqlQuery::new(query, &Vec::new())).0?);
     }
 
     let detected = _detect_version(spot)?;
@@ -143,7 +141,7 @@ fn _get_instances(spot: &OpenedSpot, custom_query: Option<&str>) -> Result<_Inst
             *version_column = detected.version().clone().into();
         }
     }
-    Ok(_to_instance_entries(result))
+    _to_instance_entries(result)
 }
 
 fn _extract_version(result: Vec<String>) -> Option<String> {
@@ -158,25 +156,29 @@ fn _extract_version(result: Vec<String>) -> Option<String> {
         .map(str::to_string)
 }
 
-fn _to_instance_entries(result: Vec<Vec<String>>) -> _InstanceEntries {
-    let hashmap: _InstanceEntries = result
+/// Consumes columns 0, 2 and 4. Instance info we cannot interpret fails the
+/// endpoint, which surfaces as a FAILURE row, rather than guessing a tenancy
+/// that would silently reshape every section.
+fn _to_instance_entries(result: Vec<Vec<String>>) -> Result<_InstanceEntries> {
+    result
         .into_iter()
-        .filter_map(|x| {
-            if x.len() < 2 {
-                log::error!(
-                    "Unexpected result from v$instance: expected at least 2 columns, got {}",
-                    x.len()
+        .map(|row| {
+            let (Some(name), Some(version), Some(tenant)) = (row.first(), row.get(2), row.get(4))
+            else {
+                anyhow::bail!(
+                    "Unexpected result from v$instance: expected at least 5 columns, got {}",
+                    row.len()
                 );
-                None
-            } else {
-                Some((
-                    InstanceName::from(x[0].as_str()),
-                    (InstanceVersion::from(x[2].clone()), Tenant::new(&x[4])),
-                ))
-            }
+            };
+            let tenant = Tenant::from_cdb_column(tenant).ok_or_else(|| {
+                anyhow::anyhow!("Unexpected v$database.cdb value '{tenant}' for instance {name}")
+            })?;
+            Ok((
+                InstanceName::from(name.as_str()),
+                (InstanceVersion::from(version.clone()), tenant),
+            ))
         })
-        .collect();
-    hashmap
+        .collect()
 }
 pub fn convert_to_num_version(version: &InstanceVersion) -> Option<InstanceNumVersion> {
     let tops = String::from(version.clone())
@@ -198,7 +200,7 @@ pub fn convert_to_num_version(version: &InstanceVersion) -> Option<InstanceNumVe
 mod tests {
     use super::*;
     use crate::config::ora_sql::Endpoint;
-    use crate::ora_sql::backend::test_support::MiniOra;
+    use crate::ora_sql::backend::test_support::{instance_row, MiniOra};
     use crate::ora_sql::backend::SpotBuilder;
 
     /// Returns the derived (version, tenant) plus every query the run issued.
@@ -322,6 +324,30 @@ mod tests {
             .is_none());
     }
 
+    #[test]
+    fn test_instance_entries_tenant_from_cdb_column() {
+        for (cdb, expected) in [("NO", Tenant::NoCdb), ("YES", Tenant::Cdb)] {
+            let entries =
+                _to_instance_entries(vec![instance_row("ORCL", "19.28.0.0.0", cdb)]).unwrap();
+            assert_eq!(
+                entries.get(&InstanceName::from("ORCL")).unwrap().1,
+                expected
+            );
+        }
+    }
+
+    #[test]
+    fn test_instance_entries_short_row_is_skipped_not_panic() {
+        for row in [vec!["ORCL".to_string()], vec!["ORCL".into(), "0".into()]] {
+            assert!(_to_instance_entries(vec![row]).is_err());
+        }
+    }
+
+    #[test]
+    fn test_instance_entries_unknown_cdb_value_fails() {
+        assert!(_to_instance_entries(vec![instance_row("ORCL", "19.28.0.0.0", "MAYBE")]).is_err());
+    }
+
     #[test]
     fn test_convert_to_num_version() {
         assert_eq!(

packages/mk-oracle/src/types.rs

@@ -190,9 +190,10 @@ pub struct InstanceVersion(String);
 #[derive(PartialEq, From, Clone, Copy, Debug, Display, Default, Into, PartialOrd)]
 pub struct InstanceNumVersion(u32);
 
+/// Used to express CDB property of instances AND applicability of queries.
 #[derive(PartialEq, Eq, Debug, Copy, Clone)]
 pub enum Tenant {
-    All,
+    All, // Only for queries: Query applies to both CDB and non-CDB instances.
     Cdb,
     NoCdb,
 }
@@ -204,12 +205,11 @@ pub enum AsmInstance {
 }
 
 impl Tenant {
-    pub fn new(tenant: &str) -> Self {
-        match tenant.to_lowercase().as_str() {
-            "all" => Tenant::All,
-            "cdb" | "yes" => Tenant::Cdb,
-            "nocdb" | "no" => Tenant::NoCdb,
-            _ => panic!("Unknown tenant type: {}", tenant),
+    pub fn from_cdb_column(cdb_column: &str) -> Option<Self> {
+        match cdb_column.to_lowercase().as_str() {
+            "yes" => Some(Tenant::Cdb),
+            "no" => Some(Tenant::NoCdb),
+            _ => None,
         }
     }
 }
@@ -361,14 +361,11 @@ mod tests {
 
     #[test]
     fn test_tenant() {
-        assert_eq!(Tenant::new("all"), Tenant::All);
-        assert_eq!(Tenant::new("cdb"), Tenant::Cdb);
-        assert_eq!(Tenant::new("nocdb"), Tenant::NoCdb);
-        assert_eq!(Tenant::new("yEs"), Tenant::Cdb);
-        assert_eq!(Tenant::new("no"), Tenant::NoCdb);
-        // panic on unknown tenant
-        let result = std::panic::catch_unwind(|| Tenant::new("unknown"));
-        assert!(result.is_err());
+        assert_eq!(Tenant::from_cdb_column("yEs"), Some(Tenant::Cdb));
+        assert_eq!(Tenant::from_cdb_column("no"), Some(Tenant::NoCdb));
+        assert!(Tenant::from_cdb_column("cdb").is_none());
+        assert!(Tenant::from_cdb_column("unknown").is_none());
+        assert!(Tenant::from_cdb_column("").is_none());
     }
 
     #[test]