dev_container: Make all `PortAttributes` fields optional (#53799)

Kunall Banerjee created 1 week ago

The Dev Container [metadata
reference](https://containers.dev/implementors/json_reference/#port-attributes)
defines all five `PortAttributes` fields (`elevateIfNeeded`, `label`,
`onAutoForward`, `protocol`, `requireLocalPort`) as optional with
defined defaults. The struct required all of them, causing
deserialization failures for any `portsAttributes` entry that omitted a
field.

Add `#[serde(default)]` to each field, wrap `label` and `protocol` in
`Option<T>` (spec default: not set), and derive `Default` on
`OnAutoForward` with `Notify` as the default variant (spec default:
`notify`). The bool fields already default to false via
`serde(default)`, matching the spec.

Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [ ] Unsafe blocks (if any) have justifying comments
- [ ] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [x] Tests cover the new/changed behavior
- [ ] Performance impact has been considered and is acceptable

Closes #53686.

Release Notes:

- Made all `PortAttributes` fields optional according to the Dev
Container spec

Change summary

crates/dev_container/src/devcontainer_json.rs | 103 ++++++++++++++++----
1 file changed, 82 insertions(+), 21 deletions(-)

Detailed changes

crates/dev_container/src/devcontainer_json.rs

@@ -19,9 +19,10 @@ pub(crate) enum PortAttributeProtocol {
     Http,
 }
 
-#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
+#[derive(Clone, Debug, Default, Deserialize, Serialize, Eq, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub(crate) enum OnAutoForward {
+    #[default]
     Notify,
     OpenBrowser,
     OpenBrowserOnce,
@@ -33,11 +34,16 @@ pub(crate) enum OnAutoForward {
 #[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub(crate) struct PortAttributes {
-    label: String,
+    #[serde(default)]
+    label: Option<String>,
+    #[serde(default)]
     on_auto_forward: OnAutoForward,
+    #[serde(default)]
     elevate_if_needed: bool,
+    #[serde(default)]
     require_local_port: bool,
-    protocol: PortAttributeProtocol,
+    #[serde(default)]
+    protocol: Option<PortAttributeProtocol>,
 }
 
 #[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -824,30 +830,30 @@ mod test {
                     (
                         "3000".to_string(),
                         PortAttributes {
-                            label: "This Port".to_string(),
+                            label: Some("This Port".to_string()),
                             on_auto_forward: OnAutoForward::Notify,
                             elevate_if_needed: false,
                             require_local_port: true,
-                            protocol: PortAttributeProtocol::Https
+                            protocol: Some(PortAttributeProtocol::Https)
                         }
                     ),
                     (
                         "db:5432".to_string(),
                         PortAttributes {
-                            label: "This Port too".to_string(),
+                            label: Some("This Port too".to_string()),
                             on_auto_forward: OnAutoForward::Silent,
                             elevate_if_needed: true,
                             require_local_port: false,
-                            protocol: PortAttributeProtocol::Http
+                            protocol: Some(PortAttributeProtocol::Http)
                         }
                     )
                 ])),
                 other_ports_attributes: Some(PortAttributes {
-                    label: "Other Ports".to_string(),
+                    label: Some("Other Ports".to_string()),
                     on_auto_forward: OnAutoForward::OpenBrowser,
                     elevate_if_needed: true,
                     require_local_port: true,
-                    protocol: PortAttributeProtocol::Https
+                    protocol: Some(PortAttributeProtocol::Https)
                 }),
                 update_remote_user_uid: Some(true),
                 remote_env: Some(HashMap::from([
@@ -1043,30 +1049,30 @@ mod test {
                     (
                         "3000".to_string(),
                         PortAttributes {
-                            label: "This Port".to_string(),
+                            label: Some("This Port".to_string()),
                             on_auto_forward: OnAutoForward::Notify,
                             elevate_if_needed: false,
                             require_local_port: true,
-                            protocol: PortAttributeProtocol::Https
+                            protocol: Some(PortAttributeProtocol::Https)
                         }
                     ),
                     (
                         "db:5432".to_string(),
                         PortAttributes {
-                            label: "This Port too".to_string(),
+                            label: Some("This Port too".to_string()),
                             on_auto_forward: OnAutoForward::Silent,
                             elevate_if_needed: true,
                             require_local_port: false,
-                            protocol: PortAttributeProtocol::Http
+                            protocol: Some(PortAttributeProtocol::Http)
                         }
                     )
                 ])),
                 other_ports_attributes: Some(PortAttributes {
-                    label: "Other Ports".to_string(),
+                    label: Some("Other Ports".to_string()),
                     on_auto_forward: OnAutoForward::OpenBrowser,
                     elevate_if_needed: true,
                     require_local_port: true,
-                    protocol: PortAttributeProtocol::Https
+                    protocol: Some(PortAttributeProtocol::Https)
                 }),
                 update_remote_user_uid: Some(true),
                 remote_env: Some(HashMap::from([
@@ -1271,30 +1277,30 @@ mod test {
                     (
                         "3000".to_string(),
                         PortAttributes {
-                            label: "This Port".to_string(),
+                            label: Some("This Port".to_string()),
                             on_auto_forward: OnAutoForward::Notify,
                             elevate_if_needed: false,
                             require_local_port: true,
-                            protocol: PortAttributeProtocol::Https
+                            protocol: Some(PortAttributeProtocol::Https)
                         }
                     ),
                     (
                         "db:5432".to_string(),
                         PortAttributes {
-                            label: "This Port too".to_string(),
+                            label: Some("This Port too".to_string()),
                             on_auto_forward: OnAutoForward::Silent,
                             elevate_if_needed: true,
                             require_local_port: false,
-                            protocol: PortAttributeProtocol::Http
+                            protocol: Some(PortAttributeProtocol::Http)
                         }
                     )
                 ])),
                 other_ports_attributes: Some(PortAttributes {
-                    label: "Other Ports".to_string(),
+                    label: Some("Other Ports".to_string()),
                     on_auto_forward: OnAutoForward::OpenBrowser,
                     elevate_if_needed: true,
                     require_local_port: true,
-                    protocol: PortAttributeProtocol::Https
+                    protocol: Some(PortAttributeProtocol::Https)
                 }),
                 update_remote_user_uid: Some(true),
                 remote_env: Some(HashMap::from([
@@ -1504,6 +1510,60 @@ mod test {
         assert_eq!(rendered, "type=tmpfs,target=/tmp,consistency=cached");
     }
 
+    #[test]
+    fn should_deserialize_port_attributes_with_missing_optional_fields() {
+        let json = r#"
+        {
+            "image": "nginx",
+            "portsAttributes": {
+                "8080": {
+                    "label": "app",
+                    "onAutoForward": "silent"
+                }
+            }
+        }
+        "#;
+
+        let result = deserialize_devcontainer_json(json);
+        assert!(
+            result.is_ok(),
+            "Expected deserialization to succeed with partial portsAttributes, got: {:?}",
+            result.err()
+        );
+
+        let devcontainer = result.unwrap();
+        let port_attrs = devcontainer.ports_attributes.unwrap();
+        let attrs = port_attrs.get("8080").unwrap();
+        assert_eq!(attrs.elevate_if_needed, false);
+        assert_eq!(attrs.require_local_port, false);
+    }
+
+    #[test]
+    fn should_deserialize_port_attributes_with_all_fields_omitted() {
+        let json = r#"
+        {
+            "image": "nginx",
+            "portsAttributes": {
+                "3000": {}
+            }
+        }
+        "#;
+
+        let result = deserialize_devcontainer_json(json);
+        assert!(
+            result.is_ok(),
+            "Expected deserialization to succeed with empty portsAttributes, got: {:?}",
+            result.err()
+        );
+
+        let devcontainer = result.unwrap();
+        let port_attrs = devcontainer.ports_attributes.unwrap();
+        let attrs = port_attrs.get("3000").unwrap();
+        assert_eq!(attrs.on_auto_forward, OnAutoForward::Notify);
+        assert_eq!(attrs.elevate_if_needed, false);
+        assert_eq!(attrs.require_local_port, false);
+    }
+
     #[test]
     fn should_fail_validation_with_workspace_mount_only() {
         let given_image_container_json = r#"
@@ -1579,6 +1639,7 @@ mod test {
             ))
         );
     }
+
     #[test]
     fn should_pass_validation_with_workspace_folder_for_docker_compose() {
         let given_image_container_json = r#"