fix: pick query params with empty values (tailcallhq#2470)

Co-authored-by: Tushar Mathur <[email protected]>
bnchi · Jul 19, 2024 · 8390cd9 · 8390cd9
1 parent 34fde45
commit 8390cd9
Show file tree

Hide file tree

Showing 7 changed files with 222 additions and 26 deletions.
diff --git a/src/core/http/query_encoder.rs b/src/core/http/query_encoder.rs
@@ -12,35 +12,48 @@ pub enum QueryEncoder {
 }
 
 impl QueryEncoder {
-    pub fn encode(&self, key: &str, raw_value: Option<ValueString>) -> Option<String> {
+    pub fn encode(&self, key: &str, raw_value: Option<ValueString>) -> String {
         if let Some(value) = raw_value {
             match &value {
                 ValueString::Value(val) => self.encode_const_value(key, val),
-                ValueString::String(val) => Some(format!("{}={}", key, val)),
+                ValueString::String(val) => format!("{}={}", key, val),
             }
         } else {
-            None
+            key.to_owned()
         }
     }
-    fn encode_const_value(&self, key: &str, value: &async_graphql::Value) -> Option<String> {
+    fn encode_const_value(&self, key: &str, value: &async_graphql::Value) -> String {
         match self {
             QueryEncoder::CommaSeparated => match value {
                 async_graphql::Value::List(list) if !list.is_empty() => {
                     let encoded_values: Vec<String> =
                         list.iter().filter_map(convert_value).collect();
-                    Some(format!("{}={}", key, encoded_values.join(",")))
+
+                    if encoded_values.is_empty() {
+                        key.to_string()
+                    } else {
+                        format!("{}={}", key, encoded_values.join(","))
+                    }
                 }
-                _ => convert_value(value).map(|val| format!("{}={}", key, val)),
+                _ => convert_value(value)
+                    .map(|val| format!("{}={}", key, val))
+                    .unwrap_or(key.to_string()),
             },
             QueryEncoder::RepeatedKey => match value {
                 async_graphql::Value::List(list) if !list.is_empty() => {
                     let encoded_values: Vec<String> = list
                         .iter()
-                        .filter_map(|val| self.encode_const_value(key, val))
+                        .map(|val| self.encode_const_value(key, val))
                         .collect();
-                    Some(encoded_values.join("&"))
+                    if encoded_values.is_empty() {
+                        key.to_string()
+                    } else {
+                        encoded_values.join("&")
+                    }
                 }
-                _ => convert_value(value).map(|val| format!("{}={}", key, val)),
+                _ => convert_value(value)
+                    .map(|val| format!("{}={}", key, val))
+                    .unwrap_or(key.to_string()),
             },
         }
     }
@@ -74,7 +87,7 @@ mod tests {
         let arg_raw_value = Some(ValueString::Value(Cow::Borrowed(&values)));
 
         let actual = encoder.encode("key", arg_raw_value);
-        let expected = Some("key=12,42,13".to_string());
+        let expected = "key=12,42,13".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -90,7 +103,7 @@ mod tests {
         let arg_raw_value = Some(ValueString::Value(Cow::Borrowed(&values)));
 
         let actual = encoder.encode("key", arg_raw_value);
-        let expected = Some("key=12&key=42&key=13".to_string());
+        let expected = "key=12&key=42&key=13".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -101,7 +114,7 @@ mod tests {
         let raw_value = Some(ValueString::String("env_value".into()));
 
         let actual = encoder.encode("key", raw_value);
-        let expected = Some("key=env_value".to_string());
+        let expected = "key=env_value".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -112,7 +125,7 @@ mod tests {
         let raw_value = Some(ValueString::String("var_value".into()));
 
         let actual = encoder.encode("key", raw_value);
-        let expected = Some("key=var_value".to_string());
+        let expected = "key=var_value".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -123,7 +136,7 @@ mod tests {
         let raw_value: Option<ValueString> = None;
 
         let actual = encoder.encode("key", raw_value);
-        let expected = None;
+        let expected = "key".to_owned();
 
         assert_eq!(actual, expected);
     }
@@ -139,7 +152,7 @@ mod tests {
         let strategy = QueryEncoder::CommaSeparated;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("ids=1,2,3".to_string());
+        let expected = "ids=1,2,3".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -155,7 +168,7 @@ mod tests {
         let strategy = QueryEncoder::RepeatedKey;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("ids=1&ids=2&ids=3".to_string());
+        let expected = "ids=1&ids=2&ids=3".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -171,7 +184,7 @@ mod tests {
         let strategy = QueryEncoder::CommaSeparated;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("values=string,42,true".to_string());
+        let expected = "values=string,42,true".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -187,7 +200,7 @@ mod tests {
         let strategy = QueryEncoder::RepeatedKey;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("values=string&values=42&values=true".to_string());
+        let expected = "values=string&values=42&values=true".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -199,7 +212,7 @@ mod tests {
         let strategy = QueryEncoder::CommaSeparated;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected: Option<String> = None;
+        let expected = "empty".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -211,7 +224,7 @@ mod tests {
         let strategy = QueryEncoder::RepeatedKey;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected: Option<String> = None;
+        let expected = "empty".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -223,7 +236,7 @@ mod tests {
         let strategy = QueryEncoder::CommaSeparated;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("single=value".to_string());
+        let expected = "single=value".to_string();
 
         assert_eq!(actual, expected);
     }
@@ -235,7 +248,7 @@ mod tests {
         let strategy = QueryEncoder::RepeatedKey;
 
         let actual = strategy.encode_const_value(key, &values);
-        let expected = Some("single=value".to_string());
+        let expected = "single=value".to_string();
 
         assert_eq!(actual, expected);
     }

diff --git a/src/core/http/request_template.rs b/src/core/http/request_template.rs
@@ -45,7 +45,7 @@ impl RequestTemplate {
         // template.
         let mustache_eval = ValueStringEval::default();
 
-        let extra_qp = self.query.iter().filter_map(|(key, value)| {
+        let extra_qp = self.query.iter().map(|(key, value)| {
             let parsed_value = mustache_eval.eval(value, ctx);
             self.query_encoder.encode(key, parsed_value)
         });
@@ -683,7 +683,7 @@ mod tests {
             let tmpl = RequestTemplate::try_from(endpoint).unwrap();
             let ctx = Context::default();
             let req = tmpl.to_request(&ctx).unwrap();
-            assert_eq!(req.url().to_string(), "http://localhost:3000/?q=1&b=1");
+            assert_eq!(req.url().to_string(), "http://localhost:3000/?q=1&b=1&c");
         }
 
         #[test]
@@ -725,7 +725,7 @@ mod tests {
             let req = tmpl.to_request(&ctx).unwrap();
             assert_eq!(
                 req.url().to_string(),
-                "http://localhost:3000/foo?b=foo&d=bar&f=baz"
+                "http://localhost:3000/foo?b=foo&d=bar&f=baz&e"
             );
         }
 

diff --git a/tests/core/snapshots/batching-group-by-optional-key.md_0.snap b/tests/core/snapshots/batching-group-by-optional-key.md_0.snap
@@ -0,0 +1,26 @@
+---
+source: tests/core/spec.rs
+expression: response
+---
+{
+  "status": 200,
+  "headers": {
+    "content-type": "application/json"
+  },
+  "body": {
+    "data": {
+      "posts": [
+        {
+          "user": {
+            "id": 1
+          },
+          "userId": 1
+        },
+        {
+          "user": null,
+          "userId": null
+        }
+      ]
+    }
+  }
+}
diff --git a/tests/core/snapshots/batching-group-by-optional-key.md_client.snap b/tests/core/snapshots/batching-group-by-optional-key.md_client.snap
@@ -0,0 +1,58 @@
+---
+source: tests/core/spec.rs
+expression: formatted
+---
+scalar Bytes
+
+scalar Date
+
+scalar Email
+
+scalar Empty
+
+scalar Int128
+
+scalar Int16
+
+scalar Int32
+
+scalar Int64
+
+scalar Int8
+
+scalar JSON
+
+scalar PhoneNumber
+
+type Post {
+  body: String
+  id: Int
+  title: String
+  user: User
+  userId: Int
+}
+
+type Query {
+  posts: [Post]
+}
+
+scalar UInt128
+
+scalar UInt16
+
+scalar UInt32
+
+scalar UInt64
+
+scalar UInt8
+
+scalar Url
+
+type User {
+  id: Int
+  name: String
+}
+
+schema {
+  query: Query
+}
diff --git a/tests/core/snapshots/batching-group-by-optional-key.md_merged.snap b/tests/core/snapshots/batching-group-by-optional-key.md_merged.snap
@@ -0,0 +1,35 @@
+---
+source: tests/core/spec.rs
+expression: formatter
+---
+schema
+  @server(port: 8000, queryValidation: false)
+  @upstream(
+    baseURL: "http://jsonplaceholder.typicode.com"
+    batch: {delay: 1, headers: [], maxSize: 1000}
+    httpCache: 42
+  ) {
+  query: Query
+}
+
+type Post {
+  body: String
+  id: Int
+  title: String
+  user: User
+    @http(
+      batchKey: ["id"]
+      path: "/users"
+      query: [{key: "id", value: "{{.value.userId}}"}, {key: "foo", value: "bar"}]
+    )
+  userId: Int
+}
+
+type Query {
+  posts: [Post] @http(path: "/posts?id=11&id=3&foo=1")
+}
+
+type User {
+  id: Int
+  name: String
+}
diff --git a/tests/execution/batching-group-by-optional-key.md b/tests/execution/batching-group-by-optional-key.md
@@ -0,0 +1,64 @@
+# Batching group by
+
+```graphql @config
+schema
+  @server(port: 8000, queryValidation: false)
+  @upstream(baseURL: "http://jsonplaceholder.typicode.com", httpCache: 42, batch: {delay: 1, maxSize: 1000}) {
+  query: Query
+}
+
+type Query {
+  posts: [Post] @http(path: "/posts?id=11&id=3&foo=1")
+}
+
+type Post {
+  id: Int
+  title: String
+  body: String
+  userId: Int
+  user: User
+    @http(
+      path: "/users"
+      query: [{key: "id", value: "{{.value.userId}}"}, {key: "foo", value: "bar"}]
+      batchKey: ["id"]
+    )
+}
+
+type User {
+  id: Int
+  name: String
+}
+```
+
+```yml @mock
+- request:
+    method: GET
+    url: http://jsonplaceholder.typicode.com/posts?id=11&id=3&foo=1
+  response:
+    status: 200
+    body:
+      - body: bar
+        id: 11
+        title: foo
+        userId: 1
+      - body: bar # no userId for bar
+        id: 3
+        title: foo
+- request:
+    method: GET
+    url: http://jsonplaceholder.typicode.com/users?id&foo=bar&id=1 # query should be id&foo=bar&id=1
+  response:
+    status: 200
+    body:
+      - id: 1
+        name: Leanne Graham
+      - id: 2
+        name: Ervin Howell
+```
+
+```yml @test
+- method: POST
+  url: http://localhost:8080/graphql
+  body:
+    query: query { posts { user { id } userId } }
+```
diff --git a/tests/execution/nullable-arg-query.md b/tests/execution/nullable-arg-query.md
@@ -19,7 +19,7 @@ type User {
 ```yml @mock
 - request:
     method: GET
-    url: http://jsonplaceholder.typicode.com/users
+    url: http://jsonplaceholder.typicode.com/users?id
   response:
     status: 200
     body: