From 14025084168d3d9ecb152fa32568bda9cf011a67 Mon Sep 17 00:00:00 2001
From: Marshall Bruner <43101405+brunerm99@users.noreply.github.com>
Date: Tue, 10 Oct 2023 19:24:23 +0200
Subject: [PATCH] give better error if required field of `url join` is invalid
 (#10589)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

# Description
Fix #10506 by adding `ExpectedNonNull` `ShellError` if required field is
entered as `$nothing`, `null`, " ", etc.

This adds a new `ShellError`, `ExpectedNonNull`, taking the expected
type and span.

# User-Facing Changes
Will get a more helpful error in the case described by #10506. Examples:
```nushell
➜ {scheme: "", host: "github.com"} | url join
Error: nu::shell::expected_non_null

  × Expected string found null.
   ╭─[entry #16:1:1]
 1 │ {scheme: "", host: "github.com"} | url join
   ·          ─┬
   ·           ╰── expected string, found null
   ╰────
```

```nushell
❯ {scheme: "https", host: null} | url join
Error: nu::shell::expected_non_null

  × Expected string found null.
   ╭─[entry #19:1:1]
 1 │ {scheme: "https", host: null} | url join
   ·                         ──┬─
   ·                           ╰── expected string, found null
   ╰────
```

# Tests + Formatting
All pass.
---
 crates/nu-command/src/network/url/join.rs | 157 ++++++++++++----------
 1 file changed, 86 insertions(+), 71 deletions(-)

diff --git a/crates/nu-command/src/network/url/join.rs b/crates/nu-command/src/network/url/join.rs
index fd0bc7568c..557b3c3943 100644
--- a/crates/nu-command/src/network/url/join.rs
+++ b/crates/nu-command/src/network/url/join.rs
@@ -223,81 +223,96 @@ impl UrlComponents {
             };
         }
 
-        // a part from port and params all other keys are strings.
-        match value.as_string() {
-            Ok(s) => {
-                if s.trim().is_empty() {
-                    Ok(self)
+        // apart from port and params all other keys are strings.
+        let s = value.as_string()?; // If value fails String conversion, just output this ShellError
+        if !Self::check_empty_string_ok(&key, &s, value_span)? {
+            return Ok(self);
+        }
+        match key.as_str() {
+            "host" => Ok(Self {
+                host: Some(s),
+                ..self
+            }),
+            "scheme" => Ok(Self {
+                scheme: Some(s),
+                ..self
+            }),
+            "username" => Ok(Self {
+                username: Some(s),
+                ..self
+            }),
+            "password" => Ok(Self {
+                password: Some(s),
+                ..self
+            }),
+            "path" => Ok(Self {
+                path: Some(if s.starts_with('/') {
+                    s
                 } else {
-                    match key.as_str() {
-                        "host" => Ok(Self {
-                            host: Some(s),
-                            ..self
-                        }),
-                        "scheme" => Ok(Self {
-                            scheme: Some(s),
-                            ..self
-                        }),
-                        "username" => Ok(Self {
-                            username: Some(s),
-                            ..self
-                        }),
-                        "password" => Ok(Self {
-                            password: Some(s),
-                            ..self
-                        }),
-                        "path" => Ok(Self {
-                            path: Some(if s.starts_with('/') {
-                                s
-                            } else {
-                                format!("/{s}")
-                            }),
-                            ..self
-                        }),
-                        "query" => {
-                            if let Some(q) = self.query {
-                                if q != s {
-                                    // if query is present it means that also params_span is set.
-                                    return Err(ShellError::IncompatibleParameters {
-                                        left_message: format!("Mismatch, query param is: {s}"),
-                                        left_span: value.span(),
-                                        right_message: format!("instead qs from params is: {q}"),
-                                        right_span: self.params_span.unwrap_or(Span::unknown()),
-                                    });
-                                }
-                            }
-
-                            Ok(Self {
-                                query: Some(format!("?{s}")),
-                                query_span: Some(value.span()),
-                                ..self
-                            })
-                        }
-                        "fragment" => Ok(Self {
-                            fragment: Some(if s.starts_with('#') {
-                                s
-                            } else {
-                                format!("#{s}")
-                            }),
-                            ..self
-                        }),
-                        _ => {
-                            nu_protocol::report_error_new(
-                                engine_state,
-                                &ShellError::GenericError(
-                                    format!("'{key}' is not a valid URL field"),
-                                    format!("Remove '{key}' col from input record"),
-                                    Some(span),
-                                    None,
-                                    vec![],
-                                ),
-                            );
-                            Ok(self)
-                        }
+                    format!("/{s}")
+                }),
+                ..self
+            }),
+            "query" => {
+                if let Some(q) = self.query {
+                    if q != s {
+                        // if query is present it means that also params_span is set.
+                        return Err(ShellError::IncompatibleParameters {
+                            left_message: format!("Mismatch, query param is: {s}"),
+                            left_span: value.span(),
+                            right_message: format!("instead qs from params is: {q}"),
+                            right_span: self.params_span.unwrap_or(Span::unknown()),
+                        });
                     }
                 }
+
+                Ok(Self {
+                    query: Some(format!("?{s}")),
+                    query_span: Some(value.span()),
+                    ..self
+                })
             }
-            _ => Ok(self),
+            "fragment" => Ok(Self {
+                fragment: Some(if s.starts_with('#') {
+                    s
+                } else {
+                    format!("#{s}")
+                }),
+                ..self
+            }),
+            _ => {
+                nu_protocol::report_error_new(
+                    engine_state,
+                    &ShellError::GenericError(
+                        format!("'{key}' is not a valid URL field"),
+                        format!("remove '{key}' col from input record"),
+                        Some(span),
+                        None,
+                        vec![],
+                    ),
+                );
+                Ok(self)
+            }
+        }
+    }
+
+    // Check if value is empty. If so, check if that is fine, i.e., not a required input
+    fn check_empty_string_ok(key: &str, s: &str, value_span: Span) -> Result<bool, ShellError> {
+        if !s.trim().is_empty() {
+            return Ok(true);
+        }
+        match key {
+            "host" => Err(ShellError::UnsupportedConfigValue(
+                "non-empty string".into(),
+                "empty string".into(),
+                value_span,
+            )),
+            "scheme" => Err(ShellError::UnsupportedConfigValue(
+                "non-empty string".into(),
+                "empty string".into(),
+                value_span,
+            )),
+            _ => Ok(false),
         }
     }