sidorares / dbus-native

D-bus protocol client and server for node.js written in native javascript
Other
260 stars 93 forks source link

API break between 0.1.1 and 0.1.2 #52

Open ecksun opened 9 years ago

ecksun commented 9 years ago

I recently upgraded from dbus-native 0.1.1 to 0.1.2 and now get crashes that appear to be because of a change in API.

Bisecting the issue I found that my code stopped working with commit f21eab6b4fe80e14ad8885b0c89e9b9ca19f9d0e

I deviced the following small test to illustrate the difference:

'use strict';
var assert = require('assert');
var dbus = require('dbus-native');

var sysbus = dbus.systemBus();
var service = sysbus.getService('org.freedesktop.NetworkManager');

service.getInterface('/org/freedesktop/NetworkManager', 'org.freedesktop.DBus.Properties', function(err, propIface) {
    assert.ifError(err);
    propIface.GetAll('org.freedesktop.NetworkManager', function(err, props) {
        assert.ifError(err);
        console.log(require('util').inspect(props, { depth: null }));
    });
});

With version 0.1.1 I get the following output:

[ [ 'Devices',
    [ [ '/org/freedesktop/NetworkManager/Devices/0',
        '/org/freedesktop/NetworkManager/Devices/1',
        '/org/freedesktop/NetworkManager/Devices/2',
        '/org/freedesktop/NetworkManager/Devices/4',
        '/org/freedesktop/NetworkManager/Devices/6',
        '/org/freedesktop/NetworkManager/Devices/7' ] ] ],
  [ 'NetworkingEnabled', [ true ] ],
  [ 'WirelessEnabled', [ true ] ],
  [ 'WirelessHardwareEnabled', [ true ] ],
  [ 'WwanEnabled', [ true ] ],
  [ 'WwanHardwareEnabled', [ true ] ],
  [ 'WimaxEnabled', [ true ] ],
  [ 'WimaxHardwareEnabled', [ true ] ],
  [ 'ActiveConnections',
    [ [ '/org/freedesktop/NetworkManager/ActiveConnection/23',
        '/org/freedesktop/NetworkManager/ActiveConnection/3',
        '/org/freedesktop/NetworkManager/ActiveConnection/2',
        '/org/freedesktop/NetworkManager/ActiveConnection/1',
        '/org/freedesktop/NetworkManager/ActiveConnection/0' ] ] ],
  [ 'PrimaryConnection',
    [ '/org/freedesktop/NetworkManager/ActiveConnection/1' ] ],
  [ 'ActivatingConnection', [ '/' ] ],
  [ 'Startup', [ false ] ],
  [ 'Version', [ '0.9.10.0' ] ],
  [ 'State', [ 70 ] ],
  [ 'Connectivity', [ 4 ] ] ]

While with version 0.1.2 I get the following:

