[PATCH proxmox 1/1] api-macro: correctly recognize fully qualified Option paths

[PATCH proxmox 1/1] api-macro: correctly recognize fully qualified Option paths

[Michael Köppl]

is_option_type only matches Option<T> and std::Option<T>, so
std::option::Option<T> and core::option::Option<T> were not considered
valid paths for Option. For optional parameters this caused a spurious
"optional types need a default or be an Option<T>" error and struct
fields failed to compile (e.g. with "description not allows on external
type").

Also drop the std::Option<T> case. That path only resolved is std is
shadowed by a local module or import and renamed imports are not handled
here anyway, as noted in is_option_type's comment.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
I stumbled upon this while reviewing Lukas' application context
injection series. I'm not quite sure if it makes sense to handle it
this way or even fix it at all, but std::option::Option should be
legitimate to use, as well as core::option::Option, so I wanted to
propose it anyway. It is a bit of a band-aid solution since we'll never
be able to cover every case here, but since the is_option_type function
even mentions that renames are explicitly not handled, std::Option<T>
should not work (since AFAICT that would require a rename, unless I'm
missing something), and the correct paths (without renaming) should
work.

 proxmox-api-macro/src/util.rs      |  6 ++++-
 proxmox-api-macro/tests/options.rs | 35 ++++++++++++++++++++++++++++++
 proxmox-api-macro/tests/types.rs   | 30 +++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/proxmox-api-macro/src/util.rs b/proxmox-api-macro/src/util.rs
index 260a168d..65ff1732 100644
--- a/proxmox-api-macro/src/util.rs
+++ b/proxmox-api-macro/src/util.rs
@@ -585,7 +585,11 @@ pub fn is_option_type(ty: &syn::Type) -> Option<&syn::Type> {
         let segs = &p.path.segments;
         let is_option = match segs.len() {
             1 => segs.last().unwrap().ident == "Option",
-            2 => segs.first().unwrap().ident == "std" && segs.last().unwrap().ident == "Option",
+            3 => {
+                (segs.first().unwrap().ident == "std" || segs.first().unwrap().ident == "core")
+                    && segs.get(1).unwrap().ident == "option"
+                    && segs.last().unwrap().ident == "Option"
+            }
             _ => false,
         };
         if !is_option {
diff --git a/proxmox-api-macro/tests/options.rs b/proxmox-api-macro/tests/options.rs
index 6d61cc10..d85950e6 100644
--- a/proxmox-api-macro/tests/options.rs
+++ b/proxmox-api-macro/tests/options.rs
@@ -39,6 +39,28 @@ pub fn test_default_macro(value: Option<isize>) -> Result<isize, Error> {
     Ok(value.unwrap_or(api_get_default!("value")))
 }

+#[api(
+    input: {
+        properties: {
+            a: {
+                description: "An optional value using the full std path",
+                optional: true,
+            },
+            b: {
+                description: "An optional value using the full core path",
+                optional: true,
+            },
+        }
+    }
+)]
+/// Return the sum of both values, treating missing values as 0.
+pub fn test_qualified_option(
+    a: std::option::Option<u64>,
+    b: ::core::option::Option<u64>,
+) -> Result<u64, Error> {
+    Ok(a.unwrap_or(0) + b.unwrap_or(0))
+}
+
 struct RpcEnv;
 impl proxmox_router::RpcEnvironment for RpcEnv {
     fn result_attrib_mut(&mut self) -> &mut Value {
@@ -86,4 +108,17 @@ fn test_invocations() {
         api_function_test_default_macro(json!({}), &API_METHOD_TEST_DEFAULT_MACRO, &mut env)
             .expect("func with option should work");
     assert_eq!(value, 5);
+
+    let value =
+        api_function_test_qualified_option(json!({}), &API_METHOD_TEST_QUALIFIED_OPTION, &mut env)
+            .expect("func with qualified option should work");
+    assert_eq!(value, 0);
+
+    let value = api_function_test_qualified_option(
+        json!({"a": 2, "b": 3}),
+        &API_METHOD_TEST_QUALIFIED_OPTION,
+        &mut env,
+    )
+    .expect("func with qualified option should work");
+    assert_eq!(value, 5);
 }
diff --git a/proxmox-api-macro/tests/types.rs b/proxmox-api-macro/tests/types.rs
index b6609cfb..c7b47907 100644
--- a/proxmox-api-macro/tests/types.rs
+++ b/proxmox-api-macro/tests/types.rs
@@ -46,6 +46,12 @@ pub struct TestStruct {
     /// An optional auto-derived value for testing:
     another: Option<String>,

+    /// Optional value using a fully qualified std path.
+    another_qualified: std::option::Option<String>,
+
+    /// Optional value using a global core path.
+    another_qualified_global: ::core::option::Option<String>,
+
     /// Disabled field.
     #[cfg(false)]
     disabled_field: String,
@@ -62,6 +68,20 @@ fn test_struct() {
                 &::proxmox_schema::StringSchema::new("An optional auto-derived value for testing:")
                     .schema(),
             ),
+            (
+                "another_qualified",
+                true,
+                &::proxmox_schema::StringSchema::new(
+                    "Optional value using a fully qualified std path.",
+                )
+                .schema(),
+            ),
+            (
+                "another_qualified_global",
+                true,
+                &::proxmox_schema::StringSchema::new("Optional value using a global core path.")
+                    .schema(),
+            ),
             (
                 "test_string",
                 false,
@@ -81,6 +101,16 @@ fn test_struct() {
                 description: "An optional auto-derived value for testing:",
                 optional: true,
             },
+            another_qualified: {
+                type: String,
+                description: "Optional value using a fully qualified std path.",
+                optional: true,
+            },
+            another_qualified_global: {
+                type: String,
+                description: "Optional value using a global core path.",
+                optional: true,
+            },
             test_string: {
                 type: String,
                 description: "A test string.",
--
2.47.3