Appropriately deserialize app_port in devcontainer.json (#53322)

KyleBarton created

Self-Review Checklist:

- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] 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
- [x] Performance impact has been considered and is acceptable

Closes #53263

Properly deserializes the app_port field, even if it is an array of
strings. Also, removes app_port from consideration when building a
docker-compose-based dev container, since the spec specifies that it
only applies to non-docker-compose dev containers.

Release Notes:

- Fixed app_port deserialization in dev container

Change summary

crates/dev_container/src/devcontainer_json.rs     | 65 ++++++++++++++--
crates/dev_container/src/devcontainer_manifest.rs | 47 ++---------
2 files changed, 66 insertions(+), 46 deletions(-)

Detailed changes

crates/dev_container/src/devcontainer_json.rs 🔗

@@ -217,8 +217,8 @@ pub(crate) struct DevContainer {
     pub(crate) override_feature_install_order: Option<Vec<String>>,
     pub(crate) customizations: Option<ZedCustomizationsWrapper>,
     pub(crate) build: Option<ContainerBuild>,
-    #[serde(default, deserialize_with = "deserialize_string_or_int")]
-    pub(crate) app_port: Option<String>,
+    #[serde(default, deserialize_with = "deserialize_app_port")]
+    pub(crate) app_port: Vec<String>,
     #[serde(default, deserialize_with = "deserialize_mount_definition")]
     pub(crate) workspace_mount: Option<MountDefinition>,
     pub(crate) workspace_folder: Option<String>,
@@ -517,7 +517,7 @@ where
     Ok(Some(mounts))
 }
 
-fn deserialize_string_or_int<'de, D>(deserializer: D) -> Result<Option<String>, D::Error>
+fn deserialize_app_port<'de, D>(deserializer: D) -> Result<Vec<String>, D::Error>
 where
     D: serde::Deserializer<'de>,
 {
@@ -530,9 +530,29 @@ where
         Int(u32),
     }
 
-    match StringOrInt::deserialize(deserializer)? {
-        StringOrInt::String(s) => Ok(Some(s)),
-        StringOrInt::Int(b) => Ok(Some(b.to_string())),
+    #[derive(Deserialize)]
+    #[serde(untagged)]
+    enum AppPort {
+        Array(Vec<StringOrInt>),
+        Single(StringOrInt),
+    }
+
+    fn normalize_port(value: StringOrInt) -> String {
+        match value {
+            StringOrInt::String(s) => {
+                if s.contains(':') {
+                    s
+                } else {
+                    format!("{s}:{s}")
+                }
+            }
+            StringOrInt::Int(n) => format!("{n}:{n}"),
+        }
+    }
+
+    match AppPort::deserialize(deserializer)? {
+        AppPort::Single(value) => Ok(vec![normalize_port(value)]),
+        AppPort::Array(values) => Ok(values.into_iter().map(normalize_port).collect()),
     }
 }
 
@@ -862,7 +882,7 @@ mod test {
                     memory: Some("8gb".to_string()),
                     storage: Some("32gb".to_string()),
                 }),