[ [ 'Devices',
    [ [ { type: 'a', child: [ { type: 'o', child: [] } ] } ],
      [ [ '/org/freedesktop/NetworkManager/Devices/0',
          '/org/freedesktop/NetworkManager/Devices/1',
          '/org/freedesktop/NetworkManager/Devices/2',
          '/org/freedesktop/NetworkManager/Devices/4',
          '/org/freedesktop/NetworkManager/Devices/6',
          '/org/freedesktop/NetworkManager/Devices/7' ] ] ] ],
  [ 'NetworkingEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WirelessEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WirelessHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WwanEnabled', [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WwanHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WimaxEnabled', [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WimaxHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'ActiveConnections',
    [ [ { type: 'a', child: [ { type: 'o', child: [] } ] } ],
      [ [ '/org/freedesktop/NetworkManager/ActiveConnection/23',
          '/org/freedesktop/NetworkManager/ActiveConnection/3',
          '/org/freedesktop/NetworkManager/ActiveConnection/2',
          '/org/freedesktop/NetworkManager/ActiveConnection/1',
          '/org/freedesktop/NetworkManager/ActiveConnection/0' ] ] ] ],
  [ 'PrimaryConnection',
    [ [ { type: 'o', child: [] } ],
      [ '/org/freedesktop/NetworkManager/ActiveConnection/1' ] ] ],
  [ 'ActivatingConnection',
    [ [ { type: 'o', child: [] } ], [ '/' ] ] ],
  [ 'Startup', [ [ { type: 'b', child: [] } ], [ false ] ] ],
  [ 'Version', [ [ { type: 's', child: [] } ], [ '0.9.10.0' ] ] ],
  [ 'State', [ [ { type: 'u', child: [] } ], [ 70 ] ] ],
  [ 'Connectivity', [ [ { type: 'u', child: [] } ], [ 4 ] ] ] ]

Obiously this is a breaking change, my question is if this is an intentional API break or if I should consider it a bug?

If it is an intentional break I think we should add some documentation detailing the new API.

sidorares commented 9 years ago

I think current ( 0.1.2 ) behaviour is consistent with pre 0.1 and what you had with 0.1.1 was an accidental bug ( most of the deserialising code is rewritten by #41 )

The idea is that by default given message top level signature and message data you should be able to generate back exactly the same binary representation of the message. This is something not necessary to most users, so that default might be changed ( but still exist behind a flag ) in favour of more compact. This is especially important when you handle json-like list of properties where "full" view of a(sv) is really difficult to read or write.

Documentation is probably on top of my priority list and any help would be appreciated

In short:

For example, if we switch to 'lightweight' results by default api might be like this

// 0.1.1 - like behaviour:
var sysbus = dbus.systemBus({ preserveTypes: true });
// new default: compact (de)searialisation of variands
var sysbus1 = dbus.systemBus();

wdyt?

ecksun commented 9 years ago

Aha, I see.

Im not very familiar with the details of dbus, could you give an example of a case where data is lost when converting from dbus types to javascript types?

In your example, you write that the default should be the default, with default do you mean the behaviour in 0.1.1? Also, would preserveTypes mean to not loose any data, that is to preserve the dbus types or does it mean that we preserver javascript types?

However I think the idea in general is good, I guess you could also expose some utility to convert from one format to the other, dunno if thats better though.

var niceSysBus = new dbus.NiceBus(dbus.systemBus());

In order to give a better response I think I need to study the dbus type system some more, do you have any good references I could read?

sidorares commented 9 years ago

If you have "ay" array and JS data like [ 1, 2, 3, "aaa" ] you can only guess actual type of elements - first 3 can be any of y,n,q,i,u,x,d,t types and fourth any of s,o,g. If data is

[  
  [ [ { type: 'b', child: [] } ], [ true ] ], 
  [ [ { type: 'i', child: [] } ], [ 123 ] ], 
  [ [ { type: 'y', child: [] } ], [ 123 ] ],
  [ [ { type: 's', child: [] } ], [ "aaa"] ],
]

then you know exactly how to serialise your data

I'll try to describe dbus type system in details (hopefully copy something from here to readme)

D-bus uses simple string type representation to describe how message should be serialised to and from raw binary data. Each character in the "signature" represents simple type, start of array/struct/hash or variant. Simple types:

name encoding js example
y unsigned byte 123
b unsigned 32 bit true
n signed 16 bit -300
q unsigned 16 bit 65000
i signed 32 bit -123000
u unsigned 32 bit 123000
x signed 64 bit 123
t unsigned 64 bit 123
d IEEE 754 double 3.14159
h unsigned 32 bit unix fd, not supported

String-like types are prefixed with length ( 8 bit for "g" and 32 for "o" and "s" )

name encoding js example
s,o 32 bit unsigned int + utf8 bytes + '\0' "test string"
g 8 bit unsigned int + ascii bytes + '\0' "iisa(ii)"

arrays, structs and maps:

array signature is "a" followed by full signature ( simple type, structure, array, struct or variant )

signature JS example
ai [1, 2, 3]
aai [ [1,2,3], [], [5,6]]

structure is described with "(" and ")" and a list if full types in between. Often used to describe single type for array or tuple element

signature JS example
i(is) [ 1, [2, "baz" ] ]
a(ii) [ [1, 2], [3, 4] ]
a(is) [ [1, "foo"], [2, "bar" ]

structure described with "{" and "}" is similar to "()" with exception that it can only contain have exactly two values - it's a tuple. Usually goes together with "a" to represent map

signature JS example
a{si} [ [ "foo", 1 ], [ "bar", 2 ] ]

Variants are prefixed with signature followed by encoded value

signature JS example
ay [ [ { type: 's', child: [] } ], [ '0.9.10.0' ] , [ { type: 'b', child: [] } ], [ false ] ] ],

( to be continued ... )

ecksun commented 9 years ago

Aha, I see, I did not think the dbus type system was so detailed. That explains quite a lot, thank you :)

I also think that you can steal most of this and put it in the documenation.

When I considered this problem I originaly did not think of the case converting from js types to dbus types (which isn't fully possible). However converting from dbus types from js types should work fine.

With that in mind though, I guess its not ever really possible to convert from js types to dbus types in a good manner, that is the "preserveTypes" mode doesnt work very well going from js -> dbus, right?

However when reading (which is what I mostly do) it should be fine providing two difrerent modes (or a way to convert from dbus types to js types).

Basically, what I will do when I get time is to rewrite my dbus-native wrapper to convert from dbus-types to javascript types. It would be nice if that utility could be provided by dbus-native.

sidorares commented 9 years ago

Automatic conversion draft:

JS -> dbus serialisation

signature JS short JS full
av [ 1, 2, true, "test" ] [ [ { type: 'd', child: [] } ], [ 1 ] ], [ { type: 'd', child: [] } ], [ 2 ] ], [ { type: 'b', child: [] } ], [ 1 ] ], [ { type: 's', child: [] } ], [ "test ] ] ]
a{sv} { test: "foo" } [ [ "test", [ { type: 's', child: [] } ], [ "test ] ] ] ]