*Dict macros don't support nested dictionaries 'a{sa{sv}}'

[zeenix]

In GitLab by @seiwill on Feb 1, 2023, 17:58

Dictionary of Dictionaries is not supported and returns an Error::PaddingNot0 when decoded by Message::body().

This code attempts to get the ActiveConnection Settings from NetworkManager. It reads the a{sa{sv}} ConnectionSettings which Optionally contains other Dict Structs.

use zbus::{blocking::{Connection, Proxy}, zvariant::{DeserializeDict, Type, OwnedObjectPath}};

#[derive(Debug, Clone, PartialEq, Default, Type, DeserializeDict)]
#[zvariant(signature = "dict")]
pub struct SettingsConnection {
    pub autoconnect: Option<bool>,
    pub id: Option<String>,
    pub interface_name: Option<String>,
    pub read_only: Option<bool>,
    pub timestamp: Option<u64>,
    #[zvariant(rename = "type")]
    pub connection_type: Option<String>,
    pub uuid: Option<String>,
}

#[derive(Debug, Clone, PartialEq, Default, Type, DeserializeDict)]
#[zvariant(signature = "dict")]
pub struct SettingsIp {
    pub method: Option<String>,
    pub addresses: Vec<String>,
}

#[derive(Debug, Clone, PartialEq, Default, Type, DeserializeDict)]
#[zvariant(signature = "a{sa{sv}}")]
pub struct ConnectionSettings {
    pub connection: Option<SettingsConnection>,
    pub ipv4: Option<SettingsIp>,
}

fn main() {
    let connection = Connection::system().unwrap();
    let proxy = Proxy::new(
        &connection,
        "org.freedesktop.NetworkManager",
        "/org/freedesktop/NetworkManager",
        "org.freedesktop.NetworkManager"
    ).unwrap();

    let active_connections: Vec<OwnedObjectPath> = proxy.get_property("ActiveConnections").unwrap();
    for ac in active_connections {
        println!("Looking up {ac:?}");

        let ac_proxy = Proxy::new(
            &connection,
            "org.freedesktop.NetworkManager",
            &ac,
            "org.freedesktop.NetworkManager.Connection.Active"
        ).unwrap();
        let conn_path: OwnedObjectPath = ac_proxy.get_property("Connection").unwrap();
        println!("  Connection Settings {conn_path:?}");

        let settings_proxy = Proxy::new(
            &connection,
            "org.freedesktop.NetworkManager",
            &conn_path,
            "org.freedesktop.NetworkManager.Settings.Connection"
        ).unwrap();

        let message = settings_proxy.call_method("GetSettings", &()).unwrap();
        println!("  Reply: {message:?}");

        // let settings_map : HashMap<String, HashMap<String, zvariant::OwnedValue>> = message.body().unwrap();
        let settings = message.body::<ConnectionSettings>().unwrap();
        println!("  {settings:?}\n");
    }
}

Running panics on the message.body().unwrap():

Looking up OwnedObjectPath(ObjectPath("/org/freedesktop/NetworkManager/ActiveConnection/5"))
  Connection Settings OwnedObjectPath(ObjectPath("/org/freedesktop/NetworkManager/Settings/23"))
  Reply: Msg { type: MethodReturn, sender: UniqueName(Str(Borrowed(":1.5"))), reply-serial: 13, body: Signature("a{sa{sv}}") }
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Variant(PaddingNot0(15))', src/main.rs:102:61
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::result::unwrap_failed
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/result.rs:1814:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/result.rs:1107:23
   4: nm_test::main
             at ./src/main.rs:102:24
   5: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5

[jonathanhood]

I'm also encountering this issue (also specifically with attempting to interact with NetworkManager). I see the exact same behavior. +1 for a fix here - or guidance on how to fix myself / workaround.

[zeenix]

I'm also encountering this issue (also specifically with attempting to interact with NetworkManager).

Sorry to hear but such information isn't helpful to maintainers usually. :( I'm aware of the issue.

guidance on how to fix myself

The code is here. A PR would be most helpful and appreciated. Having said that, I'm not sure this is solvable through the macros. I was more hoping that @sivizius will finish their work on #520 and #552, and hopefully make these macros obsolete so that all (de)serialization is handled by serde.

/ workaround

I guess you can just always use HashMap<&str, Value>.

[jonathanhood]

Thanks for the quick response! My intention was to help and not provide pressure. Sorry if my comment came across the wrong way.

The code is here. A PR would be most helpful and appreciated.

Happy to jump in. I'm guessing matrix is the best place to coordinate. I'll setup an account there. Just want to make sure I'm jumping in at a helpful spot.