-                app_port: Some("8081".to_string()),
+                app_port: vec!["8081:8081".to_string()],
                 container_env: Some(HashMap::from([
                     ("MYVAR3".to_string(), "myvar3".to_string()),
                     ("MYVAR4".to_string(), "myvar4".to_string())
@@ -1304,7 +1324,7 @@ mod test {
                     memory: Some("8gb".to_string()),
                     storage: Some("32gb".to_string()),
                 }),
-                app_port: Some("8081".to_string()),
+                app_port: vec!["8081:8081".to_string()],
                 container_env: Some(HashMap::from([
                     ("MYVAR3".to_string(), "myvar3".to_string()),
                     ("MYVAR4".to_string(), "myvar4".to_string())
@@ -1349,6 +1369,35 @@ mod test {
         assert_eq!(devcontainer.build_type(), DevContainerBuildType::Dockerfile);
     }
 
+    #[test]
+    fn should_deserialize_app_port_array() {
+        let given_json = r#"
+            // These are some external comments. serde_lenient should handle them
+            {
+                // These are some internal comments
+                "name": "myDevContainer",
+                "remoteUser": "root",
+                "appPort": [
+                    "8081:8083",
+                    "9001",
+                ],
+                "build": {
+                   	"dockerfile": "DockerFile",
+                }
+            }
+            "#;
+
+        let result = deserialize_devcontainer_json(given_json);
+
+        assert!(result.is_ok());
+        let devcontainer = result.expect("ok");
+
+        assert_eq!(
+            devcontainer.app_port,
+            vec!["8081:8083".to_string(), "9001:9001".to_string()]
+        )
+    }
+
     #[test]
     fn mount_definition_should_use_bind_type_for_unix_absolute_paths() {
         let mount = MountDefinition {

crates/dev_container/src/devcontainer_manifest.rs 🔗

@@ -1229,35 +1229,6 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true
                 }
             }
         }
-        if let Some(port) = &self.dev_container().app_port {
-            if let Some(network_service_name) = network_mode_service {
-                if let Some(service) = service_declarations.get_mut(network_service_name) {
-                    service.ports.push(DockerComposeServicePort {
-                        target: port.clone(),
-                        published: port.clone(),
-                        ..Default::default()
-                    });
-                } else {
-                    service_declarations.insert(
-                        network_service_name.to_string(),
-                        DockerComposeService {
-                            ports: vec![DockerComposeServicePort {
-                                target: port.clone(),
-                                published: port.clone(),
-                                ..Default::default()
-                            }],
-                            ..Default::default()
-                        },
-                    );
-                }
-            } else {
-                main_service.ports.push(DockerComposeServicePort {
-                    target: port.clone(),
-                    published: port.clone(),
-                    ..Default::default()
-                });
-            }
-        }
 
         service_declarations.insert(main_service_name.to_string(), main_service);
         let new_docker_compose_config = DockerComposeConfig {
@@ -1811,9 +1782,10 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true
                 }
             }
         }
-        if let Some(app_port) = &self.dev_container().app_port {
+        for app_port in &self.dev_container().app_port {
             command.arg("-p");
-            command.arg(format!("{app_port}:{app_port}"));
+            // Should just implement display for an AppPort struct which takes care of this; it might be a custom map like (literally) "8081:8080"
+            command.arg(app_port);
         }
 
         command.arg("--entrypoint");
@@ -2997,7 +2969,10 @@ mod test {
                 8082,
                 8083,
               ],
-              "appPort": "8084",
+              "appPort": [
+                8084,
+                "8085:8086",
+              ],
 
               "containerEnv": {
                 "VARIABLE_VALUE": "value",
@@ -3301,6 +3276,8 @@ chmod +x ./install.sh
                 "8083:8083".to_string(),
                 "-p".to_string(),
                 "8084:8084".to_string(),
+                "-p".to_string(),
+                "8085:8086".to_string(),
                 "--entrypoint".to_string(),
                 "/bin/sh".to_string(),
                 "sha256:610e6cfca95280188b021774f8cf69dd6f49bdb6eebc34c5ee2010f4d51cc105".to_string(),
@@ -3357,7 +3334,6 @@ chmod +x ./install.sh
                 "db:5432",
                 "db:1234",
               ],
-              "appPort": "8084",
 
               // Use 'postCreateCommand' to run commands after the container is created.
               // "postCreateCommand": "rustc --version",
@@ -3631,11 +3607,6 @@ ENV DOCKER_BUILDKIT=1
                                 published: "1234".to_string(),
                                 ..Default::default()
                             },
-                            DockerComposeServicePort {
-                                target: "8084".to_string(),
-                                published: "8084".to_string(),
-                                ..Default::default()
-                            },
                         ],
                         ..Default::default()
                     },