Make β€˜var’ attribute of a Field optional

mb created

Looking at [the spec](https://xmpp.org/extensions/xep-0004.html#protocol-field)
it seems valid not to have a `var` attribute set, at least for fields of type
`fixed` that is:

> If the element type is anything other than "fixed" (see below), it MUST
> possess a 'var' attribute that uniquely identifies the field in the context
> of the form (if it is "fixed", it MAY possess a 'var' attribute). The element
> MAY possess a 'label' attribute that defines a human-readable name for the field.

Change summary

parsers/src/caps.rs          |  6 +++-
parsers/src/data_forms.rs    | 50 ++++++++++++++++++++++++++++++++++---
parsers/src/ecaps2.rs        |  5 +++
parsers/src/media_element.rs |  2 
parsers/src/pubsub/owner.rs  |  4 +-
parsers/src/pubsub/pubsub.rs |  2 
parsers/src/server_info.rs   | 26 +++++++++---------
7 files changed, 71 insertions(+), 24 deletions(-)

Detailed changes

parsers/src/caps.rs πŸ”—

@@ -127,10 +127,12 @@ fn compute_extensions(extensions: &[DataForm]) -> Vec<u8> {
         }
         bytes.push(b'<');
         for field in extension.fields.clone() {
-            if field.var == "FORM_TYPE" {
+            if field.var.as_deref() == Some("FORM_TYPE") {
                 continue;
             }
-            bytes.append(&mut compute_item(&field.var));
+            if let Some(var) = &field.var {
+                bytes.append(&mut compute_item(var));
+            }
             bytes.append(&mut compute_items(&field.values, |value| {
                 compute_item(value)
             }));

parsers/src/data_forms.rs πŸ”—

@@ -71,7 +71,7 @@ generate_attribute!(
 #[derive(Debug, Clone, PartialEq)]
 pub struct Field {
     /// The unique identifier for this field, in the form.
-    pub var: String,
+    pub var: Option<String>,
 
     /// The type of this field.
     pub type_: FieldType,
@@ -96,7 +96,7 @@ impl Field {
     /// Create a new Field, of the given var and type.
     pub fn new(var: &str, type_: FieldType) -> Field {
         Field {
-            var: String::from(var),
+            var: Some(String::from(var)),
             type_,
             label: None,
             required: false,
@@ -128,7 +128,7 @@ impl Field {
     /// criteria differ slightly among form types.
     pub fn is_form_type(&self, ty: &DataFormType) -> bool {
         // 1. A field must have the var FORM_TYPE
-        if self.var != "FORM_TYPE" {
+        if self.var.as_deref() != Some("FORM_TYPE") {
             return false;
         }
 
@@ -170,7 +170,7 @@ impl TryFrom<Element> for Field {
         check_self!(elem, "field", DATA_FORMS);
         check_no_unknown_attributes!(elem, "field", ["label", "type", "var"]);
         let mut field = Field {
-            var: get_attr!(elem, "var", Required),
+            var: get_attr!(elem, "var", Option),
             type_: get_attr!(elem, "type", Default),
             label: get_attr!(elem, "label", Option),
             required: false,
@@ -178,6 +178,11 @@ impl TryFrom<Element> for Field {
             values: vec![],
             media: vec![],
         };
+
+        if field.type_ != FieldType::Fixed && field.var.is_none() {
+            return Err(Error::ParseError("Required attribute 'var' missing."));
+        }
+
         for element in elem.children() {
             if element.is("value", ns::DATA_FORMS) {
                 check_no_children!(element, "value");
@@ -393,6 +398,43 @@ mod tests {
         assert!(form.fields.is_empty());
     }
 
+    #[test]
+    fn test_missing_var() {
+        let elem: Element =
+            "<x xmlns='jabber:x:data' type='form'><field type='text-single' label='The name of your bot'/></x>"
+                .parse()
+                .unwrap();
+        let error = DataForm::try_from(elem).unwrap_err();
+        let message = match error {
+            Error::ParseError(string) => string,
+            _ => panic!(),
+        };
+        assert_eq!(message, "Required attribute 'var' missing.");
+    }
+
+    #[test]
+    fn test_fixed_field() {
+        let elem: Element =
+            "<x xmlns='jabber:x:data' type='form'><field type='fixed'><value>Section 1: Bot Info</value></field></x>"
+                .parse()
+                .unwrap();
+        let form = DataForm::try_from(elem).unwrap();
+        assert_eq!(form.type_, DataFormType::Form);
+        assert!(form.form_type.is_none());
+        assert_eq!(
+            form.fields,
+            vec![Field {
+                var: None,
+                type_: FieldType::Fixed,
+                label: None,
+                required: false,
+                options: vec![],
+                values: vec!["Section 1: Bot Info".to_string()],
+                media: vec![],
+            }]
+        );
+    }
+
     #[test]
     fn test_invalid() {
         let elem: Element = "<x xmlns='jabber:x:data'/>".parse().unwrap();

parsers/src/ecaps2.rs πŸ”—

@@ -94,7 +94,10 @@ fn compute_extensions(extensions: &[DataForm]) -> Result<Vec<u8>, Error> {
         ));
         bytes.push(0x1e);
         bytes.append(&mut compute_items(&extension.fields, 0x1d, |field| {
-            let mut bytes = compute_item(&field.var);
+            let mut bytes = vec![];
+            if let Some(var) = &field.var {
+                bytes.append(&mut compute_item(var));
+            }
             bytes.append(&mut compute_items(&field.values, 0x1e, |value| {
                 compute_item(value)
             }));

parsers/src/media_element.rs πŸ”—

@@ -232,7 +232,7 @@ mod tests {
             .unwrap();
         let form = DataForm::try_from(elem).unwrap();
         assert_eq!(form.fields.len(), 1);
-        assert_eq!(form.fields[0].var, "ocr");
+        assert_eq!(form.fields[0].var.as_deref(), Some("ocr"));
         assert_eq!(form.fields[0].media[0].width, Some(290));
         assert_eq!(form.fields[0].media[0].height, Some(80));
         assert_eq!(form.fields[0].media[0].uris[0].type_, "image/jpeg");

parsers/src/pubsub/owner.rs πŸ”—

@@ -229,7 +229,7 @@ mod tests {
                 title: None,
                 instructions: None,
                 fields: vec![Field {
-                    var: String::from("pubsub#access_model"),
+                    var: Some(String::from("pubsub#access_model")),
                     type_: FieldType::ListSingle,
                     label: None,
                     required: false,
@@ -276,7 +276,7 @@ mod tests {
                 title: None,
                 instructions: None,
                 fields: vec![Field {
-                    var: String::from("pubsub#access_model"),
+                    var: Some(String::from("pubsub#access_model")),
                     type_: FieldType::ListSingle,
                     label: None,
                     required: false,

parsers/src/pubsub/pubsub.rs πŸ”—

@@ -621,7 +621,7 @@ mod tests {
                     title: None,
                     instructions: None,
                     fields: vec![Field {
-                        var: String::from("pubsub#access_model"),
+                        var: Some(String::from("pubsub#access_model")),
                         type_: FieldType::ListSingle,
                         label: None,
                         required: false,

parsers/src/server_info.rs πŸ”—

@@ -44,17 +44,17 @@ impl TryFrom<DataForm> for ServerInfo {
             if field.type_ != FieldType::ListMulti {
                 return Err(Error::ParseError("Field is not of the required type."));
             }
-            if field.var == "abuse-addresses" {
+            if field.var.as_deref() == Some("abuse-addresses") {
                 server_info.abuse = field.values;
-            } else if field.var == "admin-addresses" {
+            } else if field.var.as_deref() == Some("admin-addresses") {
                 server_info.admin = field.values;
-            } else if field.var == "feedback-addresses" {
+            } else if field.var.as_deref() == Some("feedback-addresses") {
                 server_info.feedback = field.values;
-            } else if field.var == "sales-addresses" {
+            } else if field.var.as_deref() == Some("sales-addresses") {
                 server_info.sales = field.values;
-            } else if field.var == "security-addresses" {
+            } else if field.var.as_deref() == Some("security-addresses") {
                 server_info.security = field.values;
-            } else if field.var == "support-addresses" {
+            } else if field.var.as_deref() == Some("support-addresses") {
                 server_info.support = field.values;
             } else {
                 return Err(Error::ParseError("Unknown form field var."));
@@ -87,7 +87,7 @@ impl From<ServerInfo> for DataForm {
 /// Generate `Field` for addresses
 pub fn generate_address_field<S: Into<String>>(var: S, values: Vec<String>) -> Field {
     Field {
-        var: var.into(),
+        var: Some(var.into()),
         type_: FieldType::ListMulti,
         label: None,
         required: false,
@@ -122,7 +122,7 @@ mod tests {
             instructions: None,
             fields: vec![
                 Field {
-                    var: String::from("abuse-addresses"),
+                    var: Some(String::from("abuse-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,
@@ -131,7 +131,7 @@ mod tests {
                     media: vec![],
                 },
                 Field {
-                    var: String::from("admin-addresses"),
+                    var: Some(String::from("admin-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,
@@ -144,7 +144,7 @@ mod tests {
                     media: vec![],
                 },
                 Field {
-                    var: String::from("feedback-addresses"),
+                    var: Some(String::from("feedback-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,
@@ -153,7 +153,7 @@ mod tests {
                     media: vec![],
                 },
                 Field {
-                    var: String::from("sales-addresses"),
+                    var: Some(String::from("sales-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,
@@ -162,7 +162,7 @@ mod tests {
                     media: vec![],
                 },
                 Field {
-                    var: String::from("security-addresses"),
+                    var: Some(String::from("security-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,
@@ -174,7 +174,7 @@ mod tests {
                     media: vec![],
                 },
                 Field {
-                    var: String::from("support-addresses"),
+                    var: Some(String::from("support-addresses")),
                     type_: FieldType::ListMulti,
                     label: None,
                     required: false,