fix(native): Jinja - pass kwargs correctly into Python (#9276)

In Python, *args and **kwargs allow functions to accept an arbitrary number of arguments. Kwarg should be passed separately when you are calling a function. It's a separate structure.

Python example:

@template.function
def arg_named_arguments(arg1, arg2):
    return "arg1: " + arg1 + ", arg2: " + arg2

Jinja example:

arg_named_arguments1: {{ arg_named_arguments(arg2="2 arg", arg1="1 arg") }}
arg_named_arguments2: {{ arg_named_arguments(arg1="1 arg", arg2="2 arg") }}

Expected output:

arg_named_arguments1: "arg1: 1 arg, arg2: 2 arg"
arg_named_arguments2: "arg1: 1 arg, arg2: 2 arg"

Author: ovr

packages/cubejs-backend-native/src/cross/clrepr.rs

@@ -15,18 +15,24 @@ use std::collections::hash_map::{IntoIter, Iter, Keys};
 use std::collections::HashMap;
 use std::sync::Arc;
 
+#[derive(Debug, Clone)]
+pub enum CLReprObjectKind {
+    Object,
+    KWargs,
+}
+
 #[derive(Clone)]
-pub struct CLReprObject(pub(crate) HashMap<String, CLRepr>);
+pub struct CLReprObject(pub(crate) HashMap<String, CLRepr>, CLReprObjectKind);
 
 impl Default for CLReprObject {
     fn default() -> Self {
-        Self::new()
+        Self::new(CLReprObjectKind::Object)
     }
 }
 
 impl CLReprObject {
-    pub fn new() -> Self {
-        Self(HashMap::new())
+    pub fn new(kind: CLReprObjectKind) -> Self {
+        Self(HashMap::new(), kind)
     }
 
     pub fn get(&self, key: &str) -> Option<&CLRepr> {
@@ -110,6 +116,15 @@ pub enum CLRepr {
     Null,
 }
 
+impl CLRepr {
+    pub fn is_kwarg(&self) -> bool {
+        match self {
+            CLRepr::Object(obj) => matches!(obj.1, CLReprObjectKind::KWargs),
+            _ => false,
+        }
+    }
+}
+
 impl std::fmt::Display for CLRepr {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         std::fmt::Display::fmt(&self, f)
@@ -179,7 +194,7 @@ impl CLRepr {
 
             Ok(CLRepr::Array(r))
         } else if from.is_a::<JsObject, _>(cx) {
-            let mut obj = CLReprObject::new();
+            let mut obj = CLReprObject::new(CLReprObjectKind::Object);
 
             let v = from.downcast_or_throw::<JsObject, _>(cx)?;
             let properties = v.get_own_property_names(cx)?;

packages/cubejs-backend-native/src/cross/clrepr_python.rs

@@ -1,5 +1,5 @@
 use crate::cross::clrepr::CLReprObject;
-use crate::cross::{CLRepr, StringType};
+use crate::cross::{CLRepr, CLReprObjectKind, StringType};
 use pyo3::exceptions::{PyNotImplementedError, PyTypeError};
 use pyo3::types::{
     PyBool, PyComplex, PyDate, PyDict, PyFloat, PyFrame, PyFunction, PyInt, PyList, PySequence,
@@ -16,15 +16,9 @@ pub enum PythonRef {
     PyExternalFunction(Py<PyFunction>),
 }
 
-pub trait CLReprPython: Sized {
-    fn from_python_ref(v: &PyAny) -> Result<Self, PyErr>;
-    fn into_py_impl(from: CLRepr, py: Python) -> Result<PyObject, PyErr>;
-    fn into_py(self, py: Python) -> Result<PyObject, PyErr>;
-}
-
-impl CLReprPython for CLRepr {
+impl CLRepr {
     /// Convert python value to CLRepr
-    fn from_python_ref(v: &PyAny) -> Result<Self, PyErr> {
+    pub fn from_python_ref(v: &PyAny) -> Result<Self, PyErr> {
         if v.is_none() {
             return Ok(Self::Null);
         }
@@ -47,7 +41,7 @@ impl CLReprPython for CLRepr {
             Self::Int(i)
         } else if v.get_type().is_subclass_of::<PyDict>()? {
             let d = v.downcast::<PyDict>()?;
-            let mut obj = CLReprObject::new();
+            let mut obj = CLReprObject::new(CLReprObjectKind::Object);
 
             for (k, v) in d.iter() {
                 if k.get_type().is_subclass_of::<PyString>()? {
@@ -126,6 +120,16 @@ impl CLReprPython for CLRepr {
         })
     }
 
+    fn into_py_dict_impl(obj: CLReprObject, py: Python) -> Result<&PyDict, PyErr> {
+        let r = PyDict::new(py);
+
+        for (k, v) in obj.into_iter() {
+            r.set_item(k, Self::into_py_impl(v, py)?)?;
+        }
+
+        Ok(r)
+    }
+
     fn into_py_impl(from: CLRepr, py: Python) -> Result<PyObject, PyErr> {
         Ok(match from {
             CLRepr::String(v, _) => PyString::new(py, &v).to_object(py),
@@ -155,11 +159,7 @@ impl CLReprPython for CLRepr {
                 PyTuple::new(py, elements).to_object(py)
             }
             CLRepr::Object(obj) => {
-                let r = PyDict::new(py);
-
-                for (k, v) in obj.into_iter() {
-                    r.set_item(k, Self::into_py_impl(v, py)?)?;
-                }
+                let r = Self::into_py_dict_impl(obj, py)?;
 
                 r.to_object(py)
             }
@@ -189,7 +189,19 @@ impl CLReprPython for CLRepr {
         })
     }
 
-    fn into_py(self, py: Python) -> Result<PyObject, PyErr> {
+    pub fn into_py_dict(self, py: Python) -> Result<&PyDict, PyErr> {
+        Ok(match self {
+            CLRepr::Object(obj) => Self::into_py_dict_impl(obj, py)?,
+            other => {
+                return Err(PyErr::new::<PyNotImplementedError, _>(format!(
+                    "Unable to convert {:?} into PyDict",
+                    other.kind()
+                )))
+            }
+        })
+    }
+
+    pub fn into_py(self, py: Python) -> Result<PyObject, PyErr> {
         Self::into_py_impl(self, py)
     }
 }

packages/cubejs-backend-native/src/cross/mod.rs

@@ -4,7 +4,7 @@ mod clrepr_python;
 #[cfg(feature = "python")]
 mod py_in_js;
 
-pub use clrepr::{CLRepr, CLReprKind, CLReprObject, StringType};
+pub use clrepr::{CLRepr, CLReprKind, CLReprObject, CLReprObjectKind, StringType};
 
 #[cfg(feature = "python")]
-pub use clrepr_python::{CLReprPython, PythonRef};
+pub use clrepr_python::PythonRef;

packages/cubejs-backend-native/src/python/cube_config.rs

@@ -2,7 +2,7 @@ use convert_case::{Case, Casing};
 use neon::prelude::*;
 use pyo3::{PyAny, PyResult};
 
-use crate::cross::{CLRepr, CLReprObject, CLReprPython};
+use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind};
 
 pub struct CubeConfigPy {
     properties: CLReprObject,
@@ -11,7 +11,7 @@ pub struct CubeConfigPy {
 impl CubeConfigPy {
     pub fn new() -> Self {
         Self {
-            properties: CLReprObject::new(),
+            properties: CLReprObject::new(CLReprObjectKind::Object),
         }
     }
 

packages/cubejs-backend-native/src/python/entry.rs

@@ -77,9 +77,9 @@ fn python_load_model(mut cx: FunctionContext) -> JsResult<JsPromise> {
 
         let model_module = PyModule::from_code(py, &model_content, &model_file_name, "")?;
 
-        let mut collected_functions = CLReprObject::new();
-        let mut collected_variables = CLReprObject::new();
-        let mut collected_filters = CLReprObject::new();
+        let mut collected_functions = CLReprObject::new(CLReprObjectKind::Object);
+        let mut collected_variables = CLReprObject::new(CLReprObjectKind::Object);
+        let mut collected_filters = CLReprObject::new(CLReprObjectKind::Object);
 
         if model_module.hasattr("template")? {
             let template = model_module.getattr("template")?;

packages/cubejs-backend-native/src/python/python_model.rs

@@ -1,6 +1,6 @@
 use neon::prelude::*;
 
-use crate::cross::{CLRepr, CLReprObject};
+use crate::cross::{CLRepr, CLReprObject, CLReprObjectKind};
 
 pub struct CubePythonModel {
     functions: CLReprObject,
@@ -23,7 +23,7 @@ impl Finalize for CubePythonModel {}
 impl CubePythonModel {
     #[allow(clippy::wrong_self_convention)]
     pub fn to_object<'a, C: Context<'a>>(self, cx: &mut C) -> JsResult<'a, JsValue> {
-        let mut obj = CLReprObject::new();
+        let mut obj = CLReprObject::new(CLReprObjectKind::Object);
         obj.insert("functions".to_string(), CLRepr::Object(self.functions));
         obj.insert("variables".to_string(), CLRepr::Object(self.variables));
         obj.insert("filters".to_string(), CLRepr::Object(self.filters));

packages/cubejs-backend-native/src/python/runtime.rs

@@ -1,4 +1,4 @@
-use crate::cross::{CLRepr, CLReprPython};
+use crate::cross::CLRepr;
 use crate::python::neon_py::*;
 use crate::tokio_runtime_node;
 use cubesql::CubeError;
@@ -95,14 +95,19 @@ impl PyRuntime {
         let (fun, args, callback) = task.split();
 
         let task_result = Python::with_gil(move |py| -> PyResult<PyScheduledFunResult> {
-            let mut args_tuple = Vec::with_capacity(args.len());
+            let mut prep_tuple = Vec::with_capacity(args.len());
+            let mut py_kwargs = None;
 
             for arg in args {
-                args_tuple.push(arg.into_py(py)?);
+                if arg.is_kwarg() {
+                    py_kwargs = Some(arg.into_py_dict(py)?);
+                } else {
+                    prep_tuple.push(arg.into_py(py)?);
+                }
             }
 
-            let args = PyTuple::new(py, args_tuple);
-            let call_res = fun.call1(py, args)?;
+            let py_args = PyTuple::new(py, prep_tuple);
+            let call_res = fun.call(py, py_args, py_kwargs)?;
 
             let is_coroutine = unsafe { pyo3::ffi::PyCoro_CheckExact(call_res.as_ptr()) == 1 };
             if is_coroutine {

packages/cubejs-backend-native/src/template/entry.rs

@@ -110,7 +110,7 @@ impl JinjaEngine {
         let template_compile_context = CLRepr::from_js_ref(cx.argument::<JsValue>(1)?, &mut cx)?;
         let template_python_context = CLRepr::from_js_ref(cx.argument::<JsValue>(2)?, &mut cx)?;
 
-        let mut to_jinja_ctx = CLReprObject::new();
+        let mut to_jinja_ctx = CLReprObject::new(CLReprObjectKind::Object);
         to_jinja_ctx.insert("COMPILE_CONTEXT".to_string(), template_compile_context);
 
         if !template_python_context.is_null() {

packages/cubejs-backend-native/src/template/mj_value/python.rs

@@ -64,7 +64,11 @@ pub fn from_minijinja_value(from: &mjv::Value) -> Result<CLRepr, mj::Error> {
             Ok(CLRepr::Array(arr))
         }
         mjv::ValueKind::Map => {
-            let mut obj = CLReprObject::new();
+            let mut obj = CLReprObject::new(if from.is_kwargs() {
+                CLReprObjectKind::KWargs
+            } else {
+                CLReprObjectKind::Object
+            });
 
             for key in from.try_iter()? {
                 let value = if let Ok(v) = from.get_item(&key) {

packages/cubejs-backend-native/test/__snapshots__/jinja.test.ts.snap

@@ -185,7 +185,12 @@ exports[`Jinja (new api) render arguments-test.yml.jinja: arguments-test.yml.jin
   arg_seq_1: [1,2,3,4,5]
   arg_seq_2: [5,4,3,2,1]
   arg_sum_tuple: 3
-  arg_sum_map: 20"
+  arg_sum_map: 20
+  arg_kwargs1: \\"arg1: first value, arg2: second value, kwarg:(three=3 arg)\\"
+  arg_kwargs2: \\"arg1: first value, arg2: second value, kwarg:(four=4 arg,three=3 arg)\\"
+  arg_kwargs3: \\"arg1: first value, arg2: second value, kwarg:(five=5 arg,four=4 arg,three=3 arg)\\"
+  arg_named_arguments1: \\"arg1: 1 arg, arg2: 2 arg\\"
+  arg_named_arguments2: \\"arg1: 1 arg, arg2: 2 arg\\""
 `;
 
 exports[`Jinja (new api) render data-model.yml.jinja: data-model.yml.jinja 1`] = `
@@ -249,7 +254,7 @@ dump:
 exports[`Jinja (new api) render template_error_python.jinja: template_error_python.jinja 1`] = `
 [Error: could not render block: Call error: Python error: Exception: Random Exception
 Traceback (most recent call last):
-  File "jinja-instance.py", line 110, in throw_exception
+  File "jinja-instance.py", line 120, in throw_exception
 
 ------------------------- template_error_python.jinja -------------------------
    3 | 3