From 5f7425a7b4bd3de9faf779670d44f2b669f709ec Mon Sep 17 00:00:00 2001
From: Artemiy <artem-itf@yandex.ru>
Date: Sat, 6 Jan 2024 00:56:13 +0300
Subject: [PATCH] Xml errors fix (#11487)

# Description
Fixes #11264
This PR adds checks in `to xml` to output error for malformed xml
entries:
* With columns that are not one of `tag`, `attributes` or `content`
* With no `tag` when entry is not a string
* With `tag` that is not a string
This PR also replaces `attrs` with `attributes` in example and
extra_usage of `to xml` (column was originally named attrs and renamed
to attributes, but this was missed in docs)

# User-Facing Changes
`to xml` will produce error for conditions described above instead of
silently returning nothing

# Tests + Formatting
Added tests for `to xml` to check handling of malformed xml entries
---
 crates/nu-command/src/formats/to/xml.rs       | 36 +++++++++++++++----
 .../tests/format_conversions/xml.rs           | 36 +++++++++++++++++++
 2 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/crates/nu-command/src/formats/to/xml.rs b/crates/nu-command/src/formats/to/xml.rs
index 57026f445..62bc79f59 100644
--- a/crates/nu-command/src/formats/to/xml.rs
+++ b/crates/nu-command/src/formats/to/xml.rs
@@ -34,10 +34,10 @@ impl Command for ToXml {
     fn extra_usage(&self) -> &str {
         r#"Every XML entry is represented via a record with tag, attribute and content fields.
 To represent different types of entries different values must be written to this fields:
-1. Tag entry: `{tag: <tag name> attrs: {<attr name>: "<string value>" ...} content: [<entries>]}`
-2. Comment entry: `{tag: '!' attrs: null content: "<comment string>"}`
-3. Processing instruction (PI): `{tag: '?<pi name>' attrs: null content: "<pi content string>"}`
-4. Text: `{tag: null attrs: null content: "<text>"}`. Or as plain `<text>` instead of record.
+1. Tag entry: `{tag: <tag name> attributes: {<attr name>: "<string value>" ...} content: [<entries>]}`
+2. Comment entry: `{tag: '!' attributes: null content: "<comment string>"}`
+3. Processing instruction (PI): `{tag: '?<pi name>' attributes: null content: "<pi content string>"}`
+4. Text: `{tag: null attributes: null content: "<text>"}`. Or as plain `<text>` instead of record.
 
 Additionally any field which is: empty record, empty list or null, can be omitted."#
     }
@@ -46,7 +46,7 @@ Additionally any field which is: empty record, empty list or null, can be omitte
         vec![
             Example {
                 description: "Outputs an XML string representing the contents of this table",
-                example: r#"{tag: note attributes: {} content : [{tag: remember attributes: {} content : [{tag: null attrs: null content : Event}]}]} | to xml"#,
+                example: r#"{tag: note attributes: {} content : [{tag: remember attributes: {} content : [{tag: null attributes: null content : Event}]}]} | to xml"#,
                 result: Some(Value::test_string(
                     "<note><remember>Event</remember></note>",
                 )),
@@ -110,6 +110,17 @@ fn to_xml_entry<W: Write>(
     }
 
     if let Value::Record { val: record, .. } = &entry {
+        if let Some(bad_column) = find_invalid_column(record) {
+            return Err(ShellError::CantConvert {
+                to_type: "XML".into(),
+                from_type: "record".into(),
+                span: entry_span,
+                help: Some(format!(
+                    "Invalid column \"{}\" in xml entry. Only \"{}\", \"{}\" and \"{}\" are permitted",
+                    bad_column, COLUMN_TAG_NAME, COLUMN_ATTRS_NAME, COLUMN_CONTENT_NAME
+                )),
+            });
+        }
         // If key is not found it is assumed to be nothing. This way
         // user can write a tag like {tag: a content: [...]} instead
         // of longer {tag: a attributes: {} content: [...]}
@@ -144,7 +155,12 @@ fn to_xml_entry<W: Write>(
             (Value::String { val: tag_name, .. }, attrs, children) => to_tag_like(
                 entry_span, tag_name, tag_span, attrs, children, top_level, writer,
             ),
-            _ => Ok(()),
+            _ => Err(ShellError::CantConvert {
+                to_type: "XML".into(),
+                from_type: "record".into(),
+                span: entry_span,
+                help: Some("Tag missing or is not a string".into()),
+            }),
         }
     } else {
         Err(ShellError::CantConvert {
@@ -156,6 +172,14 @@ fn to_xml_entry<W: Write>(
     }
 }
 
+fn find_invalid_column(record: &Record) -> Option<&String> {
+    const VALID_COLS: [&str; 3] = [COLUMN_TAG_NAME, COLUMN_ATTRS_NAME, COLUMN_CONTENT_NAME];
+    record
+        .cols
+        .iter()
+        .find(|col| !VALID_COLS.contains(&col.as_str()))
+}
+
 /// Convert record to tag-like entry: tag, PI, comment.
 fn to_tag_like<W: Write>(
     entry_span: Span,
diff --git a/crates/nu-command/tests/format_conversions/xml.rs b/crates/nu-command/tests/format_conversions/xml.rs
index b92186e16..5946ff494 100644
--- a/crates/nu-command/tests/format_conversions/xml.rs
+++ b/crates/nu-command/tests/format_conversions/xml.rs
@@ -22,3 +22,39 @@ fn table_to_xml_text_and_from_xml_text_back_into_table() {
 
     assert_eq!(actual.out, "true");
 }
+
+#[test]
+fn to_xml_error_unknown_column() {
+    let actual = nu!(
+        cwd: "tests/fixtures/formats", pipeline(
+        r#"
+            {tag: a bad_column: b} | to xml
+        "#
+    ));
+
+    assert!(actual.err.contains("Invalid column \"bad_column\""));
+}
+
+#[test]
+fn to_xml_error_no_tag() {
+    let actual = nu!(
+        cwd: "tests/fixtures/formats", pipeline(
+        r#"
+            {attributes: {a: b c: d}} | to xml
+        "#
+    ));
+
+    assert!(actual.err.contains("Tag missing"));
+}
+
+#[test]
+fn to_xml_error_tag_not_string() {
+    let actual = nu!(
+        cwd: "tests/fixtures/formats", pipeline(
+        r#"
+            {tag: 1 attributes: {a: b c: d}} | to xml
+        "#
+    ));
+
+    assert!(actual.err.contains("not a string"));
+}