I guess you can just always use HashMap<&str, Value>.

I will investigate this to unblock me short term. Thanks for the tip.

[zeenix]

My intention was to help and not provide pressure. Sorry if my comment came across the wrong way.

Sorry for the knee-jerk reaction. It's just so much to improve and fix and very few contributors, even fewer who stick around to see their PRs through.

The code is here. A PR would be most helpful and appreciated.

Happy to jump in.

👍

I'm guessing matrix is the best place to coordinate. I'll setup an account there. Just want to make sure I'm jumping in at a helpful spot.

Yeah, for a real-time chat yes. Although I'm not always looking so there might be a delay in response.

[jonathanhood]

Just want to give a quick update here. I'm picking this up and running with it and have made decent progress here. With experimental changes - I'm seeing a{sa{sv}} types serialize and deserialize well now using the zvariant Serialize and Deserialize implementations. I'm working now on making sure optional struct fields are handled well.

Definitely not PR worthy yet, but making progress.

[zeenix]

Definitely not PR worthy yet, but making progress

👍

[jonathanhood]

I'm taking a step back and thinking about how to reapproach this problem. After significant review and work this morning I've discovered that my experimental handling of a{sa{sv} types - or really any dict of the form a{sv} - was broken. It wasn't serializing values as variants at all.

It turns out this is a shortcoming of the current Serializer and Deserializer implementations. They don't handle cases where serde knows the concrete type and calls serialize_<basic-type>/deserialize_<basic-type> but the dbus type signature is actually variant. What actually happens is that the basic type gets marshalled without the variant type signature - which is a significant bug.

I'm also finding other bugs in the gvariant implementations - for example I'm having issues with string keys over there (it looks like serialize_identifier is broken over there) and issues with string variants (I always get empty string values).

Finally, to properly handle optional keys in a dictionary - where the service handles absence of a value by simply not sending the key at all - requires the serializer to be more stateful. The serializer needs to know when a dictionary entry is being serialized and change how the serialize_some call is handled in particular.

In general, it seems that transforming Serialize and Deserialize into general use serde implementations is a pretty big lift. That is, unfortunately, pretty necessary for handling dictionaries of the form a{sv} since the variant type in there basically allows these dictionaries to be nearly arbitrarily nested at runtime.

My current knee jerk reaction to these discoveries is that a significant overhaul of these implementations is required and it will likely need to happen across multiple PRs and care will need to be taken to avoid breaking existing use cases while that work is ongoing. That means either a feature flag to select the new implementation - or building as a sidecar and later merging the worlds back together.

I would appreciate some guidance/conversation on the best path forward @zeenix

[zeenix]

They don't handle cases where serde knows the concrete type and calls serialize_<basic-type>/deserialize_<basic-type> but the dbus type signature is actually variant. What actually happens is that the basic type gets marshalled without the variant type signature - which is a significant bug.

I wouldn't call it a significant bug but rather an limitation/shortcomming. *Dict macros came after we had those implementation and not from me.

I'm also finding other bugs in the gvariant implementations

You're bound to find bugs/issues. :) Please file issues for them first before contributing PRs to ensure that we don't completely forget about them in case you don't succeed in creating a fix. Also it's good for discussing it first to see if it really is something that should and can be fixed.

Finally, to properly handle optional keys in a dictionary - where the service handles absence of a value by simply not sending the key at all - requires the serializer to be more stateful. The serializer needs to know when a dictionary entry is being serialized and change how the serialize_some call is handled in particular.

👍

My current knee jerk reaction to these discoveries is that a significant overhaul of these implementations is required

My suggestion is to take it one thing at a time.

will likely need to happen across multiple PRs

That's never a problem but keep in mind that we emphasize atomic commits and each PR can have multiple commits but of course, it's always best to also keep the PRs smaller, where it makes sense. It boils down to whether or not a PR is doing something useful on its own that we want regardless of any planned future developments on top. If yes, then it should be its own PR.

care will need to be taken to avoid breaking existing use cases while that work is ongoing. That means either a feature flag to select the new implementation

Although I've not yet seen anything you mentioned that would require any breakage, we can definitely discuss it when/where needed.

• or building as a sidecar and later merging the worlds back together.

Don't really see the point of that but you can always experiment in your own private branch until everything is ready. Just try your best to get as many non-breaking parts merged while at it and keep your branch(es) rebased on latest git main, so it's not a massive task in the end to merge the all your changes.

[zeenix]

Actually I was never really sure we can do much about this one, since it's not about putting an a{sv} in the v (variant) of another a{sv} but rather support of a different kind of (de)serialization. Now with *Dict macros deprecated (#1354), it's not even in our domain anymore.